Fix flightcontrol test and usage bugs#2273
Conversation
febcbb7 to
ab62668
Compare
| return nil, err | ||
| } | ||
| res, err := s.g.Do(ctx, "cachemap", func(ctx context.Context) (ret interface{}, retErr error) { | ||
| res, err := s.g.Do(ctx, fmt.Sprintf("cachemap-%d", index), func(ctx context.Context) (ret interface{}, retErr error) { |
There was a problem hiding this comment.
That looks pretty bad. I wonder if it might have caused some cache misses. We should pick this commit to v0.9/
There was a problem hiding this comment.
This code looks like it's existed since v0.3, so I guess it's managed to exist for a while without creating too many noticeable issues. Let me know if you want me to open the backport PR though, should it only be to v0.9 or also any versions before that?
| if err := ensureCompression(ctx, sr, desc, compressionType, s); err != nil { | ||
| return nil, err | ||
| } | ||
| refInfo := sr.Info() |
There was a problem hiding this comment.
This doesn't look quite equivalent. Previously if differ was in progress, !createIfNeeded would still wait for completion. And !forceCompression should not call differ twice, even in mediatype is different.
There was a problem hiding this comment.
And !forceCompression should not call differ twice, even in mediatype is different
Fixed
Previously if differ was in progress, !createIfNeeded would still wait for completion
Agree that used to be the behavior, but was it on purpose? It seems strange to me for the behavior of createIfNeeded to be dependent on the value of createIfNeeded in other builds that happen to be running at the same time. If a user sets createIfNeeded to false, I feel like it makes the most sense to just never block on the differ running, especially since it's related to timing and thus never a deterministic thing anyways.
Let me know if I'm missing something, I can make it behave in the old way but it will convolute the code a bit more.
d913dde to
22468dc
Compare
|
I added a unit test for GetRemote to the commit w/ changes to computeBlobChain (apparently didn't have coverage before). After pushing the PR I see it keeps failing in CI only on Windows. It doesn't appear related to my change at all, it seems to just be because it's hitting a codepath previously never hit (it gets So I updated it to just skip that test on Windows. Let me know if that's alright for now. |
Previously, the flightcontrol group was being given a key just set to the ref's ID, which meant that concurrent calls using different values of compressionType, createIfNeeded and forceCompression would incorrectly be de-duplicated. The change here splits up the flightcontrol group into a few separate calls and ensures that all the correct input variables are put into the flightcontrol keys. Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
The test was making an assertion that is no longer expected to always be true after moby#2195, which purposely made flightcontrol less deterministic. This lead to occasional failures. Signed-off-by: Erik Sipsma <erik@sipsma.dev>
|
@tonistiigi let me know what you think of the updates |
tonistiigi
left a comment
There was a problem hiding this comment.
Please verify this also works well with #2246 , whichever gets merged first.
Yep, at first glance it looks conceptually compatible even if the merge conflicts will be slightly ugly. I also left a note on that other PR to update the new |
|
I think for v0.9 let's take only the cachemap (and maybe pull) fix. |
Fixes #2270
@tonistiigi The possible bug involving the executor host's file turned out to not be an issue because in that code branch the
hostnamevar was in practice always set todefaultHostname. So this primarily fixes the bug incomputeBlobchainand a few other smaller possible ones.