From 4ae7075e277ca27864016ff514a7741b6c48ebbd Mon Sep 17 00:00:00 2001 From: Jonathan Basseri Date: Mon, 22 Jan 2018 16:28:18 -0800 Subject: [PATCH 1/4] Add benchmark for equivalence hashing. --- pkg/scheduler/core/equivalence_cache_test.go | 150 +++++++++++++++++++ 1 file changed, 150 insertions(+) diff --git a/pkg/scheduler/core/equivalence_cache_test.go b/pkg/scheduler/core/equivalence_cache_test.go index 54b903e2fbd..fb2096295f3 100644 --- a/pkg/scheduler/core/equivalence_cache_test.go +++ b/pkg/scheduler/core/equivalence_cache_test.go @@ -21,12 +21,132 @@ import ( "testing" "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/kubernetes/pkg/scheduler/algorithm" "k8s.io/kubernetes/pkg/scheduler/algorithm/predicates" ) +// makeBasicPod returns a Pod object with many of the fields populated. +func makeBasicPod() *v1.Pod { + isController := true + return &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "test-ns", + Labels: map[string]string{"app": "web", "env": "prod"}, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "v1", + Kind: "ReplicationController", + Name: "rc", + UID: "123", + Controller: &isController, + }, + }, + }, + Spec: v1.PodSpec{ + Affinity: &v1.Affinity{ + NodeAffinity: &v1.NodeAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: &v1.NodeSelector{ + NodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: "failure-domain.beta.kubernetes.io/zone", + Operator: "Exists", + }, + }, + }, + }, + }, + }, + PodAffinity: &v1.PodAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: []v1.PodAffinityTerm{ + { + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "db"}}, + TopologyKey: "kubernetes.io/hostname", + }, + }, + }, + PodAntiAffinity: &v1.PodAntiAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: []v1.PodAffinityTerm{ + { + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "web"}}, + TopologyKey: "kubernetes.io/hostname", + }, + }, + }, + }, + InitContainers: []v1.Container{ + { + Name: "init-pause", + Image: "gcr.io/google_containers/pause", + Resources: v1.ResourceRequirements{ + Limits: v1.ResourceList{ + "cpu": resource.MustParse("1"), + "mem": resource.MustParse("100Mi"), + }, + }, + }, + }, + Containers: []v1.Container{ + { + Name: "pause", + Image: "gcr.io/google_containers/pause", + Resources: v1.ResourceRequirements{ + Limits: v1.ResourceList{ + "cpu": resource.MustParse("1"), + "mem": resource.MustParse("100Mi"), + }, + }, + VolumeMounts: []v1.VolumeMount{ + { + Name: "nfs", + MountPath: "/srv/data", + }, + }, + }, + }, + NodeSelector: map[string]string{"node-type": "awesome"}, + Tolerations: []v1.Toleration{ + { + Effect: "NoSchedule", + Key: "experimental", + Operator: "Exists", + }, + }, + Volumes: []v1.Volume{ + { + VolumeSource: v1.VolumeSource{ + PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{ + ClaimName: "someEBSVol1", + }, + }, + }, + { + VolumeSource: v1.VolumeSource{ + PersistentVolumeClaim: &v1.PersistentVolumeClaimVolumeSource{ + ClaimName: "someEBSVol2", + }, + }, + }, + { + Name: "nfs", + VolumeSource: v1.VolumeSource{ + NFS: &v1.NFSVolumeSource{ + Server: "nfs.corp.example.com", + }, + }, + }, + }, + }, + } +} + type predicateItemType struct { fit bool reasons []algorithm.PredicateFailureReason @@ -641,3 +761,33 @@ func TestInvalidateAllCachedPredicateItemOfNode(t *testing.T) { } } } + +func BenchmarkEquivalenceHash(b *testing.B) { + testNamespace := "test" + + pvcInfo := predicates.FakePersistentVolumeClaimInfo{ + { + ObjectMeta: metav1.ObjectMeta{UID: "someEBSVol1", Name: "someEBSVol1", Namespace: testNamespace}, + Spec: v1.PersistentVolumeClaimSpec{VolumeName: "someEBSVol1"}, + }, + { + ObjectMeta: metav1.ObjectMeta{UID: "someEBSVol2", Name: "someEBSVol2", Namespace: testNamespace}, + Spec: v1.PersistentVolumeClaimSpec{VolumeName: "someNonEBSVol"}, + }, + { + ObjectMeta: metav1.ObjectMeta{UID: "someEBSVol3-0", Name: "someEBSVol3-0", Namespace: testNamespace}, + Spec: v1.PersistentVolumeClaimSpec{VolumeName: "pvcWithDeletedPV"}, + }, + { + ObjectMeta: metav1.ObjectMeta{UID: "someEBSVol3-1", Name: "someEBSVol3-1", Namespace: testNamespace}, + Spec: v1.PersistentVolumeClaimSpec{VolumeName: "anotherPVCWithDeletedPV"}, + }, + } + + // use default equivalence class generator + cache := NewEquivalenceCache(predicates.NewEquivalencePodGenerator(pvcInfo)) + pod := makeBasicPod() + for i := 0; i < b.N; i++ { + cache.getHashEquivalencePod(pod) + } +} From 5ab471452097835aa9273debd5130b8df6824592 Mon Sep 17 00:00:00 2001 From: Jonathan Basseri Date: Thu, 18 Jan 2018 15:49:57 -0800 Subject: [PATCH 2/4] Change equivalence hash function. This changes the equivalence class hashing function to use as inputs all the Pod fields which are read by FitPredicates. Before we used a combination of OwnerReference and PersistentVolumeClaim info, which was a close approximation. The new method ensures that hashing remains correct regardless of controller behavior. The PVCSet field can be removed from equivalencePod because it is implicitly included in the Volume list. Tests are now broken. --- pkg/scheduler/algorithm/predicates/utils.go | 71 ++++++++------------- 1 file changed, 25 insertions(+), 46 deletions(-) diff --git a/pkg/scheduler/algorithm/predicates/utils.go b/pkg/scheduler/algorithm/predicates/utils.go index ce3e9d5888e..612f85c60db 100644 --- a/pkg/scheduler/algorithm/predicates/utils.go +++ b/pkg/scheduler/algorithm/predicates/utils.go @@ -17,11 +17,8 @@ limitations under the License. package predicates import ( - "github.com/golang/glog" "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/algorithm" schedutil "k8s.io/kubernetes/pkg/scheduler/util" ) @@ -83,53 +80,35 @@ func NewEquivalencePodGenerator(pvcInfo PersistentVolumeClaimInfo) algorithm.Get return g.getEquivalencePod } -// GetEquivalencePod returns a EquivalencePod which contains a group of pod attributes which can be reused. +// GetEquivalencePod returns a equivalencePod which contains a group of pod attributes which can be reused. func (e *EquivalencePodGenerator) getEquivalencePod(pod *v1.Pod) interface{} { - // For now we only consider pods: - // 1. OwnerReferences is Controller - // 2. with same OwnerReferences - // 3. with same PVC claim - // to be equivalent - for _, ref := range pod.OwnerReferences { - if ref.Controller != nil && *ref.Controller { - if pvcSet, err := e.getPVCSet(pod); err == nil { - // A pod can only belongs to one controller, so let's return. - return &EquivalencePod{ - ControllerRef: ref, - PVCSet: pvcSet, - } - } else { - // If error encountered, log warning and return nil (i.e. no equivalent pod found) - glog.Warningf("[EquivalencePodGenerator] for pod: %v failed due to: %v", pod.GetName(), err) - return nil - } - } + // For equivalence hash to be formally correct, lists and maps + // in the equivalencePod should be normalized. (e.g. by sorting + // them) However, the vast majority of equivalent pod classes + // are expected to be created from a single pod template, so + // they will all have the same ordering. + return &equivalencePod{ + Labels: pod.Labels, + Affinity: pod.Spec.Affinity, + Containers: pod.Spec.Containers, + InitContainers: pod.Spec.InitContainers, + NodeName: &pod.Spec.NodeName, + NodeSelector: pod.Spec.NodeSelector, + Tolerations: pod.Spec.Tolerations, + Volumes: pod.Spec.Volumes, } - return nil } -// getPVCSet returns a set of PVC UIDs of given pod. -func (e *EquivalencePodGenerator) getPVCSet(pod *v1.Pod) (sets.String, error) { - result := sets.NewString() - for _, volume := range pod.Spec.Volumes { - if volume.PersistentVolumeClaim == nil { - continue - } - pvcName := volume.PersistentVolumeClaim.ClaimName - pvc, err := e.pvcInfo.GetPersistentVolumeClaimInfo(pod.GetNamespace(), pvcName) - if err != nil { - return nil, err - } - result.Insert(string(pvc.UID)) - } - - return result, nil -} - -// EquivalencePod is a group of pod attributes which can be reused as equivalence to schedule other pods. -type EquivalencePod struct { - ControllerRef metav1.OwnerReference - PVCSet sets.String +// equivalencePod is a group of pod attributes which can be reused as equivalence to schedule other pods. +type equivalencePod struct { + Labels map[string]string + Affinity *v1.Affinity + Containers []v1.Container // note about ordering + InitContainers []v1.Container // note about ordering + NodeName *string + NodeSelector map[string]string + Tolerations []v1.Toleration + Volumes []v1.Volume // note about ordering } // portsConflict check whether existingPorts and wantPorts conflict with each other From 466a499fcb5cd89042a2b8764cd3aa8699b67ea8 Mon Sep 17 00:00:00 2001 From: Jonathan Basseri Date: Thu, 18 Jan 2018 16:05:55 -0800 Subject: [PATCH 3/4] Move equivalence class hash code. This moves the equivalence hashing code from algorithm/predicates/utils.go to core/equivalence_cache.go. In the process, making the hashing function and hashing function factory both injectable dependencies is removed. --- pkg/scheduler/algorithm/predicates/BUILD | 1 - pkg/scheduler/algorithm/predicates/utils.go | 45 ------------- pkg/scheduler/algorithm/types.go | 2 - .../algorithmprovider/defaults/defaults.go | 7 -- pkg/scheduler/core/equivalence_cache.go | 62 ++++++++++++----- pkg/scheduler/core/equivalence_cache_test.go | 66 +++---------------- pkg/scheduler/core/generic_scheduler.go | 2 +- pkg/scheduler/factory/factory.go | 12 ++-- pkg/scheduler/factory/plugins.go | 11 ---- 9 files changed, 59 insertions(+), 149 deletions(-) diff --git a/pkg/scheduler/algorithm/predicates/BUILD b/pkg/scheduler/algorithm/predicates/BUILD index a028dac6259..85b38d86817 100644 --- a/pkg/scheduler/algorithm/predicates/BUILD +++ b/pkg/scheduler/algorithm/predicates/BUILD @@ -34,7 +34,6 @@ go_library( "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/labels:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/rand:go_default_library", - "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//vendor/k8s.io/apiserver/pkg/util/feature:go_default_library", "//vendor/k8s.io/client-go/listers/core/v1:go_default_library", "//vendor/k8s.io/client-go/listers/storage/v1:go_default_library", diff --git a/pkg/scheduler/algorithm/predicates/utils.go b/pkg/scheduler/algorithm/predicates/utils.go index 612f85c60db..2b9c94f9dc8 100644 --- a/pkg/scheduler/algorithm/predicates/utils.go +++ b/pkg/scheduler/algorithm/predicates/utils.go @@ -19,7 +19,6 @@ package predicates import ( "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/labels" - "k8s.io/kubernetes/pkg/scheduler/algorithm" schedutil "k8s.io/kubernetes/pkg/scheduler/util" ) @@ -67,50 +66,6 @@ func CreateSelectorFromLabels(aL map[string]string) labels.Selector { return labels.Set(aL).AsSelector() } -// EquivalencePodGenerator is a generator of equivalence class for pod with consideration of PVC info. -type EquivalencePodGenerator struct { - pvcInfo PersistentVolumeClaimInfo -} - -// NewEquivalencePodGenerator returns a getEquivalencePod method with consideration of PVC info. -func NewEquivalencePodGenerator(pvcInfo PersistentVolumeClaimInfo) algorithm.GetEquivalencePodFunc { - g := &EquivalencePodGenerator{ - pvcInfo: pvcInfo, - } - return g.getEquivalencePod -} - -// GetEquivalencePod returns a equivalencePod which contains a group of pod attributes which can be reused. -func (e *EquivalencePodGenerator) getEquivalencePod(pod *v1.Pod) interface{} { - // For equivalence hash to be formally correct, lists and maps - // in the equivalencePod should be normalized. (e.g. by sorting - // them) However, the vast majority of equivalent pod classes - // are expected to be created from a single pod template, so - // they will all have the same ordering. - return &equivalencePod{ - Labels: pod.Labels, - Affinity: pod.Spec.Affinity, - Containers: pod.Spec.Containers, - InitContainers: pod.Spec.InitContainers, - NodeName: &pod.Spec.NodeName, - NodeSelector: pod.Spec.NodeSelector, - Tolerations: pod.Spec.Tolerations, - Volumes: pod.Spec.Volumes, - } -} - -// equivalencePod is a group of pod attributes which can be reused as equivalence to schedule other pods. -type equivalencePod struct { - Labels map[string]string - Affinity *v1.Affinity - Containers []v1.Container // note about ordering - InitContainers []v1.Container // note about ordering - NodeName *string - NodeSelector map[string]string - Tolerations []v1.Toleration - Volumes []v1.Volume // note about ordering -} - // portsConflict check whether existingPorts and wantPorts conflict with each other // return true if we have a conflict func portsConflict(existingPorts schedutil.HostPortInfo, wantPorts []*v1.ContainerPort) bool { diff --git a/pkg/scheduler/algorithm/types.go b/pkg/scheduler/algorithm/types.go index f6ff3b49427..8a40bfd6928 100644 --- a/pkg/scheduler/algorithm/types.go +++ b/pkg/scheduler/algorithm/types.go @@ -75,8 +75,6 @@ type PredicateFailureReason interface { GetReason() string } -type GetEquivalencePodFunc func(pod *v1.Pod) interface{} - // NodeLister interface represents anything that can list nodes for a scheduler. type NodeLister interface { // We explicitly return []*v1.Node, instead of v1.NodeList, to avoid diff --git a/pkg/scheduler/algorithmprovider/defaults/defaults.go b/pkg/scheduler/algorithmprovider/defaults/defaults.go index 11b0a54042a..016a4fcb050 100644 --- a/pkg/scheduler/algorithmprovider/defaults/defaults.go +++ b/pkg/scheduler/algorithmprovider/defaults/defaults.go @@ -77,13 +77,6 @@ func init() { // Fit is determined by node selector query. factory.RegisterFitPredicate(predicates.MatchNodeSelectorPred, predicates.PodMatchNodeSelector) - // Use equivalence class to speed up heavy predicates phase. - factory.RegisterGetEquivalencePodFunction( - func(args factory.PluginFactoryArgs) algorithm.GetEquivalencePodFunc { - return predicates.NewEquivalencePodGenerator(args.PVCInfo) - }, - ) - // ServiceSpreadingPriority is a priority config factory that spreads pods by minimizing // the number of pods (belonging to the same service) on the same node. // Register the factory so that it's available, but do not include it as part of the default priorities diff --git a/pkg/scheduler/core/equivalence_cache.go b/pkg/scheduler/core/equivalence_cache.go index 5d9bda7eafe..d6bb34633ba 100644 --- a/pkg/scheduler/core/equivalence_cache.go +++ b/pkg/scheduler/core/equivalence_cache.go @@ -37,8 +37,7 @@ const maxCacheEntries = 100 // 2. function to get equivalence pod type EquivalenceCache struct { sync.RWMutex - getEquivalencePod algorithm.GetEquivalencePodFunc - algorithmCache map[string]AlgorithmCache + algorithmCache map[string]AlgorithmCache } // The AlgorithmCache stores PredicateMap with predicate name as key @@ -62,10 +61,9 @@ func newAlgorithmCache() AlgorithmCache { } } -func NewEquivalenceCache(getEquivalencePodFunc algorithm.GetEquivalencePodFunc) *EquivalenceCache { +func NewEquivalenceCache() *EquivalenceCache { return &EquivalenceCache{ - getEquivalencePod: getEquivalencePodFunc, - algorithmCache: make(map[string]AlgorithmCache), + algorithmCache: make(map[string]AlgorithmCache), } } @@ -208,15 +206,47 @@ func (ec *EquivalenceCache) InvalidateCachedPredicateItemForPodAdd(pod *v1.Pod, ec.InvalidateCachedPredicateItem(nodeName, invalidPredicates) } -// getHashEquivalencePod returns the hash of equivalence pod. -// 1. equivalenceHash -// 2. if equivalence hash is valid -func (ec *EquivalenceCache) getHashEquivalencePod(pod *v1.Pod) (uint64, bool) { - equivalencePod := ec.getEquivalencePod(pod) - if equivalencePod != nil { - hash := fnv.New32a() - hashutil.DeepHashObject(hash, equivalencePod) - return uint64(hash.Sum32()), true - } - return 0, false +// getEquivalenceHash computes a hash of the given pod. +// The hashing function returns the same value for any two pods that are +// equivalent from the perspective of scheduling. +func (ec *EquivalenceCache) getEquivalenceHash(pod *v1.Pod) (uint64, bool) { + equivalencePod := getEquivalencePod(pod) + hash := fnv.New32a() + hashutil.DeepHashObject(hash, equivalencePod) + return uint64(hash.Sum32()), true +} + +// equivalencePod is the set of pod attributes which must match for two pods to +// be considered equivalent for scheduling purposes. For correctness, this must +// include any Pod field which is used by a FitPredicate. +// +// NOTE: For equivalence hash to be formally correct, lists and maps in the +// equivalencePod should be normalized. (e.g. by sorting them) However, the +// vast majority of equivalent pod classes are expected to be created from a +// single pod template, so they will all have the same ordering. +type equivalencePod struct { + Namespace *string + Labels map[string]string + Affinity *v1.Affinity + Containers []v1.Container // See note about ordering + InitContainers []v1.Container // See note about ordering + NodeName *string + NodeSelector map[string]string + Tolerations []v1.Toleration + Volumes []v1.Volume // See note about ordering +} + +// getEquivalencePod returns the equivalencePod for a Pod. +func getEquivalencePod(pod *v1.Pod) *equivalencePod { + return &equivalencePod{ + Namespace: &pod.Namespace, + Labels: pod.Labels, + Affinity: pod.Spec.Affinity, + Containers: pod.Spec.Containers, + InitContainers: pod.Spec.InitContainers, + NodeName: &pod.Spec.NodeName, + NodeSelector: pod.Spec.NodeSelector, + Tolerations: pod.Spec.Tolerations, + Volumes: pod.Spec.Volumes, + } } diff --git a/pkg/scheduler/core/equivalence_cache_test.go b/pkg/scheduler/core/equivalence_cache_test.go index fb2096295f3..1ae821edc72 100644 --- a/pkg/scheduler/core/equivalence_cache_test.go +++ b/pkg/scheduler/core/equivalence_cache_test.go @@ -190,9 +190,7 @@ func TestUpdateCachedPredicateItem(t *testing.T) { }, } for _, test := range tests { - // this case does not need to calculate equivalence hash, just pass an empty function - fakeGetEquivalencePodFunc := func(pod *v1.Pod) interface{} { return nil } - ecache := NewEquivalenceCache(fakeGetEquivalencePodFunc) + ecache := NewEquivalenceCache() if test.expectPredicateMap { ecache.algorithmCache[test.nodeName] = newAlgorithmCache() predicateItem := HostPredicate{ @@ -310,9 +308,7 @@ func TestPredicateWithECache(t *testing.T) { } for _, test := range tests { - // this case does not need to calculate equivalence hash, just pass an empty function - fakeGetEquivalencePodFunc := func(pod *v1.Pod) interface{} { return nil } - ecache := NewEquivalenceCache(fakeGetEquivalencePodFunc) + ecache := NewEquivalenceCache() // set cached item to equivalence cache ecache.UpdateCachedPredicateItem( test.podName, @@ -361,27 +357,7 @@ func TestGetHashEquivalencePod(t *testing.T) { testNamespace := "test" - pvcInfo := predicates.FakePersistentVolumeClaimInfo{ - { - ObjectMeta: metav1.ObjectMeta{UID: "someEBSVol1", Name: "someEBSVol1", Namespace: testNamespace}, - Spec: v1.PersistentVolumeClaimSpec{VolumeName: "someEBSVol1"}, - }, - { - ObjectMeta: metav1.ObjectMeta{UID: "someEBSVol2", Name: "someEBSVol2", Namespace: testNamespace}, - Spec: v1.PersistentVolumeClaimSpec{VolumeName: "someNonEBSVol"}, - }, - { - ObjectMeta: metav1.ObjectMeta{UID: "someEBSVol3-0", Name: "someEBSVol3-0", Namespace: testNamespace}, - Spec: v1.PersistentVolumeClaimSpec{VolumeName: "pvcWithDeletedPV"}, - }, - { - ObjectMeta: metav1.ObjectMeta{UID: "someEBSVol3-1", Name: "someEBSVol3-1", Namespace: testNamespace}, - Spec: v1.PersistentVolumeClaimSpec{VolumeName: "anotherPVCWithDeletedPV"}, - }, - } - - // use default equivalence class generator - ecache := NewEquivalenceCache(predicates.NewEquivalencePodGenerator(pvcInfo)) + ecache := NewEquivalenceCache() isController := true @@ -607,7 +583,7 @@ func TestGetHashEquivalencePod(t *testing.T) { for _, test := range tests { for i, podInfo := range test.podInfoList { testPod := podInfo.pod - hash, isValid := ecache.getHashEquivalencePod(testPod) + hash, isValid := ecache.getEquivalenceHash(testPod) if isValid != podInfo.hashIsValid { t.Errorf("Failed: pod %v is expected to have valid hash", testPod) } @@ -668,9 +644,7 @@ func TestInvalidateCachedPredicateItemOfAllNodes(t *testing.T) { }, }, } - // this case does not need to calculate equivalence hash, just pass an empty function - fakeGetEquivalencePodFunc := func(pod *v1.Pod) interface{} { return nil } - ecache := NewEquivalenceCache(fakeGetEquivalencePodFunc) + ecache := NewEquivalenceCache() for _, test := range tests { // set cached item to equivalence cache @@ -736,9 +710,7 @@ func TestInvalidateAllCachedPredicateItemOfNode(t *testing.T) { }, }, } - // this case does not need to calculate equivalence hash, just pass an empty function - fakeGetEquivalencePodFunc := func(pod *v1.Pod) interface{} { return nil } - ecache := NewEquivalenceCache(fakeGetEquivalencePodFunc) + ecache := NewEquivalenceCache() for _, test := range tests { // set cached item to equivalence cache @@ -763,31 +735,9 @@ func TestInvalidateAllCachedPredicateItemOfNode(t *testing.T) { } func BenchmarkEquivalenceHash(b *testing.B) { - testNamespace := "test" - - pvcInfo := predicates.FakePersistentVolumeClaimInfo{ - { - ObjectMeta: metav1.ObjectMeta{UID: "someEBSVol1", Name: "someEBSVol1", Namespace: testNamespace}, - Spec: v1.PersistentVolumeClaimSpec{VolumeName: "someEBSVol1"}, - }, - { - ObjectMeta: metav1.ObjectMeta{UID: "someEBSVol2", Name: "someEBSVol2", Namespace: testNamespace}, - Spec: v1.PersistentVolumeClaimSpec{VolumeName: "someNonEBSVol"}, - }, - { - ObjectMeta: metav1.ObjectMeta{UID: "someEBSVol3-0", Name: "someEBSVol3-0", Namespace: testNamespace}, - Spec: v1.PersistentVolumeClaimSpec{VolumeName: "pvcWithDeletedPV"}, - }, - { - ObjectMeta: metav1.ObjectMeta{UID: "someEBSVol3-1", Name: "someEBSVol3-1", Namespace: testNamespace}, - Spec: v1.PersistentVolumeClaimSpec{VolumeName: "anotherPVCWithDeletedPV"}, - }, - } - - // use default equivalence class generator - cache := NewEquivalenceCache(predicates.NewEquivalencePodGenerator(pvcInfo)) + cache := NewEquivalenceCache() pod := makeBasicPod() for i := 0; i < b.N; i++ { - cache.getHashEquivalencePod(pod) + cache.getEquivalenceHash(pod) } } diff --git a/pkg/scheduler/core/generic_scheduler.go b/pkg/scheduler/core/generic_scheduler.go index da04ff45ad6..e3f47a387f0 100644 --- a/pkg/scheduler/core/generic_scheduler.go +++ b/pkg/scheduler/core/generic_scheduler.go @@ -418,7 +418,7 @@ func podFitsOnNode( if ecache != nil { // getHashEquivalencePod will return immediately if no equivalence pod found - equivalenceHash, eCacheAvailable = ecache.getHashEquivalencePod(pod) + equivalenceHash, eCacheAvailable = ecache.getEquivalenceHash(pod) } podsAdded := false // We run predicates twice in some cases. If the node has greater or equal priority diff --git a/pkg/scheduler/factory/factory.go b/pkg/scheduler/factory/factory.go index dfc5d6c3db8..3f6e6e815e0 100644 --- a/pkg/scheduler/factory/factory.go +++ b/pkg/scheduler/factory/factory.go @@ -135,6 +135,8 @@ type configFactory struct { alwaysCheckAllPredicates bool } +var _ scheduler.Configurator = &configFactory{} + // NewConfigFactory initializes the default implementation of a Configurator To encourage eventual privatization of the struct type, we only // return the interface. func NewConfigFactory( @@ -967,14 +969,8 @@ func (f *configFactory) CreateFromKeys(predicateKeys, priorityKeys sets.String, } // Init equivalence class cache - if f.enableEquivalenceClassCache && getEquivalencePodFuncFactory != nil { - pluginArgs, err := f.getPluginArgs() - if err != nil { - return nil, err - } - f.equivalencePodCache = core.NewEquivalenceCache( - getEquivalencePodFuncFactory(*pluginArgs), - ) + if f.enableEquivalenceClassCache { + f.equivalencePodCache = core.NewEquivalenceCache() glog.Info("Created equivalence class cache") } diff --git a/pkg/scheduler/factory/plugins.go b/pkg/scheduler/factory/plugins.go index 1447d9487c2..9ec28df06fc 100644 --- a/pkg/scheduler/factory/plugins.go +++ b/pkg/scheduler/factory/plugins.go @@ -75,9 +75,6 @@ type PriorityConfigFactory struct { Weight int } -// EquivalencePodFuncFactory produces a function to get equivalence class for given pod. -type EquivalencePodFuncFactory func(PluginFactoryArgs) algorithm.GetEquivalencePodFunc - var ( schedulerFactoryMutex sync.Mutex @@ -90,9 +87,6 @@ var ( // Registered metadata producers priorityMetadataProducer PriorityMetadataProducerFactory predicateMetadataProducer PredicateMetadataProducerFactory - - // get equivalence pod function - getEquivalencePodFuncFactory EquivalencePodFuncFactory ) const ( @@ -341,11 +335,6 @@ func RegisterCustomPriorityFunction(policy schedulerapi.PriorityPolicy) string { return RegisterPriorityConfigFactory(policy.Name, *pcf) } -// RegisterGetEquivalencePodFunction registers equivalenceFuncFactory to produce equivalence class for given pod. -func RegisterGetEquivalencePodFunction(equivalenceFuncFactory EquivalencePodFuncFactory) { - getEquivalencePodFuncFactory = equivalenceFuncFactory -} - // IsPriorityFunctionRegistered is useful for testing providers. func IsPriorityFunctionRegistered(name string) bool { schedulerFactoryMutex.Lock() From e9a3815a6ce6761bba69e28fb96db704e03708ea Mon Sep 17 00:00:00 2001 From: Jonathan Basseri Date: Tue, 23 Jan 2018 15:41:28 -0800 Subject: [PATCH 4/4] 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) }