From bd8f26c2d785e4ec4277d2a3556a4b65a1eef562 Mon Sep 17 00:00:00 2001 From: haoyun Date: Sun, 17 Oct 2021 12:36:36 +0800 Subject: [PATCH] fix: patchNode retry logic Signed-off-by: haoyun --- cmd/kubeadm/app/util/apiclient/idempotency.go | 30 +++++++++++-------- .../app/util/apiclient/idempotency_test.go | 3 +- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/cmd/kubeadm/app/util/apiclient/idempotency.go b/cmd/kubeadm/app/util/apiclient/idempotency.go index 5f0b928e419..978d7423731 100644 --- a/cmd/kubeadm/app/util/apiclient/idempotency.go +++ b/cmd/kubeadm/app/util/apiclient/idempotency.go @@ -19,7 +19,6 @@ package apiclient import ( "context" "encoding/json" - "fmt" "github.com/pkg/errors" @@ -278,13 +277,13 @@ func CreateOrUpdateClusterRoleBinding(client clientset.Interface, clusterRoleBin // This is a condition function meant to be used with wait.Poll. false, nil // implies it is safe to try again, an error indicates no more tries should be // made and true indicates success. -func PatchNodeOnce(client clientset.Interface, nodeName string, patchFn func(*v1.Node)) func() (bool, error) { +func PatchNodeOnce(client clientset.Interface, nodeName string, patchFn func(*v1.Node), lastError *error) func() (bool, error) { return func() (bool, error) { // First get the node object n, err := client.CoreV1().Nodes().Get(context.TODO(), nodeName, metav1.GetOptions{}) if err != nil { - // TODO this should only be for timeouts - return false, nil + *lastError = err + return false, nil // retry on any error } // The node may appear to have no labels at first, @@ -295,7 +294,8 @@ func PatchNodeOnce(client clientset.Interface, nodeName string, patchFn func(*v1 oldData, err := json.Marshal(n) if err != nil { - return false, errors.Wrapf(err, "failed to marshal unmodified node %q into JSON", n.Name) + *lastError = errors.Wrapf(err, "failed to marshal unmodified node %q into JSON", n.Name) + return false, *lastError } // Execute the mutating function @@ -303,21 +303,22 @@ func PatchNodeOnce(client clientset.Interface, nodeName string, patchFn func(*v1 newData, err := json.Marshal(n) if err != nil { - return false, errors.Wrapf(err, "failed to marshal modified node %q into JSON", n.Name) + *lastError = errors.Wrapf(err, "failed to marshal modified node %q into JSON", n.Name) + return false, *lastError } patchBytes, err := strategicpatch.CreateTwoWayMergePatch(oldData, newData, v1.Node{}) if err != nil { - return false, errors.Wrap(err, "failed to create two way merge patch") + *lastError = errors.Wrap(err, "failed to create two way merge patch") + return false, *lastError } if _, err := client.CoreV1().Nodes().Patch(context.TODO(), n.Name, types.StrategicMergePatchType, patchBytes, metav1.PatchOptions{}); err != nil { - // TODO also check for timeouts - if apierrors.IsConflict(err) { - fmt.Println("Temporarily unable to update node metadata due to conflict (will retry)") + *lastError = errors.Wrapf(err, "error patching node %q through apiserver", n.Name) + if apierrors.IsTimeout(err) || apierrors.IsConflict(err) { return false, nil } - return false, errors.Wrapf(err, "error patching node %q through apiserver", n.Name) + return false, *lastError } return true, nil @@ -327,10 +328,15 @@ func PatchNodeOnce(client clientset.Interface, nodeName string, patchFn func(*v1 // PatchNode tries to patch a node using patchFn for the actual mutating logic. // Retries are provided by the wait package. func PatchNode(client clientset.Interface, nodeName string, patchFn func(*v1.Node)) error { + var lastError error // wait.Poll will rerun the condition function every interval function if // the function returns false. If the condition function returns an error // then the retries end and the error is returned. - return wait.Poll(constants.APICallRetryInterval, constants.PatchNodeTimeout, PatchNodeOnce(client, nodeName, patchFn)) + err := wait.Poll(constants.APICallRetryInterval, constants.PatchNodeTimeout, PatchNodeOnce(client, nodeName, patchFn, &lastError)) + if err == nil { + return nil + } + return lastError } // GetConfigMapWithRetry tries to retrieve a ConfigMap using the given client, diff --git a/cmd/kubeadm/app/util/apiclient/idempotency_test.go b/cmd/kubeadm/app/util/apiclient/idempotency_test.go index a635622d4b7..35976333bd9 100644 --- a/cmd/kubeadm/app/util/apiclient/idempotency_test.go +++ b/cmd/kubeadm/app/util/apiclient/idempotency_test.go @@ -74,11 +74,12 @@ func TestPatchNodeNonErrorCases(t *testing.T) { if err != nil { t.Fatalf("failed to create node to fake client: %v", err) } + var lastError error conditionFunction := PatchNodeOnce(client, tc.lookupName, func(node *v1.Node) { node.Annotations = map[string]string{ "updatedBy": "test", } - }) + }, &lastError) success, err := conditionFunction() if err != nil { t.Fatalf("did not expect error: %v", err)