solver: correctly set the content selector with multiple bind mounts references #4270
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
jsternberg
force-pushed
the
invalid-caching-multiple-bind-mounts
branch
from
3b7fc6c to
ba6fd05
Compare
jsternberg
changed the title
jsternberg
force-pushed
the
invalid-caching-multiple-bind-mounts
branch
from
ba6fd05 to
b3fc6fa
Compare
…references Correctly set the content based selector when multiple bind mounts refer to the same source. Previously, a selector that referred to the root filesystem would be ignored. This is because a blank selector refers to the root filesystem. When two bind mounts referred to the same dependency, one mount would add a selector while the other would be skipped. This caused the cache key to be only computed based on the more narrow filesystem which caused erroneous cache hits. Now, the creation of the selector includes the root filesystem for consideration. It fills in `/` as the selector and then removes it later so that we don't narrow the selection in an invalid way. Signed-off-by: Jonathan A. Sternberg <[email protected]>
jsternberg
force-pushed
the
invalid-caching-multiple-bind-mounts
branch
from
b3fc6fa to
dc60842
Compare
tonistiigi
approved these changes
Sep 25, 2023
jsternberg
added a commit
to jsternberg/buildkit
that referenced
this pull request
Sep 26, 2023
This is a follow up to moby#4270 which adds an integration test for the fixed behavior. Signed-off-by: Jonathan A. Sternberg <[email protected]>
jsternberg
added a commit
to jsternberg/buildkit
that referenced
this pull request
Sep 27, 2023
This updates moby#4270 to add an integration test and also merge some of the logic for how the selectors are created. Now, `toSelectors` will perform the root path detection instead of some custom logic in `getMountDeps`. `dedupePaths` has also been updated to check if the number of paths is 1 or less so it can avoid an allocation when the function is a no-op. Signed-off-by: Jonathan A. Sternberg <[email protected]>
jsternberg
added a commit
to jsternberg/buildkit
that referenced
this pull request
Sep 27, 2023
This updates moby#4270 to add an integration test and also merge some of the logic for how the selectors are created. Now, `toSelectors` will perform the root path detection instead of some custom logic in `getMountDeps`. `dedupePaths` has also been updated to check if the number of paths is 1 or less so it can avoid an allocation when the function is a no-op. Signed-off-by: Jonathan A. Sternberg <[email protected]>
jsternberg
added a commit
to jsternberg/buildkit
that referenced
this pull request
Nov 15, 2023
This updates moby#4270 to add an integration test and also merge some of the logic for how the selectors are created. Now, `toSelectors` will perform the root path detection instead of some custom logic in `getMountDeps`. `dedupePaths` has also been updated to check if the number of paths is 1 or less so it can avoid an allocation when the function is a no-op. Signed-off-by: Jonathan A. Sternberg <[email protected]>
jsternberg
added a commit
to jsternberg/buildkit
that referenced
this pull request
Dec 4, 2023
This updates moby#4270 to add an integration test and also merge some of the logic for how the selectors are created. Now, `toSelectors` will perform the root path detection instead of some custom logic in `getMountDeps`. `dedupePaths` has also been updated to check if the number of paths is 1 or less so it can avoid an allocation when the function is a no-op. Signed-off-by: Jonathan A. Sternberg <[email protected]>
jsternberg
added a commit
to jsternberg/buildkit
that referenced
this pull request
Dec 4, 2023
This updates moby#4270 to add an integration test and also merge some of the logic for how the selectors are created. Now, `toSelectors` will perform the root path detection instead of some custom logic in `getMountDeps`. `dedupePaths` has also been updated to check if the number of paths is 1 or less so it can avoid an allocation when the function is a no-op. Signed-off-by: Jonathan A. Sternberg <[email protected]>
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.
Correctly set the content based selector when multiple bind mounts refer
to the same source. Previously, a selector that referred to the root
filesystem would be ignored. This is because a blank selector refers to
the root filesystem.
When two bind mounts referred to the same dependency, one mount would
add a selector while the other would be skipped. This caused the cache
key to be only computed based on the more narrow filesystem which caused
erroneous cache hits.
Now, the creation of the selector includes the root filesystem for
consideration. It fills in
/as the selector and then removes it laterso that we don't narrow the selection in an invalid way.
Fixes #4123.