update containerd to v2.0.1 #3828

AkihiroSuda · 2024-12-20T09:31:53Z

Fix #3768

AkihiroSuda · 2024-12-20T10:28:39Z

/retest

stmcginnis · 2024-12-20T12:32:47Z

I plan on doing some more local testing, but this looks good to me and CI is happy.

/lgtm

BenTheElder · 2025-01-07T19:42:32Z

pkg/cluster/nodeutils/util.go

@@ -82,7 +83,9 @@ func LoadImageArchive(n nodes.Node, image io.Reader) error {
 	if err != nil {
 		return err
 	}
-	cmd := n.Command("ctr", "--namespace=k8s.io", "images", "import", "--all-platforms", "--digests", "--snapshotter="+snapshotter, "-").SetStdin(image)
+	// --local=true: Disables the Transfer API mode that is enabled by default since containerd v2.0.
+	// Workaround for `ctr: rpc error: code = InvalidArgument desc = unable to initialize unpacker: no unpack platforms defined: invalid argument`


We probably need a long term solution for this?

Also: #3805 (Attempts to set --local=false instead of --local=true to resolve a different problem)

Opened an issue:

[Transfer API] ctr images import --all-platforms fails since v2.0: unable to initialize unpacker: no unpack platforms defined: invalid argument containerd/containerd#11228

PR

ctr: ctr images import --all-platforms: fix unpack containerd/containerd#11229

BenTheElder · 2025-01-07T19:43:35Z

pkg/cluster/nodeutils/util.go

+	}
+	var snapshotter string
+	switch configVersion {
+	case 2: // Introduced in containerd v1.3. Still supported in containerd v2.


we probably need to allow user supplied config patches to similarly target 2 vs 3 separately, though that doesn't need to be done in this PR

do we depend on the version of the config?

kind/pkg/cluster/internal/create/actions/config/config.go

Lines 119 to 146 in 90d9439

if len(ctx.Config.ContainerdConfigPatches) > 0 || len(ctx.Config.ContainerdConfigPatchesJSON6902) > 0 {

fns := make([]func() error, len(kubeNodes))

for i, node := range kubeNodes {

node := node // capture loop variable

fns[i] = func() error {

// read and patch the config

const containerdConfigPath = "/etc/containerd/config.toml"

var buff bytes.Buffer

if err := node.Command("cat", containerdConfigPath).SetStdout(&buff).Run(); err != nil {

return errors.Wrap(err, "failed to read containerd config from node")

}

patched, err := patch.TOML(buff.String(), ctx.Config.ContainerdConfigPatches, ctx.Config.ContainerdConfigPatchesJSON6902)

if err != nil {

return errors.Wrap(err, "failed to patch containerd config")

}

if err := nodeutils.WriteFile(node, containerdConfigPath, patched); err != nil {

return errors.Wrap(err, "failed to write patched containerd config")

}

// restart containerd now that we've re-configured it

// skip if containerd is not running

if err := node.Command("bash", "-c", `! pgrep --exact containerd || systemctl restart containerd`).Run(); err != nil {

return errors.Wrap(err, "failed to restart containerd after patching config")

}

return nil

}

}

if err := errors.UntilErrorConcurrent(fns); err != nil {

return err

do we depend on the version of the config?

user's patches will not work as intended, they will wind up merging in keys that are not valid for v3 because all the keys have new namespacing again.

(and if they simply switch to v3 keys, it will not work with older kind images)

ah, so you refer to the "existing" user patches, I think is a matter of how you see it then ... for me those are containerd specific patches, and yes, users need to adapt them to containerd 2.0 unless there is some containerd feature that allows to migrate between formats

for me those are containerd specific patches, and yes, users need to adapt them to containerd 2.0 unless there is some containerd feature that allows to migrate between formats

But then their config won't work for older kind releases/images.

The way we do this with kubeadm config patches is you can have multiple patches that are targeting different kubeadm API versions. So far that's been irrelevant because for nearly all of our history we've only had containerd 2.0 config.

But actually, that is for a future PR, because in this PR we're still using a v2.0 config (images/base/files/etc/containerd/config.toml), this part of the diff seems to be technically unecessary for now.

I see, I got it now, so we keep using containerd 2.0 version for the config and let containerd deal with the translation internall to version 3 https://github.com/containerd/containerd/blob/main/docs/cri/config.md#config-versions

Yeah, before we switch our config to v3 we should give users a way to deal with it smoothly, but we're not actually switching to v3 in this PR, even though this PR adds partial support for v3.

aojea · 2025-01-07T21:52:38Z

/lgtm

I'm positively surprised the diff is so small ... IIRC we already had issues with ctr and storage formats in the past

BenTheElder · 2025-01-07T22:05:51Z

I'm positively surprised the diff is so small ... IIRC we already had issues with ctr and storage formats in the past

I don't think this addresses all of the issues, and since CRI only has v1 and not v1alpha2 in containerd v2, we break kubernetes < 1.26 (which is probably OK, but worth noting, we have not actively broken an old k8s version for some time now).

AkihiroSuda · 2025-01-07T22:09:35Z

since CRI only has v1 and not v1alpha2 in containerd v2, we break kubernetes < 1.26

CRI v1 seems introduced in Kubernetes 1.20: kubernetes/kubernetes@9fcede9

aojea · 2025-01-07T22:12:23Z

and since CRI only has v1 and not v1alpha2 in containerd v2, we break kubernetes < 1.26 (which is probably OK, but worth noting, we have not actively broken an old k8s version for some time now).

v1 was added in 1.20 kubernetes/kubernetes#96387
it was made default in 1.23 kubernetes/kubernetes#106501
In 1.26 v1alpha2 was dropped kubernetes/kubernetes#110618, but we should support until 1.20 IIUIC

BenTheElder · 2025-01-07T23:04:16Z

CRI v1 seems introduced in Kubernetes 1.20: kubernetes/kubernetes@9fcede9

ACK, definitely no problem with that.

We will still need to make sure to note this for the release, and we should consider if this is sufficient excuse to drop other < 1.20 bits from the project. We've been very lenient about dropping support outright so far.

And re: other parts: #3828 (comment)

BenTheElder · 2025-01-07T23:10:09Z

At some point <1.19 will be widely unusable because of cgroups v2 anyhow.

Fix issue 3768 Signed-off-by: Akihiro Suda <[email protected]>

k8s-ci-robot · 2025-01-08T01:35:44Z

New changes are detected. LGTM label has been removed.

k8s-ci-robot · 2025-01-08T01:35:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AkihiroSuda
Once this PR has been reviewed and has the lgtm label, please ask for approval from aojea. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2025-01-08T02:19:46Z

@AkihiroSuda: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kind-conformance-parallel-ipv6	`ccd6c2a`	link	true	`/test pull-kind-conformance-parallel-ipv6`
pull-kind-e2e-kubernetes-1-28	`ccd6c2a`	link	true	`/test pull-kind-e2e-kubernetes-1-28`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 20, 2024

k8s-ci-robot requested review from aojea and stmcginnis December 20, 2024 09:32

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 20, 2024

k8s-ci-robot assigned stmcginnis Dec 20, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 20, 2024

AkihiroSuda force-pushed the containerd-v2 branch from 560c251 to b5b1ff6 Compare December 20, 2024 13:11

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 20, 2024

BenTheElder reviewed Jan 7, 2025

View reviewed changes

k8s-ci-robot assigned aojea Jan 7, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 7, 2025

This was referenced Jan 7, 2025

Use --local=false on amd64, to workaround containerd image loading #3805

Open

Unable to load docker desktop containerd managed images to cluster #3795

Open

update containerd to v2.0.1

ccd6c2a

Fix issue 3768 Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda force-pushed the containerd-v2 branch from b5b1ff6 to ccd6c2a Compare January 8, 2025 01:35

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 8, 2025

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 8, 2025

AkihiroSuda mentioned this pull request Jan 8, 2025

ctr: ctr images import --all-platforms: fix unpack containerd/containerd#11229

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update containerd to v2.0.1 #3828

update containerd to v2.0.1 #3828

AkihiroSuda commented Dec 20, 2024 •

edited

Loading

AkihiroSuda commented Dec 20, 2024

stmcginnis commented Dec 20, 2024

BenTheElder Jan 7, 2025

BenTheElder Jan 7, 2025 •

edited

Loading

AkihiroSuda Jan 8, 2025

AkihiroSuda Jan 8, 2025

BenTheElder Jan 7, 2025

aojea Jan 7, 2025

BenTheElder Jan 7, 2025

BenTheElder Jan 7, 2025

aojea Jan 7, 2025

BenTheElder Jan 7, 2025

BenTheElder Jan 7, 2025

aojea Jan 7, 2025 •

edited

Loading

BenTheElder Jan 7, 2025

aojea commented Jan 7, 2025

BenTheElder commented Jan 7, 2025

AkihiroSuda commented Jan 7, 2025

aojea commented Jan 7, 2025

BenTheElder commented Jan 7, 2025

BenTheElder commented Jan 7, 2025 •

edited

Loading

k8s-ci-robot commented Jan 8, 2025

k8s-ci-robot commented Jan 8, 2025

k8s-ci-robot commented Jan 8, 2025

	if len(ctx.Config.ContainerdConfigPatches) > 0 \|\| len(ctx.Config.ContainerdConfigPatchesJSON6902) > 0 {
	fns := make([]func() error, len(kubeNodes))
	for i, node := range kubeNodes {
	node := node // capture loop variable
	fns[i] = func() error {
	// read and patch the config
	const containerdConfigPath = "/etc/containerd/config.toml"
	var buff bytes.Buffer
	if err := node.Command("cat", containerdConfigPath).SetStdout(&buff).Run(); err != nil {
	return errors.Wrap(err, "failed to read containerd config from node")
	}
	patched, err := patch.TOML(buff.String(), ctx.Config.ContainerdConfigPatches, ctx.Config.ContainerdConfigPatchesJSON6902)
	if err != nil {
	return errors.Wrap(err, "failed to patch containerd config")
	}
	if err := nodeutils.WriteFile(node, containerdConfigPath, patched); err != nil {
	return errors.Wrap(err, "failed to write patched containerd config")
	}
	// restart containerd now that we've re-configured it
	// skip if containerd is not running
	if err := node.Command("bash", "-c", `! pgrep --exact containerd \|\| systemctl restart containerd`).Run(); err != nil {
	return errors.Wrap(err, "failed to restart containerd after patching config")
	}
	return nil
	}
	}
	if err := errors.UntilErrorConcurrent(fns); err != nil {
	return err

update containerd to v2.0.1 #3828

Are you sure you want to change the base?

update containerd to v2.0.1 #3828

Conversation

AkihiroSuda commented Dec 20, 2024 • edited Loading

AkihiroSuda commented Dec 20, 2024

stmcginnis commented Dec 20, 2024

Choose a reason for hiding this comment

BenTheElder Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Jan 7, 2025

BenTheElder commented Jan 7, 2025

AkihiroSuda commented Jan 7, 2025

aojea commented Jan 7, 2025

BenTheElder commented Jan 7, 2025

BenTheElder commented Jan 7, 2025 • edited Loading

k8s-ci-robot commented Jan 8, 2025

k8s-ci-robot commented Jan 8, 2025

k8s-ci-robot commented Jan 8, 2025

AkihiroSuda commented Dec 20, 2024 •

edited

Loading

BenTheElder Jan 7, 2025 •

edited

Loading

aojea Jan 7, 2025 •

edited

Loading

BenTheElder commented Jan 7, 2025 •

edited

Loading