Merge pull request #131693 from ania-borowiec/staging_repo_refactoring_action_type

Remove package protected fields from ActionType

pkg/scheduler/framework/events.go

@@ -68,26 +68,26 @@ func PodSchedulingPropertiesChange(newPod *v1.Pod, oldPod *v1.Pod) (events []Clu
 		r = unschedulablePod
 	}
 
-	podChangeExtracters := []podChangeExtractor{
+	podChangeExtractors := []podChangeExtractor{
 		extractPodLabelsChange,
 		extractPodScaleDown,
 		extractPodSchedulingGateEliminatedChange,
 		extractPodTolerationChange,
 	}
 	if utilfeature.DefaultFeatureGate.Enabled(features.DynamicResourceAllocation) {
-		podChangeExtracters = append(podChangeExtracters, extractPodGeneratedResourceClaimChange)
+		podChangeExtractors = append(podChangeExtractors, extractPodGeneratedResourceClaimChange)
 	}
 
-	for _, fn := range podChangeExtracters {
-		if event := fn(newPod, oldPod); event != none {
+	for _, fn := range podChangeExtractors {
+		if event := fn(newPod, oldPod); event != None {
 			events = append(events, ClusterEvent{Resource: r, ActionType: event})
 		}
 	}
 
 	if len(events) == 0 {
-		// When no specific event is found, we use AssignedPodOtherUpdate,
+		// When no specific event is found, we use the general Update action,
 		// which should only trigger plugins registering a general Pod/Update event.
-		events = append(events, ClusterEvent{Resource: r, ActionType: updatePodOther})
+		events = append(events, ClusterEvent{Resource: r, ActionType: Update})
 	}
 
 	return
@@ -116,14 +116,14 @@ func extractPodScaleDown(newPod, oldPod *v1.Pod) ActionType {
 		}
 	}
 
-	return none
+	return None
 }
 
 func extractPodLabelsChange(newPod *v1.Pod, oldPod *v1.Pod) ActionType {
 	if isLabelChanged(newPod.GetLabels(), oldPod.GetLabels()) {
 		return UpdatePodLabel
 	}
-	return none
+	return None
 }
 
 func extractPodTolerationChange(newPod *v1.Pod, oldPod *v1.Pod) ActionType {
@@ -135,7 +135,7 @@ func extractPodTolerationChange(newPod *v1.Pod, oldPod *v1.Pod) ActionType {
 		return UpdatePodToleration
 	}
 
-	return none
+	return None
 }
 
 func extractPodSchedulingGateEliminatedChange(newPod *v1.Pod, oldPod *v1.Pod) ActionType {
@@ -144,7 +144,7 @@ func extractPodSchedulingGateEliminatedChange(newPod *v1.Pod, oldPod *v1.Pod) Ac
 		return UpdatePodSchedulingGatesEliminated
 	}
 
-	return none
+	return None
 }
 
 func extractPodGeneratedResourceClaimChange(newPod *v1.Pod, oldPod *v1.Pod) ActionType {
@@ -152,7 +152,7 @@ func extractPodGeneratedResourceClaimChange(newPod *v1.Pod, oldPod *v1.Pod) Acti
 		return UpdatePodGeneratedResourceClaim
 	}
 
-	return none
+	return None
 }
 
 // NodeSchedulingPropertiesChange interprets the update of a node and returns corresponding UpdateNodeXYZ event(s).
@@ -167,7 +167,7 @@ func NodeSchedulingPropertiesChange(newNode *v1.Node, oldNode *v1.Node) (events
 	}
 
 	for _, fn := range nodeChangeExtracters {
-		if event := fn(newNode, oldNode); event != none {
+		if event := fn(newNode, oldNode); event != None {
 			events = append(events, ClusterEvent{Resource: Node, ActionType: event})
 		}
 	}
@@ -180,14 +180,14 @@ func extractNodeAllocatableChange(newNode *v1.Node, oldNode *v1.Node) ActionType
 	if !equality.Semantic.DeepEqual(oldNode.Status.Allocatable, newNode.Status.Allocatable) {
 		return UpdateNodeAllocatable
 	}
-	return none
+	return None
 }
 
 func extractNodeLabelsChange(newNode *v1.Node, oldNode *v1.Node) ActionType {
 	if isLabelChanged(newNode.GetLabels(), oldNode.GetLabels()) {
 		return UpdateNodeLabel
 	}
-	return none
+	return None
 }
 
 func isLabelChanged(newLabels map[string]string, oldLabels map[string]string) bool {
@@ -198,7 +198,7 @@ func extractNodeTaintsChange(newNode *v1.Node, oldNode *v1.Node) ActionType {
 	if !equality.Semantic.DeepEqual(newNode.Spec.Taints, oldNode.Spec.Taints) {
 		return UpdateNodeTaint
 	}
-	return none
+	return None
 }
 
 func extractNodeConditionsChange(newNode *v1.Node, oldNode *v1.Node) ActionType {
@@ -212,7 +212,7 @@ func extractNodeConditionsChange(newNode *v1.Node, oldNode *v1.Node) ActionType
 	if !equality.Semantic.DeepEqual(strip(oldNode.Status.Conditions), strip(newNode.Status.Conditions)) {
 		return UpdateNodeCondition
 	}
-	return none
+	return None
 }
 
 func extractNodeSpecUnschedulableChange(newNode *v1.Node, oldNode *v1.Node) ActionType {
@@ -220,12 +220,12 @@ func extractNodeSpecUnschedulableChange(newNode *v1.Node, oldNode *v1.Node) Acti
 		// TODO: create UpdateNodeSpecUnschedulable ActionType
 		return UpdateNodeTaint
 	}
-	return none
+	return None
 }
 
 func extractNodeAnnotationsChange(newNode *v1.Node, oldNode *v1.Node) ActionType {
 	if !equality.Semantic.DeepEqual(oldNode.GetAnnotations(), newNode.GetAnnotations()) {
 		return UpdateNodeAnnotation
 	}
-	return none
+	return None
 }

pkg/scheduler/framework/events_test.go

@@ -60,7 +60,7 @@ func TestNodeAllocatableChange(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			oldNode := &v1.Node{Status: v1.NodeStatus{Allocatable: test.oldAllocatable}}
 			newNode := &v1.Node{Status: v1.NodeStatus{Allocatable: test.newAllocatable}}
-			changed := extractNodeAllocatableChange(newNode, oldNode) != none
+			changed := extractNodeAllocatableChange(newNode, oldNode) != None
 			if changed != test.changed {
 				t.Errorf("nodeAllocatableChanged should be %t, got %t", test.changed, changed)
 			}
@@ -93,7 +93,7 @@ func TestNodeLabelsChange(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			oldNode := &v1.Node{ObjectMeta: metav1.ObjectMeta{Labels: test.oldLabels}}
 			newNode := &v1.Node{ObjectMeta: metav1.ObjectMeta{Labels: test.newLabels}}
-			changed := extractNodeLabelsChange(newNode, oldNode) != none
+			changed := extractNodeLabelsChange(newNode, oldNode) != None
 			if changed != test.changed {
 				t.Errorf("Test case %q failed: should be %t, got %t", test.name, test.changed, changed)
 			}
@@ -125,7 +125,7 @@ func TestNodeTaintsChange(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			oldNode := &v1.Node{Spec: v1.NodeSpec{Taints: test.oldTaints}}
 			newNode := &v1.Node{Spec: v1.NodeSpec{Taints: test.newTaints}}
-			changed := extractNodeTaintsChange(newNode, oldNode) != none
+			changed := extractNodeTaintsChange(newNode, oldNode) != None
 			if changed != test.changed {
 				t.Errorf("Test case %q failed: should be %t, not %t", test.name, test.changed, changed)
 			}
@@ -180,7 +180,7 @@ func TestNodeConditionsChange(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			oldNode := &v1.Node{Status: v1.NodeStatus{Conditions: test.oldConditions}}
 			newNode := &v1.Node{Status: v1.NodeStatus{Conditions: test.newConditions}}
-			changed := extractNodeConditionsChange(newNode, oldNode) != none
+			changed := extractNodeConditionsChange(newNode, oldNode) != None
 			if changed != test.changed {
 				t.Errorf("Test case %q failed: should be %t, got %t", test.name, test.changed, changed)
 			}
@@ -377,7 +377,7 @@ func Test_podSchedulingPropertiesChange(t *testing.T) {
 			name:   "pod's resource request is scaled up",
 			oldPod: podWithSmallRequest,
 			newPod: podWithBigRequest,
-			want:   []ClusterEvent{{Resource: unschedulablePod, ActionType: updatePodOther}},
+			want:   []ClusterEvent{{Resource: unschedulablePod, ActionType: Update}},
 		},
 		{
 			name:   "both pod's resource request and label are updated",
@@ -392,7 +392,7 @@ func Test_podSchedulingPropertiesChange(t *testing.T) {
 			name:   "untracked properties of pod is updated",
 			newPod: st.MakePod().Annotation("foo", "bar").Obj(),
 			oldPod: st.MakePod().Annotation("foo", "bar2").Obj(),
-			want:   []ClusterEvent{{Resource: unschedulablePod, ActionType: updatePodOther}},
+			want:   []ClusterEvent{{Resource: unschedulablePod, ActionType: Update}},
 		},
 		{
 			name:   "scheduling gate is eliminated",
@@ -404,7 +404,7 @@ func Test_podSchedulingPropertiesChange(t *testing.T) {
 			name:   "scheduling gate is removed, but not completely eliminated",
 			newPod: st.MakePod().SchedulingGates([]string{"foo"}).Obj(),
 			oldPod: st.MakePod().SchedulingGates([]string{"foo", "bar"}).Obj(),
-			want:   []ClusterEvent{{Resource: unschedulablePod, ActionType: updatePodOther}},
+			want:   []ClusterEvent{{Resource: unschedulablePod, ActionType: Update}},
 		},
 		{
 			name:   "pod's tolerations are updated",
@@ -417,7 +417,7 @@ func Test_podSchedulingPropertiesChange(t *testing.T) {
 			draDisabled: true,
 			newPod:      st.MakePod().ResourceClaimStatuses(claimStatusA).Obj(),
 			oldPod:      st.MakePod().Obj(),
-			want:        []ClusterEvent{{Resource: unschedulablePod, ActionType: updatePodOther}},
+			want:        []ClusterEvent{{Resource: unschedulablePod, ActionType: Update}},
 		},
 		{
 			name:   "pod claim statuses change, feature enabled",

pkg/scheduler/framework/types.go

@@ -82,16 +82,13 @@ const (
 	// Depends on the DynamicResourceAllocation feature gate.
 	UpdatePodGeneratedResourceClaim
 
-	// updatePodOther is a update for pod's other fields.
-	// It's used only for the internal event handling, and thus unexported.
-	updatePodOther
-
 	All ActionType = 1<<iota - 1
 
 	// Use the general Update type if you don't either know or care the specific sub-Update type to use.
-	Update = UpdateNodeAllocatable | UpdateNodeLabel | UpdateNodeTaint | UpdateNodeCondition | UpdateNodeAnnotation | UpdatePodLabel | UpdatePodScaleDown | UpdatePodToleration | UpdatePodSchedulingGatesEliminated | UpdatePodGeneratedResourceClaim | updatePodOther
-	// none is a special ActionType that is only used internally.
-	none ActionType = 0
+	Update = UpdateNodeAllocatable | UpdateNodeLabel | UpdateNodeTaint | UpdateNodeCondition | UpdateNodeAnnotation | UpdatePodLabel | UpdatePodScaleDown | UpdatePodToleration | UpdatePodSchedulingGatesEliminated | UpdatePodGeneratedResourceClaim
+
+	// None is a special ActionType that is only used internally.
+	None ActionType = 0
 )
 
 var (
@@ -129,8 +126,6 @@ func (a ActionType) String() string {
 		return "UpdatePodSchedulingGatesEliminated"
 	case UpdatePodGeneratedResourceClaim:
 		return "UpdatePodGeneratedResourceClaim"
-	case updatePodOther:
-		return "Update"
 	case All:
 		return "All"
 	case Update:
@@ -321,14 +316,24 @@ func (ce ClusterEvent) IsWildCard() bool {
 }
 
 // Match returns true if ClusterEvent is matched with the coming event.
+// "match" means the coming event is the same or more specific than the ce.
+// i.e., when ce.ActionType is Update, it return true if a coming event is UpdateNodeLabel
+// because UpdateNodeLabel is more specific than Update.
+// On the other hand, when ce.ActionType is UpdateNodeLabel, it doesn't return true if a coming event is Update.
+// This is based on the fact that the scheduler interprets the coming cluster event as specific event if possible;
+// meaning, if a coming event is Node/Update, it means that Node's update is not something
+// that can be interpreted as any of Node's specific Update events.
+//
 // If the ce.Resource is "*", there's no requirement for the coming event' Resource.
 // Contrarily, if the coming event's Resource is "*", the ce.Resource should only be "*".
+// (which should never happen in the current implementation of the scheduling queue.)
 //
 // Note: we have a special case here when the coming event is a wildcard event,
 // it will force all Pods to move to activeQ/backoffQ,
 // but we take it as an unmatched event unless the ce is also a wildcard one.
 func (ce ClusterEvent) Match(incomingEvent ClusterEvent) bool {
-	return ce.IsWildCard() || ce.Resource.match(incomingEvent.Resource) && ce.ActionType&incomingEvent.ActionType != 0
+	return ce.IsWildCard() ||
+		ce.Resource.match(incomingEvent.Resource) && ce.ActionType&incomingEvent.ActionType != 0 && incomingEvent.ActionType <= ce.ActionType
 }
 
 // match returns true if the resource is matched with the coming resource.

pkg/scheduler/framework/types_test.go

@@ -2135,6 +2135,18 @@ func TestCloudEvent_Match(t *testing.T) {
 			comingEvent: ClusterEvent{Resource: WildCard, ActionType: UpdateNodeLabel},
 			wantResult:  false,
 		},
+		{
+			name:        "event with resource = '*' matching with coming events carrying a too broad actionType",
+			event:       ClusterEvent{Resource: WildCard, ActionType: UpdateNodeLabel},
+			comingEvent: ClusterEvent{Resource: Pod, ActionType: Update},
+			wantResult:  false,
+		},
+		{
+			name:        "event with resource = '*' matching with coming events carrying a more specific actionType",
+			event:       ClusterEvent{Resource: WildCard, ActionType: Update},
+			comingEvent: ClusterEvent{Resource: Pod, ActionType: UpdateNodeLabel},
+			wantResult:  true,
+		},
 	}
 
 	for _, tc := range testCases {