ci: add runtime-aware Riot sharding - #19689
Conversation
Co-authored-by: mr-lee <1302416+mr-lee@users.noreply.github.com>
|
I can only run on private repositories. |
|
Codeowners resolved asResolved from the full PR diff against |
Dependency direction analysis📈 Existing violations got worse3 pre-existing violation(s) increased in severity (e.g. their target became more depended-on, or got pulled into an import cycle), though the edge itself isn't new:
|
Circular import analysis
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbfae8ba36
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| riot list --hash-only "${SUITE_NAME}" | sort | \ | ||
| ./scripts/ci_allocation_cli.py select \ |
There was a problem hiding this comment.
This commit changes this script from mode 100755 to 100644, but .gitlab/tests.yml still executes it directly, and get-riot-pip-cache-key.sh does the same while generating the pipeline. On Unix runners both paths now fail with Permission denied, preventing test configuration generation and Riot jobs from running; retain the executable bit.
Useful? React with 👍 / 👎.
| lines.append(f" - riot -v run -s --pass-env wait -- {' '.join(wait_for)}") | ||
|
|
||
| env = self.env | ||
| env = dict(self.env or {}) |
There was a problem hiding this comment.
This commit changes scripts/gen_gitlab_config.py from mode 100755 to 100644, while the tests-gen job in .gitlab-ci.yml invokes it directly as scripts/gen_gitlab_config.py --verbose. On Unix runners that job terminates immediately with Permission denied, so the generated child pipeline and all tests are unavailable; preserve mode 100755.
Useful? React with 👍 / 👎.
| nanoseconds = attributes.get(nanoseconds_key, 0) | ||
| if isinstance(nanoseconds, (int, float)) and not isinstance(nanoseconds, bool): | ||
| return float(nanoseconds) / 1_000_000_000 |
There was a problem hiding this comment.
When a Datadog job event contains neither queue_seconds nor ci.queue_time, this default turns the missing measurement into a valid 0.0. build-live-report then emits apparently complete zero-valued queue evidence, so check-ratchet can approve a candidate even though the required queue regression signal was absent; missing queue timing should fail ingestion or be represented as missing evidence.
Useful? React with 👍 / 👎.
| for strategy in ("legacy", "balanced"): | ||
| selected_jobs = [job for job in jobs if job.pipeline_id == pipeline_id and job.strategy == strategy] | ||
| if not selected_jobs: | ||
| raise AllocationError(f"live shadow CI job timings are missing for {pipeline_id} {strategy}") |
There was a problem hiding this comment.
For a partial or paginated CI job export, this only checks that at least one job exists for each pipeline and strategy. If a slow shard is missing while other shard events remain, the report still computes makespan and runner consumption from the incomplete set and can pass the promotion ratchet; validate the observed shard indices and totals for every suite and strategy before accepting CI-job timing evidence.
Useful? React with 👍 / 👎.
| if not isinstance(stage_name, str) or not stage_name: | ||
| raise AllocationError("Datadog CI job event is missing its stage name") | ||
| suite, shard_index, shard_total = suite_from_job_name(job_name, stage_name) | ||
| strategy = "balanced" if "-allocation-shadow" in job_name else "legacy" |
There was a problem hiding this comment.
When the policy promotes balanced to the active strategy, the required jobs retain their normal names and only shadow jobs carry -allocation-shadow, so this records every active balanced job as legacy. Subsequent job-history ingestion and model provenance therefore mix balanced topology timings into the legacy population; derive the strategy from the emitted allocation tag or otherwise encode it for active jobs.
Useful? React with 👍 / 👎.
| execution = { | ||
| key: value | ||
| for key, value in properties.items() | ||
| if key.startswith("riot.") and key not in {"riot.hash", "riot.ci.allocation_strategy"} | ||
| } |
There was a problem hiding this comment.
When xdist omits testsuite properties—the case the new filename fallback explicitly anticipates—both strategies produce an empty execution dictionary here. The dictionaries compare equal, so verify-junit reports execution_metadata_parity: true even though the filenames only prove the strategy and Riot hash, not Python version or other Riot execution metadata; require the expected metadata to be present or validate it against another authoritative artifact before asserting parity.
Useful? React with 👍 / 👎.
Co-authored-by: mr-lee <1302416+mr-lee@users.noreply.github.com>
BenchmarksBenchmark execution time: 2026-08-14 22:08:52 Comparing candidate commit 40503d7 in PR branch Found 0 performance improvements and 9 performance regressions! Performance is the same for 613 metrics, 10 unstable metrics.
|
Co-authored-by: mr-lee <1302416+mr-lee@users.noreply.github.com>
mr-lee
added
the
changelog/no-changelog
Co-authored-by: mr-lee <1302416+mr-lee@users.noreply.github.com>
Co-authored-by: mr-lee <1302416+mr-lee@users.noreply.github.com>
Co-authored-by: mr-lee <1302416+mr-lee@users.noreply.github.com>
1 participant
Description
Motivation
Riot suites preserve meaningful ownership and authoring boundaries, but count-based round-robin execution cannot account for large runtime differences between Riot environments. The first same-head shadow run reduced total Riot runner time but left the longest job nearly unchanged because some long poles were already a single Riot hash. This change adds fail-closed runtime test-item slicing so CI can break those poles without changing suitespec authoring or silently dropping or duplicating work.
Changes
107d2ec@1/3; pytest collects the authored command and deterministically selects a disjoint node-ID slice at runtime.Testing
scripts/lint checks, including formatting, typing, security, suitespec coverage, dependency/configuration validation, and static analysis.git diff --check.scripts/run-tests --venv 116989a tests/internal/test_ci_allocation.pywas attempted, but repository environment setup stopped before collection when the existing libddwaf 2.0.1 download returned 502 through the sandbox proxy.Risks
Runtime slicing could expose collection differences, hidden test ordering assumptions, or setup costs that differ from the fitted model. Splitting also repeats Riot activation work. The implementation limits slices to measured pytest commands, keeps sibling slices in separate jobs, preserves the total job budget, records exact inventories and JUnit identities, and leaves legacy active until live correctness, runner-time, queue, reliability, and critical-path gates pass.
Additional Notes
This is internal CI infrastructure with no customer-facing behavior change. No release note is needed; apply the
changelog/no-changeloglabel.PR by Bits - View session in Datadog
Comment @DataDog to request changes