Skip to content

ci: split the Docker restore into its own cacheable layer - #12393

Open
AnkushinDaniil wants to merge 4 commits into
masterfrom
ci/docker-restore-layer-cache
Open

ci: split the Docker restore into its own cacheable layer#12393
AnkushinDaniil wants to merge 4 commits into
masterfrom
ci/docker-restore-layer-cache

Conversation

@AnkushinDaniil

@AnkushinDaniil AnkushinDaniil commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Changes

Restructure the build stage of Dockerfile so dotnet restore runs in its own layer, before the sources are copied:

  1. Copy only the restore inputs first — global.json, nuget.config, the root Directory.Build.props/Directory.Build.targets/Directory.Packages.props (CPM), and (via COPY --parents) every *.csproj, the src/Nethermind build props, and Nethermind.Runner/packages.lock.json.
  2. RUN dotnet restore --locked-mode.
  3. COPY src/Nethermind and dotnet publish --no-restore.

Adds the # syntax=docker/dockerfile:1.7-labs directive (required for COPY --parents, which preserves the source paths of the copied project files). No descriptive code comments — rationale is here.

Why

On master the whole source tree is copied before dotnet restore, so the restore layer's cache key busts on any source change — a one-line edit re-resolves and re-downloads every NuGet package. Splitting restore into a layer keyed only on the project/lock files lets a source-only change reuse it.

