-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Update to containerd 2.0 #4630
Update to containerd 2.0 #4630
Conversation
d3986e5
to
1f9aaf1
Compare
1f9aaf1
to
87a00b5
Compare
b10c43a
to
e31951e
Compare
This is a prerequisite to upgrading to containerd 2.x See containerd/typeurl#54 Signed-off-by: Marat Radchenko <[email protected]>
Signed-off-by: Akihiro Suda <[email protected]>
Signed-off-by: Akihiro Suda <[email protected]>
Signed-off-by: Marat Radchenko <[email protected]>
e31951e
to
47809ed
Compare
47809ed
to
b6a8c1f
Compare
client/client_test.go
Outdated
@@ -224,6 +225,7 @@ var allTests = []func(t *testing.T, sb integration.Sandbox){ | |||
} | |||
|
|||
func TestIntegration(t *testing.T) { | |||
t.Setenv("CONTAINERD_ENABLE_DEPRECATED_PULL_SCHEMA_1_IMAGE", "1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, rootless still fails. And if you want to set variable here, you want to remove my workaround from hack/test
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, will have to try something else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think I know what's the problem with rootless tests. Look at this code: https://github.com/moby/buildkit/blob/master/util/testutil/workers/containerd.go#L183-L190
I think it blocks propagation of env variables from parent process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Unless sudo
is called with -E
/--preserve-env
, it uses a brand-new environment. Note that I am not sure it is a good idea to use these flags. Possibly we should just add CONTAINERD_ENABLE_DEPRECATED_PULL_SCHEMA_1_IMAGE=1
to c.ExtraEnv
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add it there if we want to support it, it should be removed soon entirely anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need to support Schema 1 on our CI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question. There's only a single test that deliberately tests schema v1. But it's up to you, maintainers, to decide. As just a contributor, I am trying to go conservative route by preserving whatever functionality existed before my changes until explicit "yeah, burn it" is said :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a skip to see if it makes it go green. If it was up to me, I would vote to mark it as deprecated and remove the CI for it.
Signed-off-by: Derek McGowan <[email protected]>
b6a8c1f
to
d0432c0
Compare
This is testing against beta, opening now to see if there are any glaring issues we should consider before 2.0 final. We plan on stabilizing the Go interface in 2.0 so future versions shouldn't require many code changes that are included here.
Requires
#4558
containerd/stargz-snapshotter#1545
containerd/fuse-overlayfs-snapshotter#72