From 339ce0e804b145ddace00b55fa23415d5d69ca9a Mon Sep 17 00:00:00 2001 From: Ramya Krishnan Date: Thu, 10 Jan 2019 17:42:20 -0800 Subject: [PATCH] Fix SelectorSpreadPriority scheduler to match all selectors. --- .../priorities/selector_spreading.go | 56 +++++++++---------- .../priorities/selector_spreading_test.go | 24 ++++---- 2 files changed, 38 insertions(+), 42 deletions(-) diff --git a/pkg/scheduler/algorithm/priorities/selector_spreading.go b/pkg/scheduler/algorithm/priorities/selector_spreading.go index 06cdc6edbb..0b225781fc 100644 --- a/pkg/scheduler/algorithm/priorities/selector_spreading.go +++ b/pkg/scheduler/algorithm/priorities/selector_spreading.go @@ -84,29 +84,11 @@ func (s *SelectorSpread) CalculateSpreadPriorityMap(pod *v1.Pod, meta interface{ }, nil } - count := int(0) - for _, nodePod := range nodeInfo.Pods() { - if pod.Namespace != nodePod.Namespace { - continue - } - // When we are replacing a failed pod, we often see the previous - // deleted version while scheduling the replacement. - // Ignore the previous deleted version for spreading purposes - // (it can still be considered for resource restrictions etc.) - if nodePod.DeletionTimestamp != nil { - klog.V(4).Infof("skipping pending-deleted pod: %s/%s", nodePod.Namespace, nodePod.Name) - continue - } - for _, selector := range selectors { - if selector.Matches(labels.Set(nodePod.ObjectMeta.Labels)) { - count++ - break - } - } - } + count := countMatchingPods(pod.Namespace, selectors, nodeInfo) + return schedulerapi.HostPriority{ Host: node.Name, - Score: int(count), + Score: count, }, nil } @@ -201,19 +183,29 @@ func (s *ServiceAntiAffinity) getNodeClassificationByLabels(nodes []*v1.Node) (m return labeledNodes, nonLabeledNodes } -// filteredPod get pods based on namespace and selector -func filteredPod(namespace string, selector labels.Selector, nodeInfo *schedulernodeinfo.NodeInfo) (pods []*v1.Pod) { - if nodeInfo.Pods() == nil || len(nodeInfo.Pods()) == 0 || selector == nil { - return []*v1.Pod{} +// countMatchingPods cout pods based on namespace and matching all selectors +func countMatchingPods(namespace string, selectors []labels.Selector, nodeInfo *schedulernodeinfo.NodeInfo) int { + if nodeInfo.Pods() == nil || len(nodeInfo.Pods()) == 0 || len(selectors) == 0 { + return 0 } + count := 0 for _, pod := range nodeInfo.Pods() { // Ignore pods being deleted for spreading purposes // Similar to how it is done for SelectorSpreadPriority - if namespace == pod.Namespace && pod.DeletionTimestamp == nil && selector.Matches(labels.Set(pod.Labels)) { - pods = append(pods, pod) + if namespace == pod.Namespace && pod.DeletionTimestamp == nil { + matches := true + for _, selector := range selectors { + if !selector.Matches(labels.Set(pod.Labels)) { + matches = false + break + } + } + if matches { + count++ + } } } - return + return count } // CalculateAntiAffinityPriorityMap spreads pods by minimizing the number of pods belonging to the same service @@ -232,11 +224,15 @@ func (s *ServiceAntiAffinity) CalculateAntiAffinityPriorityMap(pod *v1.Pod, meta firstServiceSelector = getFirstServiceSelector(pod, s.serviceLister) } //pods matched namespace,selector on current node - matchedPodsOfNode := filteredPod(pod.Namespace, firstServiceSelector, nodeInfo) + var selectors []labels.Selector + if firstServiceSelector != nil { + selectors = append(selectors, firstServiceSelector) + } + score := countMatchingPods(pod.Namespace, selectors, nodeInfo) return schedulerapi.HostPriority{ Host: node.Name, - Score: int(len(matchedPodsOfNode)), + Score: score, }, nil } diff --git a/pkg/scheduler/algorithm/priorities/selector_spreading_test.go b/pkg/scheduler/algorithm/priorities/selector_spreading_test.go index e76d879aef..eacc7bd768 100644 --- a/pkg/scheduler/algorithm/priorities/selector_spreading_test.go +++ b/pkg/scheduler/algorithm/priorities/selector_spreading_test.go @@ -186,8 +186,8 @@ func TestSelectorSpreadPriority(t *testing.T) { rcs: []*v1.ReplicationController{{Spec: v1.ReplicationControllerSpec{Selector: map[string]string{"foo": "bar"}}}}, services: []*v1.Service{{Spec: v1.ServiceSpec{Selector: map[string]string{"baz": "blah"}}}}, // "baz=blah" matches both labels1 and labels2, and "foo=bar" matches only labels 1. This means that we assume that we want to - // do spreading between all pods. The result should be exactly as above. - expectedList: []schedulerapi.HostPriority{{Host: "machine1", Score: 0}, {Host: "machine2", Score: 5}}, + // do spreading pod2 and pod3 and not pod1. + expectedList: []schedulerapi.HostPriority{{Host: "machine1", Score: 0}, {Host: "machine2", Score: 0}}, name: "service with partial pod label matches with service and replication controller", }, { @@ -201,7 +201,7 @@ func TestSelectorSpreadPriority(t *testing.T) { services: []*v1.Service{{Spec: v1.ServiceSpec{Selector: map[string]string{"baz": "blah"}}}}, rss: []*apps.ReplicaSet{{Spec: apps.ReplicaSetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}}}}, // We use ReplicaSet, instead of ReplicationController. The result should be exactly as above. - expectedList: []schedulerapi.HostPriority{{Host: "machine1", Score: 0}, {Host: "machine2", Score: 5}}, + expectedList: []schedulerapi.HostPriority{{Host: "machine1", Score: 0}, {Host: "machine2", Score: 0}}, name: "service with partial pod label matches with service and replica set", }, { @@ -214,8 +214,8 @@ func TestSelectorSpreadPriority(t *testing.T) { nodes: []string{"machine1", "machine2"}, services: []*v1.Service{{Spec: v1.ServiceSpec{Selector: map[string]string{"baz": "blah"}}}}, sss: []*apps.StatefulSet{{Spec: apps.StatefulSetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}}}}, - expectedList: []schedulerapi.HostPriority{{Host: "machine1", Score: 0}, {Host: "machine2", Score: 5}}, - name: "service with partial pod label matches with service and replica set", + expectedList: []schedulerapi.HostPriority{{Host: "machine1", Score: 0}, {Host: "machine2", Score: 0}}, + name: "service with partial pod label matches with service and stateful set", }, { pod: &v1.Pod{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"foo": "bar", "bar": "foo"}, OwnerReferences: controllerRef("ReplicationController", "name", "abc123")}}, @@ -227,9 +227,9 @@ func TestSelectorSpreadPriority(t *testing.T) { nodes: []string{"machine1", "machine2"}, rcs: []*v1.ReplicationController{{Spec: v1.ReplicationControllerSpec{Selector: map[string]string{"foo": "bar"}}}}, services: []*v1.Service{{Spec: v1.ServiceSpec{Selector: map[string]string{"bar": "foo"}}}}, - // Taken together Service and Replication Controller should match all Pods, hence result should be equal to one above. - expectedList: []schedulerapi.HostPriority{{Host: "machine1", Score: 0}, {Host: "machine2", Score: 5}}, - name: "disjoined service and replication controller should be treated equally", + // Taken together Service and Replication Controller should match no pods. + expectedList: []schedulerapi.HostPriority{{Host: "machine1", Score: 10}, {Host: "machine2", Score: 10}}, + name: "disjoined service and replication controller matches no pods", }, { pod: &v1.Pod{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"foo": "bar", "bar": "foo"}, OwnerReferences: controllerRef("ReplicaSet", "name", "abc123")}}, @@ -242,8 +242,8 @@ func TestSelectorSpreadPriority(t *testing.T) { services: []*v1.Service{{Spec: v1.ServiceSpec{Selector: map[string]string{"bar": "foo"}}}}, rss: []*apps.ReplicaSet{{Spec: apps.ReplicaSetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}}}}, // We use ReplicaSet, instead of ReplicationController. The result should be exactly as above. - expectedList: []schedulerapi.HostPriority{{Host: "machine1", Score: 0}, {Host: "machine2", Score: 5}}, - name: "disjoined service and replica set should be treated equally", + expectedList: []schedulerapi.HostPriority{{Host: "machine1", Score: 10}, {Host: "machine2", Score: 10}}, + name: "disjoined service and replica set matches no pods", }, { pod: &v1.Pod{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"foo": "bar", "bar": "foo"}, OwnerReferences: controllerRef("StatefulSet", "name", "abc123")}}, @@ -255,8 +255,8 @@ func TestSelectorSpreadPriority(t *testing.T) { nodes: []string{"machine1", "machine2"}, services: []*v1.Service{{Spec: v1.ServiceSpec{Selector: map[string]string{"bar": "foo"}}}}, sss: []*apps.StatefulSet{{Spec: apps.StatefulSetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}}}}, - expectedList: []schedulerapi.HostPriority{{Host: "machine1", Score: 0}, {Host: "machine2", Score: 5}}, - name: "disjoined service and replica set should be treated equally", + expectedList: []schedulerapi.HostPriority{{Host: "machine1", Score: 10}, {Host: "machine2", Score: 10}}, + name: "disjoined service and stateful set matches no pods", }, { pod: &v1.Pod{ObjectMeta: metav1.ObjectMeta{Labels: labels1, OwnerReferences: controllerRef("ReplicationController", "name", "abc123")}},