kubeadm: fix some retry logic in PatchNodeOnce #105343

jonyhy96 · 2021-09-29T09:29:57Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

fix the retry logic in PatchNodeOnce

Special notes for your reviewer:

None

k8s-ci-robot · 2021-09-29T09:30:05Z

Hi @jonyhy96. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

RA489 · 2021-09-29T11:22:42Z

/release-note-none

cmd/kubeadm/app/util/apiclient/idempotency.go

neolit123 · 2021-09-29T16:39:10Z

/priority awaiting-more-evidence
/triage accepted

jonyhy96 · 2021-09-30T01:35:16Z

Thanks for review! ping @neolit123

jonyhy96 · 2021-10-12T12:38:19Z

/retest

jonyhy96 · 2021-10-12T16:08:32Z

/retest

neolit123 · 2021-10-12T16:35:39Z

the kind CI failures seem somewhat related to the patch node changes here:

I1012 16:23:28.581402     214 patchnode.go:31] [patchnode] Uploading the CRI Socket information "unix:///run/containerd/containerd.sock" to the Node API object "kind-worker2" as an annotation
I1012 16:23:28.599223     214 round_trippers.go:541] GET https://kind-control-plane:6443/api/v1/nodes/kind-worker2?timeout=10s 404 Not Found in 17 milliseconds
I1012 16:23:28.612455     214 round_trippers.go:541] GET https://kind-control-plane:6443/api/v1/nodes/kind-worker2?timeout=10s 404 Not Found in 1 milliseconds
I1012 16:23:28.666911     214 round_trippers.go:541] GET https://kind-control-plane:6443/api/v1/nodes/kind-worker2?timeout=10s 404 Not Found in 2 milliseconds
I1012 16:23:28.937311     214 round_trippers.go:541] GET https://kind-control-plane:6443/api/v1/nodes/kind-worker2?timeout=10s 404 Not Found in 2 milliseconds
nodes "kind-worker2" not found
error uploading crisocket
k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/join.runKubeletStartJoinPhase
	/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/join/kubelet.go:220
k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow.(*Runner).Run.func1
	/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow/runner.go:234
k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow.(*Runner).visitAll
	/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow/runner.go:421
k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow.(*Runner).Run
....

https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/105343/pull-kubernetes-e2e-kind/1447957425293365248/build-log.txt

we should be careful with similar refactors to not break kubeadm, given the lack of unit tests.
i encourage you to test it locally with kind or with a two nodes (VMs) with bare kubeadm.

jonyhy96 · 2021-10-13T06:21:59Z

the kind CI failures seem somewhat related to the patch node changes here:

I1012 16:23:28.581402     214 patchnode.go:31] [patchnode] Uploading the CRI Socket information "unix:///run/containerd/containerd.sock" to the Node API object "kind-worker2" as an annotation
I1012 16:23:28.599223     214 round_trippers.go:541] GET https://kind-control-plane:6443/api/v1/nodes/kind-worker2?timeout=10s 404 Not Found in 17 milliseconds
I1012 16:23:28.612455     214 round_trippers.go:541] GET https://kind-control-plane:6443/api/v1/nodes/kind-worker2?timeout=10s 404 Not Found in 1 milliseconds
I1012 16:23:28.666911     214 round_trippers.go:541] GET https://kind-control-plane:6443/api/v1/nodes/kind-worker2?timeout=10s 404 Not Found in 2 milliseconds
I1012 16:23:28.937311     214 round_trippers.go:541] GET https://kind-control-plane:6443/api/v1/nodes/kind-worker2?timeout=10s 404 Not Found in 2 milliseconds
nodes "kind-worker2" not found
error uploading crisocket
k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/join.runKubeletStartJoinPhase
	/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/join/kubelet.go:220
k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow.(*Runner).Run.func1
	/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow/runner.go:234
k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow.(*Runner).visitAll
	/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow/runner.go:421
k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow.(*Runner).Run
....

https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/105343/pull-kubernetes-e2e-kind/1447957425293365248/build-log.txt

we should be careful with similar refactors to not break kubeadm, given the lack of unit tests. i encourage you to test it locally with kind or with a two nodes (VMs) with bare kubeadm.

