Merge pull request kubernetes#126050 from sanposhiho/refactor-move

cleanup: refactor the way extracting Node smaller events

Author: k8s-ci-robot

pkg/scheduler/eventhandlers.go

@@ -24,7 +24,6 @@ import (
 
 	v1 "k8s.io/api/core/v1"
 	storagev1 "k8s.io/api/storage/v1"
-	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -94,7 +93,7 @@ func (sched *Scheduler) updateNodeInCache(oldObj, newObj interface{}) {
 	logger.V(4).Info("Update event for node", "node", klog.KObj(newNode))
 	nodeInfo := sched.Cache.UpdateNode(logger, oldNode, newNode)
 	// Only requeue unschedulable pods if the node became more schedulable.
-	for _, evt := range nodeSchedulingPropertiesChange(newNode, oldNode) {
+	for _, evt := range queue.NodeSchedulingPropertiesChange(newNode, oldNode) {
 		sched.SchedulingQueue.MoveAllToActiveOrBackoffQueue(logger, evt, oldNode, newNode, preCheckForNode(nodeInfo))
 	}
 }
@@ -571,62 +570,6 @@ func addAllEventHandlers(
 	return nil
 }
 
-func nodeSchedulingPropertiesChange(newNode *v1.Node, oldNode *v1.Node) []framework.ClusterEvent {
-	var events []framework.ClusterEvent
-
-	if nodeSpecUnschedulableChanged(newNode, oldNode) {
-		events = append(events, queue.NodeSpecUnschedulableChange)
-	}
-	if nodeAllocatableChanged(newNode, oldNode) {
-		events = append(events, queue.NodeAllocatableChange)
-	}
-	if nodeLabelsChanged(newNode, oldNode) {
-		events = append(events, queue.NodeLabelChange)
-	}
-	if nodeTaintsChanged(newNode, oldNode) {
-		events = append(events, queue.NodeTaintChange)
-	}
-	if nodeConditionsChanged(newNode, oldNode) {
-		events = append(events, queue.NodeConditionChange)
-	}
-	if nodeAnnotationsChanged(newNode, oldNode) {
-		events = append(events, queue.NodeAnnotationChange)
-	}
-
-	return events
-}
-
-func nodeAllocatableChanged(newNode *v1.Node, oldNode *v1.Node) bool {
-	return !equality.Semantic.DeepEqual(oldNode.Status.Allocatable, newNode.Status.Allocatable)
-}
-
-func nodeLabelsChanged(newNode *v1.Node, oldNode *v1.Node) bool {
-	return !equality.Semantic.DeepEqual(oldNode.GetLabels(), newNode.GetLabels())
-}
-
-func nodeTaintsChanged(newNode *v1.Node, oldNode *v1.Node) bool {
-	return !equality.Semantic.DeepEqual(newNode.Spec.Taints, oldNode.Spec.Taints)
-}
-
-func nodeConditionsChanged(newNode *v1.Node, oldNode *v1.Node) bool {
-	strip := func(conditions []v1.NodeCondition) map[v1.NodeConditionType]v1.ConditionStatus {
-		conditionStatuses := make(map[v1.NodeConditionType]v1.ConditionStatus, len(conditions))
-		for i := range conditions {
-			conditionStatuses[conditions[i].Type] = conditions[i].Status
-		}
-		return conditionStatuses
-	}
-	return !equality.Semantic.DeepEqual(strip(oldNode.Status.Conditions), strip(newNode.Status.Conditions))
-}
-
-func nodeSpecUnschedulableChanged(newNode *v1.Node, oldNode *v1.Node) bool {
-	return newNode.Spec.Unschedulable != oldNode.Spec.Unschedulable && !newNode.Spec.Unschedulable
-}
-
-func nodeAnnotationsChanged(newNode *v1.Node, oldNode *v1.Node) bool {
-	return !equality.Semantic.DeepEqual(oldNode.GetAnnotations(), newNode.GetAnnotations())
-}
-
 func preCheckForNode(nodeInfo *framework.NodeInfo) queue.PreEnqueueCheck {
 	// Note: the following checks doesn't take preemption into considerations, in very rare
 	// cases (e.g., node resizing), "pod" may still fail a check but preemption helps. We deliberately

pkg/scheduler/eventhandlers_test.go

@@ -29,7 +29,6 @@ import (
 	resourcev1alpha2 "k8s.io/api/resource/v1alpha2"
 	storagev1 "k8s.io/api/storage/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	"k8s.io/klog/v2/ktesting"
@@ -53,157 +52,6 @@ import (
 	"k8s.io/kubernetes/pkg/scheduler/util/assumecache"
 )
 
-func TestNodeAllocatableChanged(t *testing.T) {
-	newQuantity := func(value int64) resource.Quantity {
-		return *resource.NewQuantity(value, resource.BinarySI)
-	}
-	for _, test := range []struct {
-		Name           string
-		Changed        bool
-		OldAllocatable v1.ResourceList
-		NewAllocatable v1.ResourceList
-	}{
-		{
-			Name:           "no allocatable resources changed",
-			Changed:        false,
-			OldAllocatable: v1.ResourceList{v1.ResourceMemory: newQuantity(1024)},
-			NewAllocatable: v1.ResourceList{v1.ResourceMemory: newQuantity(1024)},
-		},
-		{
-			Name:           "new node has more allocatable resources",
-			Changed:        true,
-			OldAllocatable: v1.ResourceList{v1.ResourceMemory: newQuantity(1024)},
-			NewAllocatable: v1.ResourceList{v1.ResourceMemory: newQuantity(1024), v1.ResourceStorage: newQuantity(1024)},
-		},
-	} {
-		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 := nodeAllocatableChanged(newNode, oldNode)
-			if changed != test.Changed {
-				t.Errorf("nodeAllocatableChanged should be %t, got %t", test.Changed, changed)
-			}
-		})
-	}
-}
-
-func TestNodeLabelsChanged(t *testing.T) {
-	for _, test := range []struct {
-		Name      string
-		Changed   bool
-		OldLabels map[string]string
-		NewLabels map[string]string
-	}{
-		{
-			Name:      "no labels changed",
-			Changed:   false,
-			OldLabels: map[string]string{"foo": "bar"},
-			NewLabels: map[string]string{"foo": "bar"},
-		},
-		// Labels changed.
-		{
-			Name:      "new node has more labels",
-			Changed:   true,
-			OldLabels: map[string]string{"foo": "bar"},
-			NewLabels: map[string]string{"foo": "bar", "test": "value"},
-		},
-	} {
-		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 := nodeLabelsChanged(newNode, oldNode)
-			if changed != test.Changed {
-				t.Errorf("Test case %q failed: should be %t, got %t", test.Name, test.Changed, changed)
-			}
-		})
-	}
-}
-
-func TestNodeTaintsChanged(t *testing.T) {
-	for _, test := range []struct {
-		Name      string
-		Changed   bool
-		OldTaints []v1.Taint
-		NewTaints []v1.Taint
-	}{
-		{
-			Name:      "no taint changed",
-			Changed:   false,
-			OldTaints: []v1.Taint{{Key: "key", Value: "value"}},
-			NewTaints: []v1.Taint{{Key: "key", Value: "value"}},
-		},
-		{
-			Name:      "taint value changed",
-			Changed:   true,
-			OldTaints: []v1.Taint{{Key: "key", Value: "value1"}},
-			NewTaints: []v1.Taint{{Key: "key", Value: "value2"}},
-		},
-	} {
-		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 := nodeTaintsChanged(newNode, oldNode)
-			if changed != test.Changed {
-				t.Errorf("Test case %q failed: should be %t, not %t", test.Name, test.Changed, changed)
-			}
-		})
-	}
-}
-
-func TestNodeConditionsChanged(t *testing.T) {
-	nodeConditionType := reflect.TypeOf(v1.NodeCondition{})
-	if nodeConditionType.NumField() != 6 {
-		t.Errorf("NodeCondition type has changed. The nodeConditionsChanged() function must be reevaluated.")
-	}
-
-	for _, test := range []struct {
-		Name          string
-		Changed       bool
-		OldConditions []v1.NodeCondition
-		NewConditions []v1.NodeCondition
-	}{
-		{
-			Name:          "no condition changed",
-			Changed:       false,
-			OldConditions: []v1.NodeCondition{{Type: v1.NodeDiskPressure, Status: v1.ConditionTrue}},
-			NewConditions: []v1.NodeCondition{{Type: v1.NodeDiskPressure, Status: v1.ConditionTrue}},
-		},
-		{
-			Name:          "only LastHeartbeatTime changed",
-			Changed:       false,
-			OldConditions: []v1.NodeCondition{{Type: v1.NodeDiskPressure, Status: v1.ConditionTrue, LastHeartbeatTime: metav1.Unix(1, 0)}},
-			NewConditions: []v1.NodeCondition{{Type: v1.NodeDiskPressure, Status: v1.ConditionTrue, LastHeartbeatTime: metav1.Unix(2, 0)}},
-		},
-		{
-			Name:          "new node has more healthy conditions",
-			Changed:       true,
-			OldConditions: []v1.NodeCondition{},
-			NewConditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}},
-		},
-		{
-			Name:          "new node has less unhealthy conditions",
-			Changed:       true,
-			OldConditions: []v1.NodeCondition{{Type: v1.NodeDiskPressure, Status: v1.ConditionTrue}},
-			NewConditions: []v1.NodeCondition{},
-		},
-		{
-			Name:          "condition status changed",
-			Changed:       true,
-			OldConditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionFalse}},
-			NewConditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}},
-		},
-	} {
-		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 := nodeConditionsChanged(newNode, oldNode)
-			if changed != test.Changed {
-				t.Errorf("Test case %q failed: should be %t, got %t", test.Name, test.Changed, changed)
-			}
-		})
-	}
-}
-
 func TestUpdatePodInCache(t *testing.T) {
 	ttl := 10 * time.Second
 	nodeName := "node"
@@ -574,91 +422,3 @@ func TestAdmissionCheck(t *testing.T) {
 		})
 	}
 }
