Clean up dockershim flags in the kubelet

[endocrimes]

/sig node
/kind cleanup
/cc @neolit123

As part of the removal of dockershim, we should also remove the deprecated CLI flags that still exist.

The pod-infra-container-image and container-runtime flags are now also deprecated and planned to be removed int v1.27. pod-infra-container-image is being integrated through CRI implementations to avoid needing double specification.

https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/config/flags.go#L92-L112

[endocrimes]

/cc @adisky

[cyclinder]

/assign
so we should remove https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/config/flags.go#L92-L112, but keep the pod-infra-container-image flag?

[adisky]

@cyclinder yes we need to keep pod-infra-container-image.

pod-infra-container-image can bemarked as deprecated since alternative via CRI available but i am not sure about the timeline since cri alternative is made available with the latest(1.23) release and we want to give sometime to container runtimes to implement it.
#103299
https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2040-kubelet-cri#pinned-images

[cyclinder]

@adisky so we should marked pod-infra-container-image as deprecated ? or wait some time ?

[adisky]

lets handle it as a separate issue and leave it for now

[cyclinder]

thanks @adisky ,I will work on this.

[hakman]

@adisky Should --container-runtime also be removed together with the logic behind it?
With the dockershim support removal, the only valid value for it is --container-runtime=remote.

[adisky]

@hakman It makes sense to remove --container-runtime from codebase but this flag has not been marked as deprecated we need to deprecate the flag first and remove in further releases

[neolit123]

removing the --container-runtime flag from 1.24 would break existing non-docker users.
as @adisky explains it should be marked as deprecated first.

for 1.24 i suggest:

• marking the flag as deprecated with fs.MarkDeprecated and keeping it for 1 year (3 releases).
• defaulting it to remote, current default is docker (option should be removed in code / flag description). this aligns with the dockershim removal and implies the only possible value.
• have a TODO above the flag linking to a k/k issue (e.g. this one) and keeping the issue open until the deprecation passes.
• this also affects https://github.com/kubernetes/kubernetes/blob/9b180d8913d7d08f0f2c0a4d60289560d4fdd7e7/test/e2e/framework/test_context.go

[hakman]

Sounds good to me @adisky @neolit123. I was planning to do it in a similar way. Will open a PR for it tomorrow.

[adisky]

Sounds good to me @adisky @neolit123. I was planning to do it in a similar way. Will open a PR for it tomorrow.

sorry @hakman I saw the reply just now created the PR #107094

[hakman]

@adisky I was thinking of removing the code behind it too, to make it a no-op. As is now, it would create more confusion. What do you think?

[adisky]

@adisky I was thinking of removing the code behind it too, to make it a no-op. As is now, it would create more confusion. What do you think?

We can do that in followup some of it is conflicting with #106907, it would be cleaner to do after #106907 gets merged

[pacoxu]

I create a pr to remove the container-runtime

It seems that pod-infra-container-image cannot be removed yet

#114017 is opened for this.

[aimuz]

#114017

Strange as it may seem, he is removing code that discards flags using fs.MarkDeprecated

[pacoxu]

#114017

Strange as it may seem, he is removing code that discards flags using fs.MarkDeprecated

The pr author updated the pull request.

[pacoxu]

Update some current status about this issue(half done):

• container-runtime: cleanup container runtime options #114017
• pod-infra-container-image removal should be postponed, and the deprecated status is also
• • CRI Pinned image support containerd/containerd#7944 is not merged yet(containerd 1.7 released) and Implement Pinned image support  containerd/containerd#6352
• • kubeadm should stop using it when it is ready
• • for gce windows test: Install-WorkerServices should not set pod-infra-container-image as it is marked for deprecation #108144
• • kubeadm: show a warning message when detecting that the sandbox image of the container runtime is inconsistent with that used by kubeadm #115610 The warning is helpful before the flag is removed.

[afbjorklund]

The warning seems to only work with containerd, since the "config" field is not available in docker or cri-o ?

#115610 (review)

is this a new cri feature?

{
  "status": {
    "conditions": [
      {
        "type": "RuntimeReady",
        "status": true,
        "reason": "",
        "message": ""
      },
      {
        "type": "NetworkReady",
        "status": false,
        "reason": "NetworkPluginNotReady",
        "message": "docker: network plugin is not ready: cni config uninitialized"
      }
    ]
  },
  "config": {
    "sandboxImage": "registry.k8s.io/pause:3.9"
  }
}

[afbjorklund]

You can remove the warning in kubeadm now, since the kubelet will ignore the flag anyway (since v1.29.0-alpha.1).

commit d5690f1

The actual flag will not be removed until 1.30, but it is not actually doing anything now (it now uses "pinned" instead)

[neolit123]

You can remove the warning in kubeadm now, since the kubelet will ignore the flag anyway (since v1.29.0-alpha.1).

