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

Do not evict pods which tolerate all NoExecute taints #93722

Merged
merged 1 commit into from
Aug 10, 2020
Merged

Do not evict pods which tolerate all NoExecute taints #93722

merged 1 commit into from
Aug 10, 2020

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Aug 5, 2020 •
edited
Loading

What type of PR is this?

/kind bug

Which issue(s) this PR fixes:
Fixes #90794

Does this PR introduce a user-facing change?:

Fixes a bug evicting pods after a taint with a limited tolerationSeconds toleration is removed from a node

/sig node
/priority important-soon

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. sig/node Categorizes an issue or PR as relevant to SIG Node. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 5, 2020
@liggitt liggitt mentioned this pull request Aug 5, 2020
Closed
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt

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 Aug 5, 2020
@k8s-ci-robot k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Aug 5, 2020
@liggitt liggitt changed the title WIP - Do not evict pods which tolerate all NoExecute taints Do not evict pods which tolerate all NoExecute taints Aug 5, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 5, 2020
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 5, 2020
liggitt
liggitt commented Aug 5, 2020
View reviewed changes
pkg/controller/nodelifecycle/scheduler/taint_manager.go
Comment on lines -361 to +362
klog.V(4).Infof("New tolerations for %v tolerate forever. Scheduled deletion won't be cancelled if already scheduled.", podNamespacedName.String())
klog.V(4).Infof("Current tolerations for %v tolerate forever, cancelling any scheduled deletion.", podNamespacedName.String())
tc.cancelWorkWithEvent(podNamespacedName)
Copy link
Member Author

@liggitt liggitt Aug 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'd like special attention on this change... the logged message explicitly called out the problematic behavior, so it seems like it was known, but it still seems incorrect to me.

If a pod infinitely tolerates all remaining taints on the node, I don't see why that should have different behavior than a node with no taints (for which we call cancelWorkWithEvent on line 348)

Copy link
Member Author

Choose a reason for hiding this comment

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

original commit in https://github.com/kubernetes/kubernetes/pull/40355/files#diff-12d91ddb84ebd95c1ccaa6b5980108f6R337-R341, comments by @davidopp at https://github.com/kubernetes/kubernetes/pull/40355/files#r100682445

eviction that has not yet started gets moved later or canceled (e.g. eviction is added to the eviction queue, then pod is updated with larger tolerationSeconds for its soonest toleration, or all taints are removed from node) -- IIUC we do want to move the eviction later (or cancel it if removing all taints) in that case?

I think "removing all taints" behavior should be equivalent to "removing all taints except ones that are tolerated infinitely"

@liggitt
Copy link
Member Author

liggitt commented Aug 5, 2020

/retest

@pires
Copy link
Contributor

pires commented Aug 5, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2020
@liggitt
Copy link
Member Author

liggitt commented Aug 5, 2020

/retest

@liggitt liggitt requested review from davidopp and karan and removed request for gmarek August 6, 2020 04:15
@liggitt
Copy link
Member Author

liggitt commented Aug 6, 2020

/cc @karan

@liggitt
Copy link
Member Author

liggitt commented Aug 6, 2020

/cc @dashpole

@k8s-ci-robot k8s-ci-robot requested a review from dashpole August 6, 2020 17:31
@liggitt liggitt removed the request for review from davidopp August 6, 2020 17:31
@dashpole
Copy link
Contributor

dashpole commented Aug 6, 2020

I've been trying to think of why the previous behavior would be desirable, but can't come up with anything. +1 to being consistent.
/lgtm

@liggitt
Copy link
Member Author

liggitt commented Aug 6, 2020

My inclination would be to include this in 1.19 and pick it back to supported releases, because:

  1. it is a correctness issue that can result in incorrect deletion of workloads
  2. the fix is very contained

I would like feedback from https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/nodelifecycle/OWNERS (though I'm not sure that full list is up to date)

@dashpole
Copy link
Contributor

dashpole commented Aug 6, 2020

maybe @k82cn has thoughts?

@sjenning
Copy link
Contributor

sjenning commented Aug 6, 2020

@ravisantoshgudimetla you worked on the taint-based eviction controller back in the day right (like... a year ago)? You see anything wrong with this?

lgtm though

@liggitt
Copy link
Member Author

liggitt commented Aug 8, 2020

/test pull-kubernetes-e2e-kind-ipv6

1 similar comment
@dims
Copy link
Member

dims commented Aug 8, 2020

/test pull-kubernetes-e2e-kind-ipv6

This was referenced Aug 8, 2020
Merged
Merged
Merged
@liggitt
Copy link
Member Author

liggitt commented Aug 8, 2020

Opened picks to 1.16-1.18 to start exercising CI.

@k82cn
Copy link
Member

k82cn commented Aug 10, 2020

Just went through the diff, LGTM overall, thanks very much 👍

Also agree to cherry-pick this PR :)

@liggitt
Copy link
Member Author

liggitt commented Aug 10, 2020

Sounds good, thanks for reviewing

/milestone v1.19

@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Aug 10, 2020
@k8s-ci-robot k8s-ci-robot merged commit 37cda82 into kubernetes:master Aug 10, 2020
@liggitt liggitt deleted the taint-evict branch August 10, 2020 21:15
k8s-ci-robot added a commit that referenced this pull request Aug 31, 2020
@k8s-ci-robot
Merge pull request #93816 from liggitt/automated-cherry-pick-of-#9372…
49154b6 
…2-upstream-release-1.16

Automated cherry pick of #93722: Do not evict pods which tolerate all NoExecute taints
k8s-ci-robot added a commit that referenced this pull request Sep 4, 2020
@k8s-ci-robot
Merge pull request #93814 from liggitt/automated-cherry-pick-of-#9372…
10c682b 
…2-upstream-release-1.18

Automated cherry pick of #93722: Do not evict pods which tolerate all NoExecute taints
k8s-ci-robot added a commit that referenced this pull request Sep 4, 2020
@k8s-ci-robot
Merge pull request #93815 from liggitt/automated-cherry-pick-of-#9372…
b12ac21 
…2-upstream-release-1.17

Automated cherry pick of #93722: Do not evict pods which tolerate all NoExecute taints
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ingvagabund ingvagabund Awaiting requested review from ingvagabund

@karan karan Awaiting requested review from karan

@dashpole dashpole Awaiting requested review from dashpole

Assignees

@pires pires

@dashpole dashpole

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. 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.

Pods in Node are deleted even if the NoExecute taint that triggered deletion is removed
7 participants
@liggitt @k8s-ci-robot @pires @dashpole @sjenning @dims @k82cn