Kubeadm upgrade stuck in "Static pod: etcd-xxxx hash: xxxx" occasionally

[ImNotAFrog]

What happened?

We encountered this error when running kubeadm upgrade apply, the logs of kubeadm printed "Static pod: etcd-xxxx hash: xxxx" repeatedly till timeout.

What did you expect to happen?

We were trying to keep some of the modifications in static pods' yaml while upgrading k8s. Our strategy is:

1. Keep a copy of original static pods' yaml in cluster using configmap when deploy the cluster.
2. When upgrading k8s, make a diff of current static pods' yaml and the saved copies in cluster, store them in yaml files.
3. Call "kubeadm upgrade apply" with param "--patches" to upgrade k8s components.
   We suppose that these operations should work.

How can we reproduce it (as minimally and precisely as possible)?

If the order of a map's fields are different between current static pods' yaml and the diff patch files, the problem will occur. e.g. if current etcd yaml has some annotations like:
metadata:
annotations:
field1: foo1
field2: foo2
and the patch file we created is:
metadata:
annotations:
field2: foo2
field1: foo1
the upgrade will stuck and failed.

Anything else we need to know?

Since map is unordered in golang, we didn't expected this behavior of kubeadm.
By looking into the source code of kubeadm, we found the possible cause of this behavior, the current procedures of kubeadm upgrading static pods are as below(using etcd as an example):

1. Make a hash of current etcd yaml
2. Make a new yaml and patch it if "--patches" exists
3. Compare if new yaml and old yaml are the same, if not, move new yaml to manifest and wait.
4. Repeatedly get pod hash from cluster, if the hash has changed, means the pod has upgraded successfully.
   The problem of this process is, kubeadm uses DeepHashObject in pkg/util/hash to generate hash code, this hash method will sort the fileds, which means the two yaml files like above will leads to a same hash code. Meanwhile, when compare yaml files, kubeadm simply comprares the files' bytes instead, see in: https://github.com/kubernetes/kubernetes/blob/bc41f37e85dd67ddb69eaa9e4da530c3c72101d7/cmd/kubeadm/app/util/staticpod/utils.go#L352
   In this case, kubeadm will replace the new yaml file to /etc/kubernetes/manifests, but the hash code of the pod won't make any difference.

Kubernetes version

$ kubeadm version
# v1.22.1

Cloud provider

OS version

# On Linux:
$ cat /etc/os-release
# paste output here
$ uname -a
# paste output here

# On Windows:
C:\> wmic os get Caption, Version, BuildNumber, OSArchitecture
# paste output here

Install tools

Container runtime (CRI) and version (if applicable)

Related plugins (CNI, CSI, ...) and versions (if applicable)

[k8s-ci-robot]

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

[ImNotAFrog]

/sig Cluster-Lifecycle

[pacoxu]

/area kubeadm
/transfer kubeadm

At my first glance, to patch etcd manifest, you may try the #2046 --patch feature. I need to dig into this later.

[neolit123]

Repeatedly get pod hash from cluster, if the hash has changed, means the pod has upgraded successfully.
The problem of this process is, kubeadm uses DeepHashObject in pkg/util/hash to generate hash code, this hash method will sort the fileds, which means the two yaml files like above will leads to a same hash code. Meanwhile, when compare yaml files, kubeadm simply comprares the files' bytes instead, see in: https://github.com/kubernetes/kubernetes/blob/bc41f37e85dd67ddb69eaa9e4da530c3c72101d7/cmd/kubeadm/app/util/staticpod/utils.go#L352
In this case, kubeadm will replace the new yaml file to /etc/kubernetes/manifests, but the hash code of the pod won't make any difference.

so when comparing yaml, we should also use DeepHashObject?

this seems like a bug.

occasionally

i don't understand why it would happen occasionally and not always thought?

[ImNotAFrog]

so when comparing yaml, we should also use DeepHashObject?

Totally agree with this, seems to be the most reasonable solution.

i don't understand why it would happen occasionally and not always thought?

In our case, if the new yaml file has the same order with the old one, this process would be skipped since ManifestFilesAreEqual returns true. And the annotation fileds order is random since its a map.

[neolit123]

Totally agree with this, seems to be the most reasonable solution.

ok, PRs welcome with a fix. we can backport this bugfix as blocking for upgrades in older versions too.