fix(suite): merge into the stored summary, not just the local one - #303
Closed
qu0b wants to merge 1 commit into
Closed
fix(suite): merge into the stored summary, not just the local one#303qu0b wants to merge 1 commit into
qu0b wants to merge 1 commit into
Conversation
CreateSuiteOutput enriches an existing summary.json rather than replacing it — it preserves the prior tests[], backfills payload sizes and tx counts from the step files, and folds in opcode data. But it only ever looked for that prior summary on local disk. Every CI worker starts a job with an empty RUNNER_TEMP, so the local suite directory never survives: suiteExists is always false, the merge never runs, and the summary is rebuilt from that one run's inputs and uploaded over whatever the store had accumulated. Two runs of the same suite with different config richness — one with tests.opcode_source set, one without — silently resolve to whichever finished last. Pass the stored summary in as a baseline. The executor fetches it through an optional RemoteSuiteSummary hook, wired to the S3 reader when results upload is configured, so the executor keeps no storage dependency and a local-only run behaves exactly as before. Materialising the step files stays keyed on the local summary alone. A stored summary says nothing about what is on local disk, and conflating the two would leave a wiped worker with nothing to upload — which is how a partially uploaded suite would become permanent. A fetch failure logs and falls back to the previous behaviour: the merge is an enrichment, never a precondition for writing the suite.
![redpandabot[bot]](https://avatars.githubusercontent.com/in/3749543?s=60&v=4){kind=small}
qu0b
added a commit
that referenced
this pull request
Aug 11, 2026
Implements @skylenet's suggestion: a size limit on pre-run bundles, 512MB default, anything over it skipped. ## Why The jochemnet bloatnet pre-run bundle is a single **9.4 GiB** `pre-run.request` — roughly 8k blocks of setup. `CreateSuiteOutput` copies it into the suite directory on every job, and the runner then uploads it. Nothing consumes it: the runner replays pre-runs from the fixtures cache, not from the suite, and the UI has no use for a bundle that size. ## What it does A pre-run step over `runner.benchmark.tests.max_pre_run_step_size` (default `512MB`) is recorded in `summary.json` and not copied, so it never reaches the bucket. `0` disables the limit. ```json "pre_run_steps": [ { "og_path": "pre_run/pre-run.request", "size_bytes": 10062313486, "omitted": true } ] ``` Two details worth flagging for review: **Checked with a `stat` before the copy, not at upload time.** Skipping it at the upload layer would still leave a 9.4 GiB write into `RUNNER_TEMP` on every job. Doing it here costs neither the write nor the transfer, and keeps the uploader generic — it has no business knowing which files are pre-run steps. **The entry is recorded as `omitted`, not dropped.** The suite stays honest about what the run replayed, and the UI gets something to check before offering the file for viewing — `TestFilesList.tsx:147` currently links `suites/<hash>/<og_path>/pre_run.request` unconditionally, which would 404 for an omitted bundle. That UI guard is a separate change; this PR gives it the flag to read. ## Tests - `TestCreateSuiteOutput_OmitsOversizedPreRunSteps` — over the limit: no file written, summary records `omitted` and the true size, test steps unaffected. - `TestCreateSuiteOutput_KeepsPreRunStepsWithinLimit` — under the limit and with the limit disabled, the bundle is stored exactly as before. ## How this relates to the other PRs - **#301** (merged) makes the upload of any large file work at all. Still wanted — it is what stops a >5 GiB file failing the whole suite upload, including the `summary.json` that the UI actually needs — but with this cap the 9.4 GiB path stops being exercised. - **#302** (skip unchanged objects) is orthogonal: it stops re-sending the ~2.96 GB of fixture payloads on every run, which this PR does not touch. Together the steady-state suite upload goes from ~12.4 GB a run to ~5 MB. - **#303** (merge the stored summary) is a different bug and independent of both. Note it also adds a parameter to `CreateSuiteOutput`, so whichever of #303/#304 merges second needs a trivial rebase — happy to fold both into an options struct if you'd rather. ## Verification `make build-core`, `make lint-core` (0 issues), full `go test ./pkg/... ./cmd/...` all pass locally.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Third in the sequence after #301 (multipart) and #302 (skip unchanged objects). Those two are about getting the suite into the bucket and not re-sending it; this one is about not degrading it once it is there.
The bug
CreateSuiteOutputis written to enrich an existingsummary.jsonrather than replace it: it preserves the priortests[], backfills payload sizes and tx counts from the step files, and folds in opcode data (suite.go:356-388on master). That merge is gated onsuiteExists, which is decided by reading<resultsDir>/suites/<hash>/summary.jsonfrom local disk.On every CI worker the results directory is under
RUNNER_TEMP(/opt/actions-runner/_work/_temp/results), which Actions wipes between jobs. SosuiteExistsis always false, the merge never runs, and each run rebuilds the summary from its own inputs and uploads it over whatever the store had accumulated.Nothing is lost today, because every field currently in play is recomputable from the step files — which is why this has gone unnoticed. It stops being true the moment two runs of the same suite differ in config richness.
tests.opcode_sourceis the clearest case: a run without it cannot recompute those counts, so it publishes a poorer summary and the richer one is gone. Whichever run finishes last wins.The change
executor.Configgains an optionalRemoteSuiteSummaryhook — a plain func, so the executor takes on no storage dependency.cmd/benchmarkoor/run.gowires it toupload.NewS3Readerwhen results upload is configured. Not configured (local runs, tests) → nil → behaviour is exactly as before.CreateSuiteOutputtakes the stored summary as a baseline and merges into it when there is no usable local one.Two things I deliberately kept separate:
Materialising the step files stays keyed on the local summary alone. A stored summary says nothing about what is on local disk. Conflating them would let a wiped worker skip the copy and have nothing to upload — which is exactly how a partially uploaded suite would become permanent.
TestCreateSuiteOutput_MergesStoredSummaryOnWipedWorkerasserts the files are still written.A fetch failure is not fatal. It logs and falls back to today's behaviour. The merge is an enrichment, never a precondition for writing the suite.
validSummaryalso replaces the inline check in both places: a truncated or test-less summary is not a merge baseline, local or remote. That is the condition the original comment warned about leaving"tests": nullbehind.Tests
TestCreateSuiteOutput_MergesStoredSummaryOnWipedWorker— fresh results dir, no opcode source, stored summary carrying opcode counts. Asserts the counts survive, the step files are still materialised, and payload sizes are still backfilled from them.TestCreateSuiteOutput_IgnoresUnusableStoredSummary— truncated / test-less / empty stored summaries all fall back to this run's own description.TestCreateSuiteOutput_MergesPayloadSizesOnSecondRun(existing) still passes, so the local merge path is untouched.What I left out
Concurrent writers. Six clients run the same suite in parallel and each uploads at the end, so this is read-modify-write without a compare-and-swap: two workers can both read baseline R and the second write wins. It does not matter while every field is content-derived and each worker computes identical bytes, which is the case today. If enrichment that varies per run ever lands, the fix is a conditional
PutObject—IfMatchis onPutObjectInputin the SDK we already vendor, and R2 supports it — retrying the merge on 412. Not worth the machinery for a race that currently has nothing to lose.summary.jsonis still never skipped by #302. With this change the regenerated summary is usually byte-identical to the stored one and the ETag check would skip it anyway, but the explicit exception costs ~5 MB a run and covers the case a content check cannot: a summary above the 64 MiB part size falls back to size-only comparison.Verification
make build-core,make lint-core(0 issues), fullgo test ./pkg/... ./cmd/...all pass locally.