Repository navigation
Add an option to set the Dataflow container base repository (Python, Java, Go) - #40690
Conversation
Let users point the Dataflow runner's default SDK container images, and the rewritten cross-language apache/beam images, at a base repository other than gcr.io/cloud-dataflow/v1beta3. This lets jobs run in Trusted Partner Cloud (TPC) universes, which have their own registries. If the option is unset, behavior is unchanged.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40690 +/- ##
============================================
+ Coverage 56.16% 56.38% +0.21%
============================================
Files 1124 871 -253
Lines 178336 154010 -24326
Branches 1489 0 -1489
============================================
- Hits 100170 86841 -13329
+ Misses 75645 65312 -10333
+ Partials 2521 1857 -664
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
📋 Review Summary
This PR adds the --dataflow_container_base_repository (--dataflowContainerBaseRepository in Java) pipeline option across the Java, Python, and Go Dataflow runners to allow overriding the base container registry/repository for default SDK and cross-language container images. The implementation is well-structured, consistent across all three SDKs, and includes comprehensive unit test coverage.
🔍 General Feedback
- The option precedence (
sdk_container_image>dataflow_container_base_repository> default GCR repository) and trailing-slash normalization are handled cleanly and consistently across Java, Python, and Go. - In the Python SDK, container prebuilding (
--prebuild_sdk_container_engine) currently bypasses--dataflow_container_base_repositorywhen selecting the default base image to build from (see inline comment).
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
💬 Inline Findings
-
sdks/python/apache_beam/options/pipeline_options.py(line 1280): 🟡 Medium: When--prebuild_sdk_container_engineis used on Dataflow without--sdk_container_image,sdk_container_builder._get_cloud_build_id_and_image_name(insdks/python/apache_beam/runners/portability/sdk_container_builder.py) resolves the base image viaenvironments.DockerEnvironment.default_docker_image()and only rewrites theapache/beamprefix tonames.DATAFLOW_CONTAINER_IMAGE_REPOSITORYwhensdk_location == 'default'. As a result, prebuilding a container with--dataflow_container_base_repositoryset will still pull the base image fromgcr.io/cloud-dataflow/v1beta3(orapache/beam), which will fail in restricted environments where only the custom mirror repository is accessible. Consider usingapiclient.get_container_image_from_options(pipeline_options)(orget_container_base_repository_from_options) when determining the default base image insdk_container_builder.py. -
runners/google-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/options/DataflowPipelineWorkerPoolOptions.java(line 150): 🟢 Low: For consistency with other optional string getters in this interface (and sincegetDataflowContainerBaseRepository()returnsnullwhen unset, as checked inDataflowRunner.getContainerImageForJob), consider annotating the return type with@Nullable.
@Nullable String getDataflowContainerBaseRepository();
|
Assigning reviewers: R: @jrmccluskey for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
jrmccluskey
left a comment
There was a problem hiding this comment.
+1 to model findings, otherwise LGTM
Good catch on the gap, though the details differ: the prebuild base image comes from
Done: annotated the getter and setter with @nullable |
Part of #40569
The Dataflow runners hardcode
gcr.io/cloud-dataflow/v1beta3for the default SDK container images. Python and Java also use it when they rewrite cross-languageapache/beam*images. Trusted Partner Cloud (TPC) universes have their own registries (e.g.docker.example.com/my-proj/cloud-dataflow/v1beta3), so workers there can't pull these images.This PR adds an option that replaces that prefix:
--dataflow_container_base_repository--dataflowContainerBaseRepository--dataflow_container_base_repositoryapache/beam*images.sdk_container_imageandsdk_harness_container_image_overridesstill take precedence.