kubeadm: init kep for no cri socket annotation

[pacoxu]

• One-line PR description: in theory we don't need the CRI socket annotation on the Node object that kubeadm does any more.

• Issue link: kubeadm: not use CRI socket annotation on the Node object  #3929

• Other comments: an initial proposal

/cc @neolit123 @SataQiu @chendave @RA489

[neolit123]

we can omit the keps/prod-readiness changes IMO

[neolit123]

it'a tricky change that we must make either way. the alpha annotation was a workaround for the lack of cri fields in the kubelet config, but now they exist.

[neolit123]

we can omit this

[neolit123]

maybe use a feature gate?

[pacoxu]

I add two proposals here.

1. respect a list of configuration in local kubelet configuration, and in v1.27, CRI socket is the only one
2. introduce a /var/lib/kubelet/kubeadm-config.yaml to maintain node specific configuration

I add a feature gate for the proposal 2.

[neolit123]

it's one of the ways of doing it. we need to be careful and make sure this host / instance config does not cause issues for us in the future.

perhaps we can avoid it, and just parse the cri value from the existing config.yaml on upgrade. like i proposed on the linked issue.

[pacoxu]

Yes, this may be safer for cri-socket only. Probably, I should use your proposal here as this KEP only covers the cri-socket.

I wondered if we need to add such a file for other node-customized kubelet configurations.

[chendave]

Mostly some questions.

[chendave]

Does the "global" kubelet config goes to a sub-struct of ClusterConfiguration? there is no such configmap when you bootstrap a cluster with kubeadm init...

[pacoxu]

For the global kubelet config here, it means kubectl get cm -n kube-system kubelet-config. There is no global setting in ClusterConfiguration.

For init and join process

• The cri-socket can be set in NodeRegistrationOptions.containerRuntimeEndpoint in InitConfiguration or JoinConfiguration.
• Or you can override it with --cri-socket.

[chendave]

If this global config is something embedded in clusterCfg instead of configmap, how do you override this config? each node maintains one for itself? or you want to maintain a map in such global config to track different config for each node?

[pacoxu]

If this global config is something embedded in clusterCfg instead of configmap, how do you override this config?

For upgradation, the global config is in kubelet-config configmap and will not be override by clusterCfg.

For init/join, it will respect (1) the cri-socket flag, (2) the NodeRegistrationOptions.containerRuntimeEndpoint in InitConfiguration or JoinConfiguration.

In kubelet-config configmap, there can be a global one. There are two proposals here.

• Proposal 1: If containerRuntimeEndpoint is in the node specific list, we will take it from node first and skip the global one. For this proposal, containerRuntimeEndpoint in global will not be used as the containerRuntimeEndpoint has default value and is always set during init or join.
• Proposal 2: We will take the one from /var/lib/kubelet/kubeadm-config.yaml at first, if there is no containerRuntimeEndpoint in /var/lib/kubelet/kubeadm-config.yaml, the global one will take effect.

each node maintains one for itself?

containerRuntimeEndpoint in kubelet configuration after v1.27 has a default value and is always set during init or join.

