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

Jump to bottom

CRI: remove deprecated config properties (auths, configs, mirrors) #9766

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

CRI: remove deprecated config properties (auths, configs, mirrors) #9766

wants to merge 1 commit into from

Conversation

AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented Feb 6, 2024 •
edited
Loading

Note

The removal is likely going to be postponed to v2.1

Remove the following config properties in /etc/containerd/config.toml:

Property Group Property Deprecation release Target release for removal Recommendation
[plugins."io.containerd.grpc.v1.cri".registry] auths containerd v1.3 containerd v2.0 ✅ Use ImagePullSecrets. See also #8228.
[plugins."io.containerd.grpc.v1.cri".registry] configs containerd v1.5 containerd v2.0 ✅ Use config_path
[plugins."io.containerd.grpc.v1.cri".registry] mirrors containerd v1.5 containerd v2.0 ✅ Use config_path

The toml properties are still kept for printing human-readable errors, but the properties will be completely removed in v2.1.

Depends on:

@AkihiroSuda AkihiroSuda added impact/changelog area/cri Container Runtime Interface (CRI) impact/breaking labels Feb 6, 2024
@AkihiroSuda AkihiroSuda added this to the 2.0 milestone Feb 6, 2024
@AkihiroSuda
Copy link
Member Author

cc @thockin @dims

@AkihiroSuda AkihiroSuda force-pushed the remove-deprecated-cri-config branch 2 times, most recently from ac75d67 to 0ec8de1 Compare February 6, 2024 05:50
@AkihiroSuda
CRI: remove deprecated config properties (auths, configs, mirrors)
8313a1c 
Remove the following config properties in `/etc/containerd/config.toml`:

| Property Group                                  | Property   | Deprecation release | Target release for removal | Recommendation         |
|-------------------------------------------------|------------|---------------------|----------------------------|------------------------|
|`[plugins."io.containerd.grpc.v1.cri".registry]` | `auths`    | containerd v1.3     | containerd v2.0 ✅         | Use `ImagePullSecrets` |
|`[plugins."io.containerd.grpc.v1.cri".registry]` | `configs`  | containerd v1.5     | containerd v2.0 ✅         | Use `config_path`      |
|`[plugins."io.containerd.grpc.v1.cri".registry]` | `mirrors`  | containerd v1.5     | containerd v2.0 ✅         | Use `config_path`      |

The toml properties are still kept for printing human-readable errors,
but the properties will be completely removed in v2.1.

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
@k8s-ci-robot
Copy link

@AkihiroSuda: The following test 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-containerd-node-e2e 8313a1c link true /test pull-containerd-node-e2e

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/test-infra repository. I understand the commands that are listed here.

@AkihiroSuda
Copy link
Member Author

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/containerd_containerd/9766/pull-containerd-node-e2e/1754745360544174080

{ failed [FAILED] Unexpected error:
    <*fmt.wrapError | 0xc0000bd900>: 
    validate service connection: validate CRI v1 runtime API for endpoint "unix:///run/containerd/containerd.sock": rpc error: code = Unavailable desc = connection error: desc = "transport: Error while dialing: dial unix /run/containerd/containerd.sock: connect: no such file or directory"
    {
        msg: "validate service connection: validate CRI v1 runtime API for endpoint \"unix:///run/containerd/containerd.sock\": rpc error: code = Unavailable desc = connection error: desc = \"transport: Error while dialing: dial unix /run/containerd/containerd.sock: connect: no such file or directory\"",
        err: <*fmt.wrapError | 0xc0000bd8e0>{
            msg: "validate CRI v1 runtime API for endpoint \"unix:///run/containerd/containerd.sock\": rpc error: code = Unavailable desc = connection error: desc = \"transport: Error while dialing: dial unix /run/containerd/containerd.sock: connect: no such file or directory\"",
            err: <*status.Error | 0xc0009d58f0>{
                s: {
                    s: {
                        state: {
                            NoUnkeyedLiterals: {},
                            DoNotCompare: [],
                            DoNotCopy: [],
                            atomicMessageInfo: nil,
                        },
                        sizeCache: 0,
                        unknownFields: nil,
                        Code: 14,
                        Message: "connection error: desc = \"transport: Error while dialing: dial unix /run/containerd/containerd.sock: connect: no such file or directory\"",
                        Details: nil,
                    },
                },
            },
        },
    }
occurred
In [SynchronizedBeforeSuite] at: k8s.io/kubernetes/test/e2e_node/e2e_node_suite_test.go:235 @ 02/06/24 06:17:10.719
}

