[0.11] vendor github.com/containerd/containerd v1.6.20 #3736
Conversation
|
@AkihiroSuda Could you take a look at what change is breaking the digest in the CI test. This shouldn't really break. If we test this then these digests should be actually stable between releases (unless we make an exception for security issues or obvious bugs etc.). If there is some more fundamental issue, then we need to reduce the test to only cover the stability that we can actually guarantee. If (arbitrary example), for example, containerd creates a different tar or different diff for the old parameters, then we need to fix this in containerd so that at least we can pass some parameters to get the previous behavior. I know that there is a differ issue with hardlinks between different differs giving a different result that we discussed in another thread. I think we need to consider this as a bug and track it down, not declare that it is normal for the digest to only be stable for a specific snapshotter. |
thaJeztah
force-pushed
the
0.11_containerd_1.6.20
branch
3 times, most recently
from
dba0ed0
to
fbd74a3
Compare
| const expectedDigest = "sha256:9e36395384d073e711102b13bd0ba4b779ef6afbaf5cadeb77fe77dba8967d1f" | ||
| // note that this digest differs from the one in master, due to | ||
| // commit a89f482dcb3428c0297f39474eebd7de15e4792a not being included | ||
| // in this branch. | ||
| const expectedDigest = "sha256:e26093cc8a7524089a1d0136457e6c09a34176e2b2efcf99ac471baa729c7dc9" |
There was a problem hiding this comment.
Added a comment here; we'll have to update this test if we were to decide to backport that commit;
|
One failure on containerd-rootless; 1713 That error / warning comes from Lines 380 to 384 in 252ae63 But looks like it's empty (?). Perhaps needs one of bcdad5d or e849b62 to ignore |
Related to #3401 |
Ah, thanks! Looks like it's green now. I'll update things once containerd 1.6.20 is released (and clean up the commit message etc) |
(see moby/buildkit#3736) Signed-off-by: Laura Brehm <laurabrehm@hey.com>
(see moby/buildkit#3736) Signed-off-by: Laura Brehm <laurabrehm@hey.com> (cherry picked from commit 5a1cc36) Signed-off-by: Laura Brehm <laurabrehm@hey.com>
(see moby/buildkit#3736) Signed-off-by: Laura Brehm <laurabrehm@hey.com>
(see moby/buildkit#3736) Signed-off-by: Laura Brehm <laurabrehm@hey.com>
(see moby/buildkit#3736) Signed-off-by: Laura Brehm <laurabrehm@hey.com>
- wcow: support graceful termination of servercore containers full diff: microsoft/hcsshim@v0.9.6...v0.9.8 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- server: Fix connection leak when receiving ECONNRESET full diff: containerd/ttrpc@v1.1.0...v1.1.1 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
full diff: opencontainers/runc@v1.1.3...v1.1.5 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
…1005185240-3a7f492d3f1b full diff: opencontainers/image-spec@02efb9a...3a7f492 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
full diff: containerd/containerd@v1.6.18...v1.6.20 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
0.11_containerd_1.6.20
branch
from
fbd74a3
to
5572c69
Compare
thaJeztah
changed the title
|
Rebased this one, and updated to containerd 1.6.20 |
|
Looks like 3 tests failed on rootless; I think these may the same as last time #3736 (comment) but I'll give it another run to see if it was a flaky |
|
Looks like it's green now; @AkihiroSuda @tonistiigi ptal |
|
If this completely changes the output that differ produces, then I don't think we can pick anything like this for a patch release. Why would they backport such a huge behavior change? |
|
Looking at; I guess previously it would already produce a different (unpredictable) sha, depending on what's on the host. I guess that wouldn't reproduce in CI as long as CI always had the same users in |
|
That is true. Although for most of images, it would have just been |
|
I guess a workaround for the "most common" case would be to hard-code "0:0" to "root:root" as only lookup, to "somewhat" keep the happy path the same. But for any other scenario it would already be horribly broken 😞 |
There was a problem hiding this comment.
This is way too big for my liking(especially because we already had a regression in a patch release) but I don't think we have another choice than to pick it.
@AkihiroSuda @jedevc Please verify as well.
There was a problem hiding this comment.
The digest changes from containerd/containerd@063ad2f do worry me a little bit - ideally we wouldn't take this change for v0.11. We've already had minor version bumps break users in v0.11 already 😢
Is the reason for this change that we have a different version of containerd in moby? If the issue is just the tests giving different digests in those tests, could we simply disable those tests for the time being?
That said, glancing through changelogs, nothing springs out as otherwise dangerous - I'll give a tentative approval.
It is not just the tests, but if there are separate dependencies in moby and buildkitd it could lead to different behavior and fragmentation. |
|
Opened moby/moby#45326 to test the latest changes in moby/moby |
|
@thaJeztah We should update the binary as well for consistency, right? |
|
Here in BuildKit CI? Yes, probably. |
|
Ugh.. just checked the Moby PR with this change, and looks like it's still failing 😞 with yet another digest now? No clue now what's happening. |
|
@thaJeztah Did this test ever pass in moby? |
docker diffmoby#44964 (comment)[0.11] vendor: github.com/Microsoft/hcsshim v0.9.8
full diff: microsoft/hcsshim@v0.9.6...v0.9.8
[0.11] vendor: github.com/containerd/ttrpc v1.1.1
full diff: containerd/ttrpc@v1.1.0...v1.1.1
[0.11] vendor: github.com/opencontainers/runc v1.1.5
full diff: opencontainers/runc@v1.1.3...v1.1.5
[0.11] vendor: github.com/opencontainers/image-spec v1.1.0-rc2.0.20221005185240-3a7f492d3f1b
full diff: opencontainers/image-spec@02efb9a...3a7f492
[0.11] vendor: github.com/containerd/containerd v1.6.20
full diff: containerd/containerd@v1.6.18...v1.6.20