-
-func TestNodeSchedulingPropertiesChange(t *testing.T) {
-	testCases := []struct {
-		name       string
-		newNode    *v1.Node
-		oldNode    *v1.Node
-		wantEvents []framework.ClusterEvent
-	}{
-		{
-			name:       "no specific changed applied",
-			newNode:    st.MakeNode().Unschedulable(false).Obj(),
-			oldNode:    st.MakeNode().Unschedulable(false).Obj(),
-			wantEvents: nil,
-		},
-		{
-			name:       "only node spec unavailable changed",
-			newNode:    st.MakeNode().Unschedulable(false).Obj(),
-			oldNode:    st.MakeNode().Unschedulable(true).Obj(),
-			wantEvents: []framework.ClusterEvent{queue.NodeSpecUnschedulableChange},
-		},
-		{
-			name: "only node allocatable changed",
-			newNode: st.MakeNode().Capacity(map[v1.ResourceName]string{
-				v1.ResourceCPU:                     "1000m",
-				v1.ResourceMemory:                  "100m",
-				v1.ResourceName("example.com/foo"): "1"},
-			).Obj(),
-			oldNode: st.MakeNode().Capacity(map[v1.ResourceName]string{
-				v1.ResourceCPU:                     "1000m",
-				v1.ResourceMemory:                  "100m",
-				v1.ResourceName("example.com/foo"): "2"},
-			).Obj(),
-			wantEvents: []framework.ClusterEvent{queue.NodeAllocatableChange},
-		},
-		{
-			name:       "only node label changed",
-			newNode:    st.MakeNode().Label("foo", "bar").Obj(),
-			oldNode:    st.MakeNode().Label("foo", "fuz").Obj(),
-			wantEvents: []framework.ClusterEvent{queue.NodeLabelChange},
-		},
-		{
-			name: "only node taint changed",
-			newNode: st.MakeNode().Taints([]v1.Taint{
-				{Key: v1.TaintNodeUnschedulable, Value: "", Effect: v1.TaintEffectNoSchedule},
-			}).Obj(),
-			oldNode: st.MakeNode().Taints([]v1.Taint{
-				{Key: v1.TaintNodeUnschedulable, Value: "foo", Effect: v1.TaintEffectNoSchedule},
-			}).Obj(),
-			wantEvents: []framework.ClusterEvent{queue.NodeTaintChange},
-		},
-		{
-			name:       "only node annotation changed",
-			newNode:    st.MakeNode().Annotation("foo", "bar").Obj(),
-			oldNode:    st.MakeNode().Annotation("foo", "fuz").Obj(),
-			wantEvents: []framework.ClusterEvent{queue.NodeAnnotationChange},
-		},
-		{
-			name:    "only node condition changed",
-			newNode: st.MakeNode().Obj(),
-			oldNode: st.MakeNode().Condition(
-				v1.NodeReady,
-				v1.ConditionTrue,
-				"Ready",
-				"Ready",
-			).Obj(),
-			wantEvents: []framework.ClusterEvent{queue.NodeConditionChange},
-		},
-		{
-			name: "both node label and node taint changed",
-			newNode: st.MakeNode().
-				Label("foo", "bar").
-				Taints([]v1.Taint{
-					{Key: v1.TaintNodeUnschedulable, Value: "", Effect: v1.TaintEffectNoSchedule},
-				}).Obj(),
-			oldNode: st.MakeNode().Taints([]v1.Taint{
-				{Key: v1.TaintNodeUnschedulable, Value: "foo", Effect: v1.TaintEffectNoSchedule},
-			}).Obj(),
-			wantEvents: []framework.ClusterEvent{queue.NodeLabelChange, queue.NodeTaintChange},
-		},
-	}
-
-	for _, tc := range testCases {
-		gotEvents := nodeSchedulingPropertiesChange(tc.newNode, tc.oldNode)
-		if diff := cmp.Diff(tc.wantEvents, gotEvents); diff != "" {
-			t.Errorf("unexpected event (-want, +got):\n%s", diff)
-		}
-	}
-}