Use distroless container built from HEAD in Dataflow V2 integration tests - #36160
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @kennknowles, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request updates the Dataflow V2 integration test configuration to ensure that the distroless container images used for testing are built directly from the current source code (HEAD) rather than relying on potentially outdated :latest tags. This change improves the consistency and reliability of the integration tests by guaranteeing that tests run against the most recent code, aligning the V1 and V2 testing methodologies.
Highlights
- Distroless Image Definition: Defined new Gradle properties
dockerJavaDistrolessImageContaineranddockerJavaDistrolessImageNameto specifically manage the distroless Java SDK container image within the build system. - Pipeline Options Refactoring: Refactored Dataflow V2 pipeline options by introducing
runnerV2CommonPipelineOptionsand then derivingrunnerV2PipelineOptionsandrunnerV2DistrolessPipelineOptionsfor clearer and more explicit configuration of container images. - Streamlined Distroless Build Process: Replaced the previous
buildAndPushDistrolessContainerImagetask with a newbuildAndPushDistrolessDockerJavaContainertask. This new task correctly builds the distroless image from HEAD, ensuring tests use the latest code. - Updated Integration Test Task: The
examplesJavaRunnerV2IntegrationTestDistrolesstask was updated and re-positioned to depend on the newbuildAndPushDistrolessDockerJavaContainertask and now correctly utilizes therunnerV2DistrolessPipelineOptions. - New Pre-Commit Task: Introduced a new
examplesJavaDistrolessRunnerV2PreCommittask specifically for running pre-commit tests with the distroless container, enhancing the testing suite.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
a90f08f to
8b7db44
Compare
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
fe243f3 to
4a1186a
Compare
|
R: @Abacn |
|
CC @sjvanrossum |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
|
Now that it is actually using the built container, the workflow may be too short of a timeout |
|
Actually it has a 180 minute timeout but seems to be canceled after 16 minutes repeatedly. I don't know why. |
|
When approved, I will resolve the trivial conflict in the json file, or just drop that trigger file commit, then merge without waiting for the very-long very-flaky tests to rerun. |
| def buildAndPushDockerJavaContainer = tasks.register("buildAndPushDockerJavaContainer") { | ||
| def javaVer = getSupportedJavaVersion(project.findProperty('testJavaVersion') as String) | ||
|
|
||
| tasks.register('buildAndPushDistrolessContainerImage', Task.class) { | ||
| // Only Java 17 and 21 are supported. | ||
| // See https://github.com/GoogleContainerTools/distroless/tree/main/java#image-contents. | ||
| def allowed = ["java17", "java21"] | ||
| dependsOn ":sdks:java:container:${javaVer}:docker" | ||
| def defaultDockerImageName = containerImageName( | ||
| name: "${project.docker_image_default_repo_prefix}${javaVer}_sdk", | ||
| root: "apache", | ||
| tag: project.sdk_version) | ||
| doLast { | ||
| def javaVer = getSupportedJavaVersion(project.findProperty('testJavaVersion') as String) | ||
| if (!allowed.contains(javaVer)) { | ||
| throw new GradleException("testJavaVersion must be one of ${allowed}, got: ${javaVer}") | ||
| } | ||
| if (!project.hasProperty('dockerTag')) { | ||
| throw new GradleException("dockerTag is missing but required") | ||
| } | ||
| def repository = "us.gcr.io/apache-beam-testing/${System.getenv('USER')}" | ||
| def tag = project.findProperty('dockerTag') | ||
| def imageURL = "${repository}/beam_${javaVer}_sdk_distroless:${tag}" | ||
| exec { | ||
| executable 'docker' | ||
| workingDir rootDir | ||
| args = [ | ||
| 'buildx', | ||
| 'build', | ||
| '-t', | ||
| imageURL, | ||
| '-f', | ||
| 'sdks/java/container/distroless/Dockerfile', | ||
| "--build-arg=BEAM_BASE=gcr.io/apache-beam-testing/beam-sdk/beam_${javaVer}_sdk", | ||
| "--build-arg=DISTROLESS_BASE=gcr.io/distroless/${javaVer}-debian12", | ||
| '.' | ||
| ] | ||
| commandLine "docker", "tag", "${defaultDockerImageName}", "${dockerJavaImageName}" | ||
| } | ||
| exec { | ||
| executable 'docker' | ||
| args = ['push', imageURL] | ||
| commandLine "gcloud", "docker", "--", "push", "${dockerJavaImageName}" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
:sdks:java:container:${javaVer}:dockerPush should handle this if gcloud auth configure-docker ... was executed before Gradle, right?
There was a problem hiding this comment.
I believe this command is the one that does the copying, resolving according to this logic:
def dockerImageRoot = project.findProperty('dockerImageRoot') ?: "us.gcr.io/${gcpProject.replaceAll(':', '/')}/java-postcommit-it"
So it will push a copy of the image from :sdks:java:container:...:dockerPush to the java-postcommit-it repository or the dockerImageRoot that has been overridden by GHA.
Or anyhow that's my understanding of this logic. I could be wrong, or the logic could also be excessive.
There was a problem hiding this comment.
Ah, I think some amount of this is to enable command line usage by someone who wants everything to run in a different GCP project. But I do think most or all of this logic should probably/eventually be owned by the :sdks:harness module. It may take some effort to figure out how much is necessary versus how much was just someone not realizing that the SDK harness is a thing that can and should exist fully independent of the :runners:google-cloud-dataflow-java project, and not require custom logic here to be utilized.
There was a problem hiding this comment.
There are at least 3 ways to push containers in different gradle scripts: :docker -Ppush-containers; :dockerPush and invoke docker push in a custom task. It could be a separate task to clean up all of them.
Current build.gradle invokes docker push directly at least it does not add more mess so lgtm
|
|
||
| // Push docker images to a container registry for use within tests. | ||
| // NB: Tasks which consume docker images from the registry should depend on this | ||
| // task directly ('dependsOn buildAndPushDockerJavaContainer'). This ensures the correct | ||
| // task ordering such that the registry doesn't get cleaned up prior to task completion. | ||
| def buildAndPushDockerJavaContainer = tasks.register("buildAndPushDockerJavaContainer") { | ||
| def buildAndPushDistrolessDockerJavaContainer = tasks.register("buildAndPushDistrolessDockerJavaContainer") { | ||
| def javaVer = getSupportedJavaVersion(project.findProperty('testJavaVersion') as String) | ||
|
|
||
| dependsOn ":sdks:java:container:${javaVer}:docker" | ||
| dependsOn ":sdks:java:container:distroless:${javaVer}:docker" | ||
| def defaultDockerImageName = containerImageName( | ||
| name: "${project.docker_image_default_repo_prefix}${javaVer}_sdk", | ||
| root: "apache", | ||
| tag: project.sdk_version) | ||
| name: "${project.docker_image_default_repo_prefix}${javaVer}_sdk_distroless", | ||
| root: "apache", | ||
| tag: project.sdk_version) | ||
| doLast { | ||
| exec { | ||
| commandLine "docker", "tag", "${defaultDockerImageName}", "${dockerJavaImageName}" | ||
| commandLine "docker", "tag", "${defaultDockerImageName}", "${dockerJavaDistrolessImageName}" | ||
| } | ||
| exec { | ||
| commandLine "gcloud", "docker", "--", "push", "${dockerJavaImageName}" | ||
| commandLine "gcloud", "docker", "--", "push", "${dockerJavaDistrolessImageName}" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Same here for :sdks:java:container:distroless:${javaVer}:dockerPush.
4a1186a to
83fff0f
Compare
3 participants
Previously, the Dataflow V2 tests rebuilt the distroless container using an overridden base image that was the
:latesttag from the live docker repo. Now it should use the version that was built from head, and the V1 and V2 logic are roughly matching.I did not refactor to make them share code, because I don't yet know exactly which parts of the shared code would be from logical necessity.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.