From e9a3815a6ce6761bba69e28fb96db704e03708ea Mon Sep 17 00:00:00 2001 From: Jonathan Basseri Date: Tue, 23 Jan 2018 15:41:28 -0800 Subject: [PATCH] Fix equivalence cache hash tests. --- pkg/scheduler/core/equivalence_cache.go | 24 +- pkg/scheduler/core/equivalence_cache_test.go | 243 +++++-------------- 2 files changed, 77 insertions(+), 190 deletions(-) diff --git a/pkg/scheduler/core/equivalence_cache.go b/pkg/scheduler/core/equivalence_cache.go index d6bb34633ba..f6dc084d2e6 100644 --- a/pkg/scheduler/core/equivalence_cache.go +++ b/pkg/scheduler/core/equivalence_cache.go @@ -238,7 +238,7 @@ type equivalencePod struct { // getEquivalencePod returns the equivalencePod for a Pod. func getEquivalencePod(pod *v1.Pod) *equivalencePod { - return &equivalencePod{ + ep := &equivalencePod{ Namespace: &pod.Namespace, Labels: pod.Labels, Affinity: pod.Spec.Affinity, @@ -249,4 +249,26 @@ func getEquivalencePod(pod *v1.Pod) *equivalencePod { Tolerations: pod.Spec.Tolerations, Volumes: pod.Spec.Volumes, } + // DeepHashObject considers nil and empy slices to be different. Normalize them. + if len(ep.Containers) == 0 { + ep.Containers = nil + } + if len(ep.InitContainers) == 0 { + ep.InitContainers = nil + } + if len(ep.Tolerations) == 0 { + ep.Tolerations = nil + } + if len(ep.Volumes) == 0 { + ep.Volumes = nil + } + // Normalize empty maps also. + if len(ep.Labels) == 0 { + ep.Labels = nil + } + if len(ep.NodeSelector) == 0 { + ep.NodeSelector = nil + } + // TODO: Also normalize nested maps and slices. + return ep } diff --git a/pkg/scheduler/core/equivalence_cache_test.go b/pkg/scheduler/core/equivalence_cache_test.go index 1ae821edc72..4501e870b01 100644 --- a/pkg/scheduler/core/equivalence_cache_test.go +++ b/pkg/scheduler/core/equivalence_cache_test.go @@ -29,11 +29,11 @@ import ( ) // makeBasicPod returns a Pod object with many of the fields populated. -func makeBasicPod() *v1.Pod { +func makeBasicPod(name string) *v1.Pod { isController := true return &v1.Pod{ ObjectMeta: metav1.ObjectMeta{ - Name: "test-pod", + Name: name, Namespace: "test-ns", Labels: map[string]string{"app": "web", "env": "prod"}, OwnerReferences: []metav1.OwnerReference{ @@ -353,185 +353,46 @@ func TestPredicateWithECache(t *testing.T) { } } -func TestGetHashEquivalencePod(t *testing.T) { - - testNamespace := "test" +func TestGetEquivalenceHash(t *testing.T) { ecache := NewEquivalenceCache() - isController := true + pod1 := makeBasicPod("pod1") + pod2 := makeBasicPod("pod2") - pod1 := &v1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pod1", - Namespace: testNamespace, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: "v1", - Kind: "ReplicationController", - Name: "rc", - UID: "123", - Controller: &isController, - }, - }, - }, - Spec: v1.PodSpec{ - Volumes: []v1.Volume{ - { - VolumeSource: v1.VolumeSource{ - PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{ - ClaimName: "someEBSVol1", - }, - }, - }, - { - VolumeSource: v1.VolumeSource{ - PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{ - ClaimName: "someEBSVol2", - }, - }, + pod3 := makeBasicPod("pod3") + pod3.Spec.Volumes = []v1.Volume{ + { + VolumeSource: v1.VolumeSource{ + PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{ + ClaimName: "someEBSVol111", }, }, }, } - pod2 := &v1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pod2", - Namespace: testNamespace, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: "v1", - Kind: "ReplicationController", - Name: "rc", - UID: "123", - Controller: &isController, - }, - }, - }, - Spec: v1.PodSpec{ - Volumes: []v1.Volume{ - { - VolumeSource: v1.VolumeSource{ - PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{ - ClaimName: "someEBSVol2", - }, - }, - }, - { - VolumeSource: v1.VolumeSource{ - PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{ - ClaimName: "someEBSVol1", - }, - }, + pod4 := makeBasicPod("pod4") + pod4.Spec.Volumes = []v1.Volume{ + { + VolumeSource: v1.VolumeSource{ + PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{ + ClaimName: "someEBSVol222", }, }, }, } - pod3 := &v1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pod3", - Namespace: testNamespace, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: "v1", - Kind: "ReplicationController", - Name: "rc", - UID: "567", - Controller: &isController, - }, - }, - }, - Spec: v1.PodSpec{ - Volumes: []v1.Volume{ - { - VolumeSource: v1.VolumeSource{ - PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{ - ClaimName: "someEBSVol3-1", - }, - }, - }, - }, - }, - } + pod5 := makeBasicPod("pod5") + pod5.Spec.Volumes = []v1.Volume{} - pod4 := &v1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pod4", - Namespace: testNamespace, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: "v1", - Kind: "ReplicationController", - Name: "rc", - UID: "567", - Controller: &isController, - }, - }, - }, - Spec: v1.PodSpec{ - Volumes: []v1.Volume{ - { - VolumeSource: v1.VolumeSource{ - PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{ - ClaimName: "someEBSVol3-0", - }, - }, - }, - }, - }, - } + pod6 := makeBasicPod("pod6") + pod6.Spec.Volumes = nil - pod5 := &v1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pod5", - Namespace: testNamespace, - }, - } + pod7 := makeBasicPod("pod7") + pod7.Spec.NodeSelector = nil - pod6 := &v1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pod6", - Namespace: testNamespace, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: "v1", - Kind: "ReplicationController", - Name: "rc", - UID: "567", - Controller: &isController, - }, - }, - }, - Spec: v1.PodSpec{ - Volumes: []v1.Volume{ - { - VolumeSource: v1.VolumeSource{ - PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{ - ClaimName: "no-exists-pvc", - }, - }, - }, - }, - }, - } - - pod7 := &v1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pod7", - Namespace: testNamespace, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: "v1", - Kind: "ReplicationController", - Name: "rc", - UID: "567", - Controller: &isController, - }, - }, - }, - } + pod8 := makeBasicPod("pod8") + pod8.Spec.NodeSelector = make(map[string]string) type podInfo struct { pod *v1.Pod @@ -539,39 +400,41 @@ func TestGetHashEquivalencePod(t *testing.T) { } tests := []struct { + name string podInfoList []podInfo isEquivalent bool }{ - // pods with same controllerRef and same pvc claim { + name: "pods with everything the same except name", podInfoList: []podInfo{ {pod: pod1, hashIsValid: true}, {pod: pod2, hashIsValid: true}, }, isEquivalent: true, }, - // pods with same controllerRef but different pvc claim { + name: "pods that only differ in their PVC volume sources", podInfoList: []podInfo{ {pod: pod3, hashIsValid: true}, {pod: pod4, hashIsValid: true}, }, isEquivalent: false, }, - // pod without controllerRef { + name: "pods that have no volumes, but one uses nil and one uses an empty slice", podInfoList: []podInfo{ - {pod: pod5, hashIsValid: false}, + {pod: pod5, hashIsValid: true}, + {pod: pod6, hashIsValid: true}, }, - isEquivalent: false, + isEquivalent: true, }, - // pods with same controllerRef but one has non-exists pvc claim { + name: "pods that have no NodeSelector, but one uses nil and one uses an empty map", podInfoList: []podInfo{ - {pod: pod6, hashIsValid: false}, {pod: pod7, hashIsValid: true}, + {pod: pod8, hashIsValid: true}, }, - isEquivalent: false, + isEquivalent: true, }, } @@ -581,25 +444,27 @@ func TestGetHashEquivalencePod(t *testing.T) { ) for _, test := range tests { - for i, podInfo := range test.podInfoList { - testPod := podInfo.pod - hash, isValid := ecache.getEquivalenceHash(testPod) - if isValid != podInfo.hashIsValid { - t.Errorf("Failed: pod %v is expected to have valid hash", testPod) - } - // NOTE(harry): the first element will be used as target so - // this logic can't verify more than two inequivalent pods - if i == 0 { - targetHash = hash - targetPodInfo = podInfo - } else { - if targetHash != hash { - if test.isEquivalent { - t.Errorf("Failed: pod: %v is expected to be equivalent to: %v", testPod, targetPodInfo.pod) + t.Run(test.name, func(t *testing.T) { + for i, podInfo := range test.podInfoList { + testPod := podInfo.pod + hash, isValid := ecache.getEquivalenceHash(testPod) + if isValid != podInfo.hashIsValid { + t.Errorf("Failed: pod %v is expected to have valid hash", testPod) + } + // NOTE(harry): the first element will be used as target so + // this logic can't verify more than two inequivalent pods + if i == 0 { + targetHash = hash + targetPodInfo = podInfo + } else { + if targetHash != hash { + if test.isEquivalent { + t.Errorf("Failed: pod: %v is expected to be equivalent to: %v", testPod, targetPodInfo.pod) + } } } } - } + }) } } @@ -736,7 +601,7 @@ func TestInvalidateAllCachedPredicateItemOfNode(t *testing.T) { func BenchmarkEquivalenceHash(b *testing.B) { cache := NewEquivalenceCache() - pod := makeBasicPod() + pod := makeBasicPod("test") for i := 0; i < b.N; i++ { cache.getEquivalenceHash(pod) }