Verified locally (Docker 29.4)

  • Cold build of the build stage succeeds — the --parents copy set is complete for restore --locked-mode + publish (build stage ≈ 110s compute here).
  • Cached rebuild shows the intended layer boundary:
    #11 RUN … dotnet restore --locked-mode   → CACHED
    #12 COPY src/Nethermind src/Nethermind    → CACHED
    #13 RUN … dotnet publish … --no-restore   → CACHED
    
    Restore (class loggers #11) is a distinct layer before COPY src (Setup Constantinople branch and tests #12), so a source-only change busts only Setup Constantinople branch and tests #12/EIP-145 #13 (publish) and reuses the cached restore — skipping the NuGet graph resolution + package download (~30–60s) on every rebuild.

Where it pays off

Immediately on runners with a persistent layer cache — e.g. the self-hosted ubuntu-arm64-8-core used by sync-pr-gate, whose image build already completes in ~1 min from a warm cache. Follow-up: to extend the benefit to ephemeral GitHub-hosted runners (e.g. trivy), wire cache-to/from: type=gha into the buildx image builds — left out here to keep this change to the release-critical Dockerfile alone.

Types of changes

  • Optimization
  • Build-related changes

Testing

Requires testing

  • Yes
  • No

Notes on testing

Verified with a local docker build --target build (cold success + cached rebuild layer inspection, above). CI builds the full image on this PR via trivy and sync-pr-gate, which validate the change end-to-end. The published image is byte-equivalent — only the layer ordering changed; restore --locked-mode and publish are unchanged.

Documentation

  • Yes
  • No

Requires explanation in Release Notes

  • Yes
  • No

Review follow-ups

  • The COPY --parents input list intentionally tracks the transitive restore graph of Nethermind.Runner; if that graph gains a non-Directory props/targets import, the cache-cold restore will fail visibly and the list must be updated together with the graph change.
  • This Dockerfile requires the BuildKit frontend because it uses COPY --parents and the pinned docker/dockerfile:1.7-labs syntax directive. The PR's Docker CI builds validate that requirement.
  • Dockerfile.chiseled and Dockerfile.pgo are intentionally out of scope; they are maintained by the existing dockerfile update workflow and can receive the same optimization separately if desired.

@claude

claude Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @AnkushinDaniil's task in 2m 42s —— View job


PR Review: split Docker restore into its own cacheable layer

  • Read the diff and the full Dockerfile
  • Verify the COPY set is complete for restore --locked-mode (lock files, props, csproj)
  • Check BuildKit / --parents requirements across the workflows that build this Dockerfile
  • Check for restore-time MSBuild imports that aren't copied
  • Assess regressions vs. master

Verdict

Looks correct and the caching rationale is sound. The restore command (dotnet restore --locked-mode on Nethermind.Runner) is byte-identical to master; only the layer ordering and the copy set changed. I confirmed the copy set is complete:

  • Lock file: there is exactly one packages.lock.json in the repo (src/Nethermind/Nethermind.Runner/packages.lock.json) and it is copied — so --locked-mode is fully honored.
  • Root inputs: the new explicit COPY global.json nuget.config Directory.Build.props Directory.Build.targets Directory.Packages.props ./ is equivalent to the old Directory.*.props + Directory.Build.targets + global.json + nuget.config set.
  • Restore graph: restore on Nethermind.Runner only walks its transitive ProjectReference graph (production projects), so copying all src/Nethermind/**/*.csproj is a safe superset. Restore evaluates project files only — no .cs needed — so running it before COPY src/Nethermind is valid.
  • --no-restore publish still works: obj/ is in .dockerignore, so the later COPY src/Nethermind src/Nethermind layers source on top without clobbering the obj/project.assets.json written by the restore layer.

No Critical / High / Medium findings. A couple of low-severity notes below.

Findings

Low — restore-time import src/Nethermind/tests.props is intentionally not in the copy set. Many test projects do <Import Project="../tests.props" />. This is fine today because Nethermind.Runner's restore graph doesn't include test projects, so tests.props is never evaluated during restore. Worth being aware that the COPY --parents list is a hand-maintained allowlist keyed to the current Runner restore graph — if a production project in that graph ever starts importing a non-Directory.* props/targets file, the cold restore layer would break (and only surface on a cache-cold build). Consider a brief note somewhere (PR desc is fine) that this list must track the restore graph.

Low — BuildKit dependency. The # syntax=docker/dockerfile:1.7-labs directive and COPY --parents require the BuildKit frontend. sync-pr-gate.yml invokes plain docker build (not buildx); this is fine on Docker 23+ where BuildKit is the default, but would fail on a daemon with the legacy builder (DOCKER_BUILDKIT=0). It's self-validating — this very PR builds the full image via trivy and sync-pr-gate, so a green CI confirms BuildKit is active on those runners. The -labs channel also pulls the frontend image from the registry at build time, so builds need registry access for the frontend.

Low — consistency with sibling Dockerfiles. Dockerfile.chiseled and Dockerfile.pgo keep the old "copy source before restore" ordering and won't get this caching benefit. Out of scope per the PR description ("keep this change to the release-critical Dockerfile alone"), and they're maintained in lockstep by update-dockerfiles.yml, so this is just a heads-up, not a request to change them here.

Nice, minimal change — nothing blocking merge.

Comment thread Dockerfile
@AnkushinDaniil
AnkushinDaniil marked this pull request as ready for review July 26, 2026 17:36
@AnkushinDaniil
AnkushinDaniil requested a review from rubo as a code owner July 26, 2026 17:36
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @AnkushinDaniil's task in 6m 24s —— View job


PR Review: split the Docker restore into its own cacheable layer

  • Read the diff and the full Dockerfile (compared against origin/master)
  • Verify the COPY set is complete for restore --locked-mode (csproj depths, props/targets imports, lock files)
  • Verify the copy set is equivalent to master's for the publish step
  • Check BuildKit / --parents availability across every workflow that builds this Dockerfile
  • Confirm update-dockerfiles.yml still works against the restructured file
  • Check CI evidence for a cold build

Verdict

The restructure is correct, and the copy set is complete — I verified it mechanically rather than by inspection:

Check Result
csproj depth all 156 are at src/Nethermind/<P>/<P>.csproj; none at depth 1 or ≥3, so src/Nethermind/**/*.csproj matches everything
non-Directory.* imports only ../tests.props, imported exclusively by *.Test / Ethereum.*.Test projects
Runner's restore closure no ProjectReference anywhere under src/Nethermind escapes the directory, and Nethermind.Runner.csproj references production projects only → tests.props is never evaluated
src/Nethermind/Directory.Build.props imports ../../Directory.Build.props (copied) and injects a ProjectReference to Nethermind.Analyzers.csproj into every project (covered by the glob)
lock files exactly one (Nethermind.Runner/packages.lock.json), and RestorePackagesWithLockFile is set nowhere → --locked-mode enforces against the same single lock file as master
root file set {global.json, nuget.config, Directory.Build.props, Directory.Build.targets, Directory.Packages.props} is byte-for-byte the same set master copied via Directory.*.props + Directory.Build.targets + the two singles — nothing dropped for publish
--no-restore **/obj/ is in .dockerignore, so COPY src/Nethermind layers source on top without clobbering the obj/project.assets.json written by the restore layer
update-dockerfiles.yml seds only image references; unaffected by the reordering
BuildKit availability trivy.yml + sync-pr-gate.yml use docker build (BuildKit default on Docker 23+); release.yml, ci-surge.yml, ci-taiko.yml, deploy-rpc-monitors.yml, hive-tests.yml use buildx / build-push-action

CI already validates this end-to-end at the current head (9967509) — worth noting since the PR body only claims local verification: Trivy scanner built the full image cold on a GitHub-hosted runner (2m47s, pass), and both Sync hoodi (HalfPath) and Sync hoodi (Flat) built the image and actually synced Hoodi with it (pass). So the cold --parents set, the labs frontend pull, and publish --no-restore are all exercised on real infrastructure, not just locally.

One upside the PR description doesn't claim: in release.yml's --platform=linux/amd64,linux/arm64 build, the restore RUN no longer references $TARGETARCH (only publish does), so BuildKit dedupes it across both platform variants of the build stage. On master the single fused RUN is arch-distinct, so restore runs twice per release build; now it runs once.

1 Medium, 2 Low. No Critical/High. The Medium is a one-line fix and isn't a correctness problem.

Findings

Medium — # syntax=docker/dockerfile:1.7-labs is the only unpinned image reference in the repo's Docker toolchain (Dockerfile:1, inline). Every base image in all four Dockerfiles is digest-pinned and auto-bumped by update-dockerfiles.yml; even the actions in trivy.yml are SHA-pinned. 1.7-labs floats within 1.7.x, so the frontend that compiles this release-critical Dockerfile into LLB can change with no repo change — and update-dockerfiles.yml seds only mcr.microsoft.com/dotnet/*, so it will never be bumped. It also introduces the build's only Docker Hub pull (bases are MCR, which has no anonymous limit); trivy.yml and sync-pr-gate.yml build without a Hub login, so a toomanyrequests on the frontend is a new (low-probability, loud) failure mode on GitHub-hosted runners. Suggest @sha256:<digest>, and a deliberate decision about pinning the early-2024 1.7 line versus a newer labs release. Digest-pinning addresses reproducibility/supply chain, not the rate-limit exposure.

Low — the copy list is a hand-maintained allowlist tracking Runner's restore graph. Complete today (table above), but it will break on a cold build if a project in that closure ever imports a non-Directory.* props/targets, or if a csproj lands at a different depth. Blast radius is contained: it fails loudly, and trivy cold-builds the image on every PR to master. Already recorded in the PR's review-follow-ups — no change requested.

Low — the restore layer over-invalidates. The glob copies all 156 csproj, including ~70 test projects that are never evaluated during Runner's restore, so editing any test csproj busts the restore layer for no reason. Narrowing the glob would be more fragile than the cache win is worth, so the superset is the right call — just noting the cache is less sticky in practice than the "keyed only on project/lock files" framing suggests, since test-project churn is common here.

Nothing else. The diff is minimal, the restore/publish commands are unchanged from master, and moving the SPDX header to line 2 is forced (parser directives must be line 1) and breaks no lint — there's no SPDX-header check in .github/workflows/.
· ci/docker-restore-layer-cache

Comment thread Dockerfile Outdated

@rubo rubo 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.

While I appreciate the intention, the current disadvantages outweigh the benefits:

  • Doesn't actually help CI -- every build site runs on ephemeral runners with no caching configured, so the restore layer never persists across runs. It's mostly helpful for local builds (and not even for everyone).
  • Even with caching enabled, there are pitfalls: i) silent wrong-cache hits if a restore-affecting file is added/removed without updating the hand-maintained copy list; ii) with the GitHub Actions cache backend, a full .NET build can blow past the 10 GB limit and thrash, especially multi-arch.
  • Requiring the labs frontend is not only error-prone for the (minority of) folks without BuildKit, but also adds an extra Docker Hub image pull at parse time on every build -- increasing the chance of hitting rate limits.

With this said, I'd hold this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants