Reconcile NoExecute Taint #89059

chenkaiyue · 2020-03-11T18:05:34Z

What type of PR is this?
/kind bug

What happened:
When node ready condition is unknown or false, node-lifecycle-controller will add NoSchedule and NoExecute taints on it, such as:

taints:
  - effect: NoSchedule
    key: node.kubernetes.io/not-ready
    timeAdded: "2020-03-11T16:27:55Z"
  - effect: NoExecute
    key: node.kubernetes.io/not-ready
    timeAdded: "2020-03-11T16:27:57Z"

While, if these two taints are removed by others, an easy way is just deleting them manually by kubectl edit node. After a while, NoSchedule taint will be added back, but NoExecute taint won't. The taints is like this:

taints:
  - effect: NoSchedule
    key: node.kubernetes.io/not-ready
    timeAdded: "2020-03-11T16:41:23Z"

What you expected to happen:
We know that NoSchedule and NoExecute taints depend on node's condition, node ready condition will determine what taints a node should have, node-lifecycle-controller just does this job and updates node's taints according to the node condition.

Cause the node condition doesn't change, is still unknown or false, so the corresponding taints should still be there by node-lifecycle-controller's reconcile process.

So, what i expected to happen is even the taints are removed, the taints should be added back by reconciling, not only NoSchedule taints but also NoExecute taint, just like this:

taints:
  - effect: NoSchedule
    key: node.kubernetes.io/not-ready
    timeAdded: "2020-03-11T16:41:23Z"
  - effect: NoExecute
    key: node.kubernetes.io/not-ready
    timeAdded: "2020-03-11T16:41:29Z"

How to reproduce it (as minimally and precisely as possible):
1.Stop a node's kubelet to make node's condition to be unknown, systemctl stop kubelet
2.When node condition is unknown, kubectl edit node <node-id> -o yaml and delete node.kubernetes.io/unreachable NoSchedule and NoExecute taints.
3.Wait for a while, you can find that there is only node.kubernetes.io/unreachable NoSchedule taint be added, but no NoExecute taints by kubectl get node <node-id> -o yaml.

Environment:
As far as I know, 1.14,1.16 and the master branch of kubernetes have this bug.

Why this bug happened:
UnSchedule taints generated from node informer, so when node object changed, UpdateFunc will be called, what taint should be added and what taint should be deleted is calculated according to the node condition. So even we delete NoSchedule taint, after calculated and compared, taint will be added back.

For NoExecute taint, its data resource is zoneNoExecuteTainter in monitorNodeHealth, when node condition is unknown or false, node will be added into the queue of zoneNoExecuteTainter, at the same time, node will also be added into a set of the queue. So, the same node won't be add into the queue in the next cycle of monitorNodeHealth because it is already in the set.

I think maybe the set is used to prevent duplicated same node object to be enqueued because monitorNodeHealth will be ran in few seconds, but this also causes the bug as stated above. An easy way is to check whether nodes already have the responding NoExecute taints, if no, just remove node from the set and enqueue it for adding NoExecute taint; If yes, we can ignore this duplicated object as before.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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

k8s-ci-robot · 2020-03-11T18:05:42Z

Welcome @chenkaiyue!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-03-11T18:05:42Z

Hi @chenkaiyue. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

chenkaiyue · 2020-03-11T18:08:18Z

/assign @gmarek

k8s-ci-robot · 2020-03-11T18:08:19Z

@chenkaiyue: GitHub didn't allow me to assign the following users: gmare.

Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @Gmare

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.

attlee-wang · 2020-03-12T02:55:32Z

@chenkaiyue codes add function testing and formatting

chenkaiyue · 2020-03-12T04:04:58Z

@chenkaiyue codes add function testing and formatting

done

yuzhiquan · 2020-03-12T15:36:40Z

/ok-to-test

chenkaiyue · 2020-03-13T06:08:43Z

/retest

chenkaiyue · 2020-03-13T07:17:23Z

/retest

k82cn · 2020-03-22T13:44:10Z

pkg/controller/nodelifecycle/scheduler/rate_limited_queue.go

@@ -194,6 +194,15 @@ func (q *UniqueQueue) Clear() {
 	}
 }

+// SetRemove remove value from the set if value existed
+func (q *UniqueQueue) SetRemove(value string) {


prefer to rename to Remove; the other parts are LGTM :)

OK，i will correct it

k82cn · 2020-03-22T13:44:20Z

pkg/controller/nodelifecycle/scheduler/rate_limited_queue.go

@@ -280,6 +289,11 @@ func (q *RateLimitedTimedQueue) Clear() {
 	q.queue.Clear()
 }

