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 toleration comparison & merging logic #81732

Merged
merged 2 commits into from Aug 27, 2019

Conversation

@tallclair
Copy link
Member

commented Aug 21, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:

The list of tolerations on a pod is effectively represents a union of the taints tolerated by each, so it is entirely reasonable to have 2 tolerations like:

- key: foo
  operator: Equal
  value: bar
- key: foo
  operator: Equal
  value: baz

However, the old logic would consider those 2 tolerations a conflict, and overwrite one with the other in a merge.

This PR fixes the toleration helper methods to represent a logical merge and comparison. It does that by recognizing that some tolerations are supersets of others, due to wildcard matches. For example, the above tolerations would be redundant if there was a 3rd toleration: {"key": "foo", "operator": "Exists"}.

These fixes are needed to better support RuntimeClass scheduling, but also affect the alpha PodTolerationRestriction admission controller.

Special notes for your reviewer:

There are 2 design decisions I want to highlight, and am open to discussion on:

  1. The decision to "smart merge" - Since tolerations represent a union, we could perform the merge by simply appending one list to the other (and maybe eliminating exact duplicates). We still need the isSuperset logic for checking the whitelist though, and I feel that simplifying with the smart merge offers a cleaner user experience.
  2. TolerationSeconds - The eviction logic actually takes the minimum toleration time. So technically a lower toleration time would be the superset of a higher time. However, I don't think that was the intention (that logic probably wasn't considering duplicate tolerations for the same taint, but rather across all taints), so I've stuck with using the maximum toleration time for the superset. I think that is more inline with the intention of both the PodTolerationRestriction and RuntimeClass admission controllers (probably irrelevant for the latter).

Does this PR introduce a user-facing change?:

Fix an issue with toleration merging & whitelist checking in the PodTolerationRestriction admission controller.

/sig scheduling
/sig node
/priority important-soon
/milestone v1.16

I wasn't sure who to assign this to, so went with the original author and reviewers:
/assign @aveshagarwal
/cc @timothysc @smarterclayton @derekwaynecarr @davidopp @liggitt

@tallclair

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2019

Forgot to update PodTolerationRestriction. Fixed.

@tallclair tallclair force-pushed the tallclair:merge-tolerations branch from 2c5f10f to 0888c7f Aug 21, 2019
@tallclair tallclair force-pushed the tallclair:merge-tolerations branch from 0888c7f to f82f55c Aug 21, 2019
@tallclair

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2019

Rebased.

@draveness draveness referenced this pull request Aug 22, 2019
7 of 7 tasks complete
@mattjmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

/remove-sig node

Unless I'm missing something, not sure this touches sig-node code. Please feel free to add the sig back if I'm incorrect :)

@k8s-ci-robot k8s-ci-robot removed the sig/node label Aug 22, 2019
@derekwaynecarr

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

@tallclair I think the changes as noted make sense. the admission plugin is still alpha so I think the behavior change is fine as well and represent what I agree was probably oversights in the original design.

/approve
/lgtm

@tallclair tallclair force-pushed the tallclair:merge-tolerations branch from 7e7b78d to 2e08288 Aug 26, 2019
@tallclair

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2019

Rebased.

@josiahbjorgaard

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

Hi all, code freeze is coming up in just a few days (Thursday, end of day, PST), so we need to make sure that this PR will be merged for v1.16 or moved to v1.17. Looks like it is coming along, but can you comment on it's status with respect to the milestone?

@tallclair

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2019

/assign @liggitt
For approval.

Derek already lgtm'd it, Jordan said it's on his queue (please let me know if you're not going to get to it in time). I think this is on-track for 1.16

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

/approve
/retest

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, liggitt, tallclair

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

Copy link
Member

left a comment

/lgtm

@draveness

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

@k8s-ci-robot k8s-ci-robot merged commit 0eb1bfc into kubernetes:master Aug 27, 2019
24 checks passed
24 checks passed
cla/linuxfoundation tallclair authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-conformance-kind-ipv6 Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.