samuelkarp
samuelkarp reviewed Feb 6, 2024
View reviewed changes
internal/cri/config/config.go
if useConfigPath {
return warnings, errors.New("`mirrors` cannot be set when `config_path` is provided")
}
warnings = append(warnings, deprecation.CRIRegistryMirrors)
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove deprecation.CRIRegistryMirrors?

internal/cri/config/config.go
}

if len(c.Registry.Configs) != 0 {
warnings = append(warnings, deprecation.CRIRegistryConfigs)
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove deprecation.CRIRegistryConfigs?

internal/cri/config/config.go
config.Auth = &auth
c.Registry.Configs[endpoint] = config
}
warnings = append(warnings, deprecation.CRIRegistryAuths)
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove deprecation.CRIRegistryAuths?

Copy link
Member

Choose a reason for hiding this comment

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

don't have a replacement for cri auth yet..

Copy link
Member

Choose a reason for hiding this comment

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

@dmcgowan

Copy link
Member

Choose a reason for hiding this comment

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

see #7526 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like we have to postpone removal of legacy config, although we may disable it by default and require an env var like CONTAINERD_DEPRECATED_ENABLE_CONTAINERD14_CONFIG

@samuelkarp
Copy link
Member

https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/containerd_containerd/9766/pull-containerd-node-e2e/1754745360544174080/artifacts/tmp-node-e2e-f8fc27d0-cos-beta-109-17800-66-81-system.log

Feb 06 06:17:06 tmp-node-e2e-f8fc27d0-cos-beta-109-17800-66-81 containerd[3207]: containerd: load required plugin io.containerd.grpc.v1.cri: unable to load CRI image service plugin dependency: invalid cri image config: `mirrors` is no longer supported since containerd v2.0, please use `config_path` instead

We'll need the prowjob to be updated to remove its use of mirrors.

adisky
adisky reviewed Feb 6, 2024
View reviewed changes
Copy link
Contributor

@adisky adisky left a comment

Choose a reason for hiding this comment

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

registry mirrors needs to be updated at following places
https://github.com/kubernetes/test-infra/blob/master/jobs/e2e_node/containerd/config.toml#L31

https://github.com/containerd/containerd/blob/main/contrib/gce/configure.sh#L226

@AkihiroSuda AkihiroSuda mentioned this pull request Feb 6, 2024
Open
@adisky adisky mentioned this pull request Feb 7, 2024
Merged
@k8s-ci-robot
Copy link

PR needs rebase.

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/test-infra repository.

@AkihiroSuda AkihiroSuda marked this pull request as draft February 12, 2024 15:52
@AkihiroSuda AkihiroSuda mentioned this pull request Mar 5, 2024
Merged
@AkihiroSuda AkihiroSuda modified the milestones: 2.0, 2.1 Mar 17, 2024
@AkihiroSuda
Copy link
Member Author

I guess we should postpone this to v2.1

@AkihiroSuda AkihiroSuda mentioned this pull request Mar 17, 2024
Merged
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 1, 2024

This PR is stale because it has been open 90 days with no activity. This PR will be closed in 7 days unless new comments are made or the stale label is removed.

@github-actions github-actions bot added the Stale label Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samuelkarp samuelkarp samuelkarp left review comments

@adisky adisky adisky left review comments

@mikebrow mikebrow mikebrow left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
area/cri Container Runtime Interface (CRI) do-not-merge/work-in-progress impact/breaking impact/changelog needs-rebase size/L Stale
Projects
None yet
Milestone
2.1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@AkihiroSuda @k8s-ci-robot @samuelkarp @adisky @mikebrow