Skip to content

Clean scheduled benchmark job work directories - #2214

Merged
LoopedBard3 merged 3 commits into
aspnet:mainfrom
LoopedBard3:loopedbard3-fix-benchmark-db-reuse
Oct 8, 2026
Merged

LoopedBard3 merged 3 commits into
aspnet:mainfrom
LoopedBard3:loopedbard3-fix-benchmark-db-reuse

Conversation

@LoopedBard3

@LoopedBard3 LoopedBard3 commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Scheduled Docker benchmark jobs used noClean: true, which retained a new unkeyed work directory for every run and could exhaust agent disk space.

This change removes noClean from the scheduled/shared application and database job definitions reached by benchmarks-ci-azure, benchmarks-ci-01, and benchmarks-ci-02. It also removes the redundant framework-template --application.noClean true overrides.

Jobs continue cloning and building normally, then use Crank's standard per-run cleanup. No source/build/image reuse behavior is introduced, so sources and Docker images continue to refresh through their existing build paths. Generated CI entrypoints were not edited.

Validation

  • Parsed all changed YAML and verified each scenario file differs from upstream only by noClean removal
  • Re-audited the generated scheduled pipeline imports
  • Passed all 43 pod-scheduler tests
  • Passed git diff --check

Replace unkeyed noClean retention with reuseBuild in scheduled and shared benchmark configurations while preserving fresh per-run containers and database volumes.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Container-matrix variants can reuse the shared image tag incorrectly and stop refreshing nightly inputs.

Review effort: Balanced
Findings: 1 High severity

Open (1)
What changed in this PR

Replaces unbounded noClean usage with keyed build reuse to prevent scheduled benchmark agents from exhausting disk space.

Changes:

  • Enables options.reuseBuild for affected application and database jobs.
  • Removes redundant framework pipeline --application.noClean overrides.
  • Preserves fresh containers while retaining build artifacts.
File Description
src/​BenchmarksApps/​TechEmpower/​RazorPages/​razorpages.benchmarks.yml Reuses the PostgreSQL build.
src/​BenchmarksApps/​TechEmpower/​Minimal/​minimal.benchmarks.yml Reuses the PostgreSQL build.
src/​BenchmarksApps/​TechEmpower/​BlazorSSR/​blazorssr.benchmarks.yml Reuses the PostgreSQL build.
scenarios/​te.benchmarks.yml Enables reuse across TechEmpower jobs.
scenarios/​platform.benchmarks.yml Reuses the PostgreSQL build.
scenarios/​orchard.benchmarks.yml Reuses the PostgreSQL build.
scenarios/​goldilocks.benchmarks.yml Reuses the PostgreSQL build.
scenarios/​database.benchmarks.yml Reuses PostgreSQL and SQL Server builds.
scenarios/​containers.benchmarks.yml Enables container-matrix application reuse.
build/​frameworks-scenarios.yml Removes the application noClean override.
build/​frameworks-database-scenarios.yml Removes the application noClean override.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread scenarios/containers.benchmarks.yml Outdated
Keep container-matrix application builds fresh because variants share an image tag, and restore the TechEmpower scenario file's original whitespace and EOF layout.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@LoopedBard3
LoopedBard3 marked this pull request as ready for review October 6, 2026 21:08
@DrewScoggins

Copy link
Copy Markdown
Contributor

Can we get some runs with these new configs, and make sure we are not moving the needle on these scenarios?

@DrewScoggins DrewScoggins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good other than my comment.

@LoopedBard3

Copy link
Copy Markdown
Contributor Author

Follow-up on the request for validation runs:

Functional/cache validation: passed

I ran A/B smoke coverage on the Azure ARM64 relay pod using immutable Benchmarks/TFB refs and a pinned toolchain:

  • S1: Platform Fortunes with shared PostgreSQL DB reuse
  • S4: Vertx Fortunes with distinct vertx-postgres application and postgres_te DB tags
  • S3: Gin JSON with distinct gin application and mysql_te DB tags

All valid runs completed with zero bad responses/request errors. The tests verified cold keyed builds, true warm build skips for both application and DB images, correct image/tag selection, and successful missing-image recovery after deploying the draft fix from dotnet/crank#902 at dacb59f.

Crank dependency

