Adopt standalone Helix monitoring for SDK performance CI - #5307
LoopedBard3 wants to merge 7 commits into
Conversation
Pair internal asynchronous submissions with a credentialed stage monitor and pin its tool alongside the Helix SDK. Preserve public, PR, and local synchronous behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Evaluate the internal-run predicate directly while expanding root variables. Define the async flag, pool provider, and Helix credential group together so same-block variable lookup cannot omit monitor prerequisites. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Enable anonymous monitoring for selected public and PR jobs, keep internal credentials guarded, and bound the monitor to six hours without changing sender or work-item timeouts. Restore the Arcade bootstrap executable bit for Linux tool restore. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Prefix monitor test-run identities with the matrix-expanded agent job name so independent channels sharing a phase and queue are not treated as retries. Retain stable identities for job and stage retries and scope the naming change to monitored SDK flows. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
CI orchestration, authentication, and Helix result handling warrant final human review.
Pull request overview
This pull request adopts standalone Helix monitoring for SDK performance CI while preserving result reporting and artifacts.
Changes:
- Adds conditional monitoring, authentication, and timeout configuration.
- Pins the Helix monitor dependency and tool.
- Restores
dotnet.shexecutable status. - Documents the monitored SDK performance flow.
File summaries
| File | Summary |
|---|---|
eng/Version.Details.xml |
Records the Helix monitor dependency. |
eng/common/dotnet.sh |
Restores the standard executable permission. |
docs/sdk-scenarios.md |
Documents monitoring; add the new section to the manual list. |
azure-pipelines.yml |
Configures asynchronous submission and monitoring. |
.config/dotnet-tools.json |
Pins the Helix Job Monitor tool. |
Review details
Suppressed comments (1)
docs/sdk-scenarios.md:7
- The document maintains a manual section list at the top, but this new top-level section is not included in it. Add an
SDK performance pipelinelink so readers using the document navigation can reach the newly added guidance.
## SDK performance pipeline
- Files reviewed: 4/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Keep asynchronous submission, the pool provider, matrix identity prefix, and monitor parameter block unconditional. Use anonymous Helix credentials outside internal non-PR runs, and allow legitimate no-workload stages without changing workload selection or timeout budgets. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Pipeline wiring and empty-job monitor conditions remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
azure-pipelines.yml:83
- This template is instantiated unconditionally, so an internal Manual run with all three workload-selection flags false still queues
HelixJobMonitoreven thoughsdk-perf-jobs.ymlexpands to no submitter jobs. That contradicts the PR contract that such runs do not start a monitor; gate this template on the same selection conditions used for the submitter jobs.
- template: /eng/common/core-templates/job/helix-job-monitor.yml
docs/sdk-scenarios.md:21
- This documents the inverse of the requested behavior: an internal manual run with no selected workloads should omit the monitor, while a monitored stage with no Helix jobs should fail. The current YAML always emits the monitor and enables
allowNoHelixJobs, so update this text together with the pipeline fix rather than documenting a successful empty monitored stage.
job-selection parameters and schedules are unchanged: public runs always select
correctness jobs, while internal manual runs can select no workloads. Such runs still
include the monitor and may succeed without Helix jobs (`allowNoHelixJobs: true`).
This permits an empty stage; it does not suppress submitter or work-item failures.
- Files reviewed: 4/5 changed files
- Comments generated: 2
- Review effort level: Lite
| - name: EnableHelixJobMonitor | ||
| value: true |
Align monitor credential eligibility with dotnet/runtime#133885: import the Helix group and select its token for the internal project regardless of build reason. Public runs, including public PRs, remain anonymous. Preserve workload selection and monitor settings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The pipeline credential guard and corresponding documentation must be corrected before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
azure-pipelines.yml:86
- This expression selects
$(HelixApiAccessToken)for internal pull-request runs because it checks only the project. That makes the monitor non-anonymous for PRs and is inconsistent with the required non-PR-only credential policy; include the directBuild.Reasonguard in this condition as well.
helixAccessToken: ${{ iif(eq(variables['System.TeamProject'], 'internal'), '$(HelixApiAccessToken)', '') }}
- Files reviewed: 4/5 changed files
- Comments generated: 2
- Review effort level: Lite
| - ${{ if eq(variables['System.TeamProject'], 'internal') }}: | ||
| # The standalone monitor does not inherit the submitter jobs' variable groups. | ||
| - group: DotNet-HelixApi-Access |
| Internal runs, including internal PR runs, import `DotNet-HelixApi-Access`. The | ||
| always-present token parameter uses a compile-time expression to select its token | ||
| macro for the internal project and an empty string otherwise. Public runs, including | ||
| public PR runs, therefore use anonymous Helix access, not an unresolved private-token |
Keep the monitor unconditional but reject zero-job discovery, including internal manual runs with all workload selectors false. Preserve workload selection, credential scope, timeout budgets, and matrix identities. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Summary
Adopt the standalone Helix Job Monitor for the SDK performance flow in
azure-pipelines.yml/sdk-perf-jobs.yml, releasing submitter agents after submission while retaining stage completion and work-item failure reporting.allowNoHelixJobs: false: a run with no Helix jobs fails, including an internal manual run with all three workload selectors false. The monitor is not omitted for empty runs. Submitter and work-item failures continue to propagate.DotNet-HelixApi-Accessfor the internal project, including internal PR runs. The unconditional token parameter uses a compile-timeiifexpression to select the private token macro for internal runs and an empty string otherwise. Public runs, including public PRs, remain anonymous. Both credential guards use onlySystem.TeamProject, not build reason or variables defined in the same root block.Microsoft.DotNet.Helix.JobMonitoralongside the current Arcade/Helix SDK in the local tool manifest and dependency metadata. Restoreeng/common/dotnet.sh's executable bit to match its pinned Arcade source, without changing its contents.Agent.JobName. Independent channels sharing a phase and queue stay distinct, while retries of the same leg retain stable identities.Shared runtime templates and local Python submission/download behavior are unchanged. Existing
Logs_*artifacts remain; arbitrary Helix uploads are not newly copied into pipeline artifacts.Related runtime monitor work: dotnet/runtime#132807 and dotnet/runtime#133633. This independently adopts the monitor for SDK performance; those runtime changes did not cause an SDK regression.
Internal-PR credential eligibility is aligned with dotnet/runtime#133885, which is open and not merged as of this update.
Validation
Current zero-job failure revision
Commit
07aebb250a15a5b4ce8a4c87c4faa21d50ef46cewas validated locally: the updated zero-job regression failed with the previousallowNoHelixJobs: truesetting and passed with the explicitfalsesetting. All six contract checks pass across 1,008 selection/auth combinations, including monitor presence and zero-job failure configuration for internal manual runs with all workload selectors false. Internal PR group/token selection, public PR empty-token/no-group behavior, always-present async/pool/job/parameters, unchanged workload selection, matrix naming, tool pins, and Linux executable metadata remain covered. No manual CI run was queued for this revision; no live result for it is claimed here.The pinned monitor's no-job branch and its existing source tests were reviewed: once other pipeline jobs complete, zero associated Helix jobs with
AllowNoHelixJobs=falsereturns exit code 1. Submitter-failure and nonempty work-item-failure handling are unchanged. The earlier internal-PR credential and unconditional-monitor changes were validated locally; pinned-binary matrix/retry checks verified 88 distinct legs and selection of 88 latest attempts from 176 original-plus-retry records.Earlier end-to-end evidence
Internal SDK sanity run 3073057 tested the earlier commit
c813e894947b143f536e6b6075103a8f9b2af7c9, before the subsequent unconditional-monitor, internal-PR credential, and empty-run policy revisions. It usedonlySanityCheck=true,runScheduledPrivateJobs=true,runPrivateJobs=false, andrunPublicJobs=false.Logs_*artifacts were published.FS3888errors also present in main run 3072588, and the existing Roslyn sanity filter matching no benchmarks. Unrelated workloads were not changed.Earlier local checks also covered cold Linux bootstrap and tool startup, and actual pinned-SDK metadata plus the pinned monitor binary: 88 independent prior submissions remain 88 streams instead of 31; job/stage retry simulations select 88 latest attempts from 176 records. Public CI and live retries were not run. The earlier live run is not evidence of CI validation for the latest revision.
AI disclosure
Implemented and validated with assistance from GitHub Copilot.