Write kubelet env file on kubeadm upgrade node

[ereslibre]

Is this a BUG REPORT or FEATURE REQUEST?

BUG REPORT

What happened?

Since kubernetes/kubernetes@df477a9, kubeadm upgrade apply is writing the kubelet environment file if it does not exist, but kubeadm upgrade node is not.

The code path https://github.com/kubernetes/kubernetes/blob/d1e8702d36947ccc23ad820eaae7f4afbaf0b058/cmd/kubeadm/app/phases/upgrade/postupgrade.go#L164-L176 is triggered only by kubeadm upgrade apply.

Anything else we need to know?

There are two sides for this:

• Why kubeadm upgrade apply tries to create a kubelet env file if it does not exist?

  • Can this case really happen? Init and join are supposed to create this file.
  • If it's not required: remove this logic from kubeadm upgrade apply.

• If it's really needed, then kubeadm upgrade node should behave alike and write this env file if it's missing

  • Adapt kubeadm upgrade node so it will trigger this code path as kubeadm upgrade apply does.

Moving forward, I also wonder about the relationship of this environment file regarding kubeadm. If kubeadm is owning this file, would that make sense to also overwrite this file on upgrades? If this is a yes, then the logic should be a little different, overwriting this env file always during upgrades, upon kubeadm upgrade apply and kubeadm upgrade node.

[ereslibre]

/priority important-longterm
/assign

[ereslibre]

/lifecycle active

[ereslibre]

PR: kubernetes/kubernetes#85412

[neolit123]

@pickledrick hi, i saw your ticket here:
#1927

The request is for /var/lib/kubelet/kubeadm-flags.env to be updated during kubeadm upgrade. An example of how we found this was during an upgrade. We were renaming an image repository, for example, vmware/pause:3.1 to vmware.io/pause:3.1. The kubelet flag --pod-infra-container-image=` makes use of this image and during an upgrade, this was not upgraded.

as we are discussing on the PR, cluster reconf and upgrades are two separate actions that should not be mixed:
kubernetes/kubernetes#85412
kubeadm is moving away from allowing reconf during upgrade.

in the case of a pause image change, one can reconf (either manually or using a kubeadm future functionality), then perform the upgrade.

[fabriziopandini]

@ereslibre @neolit123 @boluisa
I did a little bit of investigation around this and it turned out that writing the kubelet environment file uses the KubeletExtraArgs from node registration options

https://github.com/kubernetes/kubernetes/blob/585ef375bb68f20750f09af94dc219dd84b19616/cmd/kubeadm/app/phases/kubelet/flags.go#L71-L70

As of today kubeadm is not saving the original KubeletExtraArgs used at init/join time; that's means that, if we are writing the kubelet environment file during update there is a concrete risk to lose the initial settings for the node. This is also documented in the code itself

https://github.com/kubernetes/kubernetes/blob/31b4c782c753f8d268d0cf8cbb805d74b8b24a26/cmd/kubeadm/app/util/config/cluster.go#L133

TL;DR; both upgrade apply and upgrade node should NOT write the kubelet environment file.

Expanding the concept, kubeadm today is not designed to support the upgrade the kubelet environment file due to the lack of a permanent storage for KubeletExtraArgs settings, unless we consider parsing the file itself a valid approach.

If you ask me why upgrade apply is generating the kubelet environment file if it not exists, my guess is for managing cluster created before this file was introduced. This was long time ago, so it can be removed now.

[rosti]

+1 to what @fabriziopandini said. Let's nuke the call.
We need to delete it and document it somewhere on the site. It's likely that users will continue complaining about it.
Stuff in there is unlikely to change. We can simply advice folks to reset and re-join the node if they need regenerate that file. Another possibility is to edit it by hand and restart the kubelet, but then, if they break something, they will be on their own.

[ereslibre]

I'm +1 as well to nuke the call from kubeadm upgrade apply.

If you ask me why upgrade apply is generating the kubelet environment file if it not exists, my guess is for managing cluster created before this file was introduced. This was long time ago, so it can be removed now.

Yes, this is right, it got introduced in kubernetes/kubernetes#65104 to make the upgrade from 1.10 to >1.11 work e2e.

[alex-vmw]

@ereslibre @neolit123 @boluisa
I did a little bit of investigation around this and it turned out that writing the kubelet environment file uses the KubeletExtraArgs from node registration options

FYI, my team is NOT specifying KubeletExtraArgs in the config file, but we are setting an imageRepository in the config file and relying on the kubernetes/kubernetes#70603 change to automatically set the "--pod-infra-container-image" argument in /var/lib/kubelet/kubeadm-flags.env. That said, imageRepository is also used to write /var/lib/kubelet/kubeadm-flags.env.

https://github.com/kubernetes/kubernetes/blob/585ef375bb68f20750f09af94dc219dd84b19616/cmd/kubeadm/app/phases/kubelet/flags.go#L71-L70

As of today kubeadm is not saving the original KubeletExtraArgs used at init/join time; that's means that, if we are writing the kubelet environment file during update there is a concrete risk to lose the initial settings for the node. This is also documented in the code itself
https://github.com/kubernetes/kubernetes/blob/31b4c782c753f8d268d0cf8cbb805d74b8b24a26/cmd/kubeadm/app/util/config/cluster.go#L133

While I agree that KubeletExtraArgs is NOT saved in the kubeadm-config configmap, the imageRepository is in fact saved in the kubeadm-config configmap!

TL;DR; both upgrade apply and upgrade node should NOT write the kubelet environment file.

Expanding the concept, kubeadm today is not designed to support the upgrade the kubelet environment file due to the lack of a permanent storage for KubeletExtraArgs settings, unless we consider parsing the file itself a valid approach.

If you ask me why upgrade apply is generating the kubelet environment file if it not exists, my guess is for managing cluster created before this file was introduced. This was long time ago, so it can be removed now.

Sure, If KubeletExtraArgs are updated, then upgrade apply and upgrade node should NOT write the kubelet environment file, but why not write the file if the imageRepository changes?

P.S. The exact use case for my team changing the imageRepository is described in kubernetes/kubernetes#85412 (comment)

[fabriziopandini]

@alex-vmw

FYI, my team is NOT specifying KubeletExtraArgs in the config file, but we are setting an imageRepository in the config

yes, this is the current use case, but when implementing a change we should consider all the possible conditions, and "a user specifying KubeletExtraArgs in the config" is one of them.

Sure, If KubeletExtraArgs are updated, then upgrade apply and upgrade node should NOT write the kubelet environment file, but why not write the file if the imageRepository changes?

unfortunately, when kubeadm is executed it has access only to the current version of the kubeadm-config, and as a consequence there is no reliable [1] way to detect KubeletExtraArgs or imageRepository were updated or not

[1] as already noted above, parsing the file existing on the node where kubeadm is executed is a possible approach, but as proved several occasions in the past, it is far from being reliable

However the underlying point of this discussion is that kubeadm upgrade is not designed for handling "change the cluster" use cases, and as documented here altering the kubeadm-config and running kubeadm-upgrade can lead to unexpected results.

As a kubeadm team we are working on #1698 to address the "change the cluster" story (yay!)