From 94a29efe2f636fa676f63e64bebcf5555aeea030 Mon Sep 17 00:00:00 2001 From: TommyStarK Date: Fri, 2 Dec 2022 19:57:46 +0100 Subject: [PATCH] scheduler/internal: Improving cache and heap test coverage Signed-off-by: TommyStarK --- pkg/scheduler/internal/cache/cache_test.go | 34 +++ .../internal/cache/node_tree_test.go | 5 + pkg/scheduler/internal/cache/snapshot_test.go | 212 ++++++++++++++++++ pkg/scheduler/internal/heap/heap_test.go | 60 +++++ 4 files changed, 311 insertions(+) diff --git a/pkg/scheduler/internal/cache/cache_test.go b/pkg/scheduler/internal/cache/cache_test.go index 8b74b9eca47..1a256a1f429 100644 --- a/pkg/scheduler/internal/cache/cache_test.go +++ b/pkg/scheduler/internal/cache/cache_test.go @@ -212,6 +212,10 @@ func TestAssumePodScheduled(t *testing.T) { if err := cache.AssumePod(pod); err != nil { t.Fatalf("AssumePod failed: %v", err) } + // pod already in cache so can't be assumed + if err := cache.AssumePod(pod); err == nil { + t.Error("expected error, no error found") + } } n := cache.nodes[nodeName] if err := deepEqualWithoutGeneration(n, tt.wNodeInfo); err != nil { @@ -389,6 +393,10 @@ func TestAddPodWillConfirm(t *testing.T) { if err := cache.AddPod(podToAdd); err != nil { t.Fatalf("AddPod failed: %v", err) } + // pod already in added state + if err := cache.AddPod(podToAdd); err == nil { + t.Error("expected error, no error found") + } } cache.cleanupAssumedPods(now.Add(2 * ttl)) // check after expiration. confirmed pods shouldn't be expired. @@ -733,6 +741,17 @@ func TestUpdatePodAndGet(t *testing.T) { for i, tt := range tests { t.Run(fmt.Sprintf("case_%d", i), func(t *testing.T) { cache := newCache(ttl, time.Second, nil) + // trying to get an unknown pod should return an error + // podToUpdate has not been added yet + if _, err := cache.GetPod(tt.podToUpdate); err == nil { + t.Error("expected error, no error found") + } + + // trying to update an unknown pod should return an error + // pod has not been added yet + if err := cache.UpdatePod(tt.pod, tt.podToUpdate); err == nil { + t.Error("expected error, no error found") + } if err := tt.handler(cache, tt.pod); err != nil { t.Fatalf("unexpected err: %v", err) @@ -947,6 +966,11 @@ func TestRemovePod(t *testing.T) { t.Errorf("pod was not deleted") } + // trying to remove a pod already removed should return an error + if err := cache.RemovePod(pod); err == nil { + t.Error("expected error, no error found") + } + // Node that owned the Pod should be at the head of the list. if cache.headNode.info.Node().Name != nodeName { t.Errorf("node %q is not at the head of the list", nodeName) @@ -992,6 +1016,10 @@ func TestForgetPod(t *testing.T) { if err := isForgottenFromCache(pod, cache); err != nil { t.Errorf("pod %q: %v", pod.Name, err) } + // trying to forget a pod already forgotten should return an error + if err := cache.ForgetPod(pod); err == nil { + t.Error("expected error, no error found") + } } } @@ -1220,6 +1248,12 @@ func TestNodeOperators(t *testing.T) { } else if n != nil { t.Errorf("The node object for %v should be nil", node.Name) } + + // trying to remove a node already removed should return an error + if err := cache.RemoveNode(node); err == nil { + t.Error("expected error, no error found") + } + // Check node is removed from nodeTree as well. nodesList, err = cache.nodeTree.list() if err != nil { diff --git a/pkg/scheduler/internal/cache/node_tree_test.go b/pkg/scheduler/internal/cache/node_tree_test.go index a316a0ef4cf..ba497066cf7 100644 --- a/pkg/scheduler/internal/cache/node_tree_test.go +++ b/pkg/scheduler/internal/cache/node_tree_test.go @@ -165,6 +165,11 @@ func TestNodeTree_AddNode(t *testing.T) { nodesToAdd: allNodes[:1], expectedTree: map[string][]string{"": {"node-0"}}, }, + { + name: "same node specified twice", + nodesToAdd: []*v1.Node{allNodes[0], allNodes[0]}, + expectedTree: map[string][]string{"": {"node-0"}}, + }, { name: "mix of nodes with and without proper labels", nodesToAdd: allNodes[:4], diff --git a/pkg/scheduler/internal/cache/snapshot_test.go b/pkg/scheduler/internal/cache/snapshot_test.go index 6675c728a3a..8392dd6abd3 100644 --- a/pkg/scheduler/internal/cache/snapshot_test.go +++ b/pkg/scheduler/internal/cache/snapshot_test.go @@ -24,6 +24,7 @@ import ( "github.com/google/go-cmp/cmp" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/kubernetes/pkg/scheduler/framework" st "k8s.io/kubernetes/pkg/scheduler/testing" @@ -229,3 +230,214 @@ func TestCreateUsedPVCSet(t *testing.T) { }) } } + +func TestNewSnapshot(t *testing.T) { + podWithAnnotations := st.MakePod().Name("foo").Namespace("ns").Node("node-1").Annotations(map[string]string{"custom": "annotation"}).Obj() + podWithPort := st.MakePod().Name("foo").Namespace("foo").Node("node-0").ContainerPort([]v1.ContainerPort{{HostPort: 8080}}).Obj() + podWithAntiAffitiny := st.MakePod().Name("baz").Namespace("ns").PodAntiAffinity("another", &metav1.LabelSelector{MatchLabels: map[string]string{"another": "label"}}, st.PodAntiAffinityWithRequiredReq).Node("node-0").Obj() + podsWithAffitiny := []*v1.Pod{ + st.MakePod().Name("bar").Namespace("ns").PodAffinity("baz", &metav1.LabelSelector{MatchLabels: map[string]string{"baz": "qux"}}, st.PodAffinityWithRequiredReq).Node("node-2").Obj(), + st.MakePod().Name("bar").Namespace("ns").PodAffinity("key", &metav1.LabelSelector{MatchLabels: map[string]string{"key": "value"}}, st.PodAffinityWithRequiredReq).Node("node-0").Obj(), + } + podsWithPVCs := []*v1.Pod{ + st.MakePod().Name("foo").Namespace("foo").Node("node-0").PVC("pvc0").Obj(), + st.MakePod().Name("bar").Namespace("bar").Node("node-1").PVC("pvc1").Obj(), + st.MakePod().Name("baz").Namespace("baz").Node("node-2").PVC("pvc2").Obj(), + } + testCases := []struct { + name string + pods []*v1.Pod + nodes []*v1.Node + expectedNodesInfos []*framework.NodeInfo + expectedNumNodes int + expectedPodsWithAffinity int + expectedPodsWithAntiAffinity int + expectedUsedPVCSet sets.String + }{ + { + name: "no pods no nodes", + pods: nil, + nodes: nil, + }, + { + name: "single pod single node", + pods: []*v1.Pod{ + podWithPort, + }, + nodes: []*v1.Node{ + {ObjectMeta: metav1.ObjectMeta{Name: "node-0"}}, + }, + expectedNodesInfos: []*framework.NodeInfo{ + { + Pods: []*framework.PodInfo{ + {Pod: podWithPort}, + }, + }, + }, + expectedNumNodes: 1, + }, + { + name: "multiple nodes, pods with PVCs", + pods: podsWithPVCs, + nodes: []*v1.Node{ + {ObjectMeta: metav1.ObjectMeta{Name: "node-0"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "node-1"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "node-2"}}, + }, + expectedNodesInfos: []*framework.NodeInfo{ + { + Pods: []*framework.PodInfo{ + {Pod: podsWithPVCs[0]}, + }, + }, + { + Pods: []*framework.PodInfo{ + {Pod: podsWithPVCs[1]}, + }, + }, + { + Pods: []*framework.PodInfo{ + {Pod: podsWithPVCs[2]}, + }, + }, + }, + expectedNumNodes: 3, + expectedUsedPVCSet: sets.NewString("foo/pvc0", "bar/pvc1", "baz/pvc2"), + }, + { + name: "multiple nodes, pod with affinity", + pods: []*v1.Pod{ + podWithAnnotations, + podsWithAffitiny[0], + }, + nodes: []*v1.Node{ + {ObjectMeta: metav1.ObjectMeta{Name: "node-0"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "node-1"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "node-2", Labels: map[string]string{"baz": "qux"}}}, + }, + expectedNodesInfos: []*framework.NodeInfo{ + { + Pods: []*framework.PodInfo{}, + }, + { + Pods: []*framework.PodInfo{ + {Pod: podWithAnnotations}, + }, + }, + { + Pods: []*framework.PodInfo{ + { + Pod: podsWithAffitiny[0], + RequiredAffinityTerms: []framework.AffinityTerm{ + { + Namespaces: sets.NewString("ns"), + Selector: labels.SelectorFromSet(map[string]string{"baz": "qux"}), + TopologyKey: "baz", + NamespaceSelector: labels.Nothing(), + }, + }, + }, + }, + }, + }, + expectedNumNodes: 3, + expectedPodsWithAffinity: 1, + }, + { + name: "multiple nodes, pod with affinity, pod with anti-affinity", + pods: []*v1.Pod{ + podsWithAffitiny[1], + podWithAntiAffitiny, + }, + nodes: []*v1.Node{ + {ObjectMeta: metav1.ObjectMeta{Name: "node-0", Labels: map[string]string{"key": "value"}}}, + {ObjectMeta: metav1.ObjectMeta{Name: "node-1", Labels: map[string]string{"another": "label"}}}, + }, + expectedNodesInfos: []*framework.NodeInfo{ + { + Pods: []*framework.PodInfo{ + { + Pod: podsWithAffitiny[1], + RequiredAffinityTerms: []framework.AffinityTerm{ + { + Namespaces: sets.NewString("ns"), + Selector: labels.SelectorFromSet(map[string]string{"key": "value"}), + TopologyKey: "key", + NamespaceSelector: labels.Nothing(), + }, + }, + }, + { + Pod: podWithAntiAffitiny, + RequiredAntiAffinityTerms: []framework.AffinityTerm{ + { + Namespaces: sets.NewString("ns"), + Selector: labels.SelectorFromSet(map[string]string{"another": "label"}), + TopologyKey: "another", + NamespaceSelector: labels.Nothing(), + }, + }, + }, + }, + }, + { + Pods: []*framework.PodInfo{}, + }, + }, + expectedNumNodes: 2, + expectedPodsWithAffinity: 1, + expectedPodsWithAntiAffinity: 1, + }, + } + + for _, test := range testCases { + t.Run(test.name, func(t *testing.T) { + snapshot := NewSnapshot(test.pods, test.nodes) + + if test.expectedNumNodes != snapshot.NumNodes() { + t.Errorf("unexpected number of nodes, want: %v, got: %v", test.expectedNumNodes, snapshot.NumNodes()) + } + + for i, node := range test.nodes { + info, err := snapshot.Get(node.Name) + if err != nil { + t.Errorf("unexpected error but got %s", err) + } + if info == nil { + t.Error("node infos should not be nil") + } + for j := range test.expectedNodesInfos[i].Pods { + if diff := cmp.Diff(test.expectedNodesInfos[i].Pods[j], info.Pods[j]); diff != "" { + t.Errorf("Unexpected PodInfo (-want +got):\n%s", diff) + } + } + } + + affinityList, err := snapshot.HavePodsWithAffinityList() + if err != nil { + t.Errorf("unexpected error but got %s", err) + } + if test.expectedPodsWithAffinity != len(affinityList) { + t.Errorf("unexpected affinityList number, want: %v, got: %v", test.expectedPodsWithAffinity, len(affinityList)) + } + + antiAffinityList, err := snapshot.HavePodsWithRequiredAntiAffinityList() + if err != nil { + t.Errorf("unexpected error but got %s", err) + } + if test.expectedPodsWithAntiAffinity != len(antiAffinityList) { + t.Errorf("unexpected antiAffinityList number, want: %v, got: %v", test.expectedPodsWithAntiAffinity, len(antiAffinityList)) + } + + for key := range test.expectedUsedPVCSet { + if !snapshot.IsPVCUsedByPods(key) { + t.Errorf("unexpected IsPVCUsedByPods for %s, want: true, got: false", key) + } + } + + if diff := cmp.Diff(test.expectedUsedPVCSet, snapshot.usedPVCSet); diff != "" { + t.Errorf("Unexpected usedPVCSet (-want +got):\n%s", diff) + } + }) + } +} diff --git a/pkg/scheduler/internal/heap/heap_test.go b/pkg/scheduler/internal/heap/heap_test.go index f52ba7257b7..b337e3cc387 100644 --- a/pkg/scheduler/internal/heap/heap_test.go +++ b/pkg/scheduler/internal/heap/heap_test.go @@ -32,6 +32,26 @@ type testHeapObject struct { val interface{} } +type testMetricRecorder int + +func (tmr *testMetricRecorder) Inc() { + if tmr != nil { + *tmr++ + } +} + +func (tmr *testMetricRecorder) Dec() { + if tmr != nil { + *tmr-- + } +} + +func (tmr *testMetricRecorder) Clear() { + if tmr != nil { + *tmr = 0 + } +} + func mkHeapObj(name string, val interface{}) testHeapObject { return testHeapObject{name: name, val: val} } @@ -48,8 +68,18 @@ func TestHeapBasic(t *testing.T) { const amount = 500 var i int + // empty queue + if item := h.Peek(); item != nil { + t.Errorf("expected nil object but got %v", item) + } + for i = amount; i > 0; i-- { h.Add(mkHeapObj(string([]rune{'a', rune(i)}), i)) + // Retrieve head without removing it + head := h.Peek() + if e, a := i, head.(testHeapObject).val; a != e { + t.Errorf("expected %d, got %d", e, a) + } } // Make sure that the numbers are popped in ascending order. @@ -233,3 +263,33 @@ func TestHeap_List(t *testing.T) { } } } + +func TestHeapWithRecorder(t *testing.T) { + metricRecorder := new(testMetricRecorder) + h := NewWithRecorder(testHeapObjectKeyFunc, compareInts, metricRecorder) + h.Add(mkHeapObj("foo", 10)) + h.Add(mkHeapObj("bar", 1)) + h.Add(mkHeapObj("baz", 100)) + h.Add(mkHeapObj("qux", 11)) + + if *metricRecorder != 4 { + t.Errorf("expected count to be 4 but got %d", *metricRecorder) + } + if err := h.Delete(mkHeapObj("bar", 1)); err != nil { + t.Fatal(err) + } + if *metricRecorder != 3 { + t.Errorf("expected count to be 3 but got %d", *metricRecorder) + } + if _, err := h.Pop(); err != nil { + t.Fatal(err) + } + if *metricRecorder != 2 { + t.Errorf("expected count to be 2 but got %d", *metricRecorder) + } + + h.metricRecorder.Clear() + if *metricRecorder != 0 { + t.Errorf("expected count to be 0 but got %d", *metricRecorder) + } +}