Skip to content
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.

Sign up for GitHub

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 containerd to v2.0.1 #3828

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

update containerd to v2.0.1 #3828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion images/base/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ RUN eval "$(gimme "${GO_VERSION}")" \
# stage for building containerd
FROM go-build AS build-containerd
ARG TARGETARCH GO_VERSION
ARG CONTAINERD_VERSION="v1.7.24"
ARG CONTAINERD_VERSION="v2.0.1"
ARG CONTAINERD_CLONE_URL="https://github.com/containerd/containerd"
# we don't build with optional snapshotters, we never select any of these
# they're not ideal inside kind anyhow, and we save some disk space
Expand Down
3 changes: 2 additions & 1 deletion pkg/build/nodeimage/imageimporter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@ func (c *containerdImporter) Pull(image, platform string) error {
func (c *containerdImporter) LoadCommand() exec.Cmd {
return c.containerCmder.Command(
// TODO: ideally we do not need this in the future. we have fixed at least one image
"ctr", "--namespace=k8s.io", "images", "import", "--label=io.cri-containerd.pinned=pinned", "--all-platforms", "--no-unpack", "--digests", "-",
// --local=true: see the comment in pkg/cluster/nodeutils/util.go
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we set --no-unpack, isn't the issue in the other location with unpacking?

"ctr", "--namespace=k8s.io", "images", "import", "--local=true", "--label=io.cri-containerd.pinned=pinned", "--all-platforms", "--no-unpack", "--digests", "-",
)
}

Expand Down
26 changes: 23 additions & 3 deletions pkg/cluster/nodeutils/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package nodeutils
import (
"bytes"
"encoding/json"
"fmt"
"io"
"path"
"strings"
Expand Down Expand Up @@ -82,7 +83,11 @@ 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`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably need a long term solution for this?

Copy link
Member

@BenTheElder BenTheElder Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened an issue:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

// https://github.com/containerd/containerd/issues/11228
// Expected to be fixed in containerd v2.0.2: https://github.com/containerd/containerd/pull/11229
cmd := n.Command("ctr", "--namespace=k8s.io", "images", "import", "--local=true", "--all-platforms", "--digests", "--snapshotter="+snapshotter, "-").SetStdin(image)
if err := cmd.Run(); err != nil {
return errors.Wrap(err, "failed to load image")
}
Expand All @@ -102,9 +107,24 @@ func parseSnapshotter(config string) (string, error) {
if err != nil {
return "", errors.Wrap(err, "failed to detect containerd snapshotter")
}
snapshotter, ok := parsed.GetPath([]string{"plugins", "io.containerd.grpc.v1.cri", "containerd", "snapshotter"}).(string)
configVersion, ok := parsed.Get("version").(int64)
if !ok {
return "", errors.New("failed to detect containerd snapshotter")
return "", errors.New("failed to detect containerd config version")
}
var snapshotter string
switch configVersion {
case 2: // Introduced in containerd v1.3. Still supported in containerd v2.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@aojea aojea Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

snapshotter, ok = parsed.GetPath([]string{"plugins", "io.containerd.grpc.v1.cri", "containerd", "snapshotter"}).(string)
if !ok {
return "", errors.New("failed to detect containerd snapshotter (config version 2)")
}
case 3: // Introduced in containerd v2.0.
snapshotter, ok = parsed.GetPath([]string{"plugins", "io.containerd.cri.v1.images", "snapshotter"}).(string)
if !ok {
return "", errors.New("failed to detect containerd snapshotter (config version 3)")
}
default:
return "", fmt.Errorf("unknown containerd config version: %d (supported versions: 2 and 3)", configVersion)
}
return snapshotter, nil
}
Expand Down
Loading