From d40a8f3279feb1d3aeec731225fd33f15cad68fc Mon Sep 17 00:00:00 2001 From: Ivan Shvedunov Date: Tue, 11 Oct 2016 16:31:47 +0300 Subject: [PATCH] Don't require failureDomains in PodAffinityChecker failureDomains are only used for PreferredDuringScheduling pod anti-affinity, which is ignored by PodAffinityChecker. This unnecessary requirement was making it hard to move PodAffinityChecker to GeneralPredicates because that would require passing --failure-domains to both kubelet and kube-controller-manager. --- .../pkg/scheduler/algorithm/predicates/BUILD | 1 - .../algorithm/predicates/predicates.go | 26 ++++++++++++------- .../algorithm/predicates/predicates_test.go | 12 +++------ .../algorithm/priorities/util/topologies.go | 12 ++++++--- .../algorithmprovider/defaults/defaults.go | 2 +- 5 files changed, 29 insertions(+), 24 deletions(-) diff --git a/plugin/pkg/scheduler/algorithm/predicates/BUILD b/plugin/pkg/scheduler/algorithm/predicates/BUILD index b965e853210..0393af69079 100644 --- a/plugin/pkg/scheduler/algorithm/predicates/BUILD +++ b/plugin/pkg/scheduler/algorithm/predicates/BUILD @@ -51,7 +51,6 @@ go_test( "//pkg/labels:go_default_library", "//pkg/util/codeinspector:go_default_library", "//plugin/pkg/scheduler/algorithm:go_default_library", - "//plugin/pkg/scheduler/algorithm/priorities/util:go_default_library", "//plugin/pkg/scheduler/schedulercache:go_default_library", "//vendor:k8s.io/gengo/parser", "//vendor:k8s.io/gengo/types", diff --git a/plugin/pkg/scheduler/algorithm/predicates/predicates.go b/plugin/pkg/scheduler/algorithm/predicates/predicates.go index b143053fc3f..cf2f86aafcb 100644 --- a/plugin/pkg/scheduler/algorithm/predicates/predicates.go +++ b/plugin/pkg/scheduler/algorithm/predicates/predicates.go @@ -17,6 +17,7 @@ limitations under the License. package predicates import ( + "errors" "fmt" "math/rand" "strconv" @@ -859,16 +860,14 @@ func GeneralPredicates(pod *v1.Pod, meta interface{}, nodeInfo *schedulercache.N } type PodAffinityChecker struct { - info NodeInfo - podLister algorithm.PodLister - failureDomains priorityutil.Topologies + info NodeInfo + podLister algorithm.PodLister } -func NewPodAffinityPredicate(info NodeInfo, podLister algorithm.PodLister, failureDomains []string) algorithm.FitPredicate { +func NewPodAffinityPredicate(info NodeInfo, podLister algorithm.PodLister) algorithm.FitPredicate { checker := &PodAffinityChecker{ - info: info, - podLister: podLister, - failureDomains: priorityutil.Topologies{DefaultKeys: failureDomains}, + info: info, + podLister: podLister, } return checker.InterPodAffinityMatches } @@ -903,11 +902,14 @@ func (c *PodAffinityChecker) InterPodAffinityMatches(pod *v1.Pod, meta interface return true, nil, nil } -// AnyPodMatchesPodAffinityTerm checks if any of given pods can match the specific podAffinityTerm. +// anyPodMatchesPodAffinityTerm checks if any of given pods can match the specific podAffinityTerm. // First return value indicates whether a matching pod exists on a node that matches the topology key, // while the second return value indicates whether a matching pod exists anywhere. // TODO: Do we really need any pod matching, or all pods matching? I think the latter. func (c *PodAffinityChecker) anyPodMatchesPodAffinityTerm(pod *v1.Pod, allPods []*v1.Pod, node *v1.Node, term *v1.PodAffinityTerm) (bool, bool, error) { + if len(term.TopologyKey) == 0 { + return false, false, errors.New("Empty topologyKey is not allowed except for PreferredDuringScheduling pod anti-affinity") + } matchingPodExists := false for _, existingPod := range allPods { match, err := priorityutil.PodMatchesTermsNamespaceAndSelector(existingPod, pod, term) @@ -920,7 +922,7 @@ func (c *PodAffinityChecker) anyPodMatchesPodAffinityTerm(pod *v1.Pod, allPods [ if err != nil { return false, matchingPodExists, err } - if c.failureDomains.NodesHaveSameTopologyKey(node, existingPodNode, term.TopologyKey) { + if priorityutil.NodesHaveSameTopologyKey(node, existingPodNode, term.TopologyKey) { return true, matchingPodExists, nil } } @@ -1056,7 +1058,11 @@ func (c *PodAffinityChecker) satisfiesExistingPodsAntiAffinity(pod *v1.Pod, meta } } for _, term := range matchingTerms { - if c.failureDomains.NodesHaveSameTopologyKey(node, term.node, term.term.TopologyKey) { + if len(term.term.TopologyKey) == 0 { + glog.V(10).Infof("Empty topologyKey is not allowed except for PreferredDuringScheduling pod anti-affinity") + return false + } + if priorityutil.NodesHaveSameTopologyKey(node, term.node, term.term.TopologyKey) { glog.V(10).Infof("Cannot schedule pod %+v onto node %v,because of PodAntiAffinityTerm %v", podName(pod), node.Name, term.term) return false diff --git a/plugin/pkg/scheduler/algorithm/predicates/predicates_test.go b/plugin/pkg/scheduler/algorithm/predicates/predicates_test.go index 9932d6cd437..f1705cd207b 100644 --- a/plugin/pkg/scheduler/algorithm/predicates/predicates_test.go +++ b/plugin/pkg/scheduler/algorithm/predicates/predicates_test.go @@ -21,7 +21,6 @@ import ( "os/exec" "path/filepath" "reflect" - "strings" "testing" "k8s.io/gengo/parser" @@ -30,7 +29,6 @@ import ( "k8s.io/kubernetes/pkg/api/v1" "k8s.io/kubernetes/pkg/util/codeinspector" "k8s.io/kubernetes/plugin/pkg/scheduler/algorithm" - priorityutil "k8s.io/kubernetes/plugin/pkg/scheduler/algorithm/priorities/util" "k8s.io/kubernetes/plugin/pkg/scheduler/schedulercache" ) @@ -2463,9 +2461,8 @@ func TestInterPodAffinity(t *testing.T) { } fit := PodAffinityChecker{ - info: FakeNodeInfo(*node), - podLister: algorithm.FakePodLister(test.pods), - failureDomains: priorityutil.Topologies{DefaultKeys: strings.Split(v1.DefaultFailureDomains, ",")}, + info: FakeNodeInfo(*node), + podLister: algorithm.FakePodLister(test.pods), } nodeInfo := schedulercache.NewNodeInfo(podsOnNode...) nodeInfo.SetNode(test.node) @@ -2708,9 +2705,8 @@ func TestInterPodAffinityWithMultipleNodes(t *testing.T) { } testFit := PodAffinityChecker{ - info: nodeListInfo, - podLister: algorithm.FakePodLister(test.pods), - failureDomains: priorityutil.Topologies{DefaultKeys: strings.Split(v1.DefaultFailureDomains, ",")}, + info: nodeListInfo, + podLister: algorithm.FakePodLister(test.pods), } nodeInfo := schedulercache.NewNodeInfo(podsOnNode...) nodeInfo.SetNode(&node) diff --git a/plugin/pkg/scheduler/algorithm/priorities/util/topologies.go b/plugin/pkg/scheduler/algorithm/priorities/util/topologies.go index 171daf3e20d..0f8134ee4d2 100644 --- a/plugin/pkg/scheduler/algorithm/priorities/util/topologies.go +++ b/plugin/pkg/scheduler/algorithm/priorities/util/topologies.go @@ -52,8 +52,12 @@ func PodMatchesTermsNamespaceAndSelector(pod *v1.Pod, affinityPod *v1.Pod, term return true, nil } -// nodesHaveSameTopologyKeyInternal checks if nodeA and nodeB have same label value with given topologyKey as label key. -func nodesHaveSameTopologyKeyInternal(nodeA, nodeB *v1.Node, topologyKey string) bool { +// NodesHaveSameTopologyKey checks if nodeA and nodeB have same label value with given topologyKey as label key. +// Returns false if topologyKey is empty. +func NodesHaveSameTopologyKey(nodeA, nodeB *v1.Node, topologyKey string) bool { + if len(topologyKey) == 0 { + return false + } return nodeA.Labels != nil && nodeB.Labels != nil && len(nodeA.Labels[topologyKey]) > 0 && nodeA.Labels[topologyKey] == nodeB.Labels[topologyKey] } @@ -67,12 +71,12 @@ func (tps *Topologies) NodesHaveSameTopologyKey(nodeA, nodeB *v1.Node, topologyK if len(topologyKey) == 0 { // assumes this is allowed only for PreferredDuringScheduling pod anti-affinity (ensured by api/validation) for _, defaultKey := range tps.DefaultKeys { - if nodesHaveSameTopologyKeyInternal(nodeA, nodeB, defaultKey) { + if NodesHaveSameTopologyKey(nodeA, nodeB, defaultKey) { return true } } return false } else { - return nodesHaveSameTopologyKeyInternal(nodeA, nodeB, topologyKey) + return NodesHaveSameTopologyKey(nodeA, nodeB, topologyKey) } } diff --git a/plugin/pkg/scheduler/algorithmprovider/defaults/defaults.go b/plugin/pkg/scheduler/algorithmprovider/defaults/defaults.go index f7ac0cf4021..bcd8d099a84 100644 --- a/plugin/pkg/scheduler/algorithmprovider/defaults/defaults.go +++ b/plugin/pkg/scheduler/algorithmprovider/defaults/defaults.go @@ -139,7 +139,7 @@ func defaultPredicates() sets.String { factory.RegisterFitPredicateFactory( "MatchInterPodAffinity", func(args factory.PluginFactoryArgs) algorithm.FitPredicate { - return predicates.NewPodAffinityPredicate(args.NodeInfo, args.PodLister, args.FailureDomains) + return predicates.NewPodAffinityPredicate(args.NodeInfo, args.PodLister) }, ),