kubeadm: apply retries to all API calls in idempotency.go #123271

neolit123 · 2024-02-13T16:45:49Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

The idempotency.go (perhaps not so accurately named) contains API calls that kubeadm does against an API server using client-go.

Some users seem to have unstable setups where for unknown reasons the API server can be unavailable or refuse to respond as expected.

Use PollUntilContextTimeout in all exported functions to ensure such API calls are all retry-able.

NOTE: The context passed to PollUntilContextTimeout is not propagated in the polled function. Instead the poll function creates it's own context 'ctx := context.Background()', this is to avoid breaking expectations on the side of the callers, that expect a certain type of error and not "context timeout" errors.

Additional changes:

Make all context.TODO() -> context.Background()
Update all unit tests and make sure during testing the retry interval and timeout are short
Remove the TestMutateConfigMapWithConflict test. It does not contribute much, because conflict handling is done at the API, server side, not on the side of kubeadm. This simulating this is not needed.

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#1606

Special notes for your reviewer:

NONE

Does this PR introduce a user-facing change?

kubeadm: make sure that a variety of API server requests are retried during "init", "join", "upgrade", "reset" workflows. Prior to this change some API server requests, such as, creating or updating ConfigMaps were "one-shot" - i.e. they could fail if the API server dropped connectivity for a very short period of time.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

neolit123 · 2024-02-13T16:46:21Z

sending this initial PR as WIP to check if the kind jobs are happy.
a variety of tests needs to be updated here.

pacoxu · 2024-02-15T07:48:38Z

/cc

neolit123 · 2024-02-16T07:01:37Z

this is ready for review. there is a chance that something from this PR will break our e2e test assumption, but generally "retry all the things" is what we want. it has been discussed a few times in the past...it's just that nobody took action to send the code change.

/uncc @chendave
/cc @my-git9
(i wrote most of the unit tests from scratch)
/cc @SataQiu

neolit123

might be easier to review by looking at the files instead of diff

neolit123 · 2024-02-16T07:02:47Z

cmd/kubeadm/app/phases/addons/proxy/proxy_test.go

+	// Override the default timeouts to be shorter
+	defaultTimeouts := kubeadmapi.GetActiveTimeouts()
+	defaultAPICallTimeout := defaultTimeouts.KubernetesAPICall
+	defaultTimeouts.KubernetesAPICall = &metav1.Duration{Duration: time.Microsecond * 500}
+	defer func() {
+		defaultTimeouts.KubernetesAPICall = defaultAPICallTimeout
+	}()
+


doing these overrides for tests seems OK,
i ran time go test ... and saw no major increase in time for our unit tests.

neolit123 · 2024-02-16T07:03:35Z

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

@@ -81,131 +95,189 @@ func CreateOrMutateConfigMap(client clientset.Interface, cm *v1.ConfigMap, mutat
 	return lastError
 }

-// MutateConfigMap takes a ConfigMap Object Meta (namespace and name), retrieves the resource from the server and tries to mutate it
+// mutateConfigMap takes a ConfigMap Object Meta (namespace and name), retrieves the resource from the server and tries to mutate it


making this private as it was not used anywhere

neolit123 · 2024-02-16T07:09:16Z

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

-		if !apierrors.IsNotFound(err) {
-			return nil
-		}


all of these CreateOrRetain* functions had the wrong logic IMO, but please check if i am correct.

the logic used to be:

if the error is not a "not found " error return success.

if the error is a "not found" error create the object

i don't think 1. is correct, so i changed it:

if the error is not a "not found" error retry, it could be a connectivity error

same as above

these were added when the coredns migration logic was added, so hopefully they don't break anything.
upgrades work for me locally, but let's see.

+1
CreateOrRetainDeployment is an example.

neolit123 · 2024-02-16T07:10:44Z

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

