Merge pull request #114273 from TommyStarK/unit-tests/pkg-scheduler-internal

scheduler/internal: Improving cache and heap test coverage

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 {

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],

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)
+			}
+		})
+	}
+}

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)
+	}
+}