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

Slow docker caching in CI #874

Open
bjorn3 opened this issue Oct 9, 2024 · 6 comments
Open

Slow docker caching in CI #874

bjorn3 opened this issue Oct 9, 2024 · 6 comments
Assignees
Labels
CI Meta-issues related to CI test-framework

Comments

@bjorn3
Copy link
Collaborator

bjorn3 commented Oct 9, 2024

The problem seems to be the fact that the built image including all layers is huge (~440MB). This is because building the docker container installs several big dependencies to build sudo-rs including clang and in any case start from a relatively large base image that contains rustc. While near the end clang is uninstalled again, this doesn't have any effect on the cache size as every file that existed in any layer ends up in the cache. And this can't be fixed without merging all layers into one, which would destroy caching entirely.

I propose to build sudo-rs outside of the container using the rustc and clang installed on the host and then copy the built binaries into an image based on debian:bookworm-slim which is also used for og-sudo. Rustc needs to be installed on the host anyway to even build the test framework. And I'm not certain clang is still necessary. It may have been a left over from when bindgen was used at compile time. Doing this all would significantly reduce the size of the docker image we need to cache in CI and thus make caching faster.

@squell squell added the CI Meta-issues related to CI label Oct 9, 2024
@squell
Copy link
Member

squell commented Oct 9, 2024

I need some context: which containers is this specifically talking about? (E.g. only compliance test related, or other steps as well?)

My two cents: we have a conservative MSRV so on the one hand sudo-rs of course should be buildable on the host, but we also have some steps where want to have the latest version (specifically Miri and clippy feel useful); and of course we have the "minimal version" check where we use the MSRV.

@rnijveld
Copy link
Collaborator

rnijveld commented Oct 9, 2024

The main reasoning was I believe that for local development purposes: having docker do the build allows it tot auto-update whenever you change something, and allows you to just do another call to cargo test in the test framework to run the tests again. There are ways to optimize the docker caching as well: make sure the base layer containing stuff like clang and rustc is already uploaded to wherever the caching takes place, that way the sync only needs to upload the layers created by the application, which most likely aren’t that large.

@bjorn3
Copy link
Collaborator Author

bjorn3 commented Oct 9, 2024

I need some context: which containers is this specifically talking about? (E.g. only compliance test related, or other steps as well?)

I'm talking about the compliance tests only. The rest doesn't use docker at all.

The main reasoning was I believe that for local development purposes: having docker do the build allows it tot auto-update whenever you change something, and allows you to just do another call to cargo test in the test framework to run the tests again.

The test framework can build sudo-rs right before it builds the docker image.

There are ways to optimize the docker caching as well: make sure the base layer containing stuff like clang and rustc is already uploaded to wherever the caching takes place, that way the sync only needs to upload the layers created by the application, which most likely aren’t that large.

That requires uploading them to a real registry, right? Currently CI just uploads the entire docker cache to a github actions cache every time.

@squell
Copy link
Member

squell commented Oct 9, 2024

For the compliance tests in CI, running them on the "host" Rustc for me feels like it has the further benefit that it's closer still to the tools used by our downstream packagers used for production builds (when the test framework was set up last year this wasn't an option since Debian hadn't caught up with 1.70 yet).

Of course we also need to be able to easily run the compliance tests locally (without having to manually build an image, upload it to the container, etc).

@japaric

@bjorn3 bjorn3 self-assigned this Oct 9, 2024
@bjorn3
Copy link
Collaborator Author

bjorn3 commented Oct 9, 2024

I've got a WIP branch at https://github.com/bjorn3/sudo-rs/tree/ci_changes2.

@rnijveld
Copy link
Collaborator

rnijveld commented Oct 10, 2024

The main reasoning was I believe that for local development purposes: having docker do the build allows it tot auto-update whenever you change something, and allows you to just do another call to cargo test in the test framework to run the tests again.

The test framework can build sudo-rs right before it builds the docker image.

Sounds like a good idea to me! One thing to keep in mind is to let the rust compiler target the same target as the container image (i.e. probably amd64-linux).

There are ways to optimize the docker caching as well: make sure the base layer containing stuff like clang and rustc is already uploaded to wherever the caching takes place, that way the sync only needs to upload the layers created by the application, which most likely aren’t that large.

That requires uploading them to a real registry, right? Currently CI just uploads the entire docker cache to a github actions cache every time.

That's true, it would probably be much easier to implement the above.

Maybe an additional thing we can look at is freebsd containers/jails, just so we can do the same tests for freebsd (given the linux docker container will never test any freebsd code).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Meta-issues related to CI test-framework
Projects
None yet
Development

No branches or pull requests

3 participants