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.

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

[Need Discuss] Fix containerd config_path error when containerd_registries is configed #9743

Closed

Conversation

yankay
Copy link
Member

@yankay yankay commented Feb 1, 2023

What type of PR is this?

/kind bug

What this PR does / why we need it:

After the #9566
There is a warning log when start the containerd:

containerd[58616]: time="2023-01-31T21:05:15.041655133Z" level=warning 
  msg="failed to load plugin io.containerd.grpc.v1.cri" 
  error="invalid plugin config: `mirrors` cannot be set when `config_path` is provided"

When the containerd_insecure_registries or containerd_registries are configured, the kubeadm can not install the Kubernetes. And kubelet can not start the Pod.

So the PR is try to fix the bug, following the https://github.com/containerd/containerd/blob/main/docs/hosts.md.

Which issue(s) this PR fixes:

Fixes #9741

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 1, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yankay

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

The pull request process is described here

Needs approval from an approver in each of these files:

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 k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. 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 Feb 1, 2023
@yankay yankay force-pushed the fix-containerd-config_path branch 5 times, most recently from 2e528b4 to 81cf937 Compare February 1, 2023 10:54
@yankay yankay changed the title Fix containerd config_path error when containerd_registries is configed [WIP]Fix containerd config_path error when containerd_registries is configed Feb 1, 2023
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 1, 2023
@yankay yankay force-pushed the fix-containerd-config_path branch from 81cf937 to 60bae07 Compare February 1, 2023 11:17
@yankay yankay force-pushed the fix-containerd-config_path branch from 60bae07 to 2279e98 Compare February 1, 2023 11:19
@yankay yankay changed the title [WIP]Fix containerd config_path error when containerd_registries is configed Fix containerd config_path error when containerd_registries is configed Feb 1, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 1, 2023
@yankay yankay changed the title Fix containerd config_path error when containerd_registries is configed [WIP]Fix containerd config_path error when containerd_registries is configed Feb 1, 2023
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 1, 2023
@yankay yankay changed the title [WIP]Fix containerd config_path error when containerd_registries is configed Fix containerd config_path error when containerd_registries is configed Feb 1, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 1, 2023
@yankay
Copy link
Member Author

yankay commented Feb 1, 2023

From the

The config_path and registry.mirror cannot work together.

And the registry.mirror can still be used in containerd 1.x, containerd 1.7 is intended to be the last major release of containerd 1.x before 2.0.

”Removal of this and other deprecations are not immediate. Yes, we've been considering removing all v1 deprecations when we move to v2, yes perhaps EOY perhaps sometime next year. We've been discussing the possibility of providing migration tooling for config and metadata. It is just as likely we will attempt to carry v1 and early v2 release versions of certain services (esp. CRI) at the same time to allow for opt in testing. The hosts.toml config is very similar to docker hosts.toml, if that helps.
"
From the kubernetes/kubernetes#110312 (comment)_

So there is another way to solve the issue by revert the #9566.
And add it back when the containerd remove the registry.mirror finnaly.

I'm not sure which solution is better:

  1. use the new host.toml and fix the bugs, with this PR.
  2. use the old registry.mirror, revert the add containerd config_path #9566 .

We can choose one :-)
Would you please comment it.

/hold

@yankay yankay changed the title Fix containerd config_path error when containerd_registries is configed [WIP]Fix containerd config_path error when containerd_registries is configed Feb 1, 2023
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 1, 2023
@yankay yankay changed the title [WIP]Fix containerd config_path error when containerd_registries is configed [Need Discuss] Fix containerd config_path error when containerd_registries is configed Feb 2, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 2, 2023
@yankay
Copy link
Member Author

yankay commented Feb 6, 2023

HI @oomichi @floryut @cristicalin , would you please review the PR :-)

@lengrongfu
Copy link
Contributor

Thanks to @yankay for the fix. I feel that a switch can be added to determine whether to enable config_path. It is disabled by default. It seems that mirrors will not be used for a long time.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2023
@k8s-ci-robot
Copy link
Contributor

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.

@yckaolalala
Copy link
Contributor

Hi @yankay

I opened a PR to fix mirrors in config_path .

@yankay
Copy link
Member Author

yankay commented Jun 15, 2023

Hi @yankay

I opened a PR to fix mirrors in config_path .

Thanks @yckaolalala

I will review the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No containerd with insecure registries
4 participants