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.

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

fix re-taint operation only applied to KubeletBootOp target node #699

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

YZ775
Copy link
Contributor

@YZ775 YZ775 commented Feb 1, 2024

There are problem that when CKE is unable to SSH to nodes, it adds taints even if it is not necessary.
In order to fix this, I changed re-taint process to be done only on the target node of KubeletBootOp.

@YZ775 YZ775 changed the title fix retaint operation fix re-taint operation only applied to KubeletBootOp target node Feb 2, 2024
@YZ775 YZ775 marked this pull request as ready for review February 2, 2024 02:44
@YZ775 YZ775 requested a review from masa213f February 2, 2024 02:45
@@ -183,7 +183,7 @@ func k8sOps(c *cke.Cluster, nf *NodeFilter, cs *cke.ClusterStatus, maxConcurrent
if len(nodes) < max {
max = len(nodes)
}
ops = append(ops, k8s.KubeletBootOp(nodes[:max], nf.KubeletStoppedRegisteredNodes(), apiServer, c.Name, c.Options.Kubelet, cs.NodeStatuses))
ops = append(ops, k8s.KubeletBootOp(nodes[:max], nf.SSHConnectedNodes(nf.RegisteredNodes(nodes[:max]), true, true), apiServer, c.Name, c.Options.Kubelet, cs.NodeStatuses))
Copy link
Contributor

Choose a reason for hiding this comment

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

nodes is already filterd in L181. So we need not to filter again here.

Suggested change
ops = append(ops, k8s.KubeletBootOp(nodes[:max], nf.SSHConnectedNodes(nf.RegisteredNodes(nodes[:max]), true, true), apiServer, c.Name, c.Options.Kubelet, cs.NodeStatuses))
ops = append(ops, k8s.KubeletBootOp(nodes[:max], nf.RegisteredNodes(nodes[:max]), apiServer, c.Name, c.Options.Kubelet, cs.NodeStatuses))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, it's my mistake.
I fixed it.

Copy link
Contributor

@masa213f masa213f left a comment

Choose a reason for hiding this comment

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

LGTM

@YZ775 YZ775 merged commit 53b6f80 into main Feb 2, 2024
7 checks passed
@YZ775 YZ775 deleted the fix-retaint branch February 2, 2024 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants