perf(docker): reuse runtime layers and scope Maven builds - #3194
perf(docker): reuse runtime layers and scope Maven builds#3194lokidundun wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #3194 +/- ##
============================================
- Coverage 37.77% 32.75% -5.02%
+ Complexity 6560 5543 -1017
============================================
Files 800 789 -11
Lines 68960 67806 -1154
Branches 9166 8970 -196
============================================
- Hits 26052 22213 -3839
- Misses 39841 42970 +3129
+ Partials 3067 2623 -444 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: Both mechanisms hold up. The four build stages stay byte-identical, so the shared Bake cache and the existing docker-bake-check guard still work, and the -pl ... -am scoping is content-safe because all three assembly descriptors are dependency-driven rather than module-driven. Four non-blocking points: the runtime apt layer now refreshes only when the base image digest changes, the reactor scope is no longer settable through MAVEN_ARGS, .dockerignore was not narrowed to match the new scope, and Dockerfile-hstore gained an avoidable layer. Evidence: git diff origin/master...304bea1; lines 20-33 of all four Dockerfiles hash identically; the server, pd and store assembly descriptors contain no <moduleSet>, so -am builds exactly the closure they consume; .github/workflows/cluster-test-ci.yml still runs an unfiltered full-reactor mvn clean package on every pull request, so compile coverage of the 11 dropped modules is retained.
| && sed -i "s/^restserver.url.*$/restserver.url=http:\/\/0.0.0.0:8080/g" ./conf/rest-server.properties | ||
| && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| COPY --from=build /pkg/hugegraph-server/apache-hugegraph-server-*/ /hugegraph-server/ |
There was a problem hiding this comment.
COPY, its cache key no longer includes the application source, so the layer is reused until the eclipse-temurin:11-jre-jammy digest changes. What is lost is the per-source-change reinstall: rebuilding the same commit already hit cache before, but any source change used to force a fresh apt-get install, and now nothing short of a base image update does.
This only bites on a registry-cache flow, and that flow is out of tree: docker/bake.hcl gates every cache-to behind EXPORT_CACHE, which defaults to false, and no workflow here runs bake to build or push. The PR's own numbers show the effect, with eight runtime layers restored in 2.0-9.3 s "without rerunning package installation".
Could you add a documented way to force a reinstall, for example ARG RUNTIME_DEPS_EPOCH=1 immediately before the apt block in all four files, bumped when packages need refreshing? --no-cache-filter is not an option as things stand, since the runtime stage has no AS name. A short note on the cache behaviour in docker/README.md would help whoever operates the publish flow.
| mvn install -pl hugegraph-server/hugegraph-dist,hugegraph-pd/hg-pd-dist,hugegraph-store/hg-store-dist \ | ||
| -am $MAVEN_ARGS -e -B -ntp -Dmaven.test.skip=true -Dmaven.javadoc.skip=true \ |
There was a problem hiding this comment.
🧹 The module list is hardcoded ahead of $MAVEN_ARGS, which is the only Maven knob docker/bake.hcl exposes. Maven accumulates repeated -pl values rather than letting a later one replace an earlier one, so MAVEN_ARGS can still add modules or drop them with a ! prefix, but it can no longer set the scope outright. The PR description notes that the fork experiments "enabled the same module selection through MAVEN_ARGS"; that route closes here.
Drift between the four copies is already covered by the docker-bake-check job in .github/workflows/docker-build-ci.yml, so this is only about overridability.
Suggested change: hoist the list into a build arg beside the existing ARG MAVEN_ARGS and pass it through _common.args in docker/bake.hcl.
ARG MAVEN_PROJECTS="hugegraph-server/hugegraph-dist,hugegraph-pd/hg-pd-dist,hugegraph-store/hg-store-dist"with the command becoming mvn install -pl "$MAVEN_PROJECTS" -am $MAVEN_ARGS .... All four files stay byte-identical, so the CI identity check still passes.
| mvn install $MAVEN_ARGS -e -B -ntp -Dmaven.test.skip=true -Dmaven.javadoc.skip=true \ | ||
| mvn install -pl hugegraph-server/hugegraph-dist,hugegraph-pd/hg-pd-dist,hugegraph-store/hg-store-dist \ | ||
| -am $MAVEN_ARGS -e -B -ntp -Dmaven.test.skip=true -Dmaven.javadoc.skip=true \ | ||
| && rm ./hugegraph-server/*.tar.gz ./hugegraph-pd/*.tar.gz ./hugegraph-store/*.tar.gz |
There was a problem hiding this comment.
🧹 The reactor is scoped but the build context is not, so the caching win is smaller than the benchmark suggests. .dockerignore at head excludes only build output, archives, IDE and OS files, .git, .github, **/*.md and the compose files, so hugegraph-test, hg-pd-test, hg-store-test, hugegraph-cluster-test/**, hugegraph-example, hg-pd-cli, hg-store-cli and install-dist all still land in the context and still feed the COPY . . cache key on line 25. Editing any of them therefore invalidates this shared build stage and pays for a full scoped Maven run, for modules the build no longer compiles.
A follow-up rather than a change here, but worth recording. One caveat for whoever picks it up: these directories cannot be ignored wholesale, because the root pom lists them in <modules> and Maven fails when a listed module's pom.xml is absent, so only their src/ subtrees can be excluded.
| RUN cd /hugegraph-server/conf/graphs \ | ||
| && rm hugegraph.properties && mv hstore.properties.template hugegraph.properties | ||
| RUN sed -i "s/^restserver.url.*$/restserver.url=http:\/\/0.0.0.0:8080/g" ./conf/rest-server.properties |
There was a problem hiding this comment.
🧹 Both of these RUNs edit only the tree copied on line 62, so they are invalidated together and the split just costs an image layer. This is the one file where that layer is avoidable: the plain server Dockerfile also gained a standalone sed layer, but it has a single edit with nothing to fold into.
| RUN cd /hugegraph-server/conf/graphs \ | |
| && rm hugegraph.properties && mv hstore.properties.template hugegraph.properties | |
| RUN sed -i "s/^restserver.url.*$/restserver.url=http:\/\/0.0.0.0:8080/g" ./conf/rest-server.properties | |
| RUN cd /hugegraph-server/conf/graphs \ | |
| && rm hugegraph.properties && mv hstore.properties.template hugegraph.properties \ | |
| && cd /hugegraph-server \ | |
| && sed -i "s/^restserver.url.*$/restserver.url=http:\/\/0.0.0.0:8080/g" ./conf/rest-server.properties |
The comment on line 63 then covers only part of what the merged RUN does, so it is worth widening at the same time.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: All four points from the review on 304bea1 are handled at this head: the epoch ARG, the MAVEN_PROJECTS build arg, the merged hstore layer, and the .dockerignore follow-up note in the README. The build stages are still byte-identical and the runtime reordering keeps image behaviour the same. One small point is left. The default module list now lives in docker/bake.hcl as well as in the four Dockerfiles, and CI only checks the Dockerfile copies. Evidence: git diff 36811483...230580d and git diff 304bea1 230580d. The docker-bake-check job in .github/workflows/docker-build-ci.yml diffs only the AS build stages, and its jq assertion on bake --print never reads args. The docker-build job runs plain docker build with only SOURCE_REVISION. The red hstore check (VertexCoreTest.testAddVertexWithTtlAndTtlStartTime) is the TTL timing flake already noted on #3187. That job runs Java core tests and does not use the files changed here, but it needs a rerun.
| } | ||
|
|
||
| variable "MAVEN_PROJECTS" { | ||
| default = "hugegraph-server/hugegraph-dist,hugegraph-pd/hg-pd-dist,hugegraph-store/hg-store-dist" |
There was a problem hiding this comment.
🧹 This default is now a fifth copy of the module list, and nothing checks it against the other four. A drift here would go unnoticed.
docker-bake-checkdiffs theAS buildstage of the four Dockerfiles, so a change toARG MAVEN_PROJECTS=...there has to land in all four. It never compares that ARG with this variable, and its jq assertion onbake --printchecks targets, platforms and outputs but notargs.- The
docker-buildjob runs plaindocker build --build-arg SOURCE_REVISION=..., so CI builds with the Dockerfile default. bakealways passes this value, so the images frombake.hcluse this copy.
If someone later adds a module to the Dockerfile list (a new dist, say) and misses this line, every CI check still passes, but bake builds a different reactor from the one CI tested. With a module like that, rm ./hugegraph-*/*.tar.gz or a runtime COPY --from=build could then fail only in the bake flow.
Requested change: keep one source of truth. Either set default = null here, which makes bake leave the arg unset so the Dockerfile ARG default applies while MAVEN_PROJECTS=... docker buildx bake still overrides it, or add a jq assertion to docker-bake-check that .target.pd.args.MAVEN_PROJECTS matches the ARG MAVEN_PROJECTS default in hugegraph-pd/Dockerfile. RUNTIME_DEPS_EPOCH on line 27 duplicates its default the same way. A drift there would only cause one extra cache miss, but the same change covers it.
2 participants
Purpose of the PR
Main Changes
COPY --from=buildin all four Dockerfiles, allowing dependency layers to survive application source changes.COPY.-pland-am, reducing the reactor from 38 to 27 modules while retaining required dependencies.The existing single-job Bake flow and QEMU-based ARM64 build remain in place.
Verifying these changes
Runtime dependency layer benchmark
Both variants used the same GitHub-hosted Ubuntu runner class and separate GHCR registry caches. After seeding each cache, identical configuration comments were added to trigger distribution changes and a full Maven rebuild.
Before the change, the four ARM64 runtime package installation steps ran under QEMU and took 150.6–160.5 seconds each. After the change, all eight runtime dependency layers across amd64 and arm64 were restored from registry cache in 2.0–9.3 seconds per layer, without rerunning package installation.
Cache export was measured separately:
These BuildKit vertices overlap; their durations must not be added together or treated as independent end-to-end savings.
Maven reactor benchmark
This separate experiment compared the full reactor with the scoped reactor using the same source and existing runtime layer optimization.
The distribution-job timings above are not complete image publication timings.
Correctness checks
Successful CI runs
These are fork validation runs for the implementation approach. The Maven experiments enabled the same module selection through
MAVEN_ARGS; this PR places that selection directly in the Dockerfiles. These runs do not replace CI on the final PR commit.Measurements are single-run observations using GHCR, not the official Docker Hub publication environment. Savings from separate experiments are not additive.
Commands for local verification
Run from the repository root using Bash.
Build the selected distributions:
Inspect the shared Bake configuration:
Build all four images through the shared Bake flow:
The default Bake targets include amd64 and arm64. Local multi-platform loading requires a compatible builder and the containerd image store; building ARM64 on an amd64 host also requires emulation.
Check a direct Dockerfile build:
docker buildx build \ --platform linux/amd64 \ -f hugegraph-server/Dockerfile \ -t hugegraph-standalone:pr-check \ --load .The commands above are build checks; the linked CI runs provide the integration, content-comparison, and publication validation.
Does this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No Need