-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fix infinite loop in cache loop detection algorithm #6082
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
base: master
Are you sure you want to change the base?
Conversation
For reference this is a sample of the log I added to the checkLoops function during the infinite loop
|
Signed-off-by: Israel Barbosa <[email protected]>
I don't think that's really the issue. |
How about: diff --git a/cache/remotecache/v1/utils.go b/cache/remotecache/v1/utils.go
index cf014913e..d2e3de0bf 100644
--- a/cache/remotecache/v1/utils.go
+++ b/cache/remotecache/v1/utils.go
@@ -134,13 +134,21 @@ func (s *normalizeState) removeLoops(ctx context.Context) {
}
visited := map[digest.Digest]struct{}{}
+ processed := map[digest.Digest]struct{}{}
for _, d := range roots {
- s.checkLoops(ctx, d, visited)
+ s.checkLoops(ctx, d, visited, processed)
}
}
-func (s *normalizeState) checkLoops(ctx context.Context, d digest.Digest, visited map[digest.Digest]struct{}) {
+func (s *normalizeState) checkLoops(ctx context.Context, d digest.Digest, visited map[digest.Digest]struct{}, processed map[digest.Digest]struct{}) {
+ if _, ok := processed[d]; ok {
+ return
+ }
+ defer func() {
+ processed[d] = struct{}{}
+ }()
+
it, ok := s.byKey[d]
if !ok {
return
@@ -166,7 +174,7 @@ func (s *normalizeState) checkLoops(ctx context.Context, d digest.Digest, visite
}
delete(links[l], id)
} else {
- s.checkLoops(ctx, id, visited)
+ s.checkLoops(ctx, id, visited, processed)
}
}
} That should avoid it ever re-processing the same tree over and over again when reaching it from different ends, hopefully reducing the amount of processing needed. |
@israelglar Can you see if #6129 fixes the issue for you? This whole code should be removed there. |
@tonistiigi these are the results with the new remote cache. It fixes the issue 🥳
|
fixes #6008
I was able to repeatedly have the problem with my current project, but I can't share it's code and I'm not really sure what part of it is causing it so unfortunately I can't create a repeatable environment.
But since I had the issue, after some testing and reading other people's comments I found that the issue was in cache loops removal part of the code.
The already visited digests were being removed from the visited list