The reason for this failure is because we didn’t have enough time to retry.

jonyhy96 · 2021-10-13T07:15:40Z

/test pull-kubernetes-e2e-gce-ubuntu-containerd

pacoxu · 2021-10-14T08:58:00Z

/test pull-kubernetes-e2e-kind

neolit123 · 2021-10-14T16:05:44Z

cmd/kubeadm/app/util/apiclient/idempotency.go

+		Factor:   2.0,
+		Jitter:   0,
+	}
+	err := wait.ExponentialBackoff(backOff, PatchNodeOnce(client, nodeName, patchFn, &lastError))


something is not right here, why did wait.Poll(constants.APICallRetryInterval, constants.PatchNodeTimeout... worked before and not after the change?

these interval / timeout constants have been used for a very long time in kubeadm?

wait.Poll's retry logic is diffirent from wait.ExponentialBackoff's.

failed message from https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/105343/pull-kubernetes-e2e-kind/1447957425293365248/build-log.txt

I1012 16:23:28.581402 214 patchnode.go:31] [patchnode] Uploading the CRI Socket information "unix:///run/containerd/containerd.sock" to the Node API object "kind-worker2" as an annotation I1012 16:23:28.599223 214 round_trippers.go:541] GET https://kind-control-plane:6443/api/v1/nodes/kind-worker2?timeout=10s 404 Not Found in 17 milliseconds I1012 16:23:28.612455 214 round_trippers.go:541] GET https://kind-control-plane:6443/api/v1/nodes/kind-worker2?timeout=10s 404 Not Found in 1 milliseconds I1012 16:23:28.666911 214 round_trippers.go:541] GET https://kind-control-plane:6443/api/v1/nodes/kind-worker2?timeout=10s 404 Not Found in 2 milliseconds I1012 16:23:28.937311 214 round_trippers.go:541] GET https://kind-control-plane:6443/api/v1/nodes/kind-worker2?timeout=10s 404 Not Found in 2 milliseconds

success message from https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/105343/pull-kubernetes-e2e-kind/1448583659891200000/build-log.txt

I1014 09:54:30.367606 213 patchnode.go:31] [patchnode] Uploading the CRI Socket information "unix:///run/containerd/containerd.sock" to the Node API object "kind-worker2" as an annotation I1014 09:54:30.390173 213 round_trippers.go:541] GET https://kind-control-plane:6443/api/v1/nodes/kind-worker2?timeout=10s 404 Not Found in 22 milliseconds I1014 09:54:30.895376 213 round_trippers.go:541] GET https://kind-control-plane:6443/api/v1/nodes/kind-worker2?timeout=10s 404 Not Found in 4 milliseconds I1014 09:54:31.900145 213 round_trippers.go:541] GET https://kind-control-plane:6443/api/v1/nodes/kind-worker2?timeout=10s 200 OK in 3 milliseconds I1014 09:54:31.912029 213 round_trippers.go:541] PATCH https://kind-control-plane:6443/api/v1/nodes/kind-worker2?timeout=10s 200 OK in 6 milliseconds

We can see when use DefaultBackoff in ExponentialBackoff, it steps too fast to wait for kind-worker2 registry on apiserver. When using wait.Poll it will retry in every 500ms and timeout in 2min which gives it enough time to wait.This backOff is similar to the behavior of wait.Poll that it will timeout at 127.5s.

I know that the functions are different. It seems out of scope of this PR to change the function. We should continue to use Poll with the old constants.

Done, use wait.Poll instead

neolit123

thanks, this looks good now.
please squash the commits to 1 and i will LGTM.

/remove-priority awaiting-more-evidence
/priority backlog
/approve

k8s-ci-robot · 2021-10-16T17:32:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jonyhy96, neolit123

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:

~~cmd/kubeadm/OWNERS~~ [neolit123]

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

Signed-off-by: haoyun <yun.hao@daocloud.io>

jonyhy96 · 2021-10-17T05:36:35Z

/test pull-kubernetes-unit

neolit123

/lgtm

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 29, 2021

k8s-ci-robot requested review from fabriziopandini and RA489 September 29, 2021 09:30

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 29, 2021

