Skip to content

kubeadm: Add retries for kubeadm join / UpdateStatus #91952

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jun 11, 2020
Merged

kubeadm: Add retries for kubeadm join / UpdateStatus #91952

merged 3 commits into from
Jun 11, 2020
+49 −31

Conversation

xlgao-zju
Copy link
Contributor

What type of PR is this?
/kind bug

What this PR does / why we need it:
To make update status more resilient by adding a retry loop to this operation

Which issue(s) this PR fixes:
Fixes kubernetes/kubeadm#2092

Special notes for your reviewer:
/assign @neolit123
/assign @fabriziopandini
/assign @rosti

Does this PR introduce a user-facing change?: NONE

kubeadm: Add retries for kubeadm join / UpdateStatus to make update status more resilient by adding a retry loop to this operation

Xianglin Gao added 2 commits June 10, 2020 00:12
Add retries for CreateOrUpdateRole

Unverified

The committer email address is not verified.
Learn about vigilant mode
052eb7d 
Signed-off-by: Xianglin Gao <xianglin.gxl@alibaba-inc.com>
Add retries for CreateOrUpdateRoleBinding
6d572ea 
Signed-off-by: Xianglin Gao <xianglin.gxl@alibaba-inc.com>
@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 9, 2020
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubeadm labels Jun 9, 2020
@k8s-ci-robot k8s-ci-robot requested review from detiber and timothysc June 9, 2020 16:39
@k8s-ci-robot k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 9, 2020
@xlgao-zju xlgao-zju mentioned this pull request Jun 9, 2020
Closed
@xlgao-zju
Copy link
Contributor Author

/test pull-kubernetes-integration

rosti
rosti approved these changes Jun 10, 2020
View reviewed changes
Copy link
Contributor

@rosti rosti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @xlgao-zju !
/approve

cmd/kubeadm/app/util/apiclient/idempotency.go
return errors.Wrap(err, "unable to create RBAC role")
}
return wait.PollImmediate(constants.APICallRetryInterval, constants.APICallWithWriteTimeout, func() (bool, error) {
if _, err := client.RbacV1().Roles(role.ObjectMeta.Namespace).Create(context.TODO(), role, metav1.CreateOptions{}); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to myself: context.TODO() nested in a PollImmediate() is not a viable long term thing.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rosti, xlgao-zju

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 10, 2020
@rosti
Copy link
Contributor

rosti commented Jun 10, 2020

/retest

@neolit123
Copy link
Member

i confused why we closed #91815
but the retries in CreateOrMutateConfigMap and MutateConfigMap which are used in cmd/kubeadm/app/phases/uploadconfig/uploadconfig.go are not sufficient as they only result in linear (500ms) retries for a total of 10 seconds (20 steps).

ideally these calls should retry for ~40 seconds (e.g. APICallWithWriteTimeout) and one option is to bump them to 80 steps, but this is a bit silly.

in my opining it is better to refactor them to use:

err = wait.PollImmediate(kubeadmconstants.APICallRetryInterval, kubeadmconstants.APICallWithWriteTimeout, func() (bool, error) {

@xlgao-zju
Copy link
Contributor Author

@neolit123 I have refacted CreateOrMutateConfigMap and MutateConfigMap with PollImmediate. PTAL

refact CreateOrMutateConfigMap and MutateConfigMap with PollImmediate
04ef362 
Signed-off-by: Xianglin Gao <xianglin.gxl@alibaba-inc.com>
@xlgao-zju xlgao-zju force-pushed the add-retries-for-updatestautes branch from d5000b7 to 04ef362 Compare June 10, 2020 16:32
@neolit123
Copy link
Member

/lgtm
/retest
/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 10, 2020
@k8s-ci-robot k8s-ci-robot merged commit 0a5d706 into kubernetes:master Jun 11, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jun 11, 2020
@xlgao-zju xlgao-zju deleted the add-retries-for-updatestautes branch June 11, 2020 06:46
This was referenced Jun 11, 2020
Closed
Closed
@github-actions github-actions bot mentioned this pull request Jun 16, 2020
Open
@neolit123 neolit123 mentioned this pull request Jun 22, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rosti rosti

@detiber detiber

@timothysc timothysc

Assignees

@neolit123 neolit123

@rosti rosti

@fabriziopandini fabriziopandini

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.

Add retries for kubeadm join / UpdateStatus
5 participants
@xlgao-zju @k8s-ci-robot @rosti @neolit123 @fabriziopandini