+	err := wait.PollUntilContextTimeout(context.Background(),
+		apiCallRetryInterval, kubeadmapi.GetActiveTimeouts().KubernetesAPICall.Duration,
+		true, func(_ context.Context) (bool, error) {
+			ctx := context.Background()


this is explained in the release note too.
we don't want to use the context passed from here:

func(_ context.Context) (bool, error) {

because it changes expectations of the caller site if they are expecting e.g. an API error, but instead would get a context timeout error as the "last error"

neolit123 · 2024-02-16T07:12:51Z

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

+	os.Exit(exitVal)
+}
+
+func TestCreateOrUpdateConfigMap(t *testing.T) {


new tests added for everything.
coverage for this file was around 97%

neolit123 · 2024-02-16T07:13:43Z

/hold
/triage accepted
/priority important-longterm

pacoxu

/lgtm

pacoxu · 2024-02-18T07:51:35Z

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

-		if !apierrors.IsNotFound(err) {
-			return nil
-		}


+1
CreateOrRetainDeployment is an example.

pacoxu · 2024-02-18T08:42:30Z

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

@@ -139,7 +939,7 @@ func TestPatchNode(t *testing.T) {
 				}
 			}, &lastError)
 			success, err := conditionFunction(context.Background())
-			if err != nil {
+			if err != nil && success {


tc.success is more readable to me.

yes that's better. updated
https://github.com/kubernetes/kubernetes/compare/2941a644905755c4a89f73024666dc50da471ad2..c29450eb0090e53db16c37a6468a57f6b916f827

k8s-ci-robot · 2024-02-18T08:43:29Z

LGTM label has been added.

Git tree hash: c931b8898ccebdad5ec93c091e89a256bf3d69f1

k8s-ci-robot · 2024-02-18T08:43:43Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,pacoxu]

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

The idempotency.go (perhaps not so accurately named) contains API calls that kubeadm does against an API server using client-go. Some users seem to have unstable setups where for unknown reasons the API server can be unavailable or refuse to respond as expected. Use PollUntilContextTimeout in all exported functions to ensure such API calls are all retry-able. NOTE: The context passed to PollUntilContextTimeout is not propagated in the polled function. Instead the poll function creates it's own context 'ctx := context.Background()', this is to avoid breaking expectations on the side of the callers, that expect a certain type of error and not "context timeout" errors. Additional changes: - Make all context.TODO() -> context.Background() - Update all unit tests and make sure during testing the retry interval and timeout are short. Test coverage of idempotency.go is at ~97%. - Remove the TestMutateConfigMapWithConflict test. It does not contribute much, because conflict handling is done at the API, server side, not on the side of kubeadm. This simulating this is not needed.

pacoxu · 2024-02-18T12:35:23Z

/lgtm

k8s-ci-robot · 2024-02-18T12:35:28Z

LGTM label has been added.

Git tree hash: c331b4d286def46a53c2ce1d22aab6dda8eecf1f

SataQiu

LGTM

my-git9

/lgtm

pacoxu · 2024-02-19T08:30:03Z

/unhold

neolit123 · 2024-02-19T09:04:40Z

let's run this in e2e CI and watch for problems so that i can fix them before CF.

k8s-ci-robot requested review from chendave and SataQiu February 13, 2024 16:46

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 13, 2024

k8s-ci-robot removed the do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Feb 13, 2024

neolit123 mentioned this pull request Feb 13, 2024

evaluate the retry logic for API calls kubernetes/kubeadm#1606

Closed

k8s-ci-robot requested a review from pacoxu February 15, 2024 07:48

neolit123 force-pushed the 1.30-retry-all-api-calls branch from 763b684 to d533d70 Compare February 15, 2024 18:16

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 15, 2024

neolit123 force-pushed the 1.30-retry-all-api-calls branch 2 times, most recently from 3fd4778 to a2957ae Compare February 15, 2024 19:57

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 15, 2024

neolit123 force-pushed the 1.30-retry-all-api-calls branch from a2957ae to 2941a64 Compare February 15, 2024 20:38

k8s-ci-robot requested review from my-git9 and removed request for chendave February 16, 2024 07:01

neolit123 commented Feb 16, 2024

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 16, 2024

neolit123 changed the title ~~WIP: kubeadm: apply retries to all API calls in idempotency.go~~ kubeadm: apply retries to all API calls in idempotency.go Feb 16, 2024

pacoxu approved these changes Feb 18, 2024

View reviewed changes

k8s-ci-robot assigned pacoxu Feb 18, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 18, 2024

neolit123 force-pushed the 1.30-retry-all-api-calls branch from 2941a64 to c29450e Compare February 18, 2024 11:14

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 18, 2024

k8s-ci-robot requested a review from pacoxu February 18, 2024 11:14

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 18, 2024

SataQiu reviewed Feb 19, 2024

View reviewed changes

my-git9 reviewed Feb 19, 2024

View reviewed changes

k8s-ci-robot assigned my-git9 Feb 19, 2024

my-git9 removed their assignment Feb 19, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 19, 2024

k8s-ci-robot merged commit 7225dc6 into kubernetes:master Feb 19, 2024
14 checks passed

k8s-ci-robot added this to the v1.30 milestone Feb 19, 2024

neolit123 mentioned this pull request Feb 19, 2024

kubeadm: introduce the WaitForAllControlPlaneComponents feature gate #123341

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: apply retries to all API calls in idempotency.go #123271

kubeadm: apply retries to all API calls in idempotency.go #123271

neolit123 commented Feb 13, 2024

neolit123 commented Feb 13, 2024

pacoxu commented Feb 15, 2024

neolit123 commented Feb 16, 2024

neolit123 left a comment

neolit123 Feb 16, 2024

neolit123 Feb 16, 2024

neolit123 Feb 16, 2024

pacoxu Feb 18, 2024

neolit123 Feb 16, 2024

neolit123 Feb 16, 2024

neolit123 commented Feb 16, 2024

pacoxu left a comment

pacoxu Feb 18, 2024

pacoxu Feb 18, 2024

neolit123 Feb 18, 2024

k8s-ci-robot commented Feb 18, 2024

k8s-ci-robot commented Feb 18, 2024

pacoxu commented Feb 18, 2024

k8s-ci-robot commented Feb 18, 2024

SataQiu left a comment

my-git9 left a comment

pacoxu commented Feb 19, 2024

neolit123 commented Feb 19, 2024

kubeadm: apply retries to all API calls in idempotency.go #123271

kubeadm: apply retries to all API calls in idempotency.go #123271

Conversation

neolit123 commented Feb 13, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

neolit123 commented Feb 13, 2024

pacoxu commented Feb 15, 2024

neolit123 commented Feb 16, 2024

neolit123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 commented Feb 16, 2024

pacoxu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 18, 2024

k8s-ci-robot commented Feb 18, 2024

pacoxu commented Feb 18, 2024

k8s-ci-robot commented Feb 18, 2024

SataQiu left a comment

Choose a reason for hiding this comment

my-git9 left a comment

Choose a reason for hiding this comment

pacoxu commented Feb 19, 2024

neolit123 commented Feb 19, 2024