+// SetRemove remove value from the set of the queue
+func (q *RateLimitedTimedQueue) SetRemove(value string) {


k82cn · 2020-03-22T14:09:00Z

/approve

k8s-ci-robot · 2020-03-22T14:09:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chenkaiyue, k82cn

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:

~~pkg/controller/nodelifecycle/OWNERS~~ [k82cn]

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

xiaoxubeii · 2020-03-23T03:28:21Z

/lgtm

xiaoxubeii · 2020-03-23T03:39:51Z

/release-note-none

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 11, 2020

k8s-ci-robot requested review from gmarek and k82cn March 11, 2020 18:07

chenkaiyue force-pushed the Reconcile-NoExecute-Taint branch 2 times, most recently from da6880d to 8c62c4a Compare March 12, 2020 04:04

chenkaiyue force-pushed the Reconcile-NoExecute-Taint branch from 8c62c4a to 62dd519 Compare March 12, 2020 04:06

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 12, 2020

Reconcile NoExecute Taint

b3637c9

chenkaiyue force-pushed the Reconcile-NoExecute-Taint branch from 62dd519 to b3637c9 Compare March 13, 2020 03:44

k82cn self-assigned this Mar 22, 2020

k82cn reviewed Mar 22, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 22, 2020

k8s-ci-robot assigned xiaoxubeii Mar 23, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 23, 2020

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. 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. labels Mar 23, 2020

k8s-ci-robot merged commit 0641e0c into kubernetes:master Mar 23, 2020

k8s-ci-robot added this to the v1.19 milestone Mar 23, 2020

mrajashree mentioned this pull request Nov 3, 2020

Unreachable node no longer gets Unreachable taint with NoExecute effect #96183

Closed

howieyuen mentioned this pull request Nov 26, 2020

fix nodelifecyle controller not add NoExecute taint bug #96876

Merged

This was referenced Jan 18, 2021

Cherry pick #96876 in controller to 1.19: fix nodelifecyle controller not add NoExecute taint bug #98140

Merged

Cherry pick #96876 in controller to 1.20: fix nodelifecyle controller not add NoExecute taint bug #98168

Merged

CKchen0726 mentioned this pull request Feb 10, 2021

Cherry pick #96876 in controller to 1.18: fix nodelifecyle controller not add NoExecute taint bug #98943

Merged

rambohe-ch mentioned this pull request Jan 5, 2022

[feature] adapt k8s v1.20 version openyurtio/openyurt#705

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reconcile NoExecute Taint #89059

Reconcile NoExecute Taint #89059

chenkaiyue commented Mar 11, 2020 •

edited by liggitt

k8s-ci-robot commented Mar 11, 2020

k8s-ci-robot commented Mar 11, 2020

chenkaiyue commented Mar 11, 2020 •

edited

k8s-ci-robot commented Mar 11, 2020

attlee-wang commented Mar 12, 2020

chenkaiyue commented Mar 12, 2020

yuzhiquan commented Mar 12, 2020

chenkaiyue commented Mar 13, 2020

chenkaiyue commented Mar 13, 2020

k82cn Mar 22, 2020

chenkaiyue Mar 23, 2020

k82cn Mar 22, 2020

chenkaiyue Mar 23, 2020

k82cn commented Mar 22, 2020

k8s-ci-robot commented Mar 22, 2020

xiaoxubeii commented Mar 23, 2020

xiaoxubeii commented Mar 23, 2020

Reconcile NoExecute Taint #89059

Reconcile NoExecute Taint #89059

Conversation

chenkaiyue commented Mar 11, 2020 • edited by liggitt

k8s-ci-robot commented Mar 11, 2020

k8s-ci-robot commented Mar 11, 2020

chenkaiyue commented Mar 11, 2020 • edited

k8s-ci-robot commented Mar 11, 2020

attlee-wang commented Mar 12, 2020

chenkaiyue commented Mar 12, 2020

yuzhiquan commented Mar 12, 2020

chenkaiyue commented Mar 13, 2020

chenkaiyue commented Mar 13, 2020

k82cn Mar 22, 2020

Choose a reason for hiding this comment

chenkaiyue Mar 23, 2020

Choose a reason for hiding this comment

k82cn Mar 22, 2020

Choose a reason for hiding this comment

chenkaiyue Mar 23, 2020

Choose a reason for hiding this comment

k82cn commented Mar 22, 2020

k8s-ci-robot commented Mar 22, 2020

xiaoxubeii commented Mar 23, 2020

xiaoxubeii commented Mar 23, 2020

chenkaiyue commented Mar 11, 2020 •

edited by liggitt

chenkaiyue commented Mar 11, 2020 •

edited