Pick the PriorityClass with the lowest value of priority in case more than one global default exists #59991

bsalamat · 2018-02-16T22:40:47Z

What this PR does / why we need it:
Please see the referenced issue.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #59987

Special notes for your reviewer:

Release note:

Priority admission controller picks a global default with the lowest priority value if more than one such default PriorityClass exists.

/sig scheduling
cc/ @liggitt

liggitt · 2018-02-16T22:50:06Z

plugin/pkg/admission/priority/admission.go

 	for _, pci := range list {
 		if pci.GlobalDefault {
-			return pci, nil
+			defaultPCs = append(defaultPCs, pci)


Avoid allocation here… happens on every pod admission. Start a value at nil, assign if nil, compare if non nil?

Sure. Makes sense. PTAL.

liggitt · 2018-02-16T23:40:26Z

plugin/pkg/admission/priority/admission.go

+			if defaultPC == nil {
+				// This is the first default PC found.
+				defaultPC = pci
+			} else {


else if pci.Value < defaultPC.Value { defaultPC = pci }?

liggitt · 2018-02-16T23:40:57Z

plugin/pkg/admission/priority/admission.go

+			}
+		}
+	}
+	if len(defaultPCs) > 0 {


Should be able to do the comparison in place rather than accumulating

Absolutely. Fixing a PR while running to a meeting never works. Done.

… than one global default exists

liggitt · 2018-02-17T01:10:08Z

/lgtm

liggitt · 2018-02-17T01:10:42Z

Should document on the API field what happens if multiple objects say they are the default

liggitt · 2018-02-17T01:11:52Z

plugin/pkg/admission/priority/admission.go

 	for _, pci := range list {
 		if pci.GlobalDefault {
-			return pci, nil
+			if defaultPC == nil || defaultPC.Value > pci.Value {


Heh, this is still non-deterministic, since two names can have the same priority. Sort by lowest priority, then by name?

But that's fine, right? We care about the value of the PriorityClass and also care whether any default class exists or not. If two names have the same priority, the value that we pick will still be the same. We are able to detect whether a default class exists as well.

Doesn't this set the priority class on the pod? So the name it chose would be visible? Seems like we'd want the same name chosen consistently for multiple pods submitted

Ah, looks like it doesn't set priority class, just priority

Right, it only sets the value of priority.

bsalamat · 2018-02-17T01:16:56Z

Should document on the API field what happens if multiple objects say they are the default

I guess we can. One caveat is that this behavior is coming from an admission controller, not the API itself. So, if a cluster uses a different set of admission controllers, the behavior will be different. We can mention in the documentation that "assuming default admission controller" this will be the behavior.

liggitt · 2018-02-17T01:33:13Z

For the API to be portable, I'd expect the tie-breaking behavior to be specified on the API field

bsalamat · 2018-02-17T02:29:11Z

For the API to be portable, I'd expect the tie-breaking behavior to be specified on the API field

I updated the API docs as well. PTAL. Thanks!

liggitt · 2018-02-17T16:33:02Z

staging/src/k8s.io/api/scheduling/v1alpha1/types.go

@@ -39,6 +39,9 @@ type PriorityClass struct {

 	// globalDefault specifies whether this PriorityClass should be considered as
 	// the default priority for pods that do not have any priority class.
+	// Only one PriorityClass with can be marked as `globalDefault`. Due to race conditions, more than


Calling this a race condition makes it sound like a bug, which it isn't. I would just say that only one should have it, and if multiple do, this is the behavior

Done. PTAL.

liggitt · 2018-02-19T02:21:06Z

/lgtm

ravisantoshgudimetla · 2018-02-20T22:48:52Z

plugin/pkg/admission/priority/admission_test.go

+			attributes:            admission.NewAttributesRecord(nil, nil, pcKind, "", defaultClass1.Name, pcResource, "", admission.Delete, nil),
+			expectedDefaultBefore: defaultClass1.Value,
+			expectedDefaultAfter:  defaultClass2.Value,
+		},


Can we have one more test case with 2 default priorityClasses with same priority value then the first one is choosen(as per the current logic).

When the priority values are the same, how do you want to verify that the first one is picked? Admission controller does not update the "priorityClassName" field. It only sets the value of priority.

Apologies, I wrote my earlier comment in haste. I was under the impression that this tests getDefaultPriorityClass(), it seems we are checking only getDefaultPriority() f(n) in TestDefaultPriority, so we will return always the same value but if we can call getDefaultPriorityClass() f(n) in this test, we can check for the priorityClass being used as well.

I see your point, but getDefaultPriorityClass is used for only two purposes: 1. Whether any default priority classes exists or not, 2. Find the value of default priority. That's why it does not matter which priority class it returns when there are two default priority classes with the same value.
Returning the value of the first priority class is a side effect of the current implementation. It is not necessary for the validity of the logic. For this reason, I believe we should not add a test for it. Having a test may make users and developers think that returning the first priority is a necessary part of the logic. Besides, I am not sure if a lister is guaranteed to return the list of items always in the same order.

Thanks for the clarification @bsalamat. Yeah, I think that it is a side-effect because we are using a list where order matters.

bsalamat · 2018-02-21T18:26:47Z

@deads2k Could you please approve this if you are fine with the change?

deads2k · 2018-02-21T20:28:14Z

/approve

k8s-ci-robot · 2018-02-21T20:28:23Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~api/OWNERS~~ [liggitt]
~~docs/api-reference/OWNERS~~ [liggitt]
~~pkg/apis/OWNERS~~ [liggitt]
~~plugin/pkg/admission/OWNERS~~ [deads2k]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]

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

k8s-github-robot · 2018-02-21T21:07:42Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-02-21T21:52:00Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from deads2k and derekwaynecarr February 16, 2018 22:40

liggitt reviewed Feb 16, 2018

View reviewed changes

bsalamat force-pushed the default_pc branch from aff7f5a to cd97260 Compare February 16, 2018 23:29

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 16, 2018

liggitt reviewed Feb 16, 2018

View reviewed changes

bsalamat force-pushed the default_pc branch from cd97260 to 179f523 Compare February 17, 2018 00:57

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 17, 2018

Pick the PriorityClass with the lowest value of priority in case more…

6b29282

… than one global default exists

bsalamat force-pushed the default_pc branch from 179f523 to 6b29282 Compare February 17, 2018 00:59

k8s-ci-robot assigned liggitt Feb 17, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 17, 2018

liggitt reviewed Feb 17, 2018

View reviewed changes

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 17, 2018

Add API docs for multiple PriorityClasses marked as globalDefault

5f73546

bsalamat force-pushed the default_pc branch 2 times, most recently from 702d818 to 573101e Compare February 17, 2018 02:27

bsalamat force-pushed the default_pc branch from 573101e to 7817a22 Compare February 17, 2018 02:39

liggitt reviewed Feb 17, 2018

View reviewed changes

bsalamat force-pushed the default_pc branch from 7817a22 to e9f01f9 Compare February 19, 2018 02:01

bsalamat added 2 commits February 18, 2018 18:20

Changed API doc

0b0884f

autogenerated files

af0d745

bsalamat force-pushed the default_pc branch from e9f01f9 to af0d745 Compare February 19, 2018 02:20

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 19, 2018

ravisantoshgudimetla reviewed Feb 20, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 21, 2018

k8s-github-robot merged commit 2a604f6 into kubernetes:master Feb 21, 2018

bsalamat deleted the default_pc branch February 22, 2018 03:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pick the PriorityClass with the lowest value of priority in case more than one global default exists #59991

Pick the PriorityClass with the lowest value of priority in case more than one global default exists #59991

bsalamat commented Feb 16, 2018

liggitt Feb 16, 2018

bsalamat Feb 16, 2018

liggitt Feb 16, 2018

liggitt Feb 16, 2018

bsalamat Feb 17, 2018

liggitt commented Feb 17, 2018

liggitt commented Feb 17, 2018

liggitt Feb 17, 2018

bsalamat Feb 17, 2018 •

edited

Loading

liggitt Feb 17, 2018

liggitt Feb 17, 2018

bsalamat Feb 17, 2018

bsalamat commented Feb 17, 2018 •

edited

Loading

liggitt commented Feb 17, 2018

bsalamat commented Feb 17, 2018

liggitt Feb 17, 2018

bsalamat Feb 19, 2018

liggitt commented Feb 19, 2018

ravisantoshgudimetla Feb 20, 2018

bsalamat Feb 20, 2018

ravisantoshgudimetla Feb 21, 2018

bsalamat Feb 21, 2018

ravisantoshgudimetla Feb 21, 2018

bsalamat commented Feb 21, 2018

deads2k commented Feb 21, 2018

k8s-ci-robot commented Feb 21, 2018

k8s-github-robot commented Feb 21, 2018

k8s-github-robot commented Feb 21, 2018

Pick the PriorityClass with the lowest value of priority in case more than one global default exists #59991

Pick the PriorityClass with the lowest value of priority in case more than one global default exists #59991

Conversation

bsalamat commented Feb 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Feb 17, 2018

liggitt commented Feb 17, 2018

Choose a reason for hiding this comment

bsalamat Feb 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsalamat commented Feb 17, 2018 • edited Loading

liggitt commented Feb 17, 2018

bsalamat commented Feb 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsalamat commented Feb 21, 2018

deads2k commented Feb 21, 2018

k8s-ci-robot commented Feb 21, 2018

k8s-github-robot commented Feb 21, 2018

k8s-github-robot commented Feb 21, 2018

bsalamat Feb 17, 2018 •

edited

Loading

bsalamat commented Feb 17, 2018 •

edited

Loading