From 9772ff284837886e30dd23409b7a0d5594a7e121 Mon Sep 17 00:00:00 2001 From: Kensei Nakada Date: Fri, 12 Jul 2024 19:19:25 +0900 Subject: [PATCH 1/2] cleanup: move NodeSchedulingPropertiesChange --- pkg/scheduler/eventhandlers.go | 59 +---- pkg/scheduler/eventhandlers_test.go | 240 ------------------ pkg/scheduler/internal/queue/events.go | 58 +++++ pkg/scheduler/internal/queue/events_test.go | 265 ++++++++++++++++++++ 4 files changed, 324 insertions(+), 298 deletions(-) create mode 100644 pkg/scheduler/internal/queue/events_test.go diff --git a/pkg/scheduler/eventhandlers.go b/pkg/scheduler/eventhandlers.go index 2001b9ed91e..41e18359e26 100644 --- a/pkg/scheduler/eventhandlers.go +++ b/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 diff --git a/pkg/scheduler/eventhandlers_test.go b/pkg/scheduler/eventhandlers_test.go index a99146cf567..2a656582d4f 100644 --- a/pkg/scheduler/eventhandlers_test.go +++ b/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) - } - } -} diff --git a/pkg/scheduler/internal/queue/events.go b/pkg/scheduler/internal/queue/events.go index 22b409a9ec1..15a1fb51bc1 100644 --- a/pkg/scheduler/internal/queue/events.go +++ b/pkg/scheduler/internal/queue/events.go @@ -17,6 +17,8 @@ limitations under the License. package queue import ( + v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" "k8s.io/kubernetes/pkg/scheduler/framework" ) @@ -89,3 +91,59 @@ var ( // UnschedulableTimeout is the event when a pod stays in unschedulable for longer than timeout. UnschedulableTimeout = framework.ClusterEvent{Resource: framework.WildCard, ActionType: framework.All, Label: "UnschedulableTimeout"} ) + +func NodeSchedulingPropertiesChange(newNode *v1.Node, oldNode *v1.Node) []framework.ClusterEvent { + var events []framework.ClusterEvent + + if nodeSpecUnschedulableChanged(newNode, oldNode) { + events = append(events, NodeSpecUnschedulableChange) + } + if nodeAllocatableChanged(newNode, oldNode) { + events = append(events, NodeAllocatableChange) + } + if nodeLabelsChanged(newNode, oldNode) { + events = append(events, NodeLabelChange) + } + if nodeTaintsChanged(newNode, oldNode) { + events = append(events, NodeTaintChange) + } + if nodeConditionsChanged(newNode, oldNode) { + events = append(events, NodeConditionChange) + } + if nodeAnnotationsChanged(newNode, oldNode) { + events = append(events, 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()) +} diff --git a/pkg/scheduler/internal/queue/events_test.go b/pkg/scheduler/internal/queue/events_test.go new file mode 100644 index 00000000000..ebe55304bfa --- /dev/null +++ b/pkg/scheduler/internal/queue/events_test.go @@ -0,0 +1,265 @@ +/* +Copyright 2024 The Kubernetes Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package queue + +import ( + "reflect" + "testing" + + "github.com/google/go-cmp/cmp" + v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/kubernetes/pkg/scheduler/framework" + st "k8s.io/kubernetes/pkg/scheduler/testing" +) + +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 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{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{NodeAllocatableChange}, + }, + { + name: "only node label changed", + newNode: st.MakeNode().Label("foo", "bar").Obj(), + oldNode: st.MakeNode().Label("foo", "fuz").Obj(), + wantEvents: []framework.ClusterEvent{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{NodeTaintChange}, + }, + { + name: "only node annotation changed", + newNode: st.MakeNode().Annotation("foo", "bar").Obj(), + oldNode: st.MakeNode().Annotation("foo", "fuz").Obj(), + wantEvents: []framework.ClusterEvent{NodeAnnotationChange}, + }, + { + name: "only node condition changed", + newNode: st.MakeNode().Obj(), + oldNode: st.MakeNode().Condition( + v1.NodeReady, + v1.ConditionTrue, + "Ready", + "Ready", + ).Obj(), + wantEvents: []framework.ClusterEvent{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{NodeLabelChange, 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) + } + } +} From fe96bfa348fd89cc7860547ab963798844d090d0 Mon Sep 17 00:00:00 2001 From: Kensei Nakada Date: Fri, 12 Jul 2024 19:06:07 +0900 Subject: [PATCH 2/2] cleanup: refactor the way extracting Node smaller events --- pkg/scheduler/internal/queue/events.go | 89 +++++----- pkg/scheduler/internal/queue/events_test.go | 185 ++++++++++---------- 2 files changed, 146 insertions(+), 128 deletions(-) diff --git a/pkg/scheduler/internal/queue/events.go b/pkg/scheduler/internal/queue/events.go index 15a1fb51bc1..0334757b917 100644 --- a/pkg/scheduler/internal/queue/events.go +++ b/pkg/scheduler/internal/queue/events.go @@ -37,18 +37,15 @@ const ( ) var ( - // AssignedPodAdd is the event when a pod is added that causes pods with matching affinity terms - // to be more schedulable. + // AssignedPodAdd is the event when an assigned pod is added. AssignedPodAdd = framework.ClusterEvent{Resource: framework.Pod, ActionType: framework.Add, Label: "AssignedPodAdd"} // NodeAdd is the event when a new node is added to the cluster. NodeAdd = framework.ClusterEvent{Resource: framework.Node, ActionType: framework.Add, Label: "NodeAdd"} - // AssignedPodUpdate is the event when a pod is updated that causes pods with matching affinity - // terms to be more schedulable. + // AssignedPodUpdate is the event when an assigned pod is updated. AssignedPodUpdate = framework.ClusterEvent{Resource: framework.Pod, ActionType: framework.Update, Label: "AssignedPodUpdate"} // UnscheduledPodUpdate is the event when an unscheduled pod is updated. UnscheduledPodUpdate = framework.ClusterEvent{Resource: framework.Pod, ActionType: framework.Update, Label: "UnschedulablePodUpdate"} - // AssignedPodDelete is the event when a pod is deleted that causes pods with matching affinity - // terms to be more schedulable. + // AssignedPodDelete is the event when an assigned pod is deleted. AssignedPodDelete = framework.ClusterEvent{Resource: framework.Pod, ActionType: framework.Delete, Label: "AssignedPodDelete"} // NodeSpecUnschedulableChange is the event when unschedulable node spec is changed. NodeSpecUnschedulableChange = framework.ClusterEvent{Resource: framework.Node, ActionType: framework.UpdateNodeTaint, Label: "NodeSpecUnschedulableChange"} @@ -92,44 +89,49 @@ var ( UnschedulableTimeout = framework.ClusterEvent{Resource: framework.WildCard, ActionType: framework.All, Label: "UnschedulableTimeout"} ) -func NodeSchedulingPropertiesChange(newNode *v1.Node, oldNode *v1.Node) []framework.ClusterEvent { - var events []framework.ClusterEvent - - if nodeSpecUnschedulableChanged(newNode, oldNode) { - events = append(events, NodeSpecUnschedulableChange) - } - if nodeAllocatableChanged(newNode, oldNode) { - events = append(events, NodeAllocatableChange) - } - if nodeLabelsChanged(newNode, oldNode) { - events = append(events, NodeLabelChange) - } - if nodeTaintsChanged(newNode, oldNode) { - events = append(events, NodeTaintChange) - } - if nodeConditionsChanged(newNode, oldNode) { - events = append(events, NodeConditionChange) - } - if nodeAnnotationsChanged(newNode, oldNode) { - events = append(events, NodeAnnotationChange) +// NodeSchedulingPropertiesChange interprets the update of a node and returns corresponding UpdateNodeXYZ event(s). +func NodeSchedulingPropertiesChange(newNode *v1.Node, oldNode *v1.Node) (events []framework.ClusterEvent) { + nodeChangeExtracters := []nodeChangeExtractor{ + extractNodeSpecUnschedulableChange, + extractNodeAllocatableChange, + extractNodeLabelsChange, + extractNodeTaintsChange, + extractNodeConditionsChange, + extractNodeAnnotationsChange, } - return events + for _, fn := range nodeChangeExtracters { + if event := fn(newNode, oldNode); event != nil { + events = append(events, *event) + } + } + return } -func nodeAllocatableChanged(newNode *v1.Node, oldNode *v1.Node) bool { - return !equality.Semantic.DeepEqual(oldNode.Status.Allocatable, newNode.Status.Allocatable) +type nodeChangeExtractor func(newNode *v1.Node, oldNode *v1.Node) *framework.ClusterEvent + +func extractNodeAllocatableChange(newNode *v1.Node, oldNode *v1.Node) *framework.ClusterEvent { + if !equality.Semantic.DeepEqual(oldNode.Status.Allocatable, newNode.Status.Allocatable) { + return &NodeAllocatableChange + } + return nil } -func nodeLabelsChanged(newNode *v1.Node, oldNode *v1.Node) bool { - return !equality.Semantic.DeepEqual(oldNode.GetLabels(), newNode.GetLabels()) +func extractNodeLabelsChange(newNode *v1.Node, oldNode *v1.Node) *framework.ClusterEvent { + if !equality.Semantic.DeepEqual(oldNode.GetLabels(), newNode.GetLabels()) { + return &NodeLabelChange + } + return nil } -func nodeTaintsChanged(newNode *v1.Node, oldNode *v1.Node) bool { - return !equality.Semantic.DeepEqual(newNode.Spec.Taints, oldNode.Spec.Taints) +func extractNodeTaintsChange(newNode *v1.Node, oldNode *v1.Node) *framework.ClusterEvent { + if !equality.Semantic.DeepEqual(newNode.Spec.Taints, oldNode.Spec.Taints) { + return &NodeTaintChange + } + return nil } -func nodeConditionsChanged(newNode *v1.Node, oldNode *v1.Node) bool { +func extractNodeConditionsChange(newNode *v1.Node, oldNode *v1.Node) *framework.ClusterEvent { strip := func(conditions []v1.NodeCondition) map[v1.NodeConditionType]v1.ConditionStatus { conditionStatuses := make(map[v1.NodeConditionType]v1.ConditionStatus, len(conditions)) for i := range conditions { @@ -137,13 +139,22 @@ func nodeConditionsChanged(newNode *v1.Node, oldNode *v1.Node) bool { } return conditionStatuses } - return !equality.Semantic.DeepEqual(strip(oldNode.Status.Conditions), strip(newNode.Status.Conditions)) + if !equality.Semantic.DeepEqual(strip(oldNode.Status.Conditions), strip(newNode.Status.Conditions)) { + return &NodeConditionChange + } + return nil } -func nodeSpecUnschedulableChanged(newNode *v1.Node, oldNode *v1.Node) bool { - return newNode.Spec.Unschedulable != oldNode.Spec.Unschedulable && !newNode.Spec.Unschedulable +func extractNodeSpecUnschedulableChange(newNode *v1.Node, oldNode *v1.Node) *framework.ClusterEvent { + if newNode.Spec.Unschedulable != oldNode.Spec.Unschedulable && !newNode.Spec.Unschedulable { + return &NodeSpecUnschedulableChange + } + return nil } -func nodeAnnotationsChanged(newNode *v1.Node, oldNode *v1.Node) bool { - return !equality.Semantic.DeepEqual(oldNode.GetAnnotations(), newNode.GetAnnotations()) +func extractNodeAnnotationsChange(newNode *v1.Node, oldNode *v1.Node) *framework.ClusterEvent { + if !equality.Semantic.DeepEqual(oldNode.GetAnnotations(), newNode.GetAnnotations()) { + return &NodeAnnotationChange + } + return nil } diff --git a/pkg/scheduler/internal/queue/events_test.go b/pkg/scheduler/internal/queue/events_test.go index ebe55304bfa..e01160baa2f 100644 --- a/pkg/scheduler/internal/queue/events_test.go +++ b/pkg/scheduler/internal/queue/events_test.go @@ -1,9 +1,12 @@ /* Copyright 2024 The Kubernetes Authors. + Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 + Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -25,152 +28,156 @@ import ( st "k8s.io/kubernetes/pkg/scheduler/testing" ) -func TestNodeAllocatableChanged(t *testing.T) { +func TestNodeAllocatableChange(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 string + // changed is true if it's expected that the function detects the change and returns event. + 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: "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)}, + 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) + 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) != nil + if changed != test.changed { + t.Errorf("nodeAllocatableChanged should be %t, got %t", test.changed, changed) } }) } } -func TestNodeLabelsChanged(t *testing.T) { +func TestNodeLabelsChange(t *testing.T) { for _, test := range []struct { - Name string - Changed bool - OldLabels map[string]string - NewLabels map[string]string + name string + // changed is true if it's expected that the function detects the change and returns event. + 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"}, + 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"}, + name: "new object 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) + 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) != nil + 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) { +func TestNodeTaintsChange(t *testing.T) { for _, test := range []struct { - Name string - Changed bool - OldTaints []v1.Taint - NewTaints []v1.Taint + name string + // changed is true if it's expected that the function detects the change and returns event. + 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: "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"}}, + 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) + 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) != nil + 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) { +func TestNodeConditionsChange(t *testing.T) { nodeConditionType := reflect.TypeOf(v1.NodeCondition{}) if nodeConditionType.NumField() != 6 { - t.Errorf("NodeCondition type has changed. The nodeConditionsChanged() function must be reevaluated.") + t.Errorf("NodeCondition type has changed. The nodeConditionsChange() function must be reevaluated.") } for _, test := range []struct { - Name string - Changed bool - OldConditions []v1.NodeCondition - NewConditions []v1.NodeCondition + name string + // changed is true if it's expected that the function detects the change and returns event. + 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: "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: "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 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: "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}}, + 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) + 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) != nil + if changed != test.changed { + t.Errorf("Test case %q failed: should be %t, got %t", test.name, test.changed, changed) } }) }