jonyhy96 changed the title ~~fix: make patch node once support retry when timeout~~ kubeadm: fix some retry logic in PatchNodeOnce Sep 29, 2021

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 29, 2021

neolit123 reviewed Sep 29, 2021

View reviewed changes

cmd/kubeadm/app/util/apiclient/idempotency.go Outdated Show resolved Hide resolved

neolit123 reviewed Sep 29, 2021

View reviewed changes

cmd/kubeadm/app/util/apiclient/idempotency.go Outdated Show resolved Hide resolved

k8s-ci-robot requested review from pacoxu and SataQiu September 30, 2021 13:09

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 12, 2021

jonyhy96 force-pushed the fix-patch-node-once branch from bfbc5eb to 8e5ca51 Compare October 12, 2021 12:20

neolit123 reviewed Oct 14, 2021

View reviewed changes

neolit123 reviewed Oct 16, 2021

View reviewed changes

k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. labels Oct 16, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 16, 2021

fix: patchNode retry logic

bd8f26c

Signed-off-by: haoyun <yun.hao@daocloud.io>

jonyhy96 force-pushed the fix-patch-node-once branch from 50dc3bc to bd8f26c Compare October 17, 2021 04:36

neolit123 reviewed Oct 17, 2021

View reviewed changes

k8s-ci-robot assigned neolit123 Oct 17, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2021

k8s-ci-robot merged commit 9804a83 into kubernetes:master Oct 17, 2021

k8s-ci-robot added this to the v1.23 milestone Oct 17, 2021

jonyhy96 deleted the fix-patch-node-once branch October 18, 2021 06:15

This was referenced Oct 18, 2021

REQUEST: New membership for jonyhy96 kubernetes/org#3046

Closed

kubeadm: add unit tests for PatchNode error handling #105753

Merged

pacoxu mentioned this pull request Oct 29, 2021

GetConfigMapWithRetry is a common function, should not share global d… #105645

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: fix some retry logic in PatchNodeOnce #105343

kubeadm: fix some retry logic in PatchNodeOnce #105343

jonyhy96 commented Sep 29, 2021

k8s-ci-robot commented Sep 29, 2021

RA489 commented Sep 29, 2021

neolit123 commented Sep 29, 2021

jonyhy96 commented Sep 30, 2021

jonyhy96 commented Oct 12, 2021

jonyhy96 commented Oct 12, 2021

neolit123 commented Oct 12, 2021 •

edited

jonyhy96 commented Oct 13, 2021 •

edited

jonyhy96 commented Oct 13, 2021

pacoxu commented Oct 14, 2021

neolit123 Oct 14, 2021 •

edited

jonyhy96 Oct 15, 2021

neolit123 Oct 15, 2021 •

edited

jonyhy96 Oct 15, 2021

neolit123 left a comment

k8s-ci-robot commented Oct 16, 2021

jonyhy96 commented Oct 17, 2021

neolit123 left a comment

kubeadm: fix some retry logic in PatchNodeOnce #105343

kubeadm: fix some retry logic in PatchNodeOnce #105343

Conversation

jonyhy96 commented Sep 29, 2021

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

k8s-ci-robot commented Sep 29, 2021

RA489 commented Sep 29, 2021

neolit123 commented Sep 29, 2021

jonyhy96 commented Sep 30, 2021

jonyhy96 commented Oct 12, 2021

jonyhy96 commented Oct 12, 2021

neolit123 commented Oct 12, 2021 • edited

jonyhy96 commented Oct 13, 2021 • edited

jonyhy96 commented Oct 13, 2021

pacoxu commented Oct 14, 2021

neolit123 Oct 14, 2021 • edited

Choose a reason for hiding this comment

jonyhy96 Oct 15, 2021

Choose a reason for hiding this comment

neolit123 Oct 15, 2021 • edited

Choose a reason for hiding this comment

jonyhy96 Oct 15, 2021

Choose a reason for hiding this comment

neolit123 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 16, 2021

jonyhy96 commented Oct 17, 2021

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 commented Oct 12, 2021 •

edited

jonyhy96 commented Oct 13, 2021 •

edited

neolit123 Oct 14, 2021 •

edited

neolit123 Oct 15, 2021 •

edited