feat(docker): merge image publishing into the docker build script - #19489
feat(docker): merge image publishing into the docker build script#19489deepakpanda93 wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19489 +/- ##
============================================
+ Coverage 77.17% 77.23% +0.06%
- Complexity 33948 33967 +19
============================================
Files 2575 2575
Lines 143397 143397
Branches 17579 17579
============================================
+ Hits 110662 110753 +91
+ Misses 24476 24384 -92
- Partials 8259 8260 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the docs update! The rewrite does a nice job of reframing publishing as a maintainer-only release action and explaining the multi-arch/containerd constraints. My main concern is an internal inconsistency in how users are told to override component versions and the version tag — one section says to edit the script, while another section (and the PR description) uses command-line flags. Please have a Hudi committer (e.g. @yihua) confirm the final option interface once resolved.
| You can override the Hadoop, Spark, and Hive versions from the command line. If you plan to use `setup_demo.sh`, | ||
| build the image set matching the default compose files first. For other flows, use one of the supported version | ||
| combinations under `docker/compose`. | ||
| To build one of the other version combinations under `docker/compose`, edit the version block near the top of the |
There was a problem hiding this comment.
🤖 This section tells users to override versions by editing the HADOOP_VERSION/SPARK_VERSION/HIVE_VERSION block "near the top of the script," but the Multi-Arch example below (and the PR description) overrides the same values with --hadoop-version 3.4.0 --spark-version 4.0.1 --hive-version 3.1.3 flags. These two instructions contradict each other. Since the script appears to accept --hadoop-version/--spark-version/--hive-version flags, it would be clearer (and less error-prone than hand-editing the script) to document the flags here as well, rather than telling users to modify the source.
There was a problem hiding this comment.
You are right, and the flags are the better instruction — fixed.
The script does take --hadoop-version, --spark-version and --hive-version (option parsing at lines 84-86), so telling readers to hand edit the source was both inconsistent with the example below and worse advice. That section now reads:
./build_and_publish_docker_images.sh --hadoop-version 3.3.4 --spark-version 3.5.3 --hive-version 3.1.3with a note that those are the defaults and each can be set on its own. No edit the version block instruction remains anywhere in the README.
| By default, the script builds images for the current machine architecture and derives the version tag from the root | ||
| `pom.xml`. Use `--version-tag` to set an explicit tag if needed. | ||
| The script builds images for the current machine architecture and tags each one `:latest` plus the Hudi version | ||
| taken from the root `pom.xml`. Export `VERSION_TAG` to use a different second tag: |
There was a problem hiding this comment.
🤖 This says to override the second tag by exporting VERSION_TAG (VERSION_TAG=my-test ./build_and_publish_docker_images.sh), but the PR description and the previous docs describe a --version-tag <tag> flag for the same purpose. It would help to state definitively whether the tag override is an environment variable, a --version-tag flag, or both — right now the two forms coexist in the same doc and readers won't know which one the script honors.
There was a problem hiding this comment.
Both work, and the docs now say so along with the order they resolve in. The script does:
if [ -n "$VERSION_TAG_ARG" ]; then # --version-tag
VERSION_TAG="$VERSION_TAG_ARG"
elif [ -z "${VERSION_TAG:-}" ]; then # exported VERSION_TAG
VERSION_TAG=$(get_hudi_project_version ...)
fiso it is flag, then environment variable, then the version from the root pom.xml. Confirmed by running it rather than only reading it:
| invocation | resulting tag |
|---|---|
--version-tag from-flag |
from-flag |
VERSION_TAG=from-env |
from-env |
| both together | from-flag |
The README now documents both forms side by side and states that the flag wins.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for iterating on this! This round simplifies the docs to a single Java 11 base image and re-scopes the version examples to Spark 3.x. One thing worth double-checking (inline): the "### Base image" section now says the script just builds base_java11 for Spark 3.x, but the script itself still auto-selects base_java17 when --spark-version is 4.x — so the docs and script may have drifted. Separately, the two earlier comments (editing the in-script version block vs. the --hadoop-version/--spark-version/--hive-version flags, and VERSION_TAG env var vs. --version-tag flag) don't appear to be resolved by these changes yet. Please take a look at the inline comment, and this should be ready for a Hudi committer or PMC member to take it from here.
| |---------------|---------|------------| | ||
| | `base_java11` | Java 11 | Spark 3.x | | ||
| | `base_java17` | Java 17 | Spark 4.0+ | | ||
| `build_and_publish_docker_images.sh` builds the `base_java11` module, which is what Spark 3.x needs. |
There was a problem hiding this comment.
🤖 This now says the script just "builds the base_java11 module," but build_and_publish_docker_images.sh still auto-selects base_java17 / BASE_IMAGE_TAG=java17 when --spark-version is 4.x (the SPARK_MAJOR -ge 4 branch), and that flag still accepts 4.x. Was dropping the Java 17 base from the docs intentional, or should this section still mention that a 4.x --spark-version switches to base_java17?
There was a problem hiding this comment.
Good catch, and the drift was mine — 473008de97fe took the Java 17 base out of the README but left the SPARK_MAJOR -ge 4 branch in the script, so the two disagreed.
Resolved in favour of the script, because that branch is live rather than dead: docker/hoodie/hadoop/base_java17 exists and the selection is reachable through --spark-version 4.x. Verified by running it:
$ ./build_and_publish_docker_images.sh --version-tag t1 --spark-version 4.0.1 --publish false
Using Java 17 base image for Spark 4.0.1
Building hoodie/hadoop/base_java17 as:
- apachehudi/hudi-hadoop_3.3.4-base-java17:latest
The section now reads:
build_and_publish_docker_images.shpicks the base module from--spark-version:base_java11for Spark 3.x, which is the default and what the compose files under/composeuse, andbase_java17for a 4.x version. The demo compose files are all Spark 3.x, so the Java 17 base is only reached if you ask for a 4.x--spark-versionexplicitly.
That keeps the docs truthful about what the script does without presenting Spark 4 as a supported demo path, which was the intent behind removing it in the first place.
Publishing the demo images was a manual docker push per image, and the earlier attempt at a separate push script duplicated the image list and the version variables, which is unmaintainable across two files. build_and_publish_docker_images.sh now does both, and build_docker_images.sh is removed in favour of it: - --publish <true|false>, defaulting to false, so a plain run only ever writes to the local Docker daemon. Named rather than positional so the command says what it is about to do. - --hadoop-version, --spark-version, --hive-version and --version-tag, all optional, defaulting to the set docker-compose_hadoop334_hive313_spark353 uses since that is what setup_demo.sh runs. The previous default was Hadoop 2.8.4 and Hive 2.3.10, which the demo never referenced. - --multi-arch <true|false>. buildx cannot leave a multi-platform result nowhere, so it is pushed when publishing and otherwise loaded into the local containerd store using the builder from the current docker context. A docker-container builder resolves FROM against registries only, which silently builds the later images against Docker Hub instead of against the run. - the base image follows the Spark version, so Spark 4.x selects base_java17. - every build passes HADOOP_VERSION, SPARK_VERSION, HIVE_VERSION and BASE_IMAGE_TAG, because the Dockerfiles carry differing ARG defaults and an image otherwise gets a name that does not describe its contents. - the push step reports a per-tag summary and exits non-zero if any push failed, so a partial publish is not read as success. base_java11 copied mapred-site.xml.template unconditionally, a file Hadoop 3.x does not ship, so Hadoop 3.x could not be built on the Java 11 base at all. The copy is now guarded, in POSIX form because the Dockerfile RUN uses /bin/sh. docker/README.md is updated throughout: a publishing section stating that it is a maintainer action, the version and multi-arch flags, and what keeping multi-arch images locally requires.
…oop default The Java 17 base and the Spark 4.x combination are documented but not usable: the YARN services in those images do not start on Java 17. Describing them alongside the working Spark 3.x flow sends people down a path that fails, so the README now covers only the Java 11 base that the script builds. base_java11 was also the last Dockerfile in the set still defaulting to Hadoop 2.8.4 while every downstream image defaulted to 3.3.4, so a bare docker build of the base produced a parent that no other image would resolve. It now defaults to 3.3.4 like the rest. This only affects builds that pass no --build-arg, since the script always passes one; 2.8.4 remains reachable with --hadoop-version.
Three inconsistencies from the review: - The version override section told readers to edit the block at the top of the script, while the script takes --hadoop-version, --spark-version and --hive-version. Document the flags instead of hand editing source. - The version tag was described only as an exported VERSION_TAG, while --version-tag also exists. Both work, so state both and the order they resolve in: flag, then environment, then the root pom. - The base image section said the script builds base_java11, but it selects base_java17 when --spark-version is 4.x. Say so, while making clear the demo compose files are all Spark 3.x.
473008d to
4b64f42
Compare
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the docs update! The rewrite around the new build_and_publish_docker_images.sh reads well and the base-image, version-default, and multi-arch sections are much clearer than before. One critical accuracy concern: the Publishing section promises a non-zero exit on partial push failure, but the current script does not actually exit non-zero in that case. Please have a Hudi committer or PMC member confirm the intended contract.
| ``` | ||
|
|
||
| Each image is pushed under both the `latest` tag and the version tag, the latter taken from the root `pom.xml` | ||
| unless `VERSION_TAG` overrides it. The script reports a per-tag summary and exits non-zero if any push fails, so a |
There was a problem hiding this comment.
🤖 This section states the script "exits non-zero if any push fails, so a partial publish is not mistaken for a successful one," but the current push_images() in build_and_publish_docker_images.sh doesn't actually do that. Its failure branch only prints Some pushes failed. Review logs above. with no exit 1/return 1, and since each docker push runs inside an if (exempt from set -e) and the final command is an echo, the script exits 0 even when some images (or all, if inspect finds them missing) failed to push. Because publishing overwrites the shared apachehudi/... repos, a maintainer gating a release on the exit code would read a partial publish as success and leave the registry mixed (some images at the new tag, some stale). Could we either add an exit 1 when FAILURE_COUNT > 0 in the script, or soften this sentence to match the actual behavior? The per-tag summary claim is accurate; it's just the exit-code guarantee that isn't.
4 participants
Describe the issue this Pull Request addresses
Supersedes #14101, which was opened against a
docker/build_docker_images.shthat has since beenrewritten on master. Rebasing it would have reverted that work, so this is a fresh port of the same
idea onto the current script. All review comments from #14101 are addressed here.
Publishing the Hudi demo images to the
apachehudiDocker Hub organisation is a manualdocker pushper image today, as documented under Upload Updated Image to Repository on DockerHub. There is no scripted path, so a release publishes nine images by hand across two tags each.
Summary and Changelog
docker/build_and_publish_docker_images.shreplacesdocker/build_docker_images.shand does bothjobs, which is what @xushiyan asked for on #14101 rather than a second script duplicating the image
list and version variables.
--publish <true|false>, defaultfalse, so a plain run only ever writes to the local Dockerdaemon. Named rather than positional, so the command states what it is about to do.
--hadoop-version,--spark-version,--hive-version,--version-tag, all optional.--multi-arch <true|false>, defaultfalse.base_java17.HADOOP_VERSION,SPARK_VERSION,HIVE_VERSIONandBASE_IMAGE_TAG. TheDockerfiles carry differing
ARGdefaults, so without this an image can get a name that does notdescribe its contents.
:latestand the version from the rootpom.xml, overridable with--version-tag.publish is not read as success.
--helpprints the usage, including who publishing is for.Two things worth reviewer attention:
Default versions changed. They were Hadoop 2.8.4 / Hive 2.3.10, but
setup_demo.shrunsdocker-compose_hadoop334_hive313_spark353, so a bare build produced a set the demo neverreferenced. Defaults are now Hadoop 3.3.4 / Hive 3.1.3 / Spark 3.5.3 to match. The old combination
is still reachable with the version flags.
Multi-arch without publishing.
buildxcannot leave a multi-platform result nowhere: it eitherpushes or loads. With
--publish trueit pushes, which is also what makes each image visible to thenext one, since every image after the base starts
FROMthe previous one. Without it the images areloaded into the local containerd image store, using the builder from the current docker context.
Both are required: the legacy image store cannot hold a multi-platform image, and a
docker-containerbuilder resolvesFROMagainst registries only, so it silently builds the laterimages against whatever is already on Docker Hub rather than against the run. The script checks for
the containerd store and stops with an explanation instead.
One unrelated bug fixed because it blocked the above.
base_java11copied/etc/hadoop/mapred-site.xml.templateunconditionally, a file Hadoop 3.x does not ship, so Hadoop3.x could not be built on the Java 11 base at all. The copy is now guarded. The guard is POSIX
[ -f ... ]rather than the[[ -f ... ]]used by the legacybasemodule, because the DockerfileRUNexecutes under/bin/sh, which isdashin these images;[[is not a command there, so thecondition is always false and the copy would be skipped even when the file exists. Happy to split
this into its own PR if reviewers prefer.
docker/README.mdis updated throughout: a publishing section stating this is a maintainer action,the version and multi-arch flags, and what keeping multi-arch images locally requires.
Review round. Three places where the README and the script had drifted apart:
while the script takes
--hadoop-version,--spark-versionand--hive-version. It documentsthe flags now; no hand editing instruction remains.
VERSION_TAG, while--version-tagalsoexists. Both work, so the README states both and the order they resolve in: flag, then
environment variable, then the version from the root
pom.xml.base_java11while the script still selectsbase_java17for a 4.x--spark-version. The section describes both again, and notes that thedemo compose files are all Spark 3.x so the Java 17 base is only reached on request.
Impact
Publishing the demo image set becomes one command for maintainers. A plain build now produces the
images
setup_demo.shactually consumes. Hadoop 3.x is buildable on the Java 11 base.build_docker_images.shis removed, so anyone invoking it by name will need to switch. Nothing inthe repository referenced it other than the README.
Risk Level
low
Documentation Update
docker/README.mdis updated in this PR. No Hudi website change needed.Verification
Full image set built. A default run built all nine images on Docker 28.1.1 / macOS arm64, each
carrying both tags,
:latestand thepom.xmlversion:These names are exactly what
docker-compose_hadoop334_hive313_spark353_{amd64,arm64}.ymlreferences, which the previous defaults did not produce.
Argument handling was checked by stubbing
dockeronPATHand asserting the resultingcommand lines:
docker buildbuildxbuildx --pushbuildx --loaddocker push--publish true--multi-arch true--multi-arch true --publish trueReview round checks. The three README corrections were verified against the script rather than
by reading it:
--version-tag from-flagfrom-flagVERSION_TAG=from-envfrom-envfrom-flagbash -nandsh -nboth pass on the script, and every flag it parses is documented in the README.Multi-arch was exercised for
base_java11anddatanodeonlinux/amd64,linux/arm64,confirming the Hadoop 3.3.4 guard and that
datanoderesolves its parent from the local containerdstore rather than pulling it. The remaining seven were not built multi-arch end to end, as that run
is dominated by ~660 MB Apache mirror downloads per architecture.
Contributor's checklist