--container-runtime-endpoint string The endpoint of container runtime service. Unix Domain Sockets are supported on Linux, while npipe and tcp endpoints are supported on Windows. Examples:'unix:///path/to/runtime.sock', 'npipe:////./pipe/runtime' (default "unix:///run/containerd/containerd.sock") (DEPRECATED: This parameter should be set via the config file specified by the Kubelet's --config flag. See https://kubernetes.io/docs/tasks/administer-cluster/kubelet-config-file/ for more information.)

The flag is like above.

or you want to maintain a map in such global config to track different config for each node?

No.

[chendave]

Can you pls elaborate a little bit? How the node-specific config maintained in the kubeadm code?

[pacoxu]

I mean that we can maintain it in cmd/kubeadm/app/constants/constants.go or somewhere.

nodeSpecificKubeletConfigurations =  map[string]string{
	"containerRuntimeEndpoint": "the endpoint of container runtime service",
	"imageServiceEndpoint": "the endpoint of container image service",
	"providerID": "sets the unique ID of the instance that an external provider",
}

The list can be extended and I am not sure if the list should be modified by a user in a global config nodeSpecificKubeletConfiugrations, which means a new array in ClusterConfiguration. The default values can be containerRuntimeEndpoint and imageServiceEndpoint.

[neolit123]

i think we can take the patches approach - any locally stored config file can act as a patch over the config from kubelet-config cm. but we may only consider some fields from the kubeletconfiguration v1beta1. this becomes tricky when v1beta1 is replaced by e.g. v1

[neolit123]

any locally stored config file can act as a patch over the config from kubelet-config cm

this can also be tricky in some circumstances where we don't want to patch a field e.g. during upgrade, but hopefully it's not an issue

[pacoxu]

If we use the --patches feature here to add a default patch dir on node, this can be done in this way.

However, if users want to specify a new patch dir, he has to merge the default patch files or content to the new one. That would be a little weird.

[neolit123]

i meant a generic patches approach where a local config file overrides the global config downloaded from the cm. not the --patches feature.

this local file must be stored somewhere. perhaps in the same dir as config.yaml, but called config-instance.yaml. IMO it has a number of tricky aspects that need to be covered in the design doc for init, join, upgrade.

[chendave]

Does the config file here support to config the containerRuntimeEndpoint? I tried to bootstrap a cluster with --cri-socket option, but this config is not persistented in the /var/lib/kubelet/config.yaml. If the endpoint is already supported to config in kubelet config file, then this is the logic we should update.

[pacoxu]

Does the config file here support to config the containerRuntimeEndpoint? I tried to bootstrap a cluster with --cri-socket option, but this config is not persistented in the /var/lib/kubelet/config.yaml. If the endpoint is already supported to config in kubelet config file, then this is the logic we should update.

containerRuntimeEndpoint is added in v1.27. In the past and also v1.27, we are using the --container-runtime-endpoint in /var/lib/kubelet/kubeadm-flags.env.

• Before v1.27, it cannot be added in the kubeletConfiguration.
• And also, we saved the cri-socket in the annotation, and that is why we don't have to read and parse /var/lib/kubelet/kubeadm-flags.env here.

If we start the process that not using cri-socket annotation in v1.28, we set it in the /var/lib/kubelet/config.yaml.
Then the flag in /var/lib/kubelet/kubeadm-flags.env can be cleaned. (This should be mentioned in this KEP. I will update it.)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pacoxu
Once this PR has been reviewed and has the lgtm label, please assign vincepri for approval. For more information see the Kubernetes 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:

• keps/sig-cluster-lifecycle/OWNERS

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

[neolit123]

@pacoxu are there any blockers / concerns or can this KEP work proceed with an implementation POC PR?

if we have an alpha FG + tests we can establish if the change will work as expected. this feels like the safest route.

also, can we target .28?

i need to re-read the whole KEP again.

[pacoxu]

Of course, I will start to working on a demo PR this week or next. I think there is no blocker.
We could get this done in v1.28 release cycle.

[neolit123]

Of course, I will start to working on a demo PR this week or next. I think there is no blocker.
We could get this done in v1.28 release cycle.

ok, i didn't see a feature gate in the kep. imo, we should use it as the user controlled switch. e.g. NodeLocalCRISocket
#3930 (comment)

[pacoxu]

https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/3983-drop-in-configuration
kubelet introduces a --config-dir for drop-in configuration.

We may alter to use it then. #3983 is beta in v1.29.

[neolit123]

https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/3983-drop-in-configuration kubelet introduces a --config-dir for drop-in configuration.

We may alter to use it then. #3983 is beta in v1.29.

i am generally -1 (but we can discuss it), since it implies a mechanism for systemd like drop-ins.
kubeadm on the other hand already uses patches for this node level config.

i believe i commented on that KEP, already.

[pacoxu]

i believe i commented on that KEP, already.

Yes. I must miss that 🤣

i am generally -1 (but we can discuss it), since it implies a mechanism for systemd like drop-ins. kubeadm on the other hand already uses patches for this node-level config.

I still cannot decide on it. Is the new drop-in a better solution than the current kubeadm way?

let's keep it as is until the direction is clear and beyond doubt.

[neolit123]

i believe i commented on that KEP, already.

Yes. I must miss that 🤣

i am generally -1 (but we can discuss it), since it implies a mechanism for systemd like drop-ins. kubeadm on the other hand already uses patches for this node-level config.

I still cannot decide on it. Is the new drop-in a better solution than the current kubeadm way?

let's keep it as is until the direction is clear and beyond doubt.

here are my comments on the KEP PR
#4031 (review)

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[neolit123]

@pacoxu you closed
#3929

what should we do with this PR?

[pacoxu]

i meant a generic patches approach where a local config file overrides the global config downloaded from the cm. not the --patches feature.

this local file must be stored somewhere. perhaps in the same dir as config.yaml, but called config-instance.yaml. IMO it has a number of tricky aspects that need to be covered in the design doc for init, join, upgrade.

Above is @neolit123 your proposal in comment here https://github.com/kubernetes/enhancements/pull/3930/files#r1177682829.

I prefer this solution.

BTW, /var/lib/kubelet/kubeadm-flags.env will only have --container-runtime-endpoint later. I would like to make this cri-socket-annotation and runtime endpoint configuration to config-instance.yaml under /var/lib/kubelet.

[neolit123]

/close
#3929 was closed
let's revisit in the future.

kubernetes/kubernetes#122734 (comment)
cc @HirazawaUi

[k8s-ci-robot]

@neolit123: Closed this PR.

In response to this:

/close
#3929 was closed
let's revisit in the future.

kubernetes/kubernetes#122734 (comment)
cc @HirazawaUi

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.