Update containerd dependency #3782
Conversation
gabriel-samfira
force-pushed
the
update-containerd
branch
from
1314c22
to
6d7ef59
Compare
| @@ -6562,7 +6562,7 @@ FROM scratch | |||
| COPY --from=0 / / | |||
| `) | |||
|
|
|||
| const expectedDigest = "sha256:14b7856c37777ad7e8f4a801cb98abec98c5ae0f4a8dbb152447d18f1c1a3ba9" | |||
| const expectedDigest = "sha256:d286483eccf4d57c313a3f389cdc196e668d914d319c574b15aabdf1963c5eeb" | |||
There was a problem hiding this comment.
Apparently, that digest may change based on snapshotter configuration and other variables.
There was a problem hiding this comment.
previous run resulted in this: https://github.com/moby/buildkit/actions/runs/4638010045/jobs/8207529124#step:7:1814
There was a problem hiding this comment.
Which may have happened because we update containerd. Seems like last time this was changed was when containerd was updated to 1.7.0-rc3: 98deacf#diff-93731187c2a292b7e92b852225675f88b41b7634dafd66cfeab50697bd806ddbR6561
There was a problem hiding this comment.
Still curious to know which commit introduced this change
There was a problem hiding this comment.
😭
As long as we have this test we need to find the exact cause of these changes.
If this breaks with every containerd release we need to consider forking the differ and starting to maintain our own one that we keep stable(or define "differ compatibility version" that can be set by user).
There was a problem hiding this comment.
It seems that the difference is due to the fact that the resulting OCI tarball no longer has a manifest.json inside of it.
With the current master branch, it seems we still have a manifest.json even if we explicitly specify we want an oci image, which should skip adding the docker manifest. This seems to have been fixed in containerd/containerd#8213. A fix which this update pulls in.
So it seems this update also fixes a bug 😄.
Apologies for the late reply on this. I have been traveling for the paste couple of weeks.
There was a problem hiding this comment.
Here are the contents of the master branch vs this branch (the blobs folder is omitted):
The master branch:
root@buildkit:~/test/b# sha256sum $HOME/test/a/ex/*
3fc4c7275c73ef20e8b5756d54e0e4dbd12b8df91505410c5e9ff55957c06007 /root/test/a/ex/index.json
4be49e4e12d735ac51e8acd43a62ca3c2135f66d86f1f6b419aae17c8d507b28 /root/test/a/ex/manifest.json
18f0797eab35a4597c1e9624aa4f15fd91f6254e5538c1e0d193b2a95dd4acc6 /root/test/a/ex/oci-layoutThis branch:
root@buildkit:~/test/b# sha256sum $HOME/test/b/ex/*
3fc4c7275c73ef20e8b5756d54e0e4dbd12b8df91505410c5e9ff55957c06007 /root/test/b/ex/index.json
18f0797eab35a4597c1e9624aa4f15fd91f6254e5538c1e0d193b2a95dd4acc6 /root/test/b/ex/oci-layout
gabriel-samfira
force-pushed
the
update-containerd
branch
from
6d7ef59
to
e20069b
Compare
|
Rebased on top of master and resolved conflicts. |
|
Not sure if the failure is related. Seems to happen on |
|
Any chance we could merge this? It would unblock a few other PRs 😄 |
|
@tonistiigi Should this be in v0.13 or in v0.12? containerd v2.0 isn't likely going to be released in the next couple of months, and I guess we don't want to use non-tagged commit of containerd in a GA version of BuildKit? |
|
@AkihiroSuda Good call. @gabriel-samfira If you need a specific commit is it possible it could be added to 1.7 branch. |
The needed commits (30) were added by this PR: containerd/containerd#8043 It is unlikely they will be backported to the stable 1.7 branch. 😄 Do we need to wait for the next stable release before updating the dependency, or is there a point during the development cycle of buildkit when we can update to non tagged commit of containerd? Edit: this update is needed for a few other PRs to actually build and for buildkit to work on Windows. But this PR does not necessarily have to be a part of 0.12. It can wait for the next version. There are a few more PRs that need to merge before buildkit actually starts working on windows. So no rush yet. |
Updating non-tagged is fine during dev but might be problematic for releases. Atm. it looks like we would want to release v0.12 buildkit before containerd tags a next release. Otoh it would be nice if we could get some of this work into v0.12 so it can be tested early. |
|
Shall we merge this into master for v0.13, and create a new branch for v0.12? |
|
I am ok with whatever you folks decide here. I would love to be able to convert some branches from draft to ready for review as soon as this merges. In the meantime I will focus on the branches that don't depend on this. |
|
I think we need ~3 more weeks before we can cut v0.12 . We can create a feature branch for you if you like until that. cc @jedevc |
That's ok. I think we can focus on some of the other PRs that don't require this until v0.12 is cut, if you folks have some cycles. There are 2 in particular that do not depend in this (the handle paths on Windows one and the add windows service support). I can rebase the rest and wait for v0.12 to be cut. |
|
On other thing to take into account is that the next containerd release from their main branch will be v2.0.0. Given that the v1.7 branch has been cut, that means that the main branch should be renamed to technically that would also mean that BuildKit could consume (parts of) containerd side-by-side with v1.x (so if the code needed is in isolated parts of BuildKit; those parts could use the v2 dependency. Not sure if that'd work though 😅 I do share concerns about using a non-released version; more so because it currently has the wrong module name, but also because we can't easily update if there's (security) fixes without fast-forwarding to "head" of upstream main |
This will be fun 🥲 .
I agree. Was thinking that it may be ok at the beginning of the development cycle for the next release, updating to the latest stable containerd tag before we cut a new release of buildkit. But given the impending transition, I think you folks will need to decide how the update to containerd 2.0 will proceed. If we go with the feature branch approach, what would be the process of merging it into |
|
Did a quick PR to see how hard containerd explodes when renamed to v2, but it's gonna be a lot of fun, as there's circular dependencies, so all other repositories in the containerd org probably need to bump to v2 as well (and be renamed); containerd/containerd#8478 |
This will most likely get easier as the containerd project consolidates versions across packages and cuts proper tags for the new versions. I don't think the API will change in any significant way. The current HEAD works with buildkit. That being said, if updating to containerd 2.0 will mean waiting a significant amount of time, I think a feature branch would be desirable in order to move the Windows enablement forward. Getting that work reviewed and merged into a feature branch which can then be merged into master once everything is in place with containerd 2.0 would be great. |
|
@tonistiigi Would it be possible to create the feature branch for the windows work? I could re-target some of the current PRs against that branch and keep it in sync with master. As a side note, I don't think containerd 2.0 will break things, as the current HEAD still works well with buildkitd. We will most likely be able to switch to 2.0 in future stable releases, at which point, the Windows work will be able to merge into master easily. A feature branch would be nice until that point. It would allow people to test things out more easily. |
|
The needed commits have been backported to containerd 1.7 and will probably be available in the next point release. I will update this PR when that happens. |
This update includes a number of improvements needed for native Windows support, as well as some bugfixes. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
gabriel-samfira
force-pushed
the
update-containerd
branch
from
e20069b
to
2c09695
Compare
|
I've updated this PR to target the new stable release of containerd (v1.7.2). The latest version includes the needed commits. |
Updates containerd to the latest stable release. This release includes fixes and features that are needed to enable native Windows support in buildkit.