-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
ci: fix docker cache hash collision #58549
Conversation
@bors r+ p=50 |
📌 Commit 9c2dc109fe192ef880b17ebc3b14c862707e9358 has been approved by |
Removed the backport nominations, stable and beta works fine with the old hashing code and we're not going to backport CI changes to beta 10 days before the released, or even more to stable. Avoiding the backport will save us another cycle of timeout->retry. |
⌛ Testing commit 9c2dc109fe192ef880b17ebc3b14c862707e9358 with merge 4744156ef241eecc5e41435b7e774718f6361b3f... |
💔 Test failed - checks-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors retry |
⌛ Testing commit 9c2dc109fe192ef880b17ebc3b14c862707e9358 with merge 79cb5f6e34b9ff36279632af05e8a5343d0fcacb... |
💔 Test failed - checks-travis |
💔 Test failed - checks-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Before this commit the hash used to cache docker images was calculated from the image's files and the files in the scripts/ directory. This worked fine when all the files used by an image were in those directories, but some images pull files from other images, causing hash collisions in some cases. This commit changes the hash to include the files of all the docker images, causing a rebuild of all the images when a single one changes. That's a bit heavy-handed, but we have no way to track which files an image pulls in and hash collisions are really painful to deal with.
9c2dc10
to
2b2045d
Compare
@bors r=Mark-Simulacrum |
📌 Commit 2b2045d has been approved by |
⌛ Testing commit 2b2045d with merge 97886206790a30050e82679c9779a802fc12db8f... |
💔 Test failed - checks-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors retry |
…crum ci: fix docker cache hash collision #58416 uncovered a bug in our caching for docker images: if the image `foo` pulls files from the image `bar` and a file in `bar` changed, the hash of `foo` will be the same even though it should be different. In that PR's case, `dist-i686-linux` pulls scripts from `dist-x86_64-linux`, and the PR only changed those scripts, causing an hash collision for `dist-i686-linux`. We have to fix this, since the image will be rebuilt every time bors switches from testing master to testing beta/stable (and when it switches back), making CI way more painful than it currently is. The approach used by this PR is to just include all the files in `src/ci/docker` in the hash. It's a bit heavy-handed and it will cause a rebuild of all the images every time a single image changes, but it's the best I can think of. r? @Mark-Simulacrum cc @alexcrichton @kennytm
☀️ Test successful - checks-travis, status-appveyor |
…r=pietroalbini Calculate Docker cache hash precisely from Dockerfile's dependencies rust-lang#58549 changed the Docker cache calculation to include every file under `src/ci/docker`, so that when files under `dist-x86_64-linux` is changed, its dependent image `dist-i686-linux` will also be rebuilt. However, this ultraconservative solution caused the `dist-i686-linux` to be rebuilt every time an irrelevant Dockerfile (e.g. the PowerPC ones) is changed, which increases the building time beyond 3 hours and forcing a spurious but expected failure. This commit instead parses the Dockerfile itself and look for the actual dependencies. The scripts needs to be copied into the Docker image, which must be done with the COPY command, so we just need to find all lines with a COPY command and add the source file into the hash calculator. Note: this script only handles single-lined COPY command in the form `COPY src1 src2 src3 dst`, since these are the only variant used inside this repository.
#58416 uncovered a bug in our caching for docker images: if the image
foo
pulls files from the imagebar
and a file inbar
changed, the hash offoo
will be the same even though it should be different. In that PR's case,dist-i686-linux
pulls scripts fromdist-x86_64-linux
, and the PR only changed those scripts, causing an hash collision fordist-i686-linux
.We have to fix this, since the image will be rebuilt every time bors switches from testing master to testing beta/stable (and when it switches back), making CI way more painful than it currently is.
The approach used by this PR is to just include all the files in
src/ci/docker
in the hash. It's a bit heavy-handed and it will cause a rebuild of all the images every time a single image changes, but it's the best I can think of.r? @Mark-Simulacrum
cc @alexcrichton @kennytm