ExecOp: update content-cache mount logic #4624
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tonistiigi
reviewed
Feb 6, 2024
This refactors the content-based cache to be that little bit tidier. In
addition to adding comments that explain *why* we're even bothering,
this restructures the code to avoid being unclear.
To explain the changes in a little more detail (since it's not
abundantly clear why this translation is valid), the initial condition
looks like:
if (!m.Readonly || m.Dest == pb.RootMount) && m.Output != -1 {
deps[m.Input].NoContentBasedHash = true
We can apply De Morgan's law recursively to invert the condition and the
result:
deps[m.Input].NoContentBasedHash = true
if (m.Readonly && m.Dest != pb.RootMount) || m.Output == -1 {
deps[m.Input].NoContentBasedHash = false
With all the juggling of NoContentBasedCache, we invert the variable
name to be ContentBasedCache (and invert everywhere it's used as well):
if (m.Readonly && m.Dest != pb.RootMount) || m.Output == -1 {
deps[m.Input].ContentBasedHash = true
This reads a bit easier, but now we split this into two separate
branches for readability (and so we can comment each one in more detail
separately):
if m.Readonly && m.Dest != pb.RootMount {
deps[m.Input].ContentBasedHash = true
}
if m.Output == -1 {
deps[m.Input].ContentBasedHash = true
}
While this has been the behavior for ages, I think it makes sense to
deliberately this behavior slightly. It doesn't make sense to me that we
should only disallow read-only root mounts, but no-output root mounts
are allowed - there's no reason these shouldn't behave identically, by
splitting these out:
if m.Readonly {
deps[m.Input].ContentBasedHash = true
}
if m.Output == -1 {
deps[m.Input].ContentBasedHash = true
}
if m.Dest == pb.RootMount {
deps[m.Input].ContentBasedHash = false
}
There is a small chance that this is a breaking change for some users,
however, 1. SkipOutput (-1) is very rare and not often used in the wild
(except for dockerfiles, where it's only used for non-root mounts), and 2.
will only cause a cache miss.
Signed-off-by: Justin Chadwell <me@jedevc.com>
These mounts are actually safe, as suggested by Erik on slack: > Is it correct that this wouldn’t be a problem in the case where the > selector of the mount is just “/“? Because then there’s no “hidden” > files. > > If so, maybe there’s a path to enabling content cache for rw mounts > that are from “/“. Then you could also get the same end behavior by > not using selectors and instead always copying the subdir you want to > mount to scratch. This patch adds a check for this case, and explicitly enables content-based cache for these cases. Co-authored-by: Erik Sipsma <erik@sipsma.dev> Signed-off-by: Justin Chadwell <me@jedevc.com>
This allows LLB-directed content-based cache enablement for each mount. Some mounts may not be explicitly unabled (because it would be unsafe) - for these cases we explicitly error out. Signed-off-by: Justin Chadwell <me@jedevc.com>
These test all of the new behavior: - Checks for old default no content cache - Checks for old read-only and no-output allowed content cache - Checks for new root selector allowed content cache - Checks for new caller options that allow enabling/disabling it Signed-off-by: Justin Chadwell <me@jedevc.com>
jedevc
force-pushed
the
control-mount-content-cache
branch
from
fa2435c
to
ed2efe3
Compare
I think this should do the job? https://github.com/jedevc/buildkit/blob/control-mount-content-cache/solver/pb/caps.go#L60 |
tonistiigi
approved these changes
Feb 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See discussion in Slack: https://dockercommunity.slack.com/archives/C7S7A40MP/p1706711391769259
This is split into 4 commits for readability:
exec: refactor content-based cache detectionexec: allow content-cache for root selected mounts(with @sipsma for coming up with the idea)exec: allow caller-controlled content-based cachetest: add new content-cache exec mount tests