Repository navigation
[Improve][E2E] Add multi-arch Spark test images - #11664
Conversation
Signed-off-by: FenjuFu <[email protected]>
|
Thanks for your contribution! This is a solid addition to the project, and your attention to detail really shows. Contributors like you are what keep this community moving forward. Feel free to keep the PRs coming — we're always glad to have you here! Please ignore the following message if you have joined in the Slack~ |
|
The Build check failed, but its job log is no longer available through GitHub Actions, and contributor permissions do not allow re-requesting the upstream check suite. The other checks pass. Could a maintainer please rerun/approve the Build check so I can address any actionable failure if it reproduces? |
DanielLeens
left a comment
There was a problem hiding this comment.
Thanks so much for taking this on, and welcome to the SeaTunnel community! Apple-Silicon-unfriendly test images are a real pain for local development, and building a project-owned, reproducible, multi-arch Spark E2E image is exactly the right direction to fix that. I went through the full diff, the linked issue, and the CI run on your fork commit by commit, and I did find one significant issue that needs to be resolved before this can merge — but the core idea and the Dockerfile/bake setup themselves are solid. Nothing here should be discouraging; this is very close.
What Problem Does This PR Solve?
User pain point: E2E tests for Spark 2/3 currently pull tyrantlucifer/spark:2.4.6 and tyrantlucifer/spark:3.3.0 — third-party, amd64-only images. On Apple Silicon (arm64) machines, Docker has to emulate amd64, which is slow and often unstable (tracked in #11638).
Fix approach: Add a project-owned Dockerfile + docker-bake.hcl that build multi-arch (linux/amd64 + linux/arm64) Spark 2.4.6/Hadoop 2.7 and Spark 3.3.0/Hadoop 3 images from the official Apache Spark archive, publish them via a new manually-triggered workflow to apache/seatunnel:e2e-spark-2.4.6 / apache/seatunnel:e2e-spark-3.3.0, and repoint Spark2Container/Spark3Container at those tags.
One-sentence summary: A reasonable project-owned multi-arch Spark test image is introduced, but the container classes were switched over to the new tags before those tags actually exist anywhere, which currently breaks the E2E build wholesale.
1. Code Change Review
1.1 Core Logic Analysis
The change touches exactly two production-relevant lines plus new CI/test infra:
seatunnel-e2e/seatunnel-e2e-common/src/test/java/org/apache/seatunnel/e2e/common/container/spark/Spark2Container.java(getDockerImage()):"tyrantlucifer/spark:2.4.6"→"apache/seatunnel:e2e-spark-2.4.6".../spark/Spark3Container.java(getDockerImage()):"tyrantlucifer/spark:3.3.0"→"apache/seatunnel:e2e-spark-3.3.0"- New:
seatunnel-e2e/seatunnel-e2e-common/src/test/docker/spark/{Dockerfile,docker-bake.hcl,entrypoint.sh} - New:
.github/workflows/publish-e2e-spark-images.yaml(workflow_dispatchonly, gatedif: github.repository == 'apache/seatunnel')
Before/after (the actual behavioral change):
// before
protected String getDockerImage() {
return "tyrantlucifer/spark:2.4.6";
}
// after
protected String getDockerImage() {
return "apache/seatunnel:e2e-spark-2.4.6";
}Critical path: TestContainerId.SPARK_2_4 / SPARK_3_3 are consumed by dozens of connector/transform/engine E2E test classes (Hudi, Kafka, Paimon, Iceberg, Druid, Pulsar, Lance, file-local/sftp, FakeSourceToAssertIT, etc.) through AbstractTestSparkContainer.startUp(), which does:
GenericContainer<>(getDockerImage())
.withEnv("SPARK_MODE", "master")
.waitingFor(LogMessageWaitStrategy(".*Master: Starting Spark master at.*"))
So this is squarely the normal execution path, not a boundary/recovery case: every parameterized E2E test that includes Spark as one of its target engines calls getDockerImage() directly during container startup on every CI run.
Runtime chain that actually executes on CI right now:
Maven IT job (e.g. all-connectors-it-2, paimon-connector-it, engine-v2-it, ...)
-> JUnit5 @TestContainer parameter resolution
-> AbstractTestSparkContainer.startUp()
-> new GenericContainer<>("apache/seatunnel:e2e-spark-2.4.6" | "...e2e-spark-3.3.0")
-> Testcontainers RemoteDockerImage pull
-> Docker Hub GET manifest apache/seatunnel:e2e-spark-2.4.6
-> HTTP 404 "manifest unknown"
-> ContainerFetchException -> ContainerLaunchException -> ParameterResolutionException
-> test class ERROR (not merely the Spark-parameterized case — the whole test class aborts)
I verified this is exactly what is happening, not a hypothesis: pulling the job logs from the fork's failed Build run (head 2a36c68a) shows the identical stack trace repeated across all-connectors-it-1..8, jdbc-connectors-it-part-1/2/3/6/7, kafka-connector-it, connector-redis-it, doris-connector-it, kudu-connector-it, paimon-connector-it, rocketmq-connector-it, amazonSqs-connector-it, connector-file-local-it, connector-file-sftp-it, transform-v2-it-part-1/2, engine-v2-it:
Caused by: com.github.dockerjava.api.exception.NotFoundException: Status 404:
{"message":"manifest for apache/seatunnel:e2e-spark-2.4.6 not found: manifest unknown: manifest unknown"}
(and the same for e2e-spark-3.3.0). I also confirmed on Docker Hub directly that the apache/seatunnel repository currently has exactly 7 tags, all real ~3.18 GB release distributions pushed by the asfjenkins account — there is no e2e-spark-* tag there yet, because the only thing that can create it is the new workflow_dispatch workflow, and nobody has run it.
Key findings:
- The normal path reaches the changed lines immediately and unconditionally — this is not an edge case.
- The scenario covered by the fix (multi-arch, project-owned image) is the right one; the Dockerfile/bake/entrypoint design itself is clean and idiomatic (tini as PID 1,
set -euo pipefail,--chmod=755on copy, buildx bake with two targets each building both platforms). - This is a precise fix for the wrong sequencing — repointing the container classes must happen after the images are published and verified pullable, not in the same commit as the workflow that would publish them. As merged today, it is not a working fix yet; it is a fix that requires an out-of-band manual action by someone with
apache/seatunnelwrite access before it stops being a regression. - Remaining blind spot: even once published, the new
entrypoint.shhas never been booted anywhere (author's own PR description: "Local Docker validation was not available... Maven verification... produced a truncated ZIP"). The readiness wait strategy (LogMessageWaitStrategy(".*Master: Starting Spark master at.*"), 2‑minute timeout) depends onspark-class org.apache.spark.deploy.master.Master --host ...producing that exact log line — very likely correct since it's the same Spark class previously wrapped bytyrantlucifer's image, but genuinely unverified end-to-end. - Correctness depends entirely on external state (Docker Hub tag existence) that this PR does not, and by construction of a
workflow_dispatch-only +if: github.repository == 'apache/seatunnel'gate, cannot itself establish. That precondition is not encoded anywhere as a CI gate (e.g., no check that skips/soft-fails Spark E2E tests when the new tag is absent), so the failure mode is a hard break of the entire matrix rather than a scoped, isolated failure of only the two new Spark tests.
1.2 Compatibility Impact
Verdict: Fully compatible.
No public API, Option name/default, wire protocol, checkpoint/savepoint format, or documented runtime behavior is touched. This is purely CI/E2E test infrastructure (test container image source + a new manual publish workflow). There is no impact on end users running SeaTunnel jobs.
1.3 Performance / Side-Effect Analysis
- CPU/network: the new publish workflow builds two Spark distributions for two platforms each via QEMU-emulated
buildx bake;timeout-minutes: 60is a reasonable ceiling given the tarballs are a few hundred MB per arch and only runs on manual dispatch — no impact on the normal per-PR CI cost. - No new production dependency, no new runtime thread/lock/retry behavior.
- Resource release:
AbstractTestSparkContainer.tearDown()is unchanged and still stops the container and cleans the bind-mounted volume — fine. - Idempotency of the publish workflow: re-running
docker buildx bake --pushoverwrites the same mutable tag with no version/date suffix and no digest pinning on the Java side. Low risk for E2E fixtures, but worth knowing (see Issue 6).
1.4 Error Handling and Logging
entrypoint.sh's case statement fails loudly and correctly for any SPARK_MODE other than master (echo ... >&2; exit 1) rather than silently doing nothing — good practice, no swallowed exceptions. The Dockerfile's RUN test -n ... guards on required build args are also a sound fail-fast pattern.
Issue 1 — Container image reference switched before the publishing workflow has ever been run, breaking the entire Testcontainers-based E2E matrix
- Location:
seatunnel-e2e/seatunnel-e2e-common/src/test/java/org/apache/seatunnel/e2e/common/container/spark/Spark2Container.java:49,.../spark/Spark3Container.java:49(paired with.github/workflows/publish-e2e-spark-images.yaml:19beingworkflow_dispatch-only) - Problem description: The two container classes now request
apache/seatunnel:e2e-spark-2.4.6/apache/seatunnel:e2e-spark-3.3.0, but nothing in this PR (or anywhere else) actually creates those tags automatically — the publish workflow only runs when a maintainer manually triggers it. Verified empirically: the fork's CI run on this exact head commit fails ~30+ unrelated IT jobs (all-connectors-it-*,jdbc-connectors-it-part-*,kafka,redis,doris,kudu,paimon,rocketmq,amazonSqs,file-local,file-sftp,transform-v2-it-part-*,engine-v2-it) withNotFoundException: manifest for apache/seatunnel:e2e-spark-2.4.6/3.3.0 not found, and Docker Hub currently lists only 7 official release tags forapache/seatunnel, noe2e-spark-*tag. - Potential risk: Merging as-is regresses CI for every module that includes Spark as a target engine — not scoped to Spark-only tests, since the container fetch failure aborts the whole JUnit test class via
ParameterResolutionException. It stays broken indefinitely until a maintainer withapache/seatunnelwrite access manually triggers the new workflow (an out-of-band step this PR cannot itself guarantee happens before/at merge time). - Best improvement:
- Option A (recommended): Split into two PRs / merge order — merge the workflow only first, have a maintainer trigger
publish-e2e-spark-images, confirm both tags are pullable on Docker Hub, then merge theSpark2Container/Spark3Containerreference switch as a small follow-up. - Option B: Keep it one PR, but make the workflow run automatically on a path-filtered push to
dev(with agithub.repositoryguard) so the image is guaranteed to exist by the time this same commit lands ondev, instead of relying on a human to remember to click "Run workflow".
- Option A (recommended): Split into two PRs / merge order — merge the workflow only first, have a maintainer trigger
- Severity: High
- Raised by another reviewer: No (FenjuFu did comment on the PR that the "Build" check failed and asked a maintainer to look, but did not diagnose the root cause — I'm providing the concrete diagnosis here).
Issue 2 — Publishing E2E test images into the official release Docker Hub namespace
- Location:
.github/workflows/publish-e2e-spark-images.yaml:1-42(compare with the existing.github/workflows/publish-docker.yaml, which pushes official release images to the sameapache/seatunnelDocker Hub repository using the samesecrets.DOCKERHUB_USER/secrets.DOCKERHUB_TOKEN, triggered bypush: tags: '*'as part of the ASF release process) - Problem description:
apache/seatunnelon Docker Hub is, in practice, the official release-artifact repository (currently 7 tags, all multi-GB full distributions pushed by theasfjenkinsaccount). This PR addse2e-spark-2.4.6/e2e-spark-3.3.0tags into that same repository using the same production release credentials, from a new, separately-triggerable workflow. - Potential risk: Mixes CI-only test fixtures into the tag namespace that downstream users/scripts treat as "the official SeaTunnel distribution," and extends who/what can push to that namespace using release-grade Docker Hub credentials — a supply-chain surface change for an Apache release artifact that's worth a maintainer/PMC-infra sign-off, not something to wave through as an ordinary E2E improvement.
- Best improvement:
- Option A: Publish to a dedicated, clearly test-scoped location instead — e.g. GHCR under the ASF org (
ghcr.io/apache/seatunnel-e2e) or a separate Docker Hub repo/org reserved for CI fixtures — so no release credentials are needed and the release tag namespace stays clean. - Option B: If reusing
apache/seatunnelis intentional, at minimum get explicit maintainer/infra confirmation that DOCKERHUB_TOKEN is fine to use this way, and document in the workflow why it's safe (blast-radius, who can trigger it, review requirements for who hasworkflow_dispatchrights).
- Option A: Publish to a dedicated, clearly test-scoped location instead — e.g. GHCR under the ASF org (
- Severity: Medium
- Raised by another reviewer: No
Issue 3 — New Spark master entrypoint has never been booted or log-verified end-to-end
- Location:
seatunnel-e2e/seatunnel-e2e-common/src/test/docker/spark/entrypoint.sh:22-27, cross-referenced with the wait condition inAbstractTestSparkContainer(LogMessageWaitStrategy(".*Master: Starting Spark master at.*"), 2-minute timeout — not part of this diff, but is the acceptance gate for the new image) - Problem description:
entrypoint.shexecsspark-class org.apache.spark.deploy.master.Master --host ...directly rather than the vendor images' usual wrapper scripts. This is very likely to emit the expectedMaster: Starting Spark master at ...log line since it's the same underlying Spark class, but the PR description itself states Docker validation could not be run locally, and CI can't reach this code path either yet (Issue 1) — so, as of this review, nobody has observed the new image actually pass its own container-readiness check. - Potential risk: If the log line format, startup timing, or default port binding differs even slightly from what
LogMessageWaitStrategyexpects, every Spark E2E test will time out after 2 minutes rather than fail fast — this would only surface once Issue 1 is fixed and the image is pullable, likely well after this PR has already landed. - Best improvement: Once Issue 1 is resolved and the images are published, run the actual E2E suite against them (or at minimum one representative test per engine, e.g.
FakeSourceToAssertITwith Spark 2 and Spark 3) before considering this done, and record the result in the PR description. - Severity: Medium
- Raised by another reviewer: No
Issue 4 — No inline comments on the deliberate SPARK_MODE scope limitation
- Location:
seatunnel-e2e/seatunnel-e2e-common/src/test/docker/spark/entrypoint.sh:22-29 - Problem description: The entrypoint only implements
SPARK_MODE=masterand hard-fails on anything else. That's a deliberate, reasonable scope decision (today only a Spark master container is ever started — seeAbstractTestSparkContainer's own doc comment: "if we want to use cluster mode, we need to start a worker"), but nothing in the script or the Dockerfile explains why worker mode isn't supported yet, which will look like an oversight to the next contributor who tries to add worker support. - Potential risk: Low on its own, but "nontrivial new script with unexplained hard-fail branch" is exactly the kind of thing that costs someone else debugging time later.
- Best improvement: A short comment above the
caseblock noting that only master mode is needed today (standalone/client-mode job submission), and worker mode is intentionally unimplemented pending future cluster-mode E2E support. - Severity: Medium
- Raised by another reviewer: No
Issue 5 — No concurrency guard on the manual publish workflow
- Location:
.github/workflows/publish-e2e-spark-images.yaml:16-42 - Problem description: The workflow has no
concurrency:group, so two manual triggers close together (e.g., a retry after a perceived hang) could race and interleave twodocker buildx bake --pushruns against the same tags. - Potential risk: Low — this is a manually-triggered, infrequent workflow, but a
concurrencygroup is a one-line safeguard against a confusing partial-overwrite outcome. - Best improvement: Add
concurrency: { group: publish-e2e-spark-images, cancel-in-progress: false }. - Severity: Low
- Raised by another reviewer: No
Issue 6 — Published tags are mutable with no reproducibility/versioning signal
- Location:
seatunnel-e2e/seatunnel-e2e-common/src/test/docker/spark/docker-bake.hcl:29-43 - Problem description:
apache/seatunnel:e2e-spark-2.4.6/e2e-spark-3.3.0are simple version tags with no build-date/content-hash suffix, and Testcontainers uses the default (non-AlwaysPull) pull policy, so a stale locally-cached layer on a given CI runner or dev machine won't necessarily match a freshly rebuilt tag after a Dockerfile fix. - Potential risk: Low — common tradeoff for CI fixture images, but worth knowing if a future Dockerfile bugfix needs to roll out predictably everywhere.
- Best improvement: Optional: append a short build-date or Dockerfile-content-hash suffix, or document that re-running the workflow is expected to occasionally require a manual
docker pullrefresh on stale runners. - Severity: Low
- Raised by another reviewer: No
2. Code Quality Assessment
2.1 Coding Standards
License headers are present and correct on all new files (confirmed by the fork's own "License header" CI job passing). Dockerfile/docker-bake.hcl are clean and idiomatic. See Issue 4 for the one missing explanatory comment.
2.2 Test Coverage and Test Stability
This PR changes E2E test infrastructure (container image source), so the mandatory stability analysis applies. I checked for hard sleeps, assertion/async races, unreleased resources, order dependence, and weak readiness signals in the changed files — none of those specific anti-patterns are newly introduced; the readiness wait strategy is unchanged and reused as-is. However, "does the test infra actually work" is the real question here, and the answer today, verified against the actual CI run, is no — not because of a flaky readiness signal, but because the referenced image doesn't exist yet.
Stability rating: High risk (path:line evidence: Spark2Container.java:49, Spark3Container.java:49, confirmed by fork run 92307336191 and 20+ sibling IT jobs at commit 2a36c68a0d641809d0dd1260fa2ee0bc7fcafec3, all failing with manifest for apache/seatunnel:e2e-spark-2.4.6/3.3.0 not found). This maps to Issue 1 above (High severity), per the mandatory rule that a "High risk" stability rating must become a formal High-severity issue.
2.3 Documentation Updates
No user-facing behavior changed, and I found no other code or docs referencing the old tyrantlucifer/spark:* images or the new e2e-spark-* tags outside the 6 changed files, so no docs/en/docs/zh updates are required for this PR.
3. Architectural Soundness
3.1 Elegance of the Solution
The Dockerfile/bake/entrypoint design is a precise, well-scoped long-term solution for the multi-arch problem itself. The rollout sequencing (Issue 1) and namespace choice (Issue 2) are the parts that need rework before this is a complete, production-safe long-term solution rather than a partially-working one.
3.2 Maintainability
Good: single parameterized Dockerfile + bake file for both Spark versions, minimal surface area, no connector-specific logic leaking into the engine or core.
3.3 Extensibility
Adding a third Spark version later is a small, mechanical change to docker-bake.hcl plus a new getDockerImage() override — reasonable. Worker-mode support (if ever needed) would require extending entrypoint.sh's case statement, which is currently undocumented (Issue 4).
3.4 Historical-Version Compatibility
Not applicable — this PR does not touch seatunnel-engine/seatunnel-api, checkpoint/savepoint, serialization, or any historically persisted state; it is scoped to E2E test container images.
4. Issue Summary
| Number | Issue | Location | Severity |
|---|---|---|---|
| 1 | Container images repointed before the publish workflow has ever run; breaks the entire Testcontainers E2E matrix | Spark2Container.java:49, Spark3Container.java:49 | High |
| 2 | E2E test images pushed into the official release Docker Hub namespace using release credentials | .github/workflows/publish-e2e-spark-images.yaml:1-42 | Medium |
| 3 | New Spark master entrypoint never booted/log-verified end-to-end | entrypoint.sh:22-27 | Medium |
| 4 | No comment explaining the deliberate SPARK_MODE=master-only scope | entrypoint.sh:22-29 | Medium |
| 5 | No concurrency guard on the manual publish workflow | publish-e2e-spark-images.yaml:16-42 | Low |
| 6 | Mutable, unversioned published tags | docker-bake.hcl:29-43 | Low |
5. Merge Recommendation
Conclusion: Ready to merge after fixes
-
Blockers — must be fixed:
- Issue 1 (High): resolve the sequencing gap before the two container-reference lines land on
dev— either split into "workflow first, container switch second" once the images are confirmed pullable, or wire the publish workflow to run automatically ahead of/alongside this change so the tags are guaranteed to exist. This is the one item that actually breaks CI today and must not be merged as-is.
- Issue 1 (High): resolve the sequencing gap before the two container-reference lines land on
-
Recommended fixes — non-blocking:
- Issue 2 (Medium): get explicit maintainer/infra sign-off on reusing the official
apache/seatunnelDocker Hub namespace and release credentials for CI fixtures, or move to a dedicated test-image location. - Issue 3 (Medium): once images are published, actually run at least one Spark 2 and one Spark 3 E2E test against them and confirm the readiness wait strategy passes.
- Issue 4 (Medium): add a short comment on the entrypoint's master-only scope.
- Issues 5–6 (Low): concurrency guard, tag versioning — nice-to-haves.
- Issue 2 (Medium): get explicit maintainer/infra sign-off on reusing the official
Overall assessment: the underlying problem (amd64-only third-party Spark images hurting Apple Silicon contributors) is real and well worth fixing, and the technical design of the Dockerfile/bake/entrypoint is sound. The blocking issue isn't the image design — it's that the container classes were switched over before the new tags exist anywhere reachable, which is empirically breaking the fork's CI run across roughly 20+ unrelated IT jobs right now. Once Issue 1 is resolved (images published and verified pullable before or together with the reference switch), and ideally Issue 2 is cleared with a maintainer, this looks like a genuinely good improvement. No other reviewer has posted a formal review on this PR yet, so there is no existing verdict to agree or disagree with here — this is the first full review.
No blocking issues exist beyond Issue 1's sequencing problem — I want to be clear this isn't a case of many small things stacking up; it's really one structural gap (the chicken-and-egg between "workflow that creates the tag" and "code that requires the tag") plus a namespace question worth a second pair of eyes. Great first contribution — looking forward to seeing this land once the sequencing is sorted out!
|
Thanks for the detailed review — you're absolutely right about the sequencing issue. The new apache/seatunnel:e2e-spark-* tags are not guaranteed to exist before the manual publish workflow is run in the upstream repository, so switching Spark2Container / Spark3Container in the same PR can break the Spark E2E matrix. I'll update this PR to keep the image build/publish infrastructure separate, and only switch the Java-side image references after the images have been published and verified pullable from apache/seatunnel. Thanks again for the careful review and the clear explanation. |
|
Hi @FenjuFu, thanks for the quick follow-up on both points! On the sequencing point — yes, that's exactly it, and your plan (keep the image build/publish infra in this PR, defer switching On the Build check — I just re-pulled the job logs for the fork's
One unrelated data point so it doesn't get conflated with the above: one of the ~27 failing jobs on that same run, Looking forward to the follow-up once the split is ready! |
Signed-off-by: FenjuFu <[email protected]>
|
Thanks for the detailed follow-up. I addressed the rollout sequencing issue in commit 8ffec55:
The Java-side switch is now deferred until the new images have been published and verified pullable. I also updated the PR description so it no longer claims that this PR changes the container references. Validation completed: I have left the Docker Hub namespace/credential decision and tag policy unchanged for maintainer/infra confirmation. |
DanielLeens
left a comment
There was a problem hiding this comment.
What Problem Does This PR Solve?
Spark E2E tests currently pull tyrantlucifer/spark:2.4.6 and tyrantlucifer/spark:3.3.0 — third-party images that are amd64-only. On Apple Silicon (arm64) machines this forces QEMU emulation, which is slow and often unstable (tracked in #11638). This PR lays the groundwork for a project-owned, reproducible, genuinely multi-arch replacement: a parameterized Dockerfile, a Buildx Bake file that builds linux/amd64 + linux/arm64 for both Spark 2.4.6/Hadoop 2.7 and Spark 3.3.0/Hadoop 3, and a manually-triggered Apache-only workflow that publishes those images to apache/seatunnel:e2e-spark-2.4.6 / apache/seatunnel:e2e-spark-3.3.0. Importantly, this revision does not touch Spark2Container/Spark3Container — the Java-side switch is explicitly deferred to a follow-up, after the images exist and are verified pullable. This is a pure infra-addition PR: +165/-0 across 4 new files, no production or existing-test code touched.
This is a full re-review of the current head, which supersedes the earlier reviewed commit. The previously reported blocking issue (premature Spark2Container/Spark3Container switch) has been resolved by reverting those two files entirely — they are not part of this diff at all.
1. Code Change Review
1.1 Core Logic Analysis
Four new files, no existing files modified:
.github/workflows/publish-e2e-spark-images.yaml(46 lines) —workflow_dispatch-only, gatedif: github.repository == 'apache/seatunnel'seatunnel-e2e/seatunnel-e2e-common/src/test/docker/spark/Dockerfile(45 lines).../spark/docker-bake.hcl(43 lines).../spark/entrypoint.sh(31 lines)
Dockerfile (Dockerfile:17-45): single-stage build using eclipse-temurin:8-jre-jammy, with test -n fail-fast guards on required build args (SPARK_VERSION, HADOOP_PROFILE), --fail --retry 3 on the download, apt-list cleanup to keep image size down, tini as PID 1 for correct signal handling of a long-running JVM process, and --chmod=755 on COPY to avoid a separate RUN chmod layer. These are idiomatic, well-considered patterns.
docker-bake.hcl (docker-bake.hcl:19-43): a shared spark target sets platforms = ["linux/amd64", "linux/arm64"], and two version-specific targets (spark-2.4.6, spark-3.3.0) inherit it and set a single tag each. Each target sets the same tag for both platforms, so docker buildx bake --push (via the docker-container driver from docker/setup-buildx-action@v3) produces a proper multi-arch manifest list per tag, not two competing single-arch pushes. This means the eventual consumer (Testcontainers on the Java side) needs zero runtime arch-detection logic — Docker itself resolves the correct platform variant on pull, and there is no scenario where CI would silently run the wrong-arch image under emulation once this is wired up, since manifest-list resolution is native, not emulated.
entrypoint.sh (entrypoint.sh:19-31): starts spark-class org.apache.spark.deploy.master.Master via exec (correctly replaces the shell so signals from tini propagate straight to the JVM, no orphaned wrapper process), with a case statement that fails loudly on any unsupported SPARK_MODE. Compared to the previously-reviewed commit, this now carries an explanatory comment documenting the master-only scope decision. The default SPARK_MASTER_HOST fallback (spark-master) matches AbstractTestSparkContainer.java:61's .withNetworkAliases("spark-master") — the network alias the future consumer would resolve against — so the entrypoint's defaults are already consistent with the existing container harness.
Workflow (publish-e2e-spark-images.yaml:17-46): workflow_dispatch-only, gated to apache/seatunnel, with permissions: contents: read (minimal), docker/login-action@v3 + docker/setup-qemu-action@v3 + docker/setup-buildx-action@v3 at the same pinned versions already used by the existing publish-docker.yaml release-image job. Compared to the previously-reviewed commit, a concurrency group (group: publish-e2e-spark-images, cancel-in-progress: false) has been added, closing the race window where two manual triggers could interleave two --push runs.
Key finding on the critical path: I confirmed by diffing against the previous head and reading Spark2Container.java/Spark3Container.java directly that getDockerImage() in both still returns the original tyrantlucifer/spark:* tags — this diff contains zero Java changes. Nothing in the normal E2E execution path is affected by this PR at all today; the previously-identified High-severity blocker (container classes requesting a tag that doesn't exist yet) no longer applies, because the tag-requesting side was reverted rather than merged ahead of the tag-creating side.
Verification against fork CI: the fork's Build run on this exact head shows every IT job that previously failed with a "manifest not found" error for the new tags — the full connector/JDBC/transform/engine IT matrix — now completing with success on both JDK 8 and JDK 11. This is direct, empirical confirmation that the sequencing fix works and the E2E matrix is healthy again on this head. The only failing job in that run is an unrelated documentation dead-link checker; this PR touches no docs content, so that failure is out of scope.
1.2 Compatibility Impact
Fully compatible. No public API, Option name/default, wire protocol, checkpoint/savepoint format, or documented runtime behavior is touched, and no existing test-harness Java file is touched either. This is purely new CI/test infrastructure that nothing currently references.
1.3 Performance / Side-Effect Analysis
The publish workflow only runs on manual dispatch, adding zero cost to normal per-PR CI. timeout-minutes: 60 is a reasonable ceiling for four QEMU-emulated builds. Registry storage cost is modest relative to the existing full-distribution release tags in the same Docker Hub repository. No new production dependency or resource-release-path change. One low-severity note: re-running docker buildx bake --push overwrites the same mutable tag with no digest pinning or date/hash suffix — acceptable for CI fixture images, but worth a mention below.
1.4 Error Handling and Logging
The Dockerfile's test -n guards fail the build immediately and clearly if required args are missing, rather than producing a confusing downstream curl 404. entrypoint.sh's case statement fails loudly for any unsupported SPARK_MODE rather than silently doing nothing. No swallowed exceptions anywhere in this diff. No blocking issues in this category.
Issue 1 — Published tags reuse the official release Docker Hub namespace and credentials
- Location:
.github/workflows/publish-e2e-spark-images.yaml:35-39 - Problem description:
apache/seatunnelon Docker Hub is, in practice, the official release-artifact repository. This PR addse2e-spark-*CI-fixture tags into that same namespace using the same production credentials already used for release publishing. - Potential risk: Low incremental risk since the credentials already have push access to this exact repository, but it mixes test-fixture tags into a namespace downstream users/scripts may treat as "the official distribution list."
- Best improvement: Get explicit maintainer/infra confirmation that reusing
apache/seatunnel+ the release token for CI fixtures is acceptable, or move to a dedicated location (a separate repo/org, or GHCR under the ASF org) in a follow-up. - Severity: Medium
- Raised by another reviewer: No (carried forward from my own earlier round on this PR; the author's own comment explicitly left this decision for maintainer/infra confirmation).
Issue 2 — New entrypoint has never actually been booted or log-verified
- Location:
entrypoint.sh:22-26, cross-referenced withAbstractTestSparkContainer.java:72-75(2-minuteLogMessageWaitStrategytimeout) - Problem description: Because the publish workflow hasn't been triggered by a maintainer yet, and nothing on the Java side references the new tags in this diff, no CI run has actually built and booted this image end-to-end.
- Potential risk: If the log format, startup timing, or port binding differs even slightly from what the readiness wait strategy expects, every Spark E2E test would time out once a future PR wires this image in — but this risk is fully contained to that future PR and cannot manifest from merging this one.
- Best improvement: Before or as part of the follow-up PR that switches
Spark2Container/Spark3Container, have a maintainer trigger the publish workflow once and run at least one representative Spark 2 and Spark 3 E2E test against the new tags on both architectures. - Severity: Medium (informational for this PR; will become blocking for the follow-up switch PR if left unverified)
- Raised by another reviewer: No
2. Code Quality Assessment
2.1 Coding Standards
License headers are present and correctly formatted on all four new files, confirmed both by direct inspection and by the fork's license-header CI job passing. The Dockerfile, bake file, and entrypoint script are clean, idiomatic, and consistent with existing repository conventions. The explanatory comment on the master-only scope decision requested in the prior round has been added.
2.2 Test Coverage and Test Stability
This PR adds test infrastructure but does not yet wire it into any test execution path — no container class or E2E test references these new images. I checked for the standard flaky-pattern anti-patterns (hard sleeps, async/assertion races, unreleased resources, order dependence, weak readiness signals): none are newly introduced, and the one readiness-signal dependency that does exist is unchanged and not exercised by this diff.
Has this been validated on both architectures? No, and it cannot be from within this PR alone — the publish workflow is dispatch-only and gated to apache/seatunnel, so it has never actually run. This is fully explained by Issue 2 above.
Stability rating: Stable for this PR as merged — nothing in the normal or CI execution path reaches the new image, so there is no flakiness risk introduced today. The new image's own correctness remains unverified and must be confirmed before the follow-up Java-side switch, tracked as Issue 2, not as a test-stability defect in this diff.
2.3 Documentation Updates
No user-facing behavior changed, and no other code in the repository references the old vendor image or the new tags outside these four files, so no docs/en/docs/zh updates are required for this PR. Worth flagging for the follow-up PR: once the Spark E2E images are actually switched, any contributor-facing E2E setup documentation mentioning the vendor image name should be updated.
3. Architectural Soundness
3.1 Elegance of the Solution
The Dockerfile/bake/entrypoint trio is a precise, minimal, well-scoped solution to the actual problem. Splitting "build and publish the image" from "switch the Java-side reference" into two separate changes — as this revision now does — is exactly the right sequencing discipline for a change spanning an external, human-triggered side effect and an internal code change.
3.2 Maintainability
Good: a single parameterized Dockerfile and bake file cover both Spark versions via build args, minimizing duplication. No connector-specific logic leaks into the engine or core.
3.3 Extensibility
Adding a third Spark version later is a small, mechanical change: one new target block plus, eventually, a new getDockerImage() override. Worker-mode support, if ever needed, would extend entrypoint.sh's case statement, now explicitly documented as intentionally out of scope for today.
3.4 Historical-Version Compatibility
Not applicable. This PR does not touch seatunnel-engine, seatunnel-api, checkpoint/savepoint, or any historically persisted state.
4. Issue Summary
| Number | Issue | Location | Severity |
|---|---|---|---|
| 1 | E2E test images published into the official release Docker Hub namespace/credentials | .github/workflows/publish-e2e-spark-images.yaml:35-39 |
Medium |
| 2 | New Spark master entrypoint never booted/log-verified end-to-end | entrypoint.sh:22-26 |
Medium |
| 3 | Published tags are mutable with no build-date/content-hash suffix | docker-bake.hcl:29-43 |
Low |
No High-severity or blocking issues remain. The previously-reported blocker has been fully resolved by reverting the container-reference files out of this diff, and the fork's CI run on the current head confirms every previously-broken Spark-touching IT job is now green.
5. Merge Recommendation
Conclusion: Ready to merge
-
Blockers — must be fixed
- None. The sequencing issue that made the earlier commit unmergeable has been correctly and completely resolved.
-
Recommended fixes — non-blocking
- Issue 1 (Medium): get explicit maintainer/infra sign-off on reusing the release Docker Hub namespace and credentials for CI fixture images.
- Issue 2 (Medium): once triggered, run at least one Spark 2 and one Spark 3 E2E test against the new tags on both architectures before switching the container classes over.
- Issue 3 (Low): optional — consider a build-date or content-hash tag suffix for future reproducibility.
Overall assessment: this is a well-scoped, correctly-sequenced piece of CI infrastructure. The multi-arch design (a shared tag across both platforms in a single bake target) is the right approach and needs no custom arch-selection logic on the consumer side, the workflow reuses established, already-vetted patterns, and — most importantly — this revision no longer touches any file in the normal or CI execution path, so it cannot regress anything today. Good, careful follow-through on the prior review's feedback.
davidzollo
left a comment
There was a problem hiding this comment.
+1
LGTM
Thanks for your contribution! This is a solid addition to the project, and your attention to detail really shows. Contributors like you are what keep this community moving forward. Feel free to keep the PRs coming — we're always glad to have you here!
Please ignore the following message if you have joined in the Slack~
To facilitate focused communication among contributors, we’ve created a brand-new dedicated #contributors channel in official Slack workspace.
Slack workspace link: https://s.apache.org/seatunnel-slack
This channel is designed for technical discussions, feature design brainstorming, roadmap sync and direct communication with project core Contributors. Everyone who contributes code, docs, test cases or community ideas is warmly invited to join, share thoughts and collaborate together under The Apache Way. Welcome to join 😄
Purpose
Provide project-owned, reproducible Spark E2E images for both
linux/amd64andlinux/arm64, addressing the Apple Silicon limitation tracked in #11638.Changes
linux/amd64andlinux/arm64apache/seatunnelSpark2ContainerandSpark3Containeron the existing images until the new tags have been published and verified pullableThe proposed publish destination is:
apache/seatunnel:e2e-spark-2.4.6apache/seatunnel:e2e-spark-3.3.0Switching the Java-side image references is intentionally deferred to a follow-up after the images exist. The seven shared Flink images (1.13, 1.14, 1.15, 1.16, 1.17, 1.18, and 1.20) are also left for follow-up after the Spark path is proven.
Validation
git diff --check.github/workflows/publish-e2e-spark-images.yamlsuccessfully with PyYAMLeclipse-temurin:8-jre-jammypublishes both amd64 and arm64 variantsThe repository Maven Wrapper remains unusable in this environment because its downloaded Maven ZIP is truncated (
zip END header not found). A checksum-verified standalone Maven 3.8.4 was also attempted, butspotless:applymade no progress while resolving the multi-module build and was terminated without changing files.Part of #11638.