Skip to content
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

Revert "UpdateOrCreateToken get secrets err handling optimization" #115068

Merged
merged 1 commit into from
Jan 15, 2023
Merged

Revert "UpdateOrCreateToken get secrets err handling optimization" #115068

merged 1 commit into from
Jan 15, 2023

Conversation

neolit123
Copy link
Member

@neolit123 neolit123 commented Jan 13, 2023 •
edited

Reverts #115000

the PR caused a panic in kubeadm init to fail:

I0113 07:57:13.544782     300 round_trippers.go:553] GET https://172.17.0.7:6443/api/v1/namespaces/kube-system/secrets/bootstrap-token-82xr2k?timeout=10s 404 Not Found in 2 milliseconds
a token with id "82xr2k" already exists
k8s.io/kubernetes/cmd/kubeadm/app/phases/bootstraptoken/node.UpdateOrCreateTokens
	cmd/kubeadm/app/phases/bootstraptoken/node/token.go:49
k8s.io/kubernetes/cmd/kubeadm/app/phases/bootstraptoken/node.CreateNewTokens
	cmd/kubeadm/app/phases/bootstraptoken/node/token.go:35
k8s.io/kubernetes/cmd/kubeadm/app/phases/copycerts.createShortLivedBootstrapToken
	cmd/kubeadm/app/phases/copycerts/copycerts.go:73
k8s.io/kubernetes/cmd/kubeadm/app/phases/copycerts.UploadCerts
	cmd/kubeadm/app/phases/copycerts/copycerts.go:95
k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/init.runUploadCerts
	cmd/kubeadm/app/cmd/phases/init/uploadcerts.go:71
k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow.(*Runner).Run.func1
	cmd/kubeadm/app/cmd/phases/workflow/runner.go:259
k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow.(*Runner).visitAll
	cmd/kubeadm/app/cmd/phases/workflow/runner.go:446
k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases/workflow.(*Runner).Run
	cmd/kubeadm/app/cmd/phases/workflow/runner.go:232
k8s.io/kubernetes/cmd/kubeadm/app/cmd.newCmdInit.func1

https://storage.googleapis.com/kubernetes-jenkins/logs/ci-kubernetes-e2e-kubeadm-kinder-kubelet-1-25-on-latest/1613806184970588160/build-log.txt

https://testgrid.k8s.io/sig-cluster-lifecycle-kubeadm#kubeadm-kinder-kubelet-1-25-on-latest

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 13, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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 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. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 13, 2023
@neolit123
Copy link
Member Author

/kind regression
/priority important-soon
/triage accepted
/release-note-none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/regression Categorizes issue or PR as related to a regression from a prior release. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on. 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. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 13, 2023
@neolit123
Copy link
Member Author

cc @helen-frank @pacoxu

@neolit123
Copy link
Member Author

neolit123 commented Jan 13, 2023 •
edited

i propose we:

helen-frank
helen-frank reviewed Jan 14, 2023
View reviewed changes
cmd/kubeadm/app/phases/bootstraptoken/node/token.go
return err
}
if secret != nil && failIfExists {
if secret != nil && err == nil && failIfExists {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the code I wrote before is correct.

else if err != nil && !apierrors.IsNotFound(err) {
			return err
}

Copy link
Contributor

Choose a reason for hiding this comment

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

If you are not available, I will test this problem as soon as possible(It is expected to be the day after tomorrow.). There are some travel plans recently 😀.

Copy link
Member Author

Choose a reason for hiding this comment

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

we should revert the change to unblock the ci first.

#115068 (comment)

looking for lgtm.

Copy link
Contributor

Choose a reason for hiding this comment

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

we should revert the change to unblock the ci first.

#115068 (comment)

looking for lgtm.

got it

helen-frank added a commit to helen-frank/karmada that referenced this pull request Jan 14, 2023
@helen-frank
fix bug(when err == notfound, the logical judgment may be considered …
6eda942 
…as existing) refer to kubernetes/kubernetes#115068

Signed-off-by: helen <haitao.zhang@daocloud.io>
@neolit123
Copy link
Member Author

/retest

@helen-frank
Copy link
Contributor

/lgtm

@k8s-ci-robot
Copy link
Contributor

@helen-frank: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

@pacoxu
Copy link
Member

pacoxu commented Jan 15, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 15, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 6a2cc6c26bb6d933ae29160095f318dca126d0ff

@k8s-ci-robot k8s-ci-robot merged commit fd885b7 into kubernetes:master Jan 15, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.27 milestone Jan 15, 2023
@lonelyCZ lonelyCZ mentioned this pull request Feb 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@helen-frank helen-frank helen-frank left review comments

@chendave chendave Awaiting requested review from chendave

@pacoxu pacoxu Awaiting requested review from pacoxu

Assignees

@pacoxu pacoxu

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/regression Categorizes issue or PR as related to a regression from a prior release. 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-none Denotes a PR that doesn't merit a release note. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.27
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@neolit123 @k8s-ci-robot @helen-frank @pacoxu