which k/k pr did that?

The actual flag will not be removed until 1.30, but it is not actually doing anything now (it now uses "pinned" instead)

we can remove the flag and warning in kubeadm 1.29, but we don't need other actions because the kubelet will pin the pause image using cri. is that correct?

EDIT: kubeadm can continue showing this warning:
#115610 (comment)

[neolit123]

we can remove the flag and warning in kubeadm 1.29

i can send the kubeadm pr for this.

[neolit123]

actually we are facing a bit of an issue after the recent change to support N-3 kubelet upgrades. please see:
kubernetes/kubeadm#2626 (comment)

ideally, the flag should be removed after 1.29 goes out of support (if my math is correct).

[afbjorklund]

which k/k pr did that?

• pkg/kubelet: allow sandbox image pinning from CRI #118544

[neolit123]

the flag cannot be removed in 1.30 if we follow the rules:
kubernetes/kubeadm#2626 (comment)

[pacoxu]

• pkg/kubelet: allow sandbox image pinning from CRI #118544

the flag cannot be removed in 1.30 if we follow the rules:
kubernetes/kubeadm#2626 (comment)

/cc @mrunalp @SergeyKanzhelev
we probably have to revert the #118544 update the deprecate plan per the deprecate/skew policy mentioned by @neolit123 and Jordon.

[pacoxu]

/unassign @cyclinder

[neolit123]

• pkg/kubelet: allow sandbox image pinning from CRI #118544

the flag cannot be removed in 1.30 if we follow the rules:
kubernetes/kubeadm#2626 (comment)

/cc @mrunalp @SergeyKanzhelev we probably have to revert the #118544 per the deprecate/skew policy mentioned by @neolit123 and Jordon.

no need to revert #118544. the flag must remain as a no-op for a GA deprecation period. not just one release.

[pacoxu]

https://github.com/kubernetes/kubernetes/pull/118544/files#diff-3a14a9838132c6668f4d100c26afa05b471b149c469edfece9b84fcd058c8a4bR204

_ = cmd.Flags().MarkDeprecated("pod-infra-container-image", "--pod-infra-container-image will be removed in 1.30. Image garbage collector will get sandbox image information from CRI.")

This line should be changed if I understand correctly.

kubernetes/cmd/kubelet/app/server.go

Lines 203 to 204 in 6897b12

	klog.InfoS("--pod-infra-container-image will not be pruned by the image garbage collector in kubelet and should also be set in the remote runtime")
	_ = cmd.Flags().MarkDeprecated("pod-infra-container-image", "--pod-infra-container-image will be removed in 1.30. Image garbage collector will get sandbox image information from CRI.")

kubernetes/pkg/kubelet/config/flags.go

Line 54 in 6897b12

fs.MarkDeprecated("pod-infra-container-image", "will be removed in a future release. Image garbage collector will get sandbox image information from CRI.")

[neolit123]

This line should be changed if I understand correctly.

the deprecation timeline in the note can be adjusted if SIG node agrees.

[afbjorklund]

Now we can see the kubelet starting to garbage collect the sandbox image, in Docker...

"Image garbage collection failed multiple times in a row" err="wanted to free 47880955494 bytes, but freed 0 bytes space with errors in image deletion: rpc error: code = Unknown desc = Error response from daemon: conflict: unable to remove repository reference \"registry.k8s.io/pause:3.9\" (must force) - container 0aa790817599 is using its referenced image e6f181688397"

(it gets a bit clean-happy, when you run kubernetes-in-container and it sees host /var)

Fortunately the cleaning failed, since there were still some kube-system pods using it.

[carlory]

/assign

[pacoxu]

From containerd side, containerd v1.7.3 and containerd 1.6.22 includes the support of pinned image:

• Pinned image support ([release/1.6] Pinned image support containerd/containerd#8720)

• 1.6.25+ and containerd 1.7.10+ fixed a bug cri: fix using the pinned label to pin image ([release/1.7] cri: fix using the pinned label to pin image containerd/containerd#9381).

However, there are still two bugfix pull requests in progress:

• fix pinned the preloaded pause image containerd/containerd#9460
• cri: pin the sandbox image by comparing to config.SandboxImage containerd/containerd#8866

[adisky]

However, there are still two bugfix pull requests in progress:

containerd/containerd#9460
containerd/containerd#8866

Are those PR's really a blocker? one of them is for adding the pinned label for preloaded image which can be done via ctr as well while importing the image. Kind is planning to leverage it without those PR's,
containerd/containerd#9476 (comment)

[pacoxu]

The two WIP PR are not blockers.
containerd/containerd#9381 is not a blocker as well.(It will pin some more than expected.)

To use that feature, we at least should use containerd v1.7.3+ and containerd 1.6.22+.