Merge pull request #34543 from ivan4th/dont-require-failure-domains-for-pod-affinity-checker

Automatic merge from submit-queue

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`.

plugin/pkg/scheduler/algorithm/predicates/BUILD

@@ -43,7 +43,6 @@ go_test(
     deps = [
         "//pkg/api/v1: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/apimachinery/pkg/api/resource",
         "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1",

plugin/pkg/scheduler/algorithm/predicates/predicates.go

@@ -17,6 +17,7 @@ limitations under the License.
 package predicates
 
 import (
+	"errors"
 	"fmt"
 	"math/rand"
 	"strconv"
@@ -876,14 +877,12 @@ func GeneralPredicates(pod *v1.Pod, meta interface{}, nodeInfo *schedulercache.N
 type PodAffinityChecker struct {
 	info      NodeInfo
 	podLister algorithm.PodLister
-	failureDomains priorityutil.Topologies
 }
 
-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},
 	}
 	return checker.InterPodAffinityMatches
 }
@@ -915,11 +914,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
 	namespaces := priorityutil.GetNamespacesFromPodAffinityTerm(pod, term)
 	selector, err := metav1.LabelSelectorAsSelector(term.LabelSelector)
@@ -934,7 +936,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
 			}
 		}
@@ -1067,7 +1069,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

plugin/pkg/scheduler/algorithm/predicates/predicates_test.go

@@ -19,14 +19,12 @@ package predicates
 import (
 	"fmt"
 	"reflect"
-	"strings"
 	"testing"
 
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/kubernetes/pkg/api/v1"
 	"k8s.io/kubernetes/plugin/pkg/scheduler/algorithm"
-	priorityutil "k8s.io/kubernetes/plugin/pkg/scheduler/algorithm/priorities/util"
 	"k8s.io/kubernetes/plugin/pkg/scheduler/schedulercache"
 )
 
@@ -2574,7 +2572,6 @@ func TestInterPodAffinity(t *testing.T) {
 		fit := PodAffinityChecker{
 			info:      FakeNodeInfo(*node),
 			podLister: algorithm.FakePodLister(test.pods),
-			failureDomains: priorityutil.Topologies{DefaultKeys: strings.Split(v1.DefaultFailureDomains, ",")},
 		}
 		nodeInfo := schedulercache.NewNodeInfo(podsOnNode...)
 		nodeInfo.SetNode(test.node)
@@ -2904,7 +2901,6 @@ func TestInterPodAffinityWithMultipleNodes(t *testing.T) {
 			testFit := PodAffinityChecker{
 				info:      nodeListInfo,
 				podLister: algorithm.FakePodLister(test.pods),
-				failureDomains: priorityutil.Topologies{DefaultKeys: strings.Split(v1.DefaultFailureDomains, ",")},
 			}
 			nodeInfo := schedulercache.NewNodeInfo(podsOnNode...)
 			nodeInfo.SetNode(&node)

plugin/pkg/scheduler/algorithm/priorities/util/topologies.go

@@ -49,8 +49,12 @@ func PodMatchesTermsNamespaceAndSelector(pod *v1.Pod, namespaces sets.String, se
 	return true
 }
 
-// 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]
 }
 
@@ -64,12 +68,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)
 	}
 }

plugin/pkg/scheduler/algorithmprovider/defaults/defaults.go

@@ -140,7 +140,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)
 			},
 		),