Merge pull request #59991 from bsalamat/default_pc

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

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

**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**:

```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

api/openapi-spec/swagger.json

@@ -84665,7 +84665,7 @@
       "type": "string"
      },
      "globalDefault": {
-      "description": "globalDefault specifies whether this PriorityClass should be considered as the default priority for pods that do not have any priority class.",
+      "description": "globalDefault specifies whether this PriorityClass should be considered as the default priority for pods that do not have any priority class. Only one PriorityClass can be marked as `globalDefault`. However, if more than one PriorityClasses exists with their `globalDefault` field set to true, the smallest value of such global default PriorityClasses will be used as the default priority.",
       "type": "boolean"
      },
      "kind": {

api/swagger-spec/scheduling.k8s.io_v1alpha1.json

@@ -799,7 +799,7 @@
      },
      "globalDefault": {
       "type": "boolean",
-      "description": "globalDefault specifies whether this PriorityClass should be considered as the default priority for pods that do not have any priority class."
+      "description": "globalDefault specifies whether this PriorityClass should be considered as the default priority for pods that do not have any priority class. Only one PriorityClass can be marked as `globalDefault`. However, if more than one PriorityClasses exists with their `globalDefault` field set to true, the smallest value of such global default PriorityClasses will be used as the default priority."
      },
      "description": {
       "type": "string",

docs/api-reference/scheduling.k8s.io/v1alpha1/definitions.html

@@ -1341,7 +1341,7 @@ Examples:<br>
 </tr>
 <tr>
 <td class="tableblock halign-left valign-top"><p class="tableblock">globalDefault</p></td>
-<td class="tableblock halign-left valign-top"><p class="tableblock">globalDefault specifies whether this PriorityClass should be considered as the default priority for pods that do not have any priority class.</p></td>
+<td class="tableblock halign-left valign-top"><p class="tableblock">globalDefault specifies whether this PriorityClass should be considered as the default priority for pods that do not have any priority class. Only one PriorityClass can be marked as <code>globalDefault</code>. However, if more than one PriorityClasses exists with their <code>globalDefault</code> field set to true, the smallest value of such global default PriorityClasses will be used as the default priority.</p></td>
 <td class="tableblock halign-left valign-top"><p class="tableblock">false</p></td>
 <td class="tableblock halign-left valign-top"><p class="tableblock">boolean</p></td>
 <td class="tableblock halign-left valign-top"><p class="tableblock">false</p></td>

pkg/apis/scheduling/types.go

@@ -44,8 +44,11 @@ type PriorityClass struct {
 	// receive when they have the name of this class in their pod spec.
 	Value int32
 
-	// GlobalDefault specifies whether this PriorityClass should be considered as
+	// globalDefault specifies whether this PriorityClass should be considered as
 	// the default priority for pods that do not have any priority class.
+	// Only one PriorityClass can be marked as `globalDefault`. However, if more than
+	// one PriorityClasses exists with their `globalDefault` field set to true,
+	// the smallest value of such global default PriorityClasses will be used as the default priority.
 	// +optional
 	GlobalDefault bool
 

plugin/pkg/admission/priority/admission.go

@@ -231,12 +231,17 @@ func (p *PriorityPlugin) getDefaultPriorityClass() (*scheduling.PriorityClass, e
 	if err != nil {
 		return nil, err
 	}
+	// In case more than one global default priority class is added as a result of a race condition,
+	// we pick the one with the lowest priority value.
+	var defaultPC *scheduling.PriorityClass
 	for _, pci := range list {
 		if pci.GlobalDefault {
-			return pci, nil
+			if defaultPC == nil || defaultPC.Value > pci.Value {
+				defaultPC = pci
+			}
 		}
 	}
-	return nil, nil
+	return defaultPC, nil
 }
 
 func (p *PriorityPlugin) getDefaultPriority() (int32, error) {

plugin/pkg/admission/priority/admission_test.go

@@ -189,6 +189,14 @@ func TestDefaultPriority(t *testing.T) {
 			expectedDefaultBefore: scheduling.DefaultPriorityWhenNoDefaultClassExists,
 			expectedDefaultAfter:  defaultClass1.Value,
 		},
+		{
+			name:                  "multiple default classes resolves to the minimum value among them",
+			classesBefore:         []*scheduling.PriorityClass{defaultClass1, defaultClass2},
+			classesAfter:          []*scheduling.PriorityClass{defaultClass2},
+			attributes:            admission.NewAttributesRecord(nil, nil, pcKind, "", defaultClass1.Name, pcResource, "", admission.Delete, nil),
+			expectedDefaultBefore: defaultClass1.Value,
+			expectedDefaultAfter:  defaultClass2.Value,
+		},
 		{
 			name:                  "delete default priority class",
 			classesBefore:         []*scheduling.PriorityClass{defaultClass1},

staging/src/k8s.io/api/scheduling/v1alpha1/generated.proto

@@ -43,6 +43,9 @@ message PriorityClass {
 
   // globalDefault specifies whether this PriorityClass should be considered as
   // the default priority for pods that do not have any priority class.
+  // Only one PriorityClass can be marked as `globalDefault`. However, if more than
+  // one PriorityClasses exists with their `globalDefault` field set to true,
+  // the smallest value of such global default PriorityClasses will be used as the default priority.
   // +optional
   optional bool globalDefault = 3;
 

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 can be marked as `globalDefault`. However, if more than
+	// one PriorityClasses exists with their `globalDefault` field set to true,
+	// the smallest value of such global default PriorityClasses will be used as the default priority.
 	// +optional
 	GlobalDefault bool `json:"globalDefault,omitempty" protobuf:"bytes,3,opt,name=globalDefault"`
 

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

@@ -31,7 +31,7 @@ var map_PriorityClass = map[string]string{
 	"":              "PriorityClass defines mapping from a priority class name to the priority integer value. The value can be any valid integer.",
 	"metadata":      "Standard object's metadata. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#metadata",
 	"value":         "The value of this priority class. This is the actual priority that pods receive when they have the name of this class in their pod spec.",
-	"globalDefault": "globalDefault specifies whether this PriorityClass should be considered as the default priority for pods that do not have any priority class.",
+	"globalDefault": "globalDefault specifies whether this PriorityClass should be considered as the default priority for pods that do not have any priority class. Only one PriorityClass can be marked as `globalDefault`. However, if more than one PriorityClasses exists with their `globalDefault` field set to true, the smallest value of such global default PriorityClasses will be used as the default priority.",
 	"description":   "description is an arbitrary string that usually provides guidelines on when this priority class should be used.",
 }