solver: Release unused refs in LoadWithParents #3815
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
Before this any refs that were loaded but not put into results would be leaked. Signed-off-by: Erik Sipsma <erik@sipsma.dev>
sipsma
force-pushed
the
release-loaded-refs
branch
from
82d4a05
to
14b0e9d
Compare
tonistiigi
reviewed
Apr 25, 2023
| @@ -217,6 +217,11 @@ func (c *cacheManager) LoadWithParents(ctx context.Context, rec *CacheRecord) ([ | |||
| r.Release(context.TODO()) | |||
| } | |||
| } | |||
| for _, r := range m { | |||
There was a problem hiding this comment.
Shouldn't this be in the defer of filterResults?
There was a problem hiding this comment.
filterResults is recursive so it seemed complicated to try to update it relative to just emptying it out here. Figured it made sense still since m isn't constructed by filterResults
tonistiigi
approved these changes
Apr 25, 2023
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.
Before this any refs that were loaded but not put into results would be leaked.
cc @tonistiigi noticed in certain load tests I was running w/ Dagger that cache refs were leaking, this fixes it. I am still trying to understand the exact circumstances it arises in.I know for sure it only happens when using a cacheManager constructed from a remote cache import. And I also think it might happen when there are multiple results that end up with the same layers (i.e. two image refs resolve to the same set of layers). However, the context in which this was triggered involved cache manifest json constructed by custom code, not by the normal buildkit code, so I don't know if it's possible to trigger this using a cache manifest generated by buildkit upstream.Either way, I think this fix is desirable upstream since we should never leave refs unreleased anyways.
EDIT: @tonistiigi I was able to reproduce this same bug purely with Buildkit, see the newly added integration test, which fails without this change present.
The problem happens when a remote cache import has multiple records with the same layers. In this case the resultID is calculated as a hash of the layer digests, so both records are associated with the same resultID. This causes them both to be loaded in
LoadWithParentsin the remote cache result storage.filterResultsthen only chooses one of them to use for the returnedresultslist, but it never releases the refs for the others it didn't end up choosing.