reuseBuild is not safe across agent restart/image loss without dotnet/crank#902: the keyed build directory can survive while the Docker image is absent, causing Crank to skip the build and fail at docker create. The Crank fix must land and be deployed together with this Benchmarks change.

Performance screening: not statistically resolved

The precommitted 30-second pairs produced mixed results:

Scenario RPS Mean latency P90 P99
S1 Platform Fortunes +4.8% -4.6% -10.6% -14.8%
S4 Vertx Fortunes -4.0% +5.3% +21.9% +39.9%
S3 Gin JSON -4.5% +7.7% +12.7% +18.1%

Short cold/warm samples were mixed from neutral to better, while the interleaved old-first long pairs were sensitive to ordering/environment and sometimes triggered the missing-image fallback rebuild. This is not enough evidence to call either a regression or an improvement.

Next step

Run balanced, randomized 5x5 measured pairs on a reserved pod at normal duration before drawing a precision performance conclusion.

Remove the PR-added reuseBuild settings so scheduled jobs continue cloning and building normally while their per-run work directories are cleaned.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@LoopedBard3 LoopedBard3 changed the title Reuse builds for scheduled benchmark jobs Clean scheduled benchmark job work directories Oct 7, 2026
@LoopedBard3

Copy link
Copy Markdown
Contributor Author

Design update: based on the source/image freshness concerns raised during validation, this PR has been narrowed to only remove noClean.

The PR no longer adds options.reuseBuild. Scheduled jobs will continue cloning sources and building Docker images through their normal paths, and Crank will clean each per-run work directory afterward. This prevents the unbounded accumulation of unkeyed work directories without introducing persistent source/build/image caching.

The earlier reuseBuild cache and performance test results were useful for identifying behavior, but they no longer apply to the final design of this PR. The final diff and validation now cover cleanup-only behavior.

@sebastienros

Copy link
Copy Markdown
Member

what needs to be checked is how long the databased jobs take to be ready (only these jobs). We might have to download the image between each run without that.

@LoopedBard3
LoopedBard3 marked this pull request as draft October 8, 2026 01:38
@LoopedBard3
LoopedBard3 marked this pull request as ready for review October 8, 2026 05:28
@LoopedBard3

LoopedBard3 commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor Author

@sebastienros

Copilot Investigated/Written:

Good point. I checked the Crank cleanup flow and ran database-only comparisons both locally and on the production Linux Azure ARM64 DB agent.

noClean affects two separate things:

  • it skips deletion of the random source/work directory;
  • during Docker cleanup it changes docker rmi --force to docker rmi --force --no-prune.

Both paths still remove the named job image and the database container/anonymous volumes. Docker's --no-prune only preserves untagged parent image records; it does not control BuildKit's build cache.

For the production test I ran three noClean: true jobs and three normal-cleanup jobs, in a balanced order, using the same pinned FrameworkBenchmarks PostgreSQL source and Dockerfile.

Metric noClean mean Cleanup mean noClean median Cleanup median
Crank build time 3.448s 3.139s 4.010s 2.362s
DB start time 1.598s 1.591s 1.612s 1.548s
Submit-to-ready 25.681s 27.158s 27.037s 27.199s

All six runs completed successfully. Every run performed the expected docker build --pull registry metadata check and reported that postgres:18-trixie was up to date. There were zero Pulling fs layer, Downloading, Download complete, or Extracting markers. The cleanup runs therefore did not download the database image layers again.

The median submit-to-ready difference was only 0.162 seconds, and build/start times were not worse with cleanup. The meaningful difference was retention: the three noClean runs created three distinct retained unkeyed work paths, while the three cleanup runs followed normal job deletion.

This matches the local Docker/Crank test, where warm PostgreSQL builds were 7.535 seconds with noClean and 7.290/7.115 seconds with normal cleanup, with cached Dockerfile steps and no layer downloads.

Based on both environments, removing noClean fixes the per-run work-directory growth without forcing Docker image downloads or materially increasing database build/readiness time.

/Not AI: I will also watch to make sure this doesn't add any instability to the test runs over the next few days👍.

@DrewScoggins DrewScoggins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@LoopedBard3
LoopedBard3 merged commit d944183 into aspnet:main Oct 8, 2026
2 checks passed
@LoopedBard3
LoopedBard3 deleted the loopedbard3-fix-benchmark-db-reuse branch October 8, 2026 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants