Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move docker build from build script to xtask #2127

Merged
merged 7 commits into from
Oct 27, 2023
Merged

Conversation

divergentdave
Copy link
Contributor

@divergentdave divergentdave commented Oct 13, 2023

This closes #2081, removing the build script that was calling docker build. A new xtask package is added, and its executable is aliased to run as a cargo subcommand. Now, cargo xtask test-docker will invoke docker buildx bake to build the same images, and pass the image IDs through environment variables to cargo test, rather than including image contents in the source. All tests that depend on the container images are put behind the testcontainer feature flag. Testing in CI invokes cargo test with the testcontainer feature via cargo xtask test-docker-with-images instead, using images loaded from a preceding CI job.

@divergentdave divergentdave marked this pull request as ready for review October 14, 2023 01:03
@divergentdave divergentdave requested a review from a team as a code owner October 14, 2023 01:03
Copy link
Contributor

@tgeoghegan tgeoghegan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned that we don't run cargo xtask test-docker anywhere in CI (unless I missed it). Maybe if test-docker were broken out into two subcommands, one for building the container images and one that uses them in a test, then the former could be used in the janus_interop_docker job and the latter could be used in janus_build?

Or if we ran cargo xtask test-docker in janus_build after loading the Docker images tarball emitted by janus_interop_docker, would docker buildx bake swiftly notice that the images it wants to build are already present in the local repository and move on, or would it insist on rebuilding them?

Comment on lines 184 to 193
- name: Upload artifact
uses: actions/upload-artifact@v3
Copy link
Contributor

@tgeoghegan tgeoghegan Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will upload the three docker images, which are 40ish MB apiece, on every PR build. I'm concerned about our utilization of artifacts storage. The GitHub docs discuss a 90 day default retention period for artifacts, but nothing about a maximum on storage. Maybe we should go ahead with this and see if we start getting angry emails about using up our storage quota?

Or maybe we should preemptively tighten the retention period down to maybe 1 day, since we expect the artifacts emitted by this job to be immediately consumed by the test job.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems easier to put #[cfg(feature = "testcontainer")] at the module level, since I don't think we ever intend to support testing Daphne outside of a container.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one test, janus_in_process_daphne, that can be run using an in-process Janus leader and a Daphne helper in a container. It's disabled for the time being until we can upgrade Daphne, but I'd like to keep it available with default features.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 🙌🏻

Copy link
Member

@branlwyd branlwyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still wary of moving tests out of the default cargo test workflow -- folks, especially newer contributors, are likely to encounter mysterious failures in CI that they didn't see locally because they didn't run the tests locally. But let's see how it goes; we can always point folks at the README.

.github/workflows/ci-build.yml Outdated Show resolved Hide resolved
@divergentdave
Copy link
Contributor Author

I'm concerned that we don't run cargo xtask test-docker anywhere in CI (unless I missed it). Maybe if test-docker were broken out into two subcommands, one for building the container images and one that uses them in a test, then the former could be used in the janus_interop_docker job and the latter could be used in janus_build?

Or if we ran cargo xtask test-docker in janus_build after loading the Docker images tarball emitted by janus_interop_docker, would docker buildx bake swiftly notice that the images it wants to build are already present in the local repository and move on, or would it insist on rebuilding them?

I added separate subcommands for just building container images and just running tests. I changed the CI to use cargo xtask test-docker-with-images on the consuming end. I think I'd prefer using docker/bake-action instead of cargo xtask build-docker on the producing end still, because that sets up the GitHub Actions caching backend for us, and I would still have to go through some other hoops to tag, save, and upload the images.

Getting rid of artifacts uploads/downloads and relying on layer caching might work, plus keeping the needs relationship between jobs would help with cache hit rates when things are rebuilt. I'll try that out in a bit.

@divergentdave divergentdave merged commit 416af2f into main Oct 27, 2023
8 checks passed
@divergentdave divergentdave deleted the david/xtask-tests branch October 27, 2023 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposed design for removing container image blobs
3 participants