From 0dad8dd459b2b7ababde08109ed1fb0e224ac41a Mon Sep 17 00:00:00 2001 From: Avesh Agarwal Date: Tue, 1 Aug 2017 15:03:51 -0400 Subject: [PATCH] Do not allow empty topology key for pod affinities. --- pkg/api/types.go | 4 +- pkg/api/validation/validation.go | 31 ++++++-------- pkg/api/validation/validation_test.go | 42 ++++++++++++++++--- .../algorithm/priorities/util/topologies.go | 1 - 4 files changed, 49 insertions(+), 29 deletions(-) diff --git a/pkg/api/types.go b/pkg/api/types.go index ab9b7056d4f..2e8a1505bcb 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -2026,9 +2026,7 @@ type PodAffinityTerm struct { // the labelSelector in the specified namespaces, where co-located is defined as running on a node // whose value of the label with key topologyKey matches that of any node on which any of the // selected pods is running. - // For PreferredDuringScheduling pod anti-affinity, empty topologyKey is interpreted as "all topologies" - // ("all topologies" here means all the topologyKeys indicated by scheduler command-line argument --failure-domains); - // for affinity and for RequiredDuringScheduling pod anti-affinity, empty topologyKey is not allowed. + // Empty topologyKey is not allowed. // +optional TopologyKey string } diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index f6bd510bfdd..a3b476312fd 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -2429,7 +2429,7 @@ func ValidatePreferredSchedulingTerms(terms []api.PreferredSchedulingTerm, fldPa } // validatePodAffinityTerm tests that the specified podAffinityTerm fields have valid data -func validatePodAffinityTerm(podAffinityTerm api.PodAffinityTerm, allowEmptyTopologyKey bool, fldPath *field.Path) field.ErrorList { +func validatePodAffinityTerm(podAffinityTerm api.PodAffinityTerm, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(podAffinityTerm.LabelSelector, fldPath.Child("matchExpressions"))...) @@ -2438,32 +2438,29 @@ func validatePodAffinityTerm(podAffinityTerm api.PodAffinityTerm, allowEmptyTopo allErrs = append(allErrs, field.Invalid(fldPath.Child("namespace"), name, msg)) } } - if !allowEmptyTopologyKey && len(podAffinityTerm.TopologyKey) == 0 { - allErrs = append(allErrs, field.Required(fldPath.Child("topologyKey"), "can only be empty for PreferredDuringScheduling pod anti affinity")) + if len(podAffinityTerm.TopologyKey) == 0 { + allErrs = append(allErrs, field.Required(fldPath.Child("topologyKey"), "can not be empty")) } - if len(podAffinityTerm.TopologyKey) != 0 { - allErrs = append(allErrs, unversionedvalidation.ValidateLabelName(podAffinityTerm.TopologyKey, fldPath.Child("topologyKey"))...) - } - return allErrs + return append(allErrs, unversionedvalidation.ValidateLabelName(podAffinityTerm.TopologyKey, fldPath.Child("topologyKey"))...) } // validatePodAffinityTerms tests that the specified podAffinityTerms fields have valid data -func validatePodAffinityTerms(podAffinityTerms []api.PodAffinityTerm, allowEmptyTopologyKey bool, fldPath *field.Path) field.ErrorList { +func validatePodAffinityTerms(podAffinityTerms []api.PodAffinityTerm, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} for i, podAffinityTerm := range podAffinityTerms { - allErrs = append(allErrs, validatePodAffinityTerm(podAffinityTerm, allowEmptyTopologyKey, fldPath.Index(i))...) + allErrs = append(allErrs, validatePodAffinityTerm(podAffinityTerm, fldPath.Index(i))...) } return allErrs } // validateWeightedPodAffinityTerms tests that the specified weightedPodAffinityTerms fields have valid data -func validateWeightedPodAffinityTerms(weightedPodAffinityTerms []api.WeightedPodAffinityTerm, allowEmptyTopologyKey bool, fldPath *field.Path) field.ErrorList { +func validateWeightedPodAffinityTerms(weightedPodAffinityTerms []api.WeightedPodAffinityTerm, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} for j, weightedTerm := range weightedPodAffinityTerms { if weightedTerm.Weight <= 0 || weightedTerm.Weight > 100 { allErrs = append(allErrs, field.Invalid(fldPath.Index(j).Child("weight"), weightedTerm.Weight, "must be in the range 1-100")) } - allErrs = append(allErrs, validatePodAffinityTerm(weightedTerm.PodAffinityTerm, allowEmptyTopologyKey, fldPath.Index(j).Child("podAffinityTerm"))...) + allErrs = append(allErrs, validatePodAffinityTerm(weightedTerm.PodAffinityTerm, fldPath.Index(j).Child("podAffinityTerm"))...) } return allErrs } @@ -2477,13 +2474,11 @@ func validatePodAntiAffinity(podAntiAffinity *api.PodAntiAffinity, fldPath *fiel // fldPath.Child("requiredDuringSchedulingRequiredDuringExecution"))...) //} if podAntiAffinity.RequiredDuringSchedulingIgnoredDuringExecution != nil { - // empty topologyKey is not allowed for hard pod anti-affinity - allErrs = append(allErrs, validatePodAffinityTerms(podAntiAffinity.RequiredDuringSchedulingIgnoredDuringExecution, false, + allErrs = append(allErrs, validatePodAffinityTerms(podAntiAffinity.RequiredDuringSchedulingIgnoredDuringExecution, fldPath.Child("requiredDuringSchedulingIgnoredDuringExecution"))...) } if podAntiAffinity.PreferredDuringSchedulingIgnoredDuringExecution != nil { - // empty topologyKey is allowed for soft pod anti-affinity - allErrs = append(allErrs, validateWeightedPodAffinityTerms(podAntiAffinity.PreferredDuringSchedulingIgnoredDuringExecution, true, + allErrs = append(allErrs, validateWeightedPodAffinityTerms(podAntiAffinity.PreferredDuringSchedulingIgnoredDuringExecution, fldPath.Child("preferredDuringSchedulingIgnoredDuringExecution"))...) } return allErrs @@ -2498,13 +2493,11 @@ func validatePodAffinity(podAffinity *api.PodAffinity, fldPath *field.Path) fiel // fldPath.Child("requiredDuringSchedulingRequiredDuringExecution"))...) //} if podAffinity.RequiredDuringSchedulingIgnoredDuringExecution != nil { - // empty topologyKey is not allowed for hard pod affinity - allErrs = append(allErrs, validatePodAffinityTerms(podAffinity.RequiredDuringSchedulingIgnoredDuringExecution, false, + allErrs = append(allErrs, validatePodAffinityTerms(podAffinity.RequiredDuringSchedulingIgnoredDuringExecution, fldPath.Child("requiredDuringSchedulingIgnoredDuringExecution"))...) } if podAffinity.PreferredDuringSchedulingIgnoredDuringExecution != nil { - // empty topologyKey is not allowed for soft pod affinity - allErrs = append(allErrs, validateWeightedPodAffinityTerms(podAffinity.PreferredDuringSchedulingIgnoredDuringExecution, false, + allErrs = append(allErrs, validateWeightedPodAffinityTerms(podAffinity.PreferredDuringSchedulingIgnoredDuringExecution, fldPath.Child("preferredDuringSchedulingIgnoredDuringExecution"))...) } return allErrs diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 6f0940d03e1..542bcda1aec 100644 --- a/pkg/api/validation/validation_test.go +++ b/pkg/api/validation/validation_test.go @@ -4655,8 +4655,8 @@ func TestValidatePod(t *testing.T) { }), }, }, - "invalid pod affinity, empty topologyKey is not allowed for hard pod affinity": { - expectedError: "can only be empty for PreferredDuringScheduling pod anti affinity", + "invalid hard pod affinity, empty topologyKey is not allowed for hard pod affinity": { + expectedError: "can not be empty", spec: api.Pod{ ObjectMeta: metav1.ObjectMeta{ Name: "123", @@ -4682,8 +4682,8 @@ func TestValidatePod(t *testing.T) { }), }, }, - "invalid pod anti-affinity, empty topologyKey is not allowed for hard pod anti-affinity": { - expectedError: "can only be empty for PreferredDuringScheduling pod anti affinity", + "invalid hard pod anti-affinity, empty topologyKey is not allowed for hard pod anti-affinity": { + expectedError: "can not be empty", spec: api.Pod{ ObjectMeta: metav1.ObjectMeta{ Name: "123", @@ -4709,8 +4709,8 @@ func TestValidatePod(t *testing.T) { }), }, }, - "invalid pod anti-affinity, empty topologyKey is not allowed for soft pod affinity": { - expectedError: "can only be empty for PreferredDuringScheduling pod anti affinity", + "invalid soft pod affinity, empty topologyKey is not allowed for soft pod affinity": { + expectedError: "can not be empty", spec: api.Pod{ ObjectMeta: metav1.ObjectMeta{ Name: "123", @@ -4739,6 +4739,36 @@ func TestValidatePod(t *testing.T) { }), }, }, + "invalid soft pod anti-affinity, empty topologyKey is not allowed for soft pod anti-affinity": { + expectedError: "can not be empty", + spec: api.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "123", + Namespace: "ns", + }, + Spec: validPodSpec(&api.Affinity{ + PodAntiAffinity: &api.PodAntiAffinity{ + PreferredDuringSchedulingIgnoredDuringExecution: []api.WeightedPodAffinityTerm{ + { + Weight: 10, + PodAffinityTerm: api.PodAffinityTerm{ + LabelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "key2", + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{"value1", "value2"}, + }, + }, + }, + Namespaces: []string{"ns"}, + }, + }, + }, + }, + }), + }, + }, "invalid toleration key": { expectedError: "spec.tolerations[0].key", spec: api.Pod{ diff --git a/plugin/pkg/scheduler/algorithm/priorities/util/topologies.go b/plugin/pkg/scheduler/algorithm/priorities/util/topologies.go index 2764d9a0acb..511ffc13706 100644 --- a/plugin/pkg/scheduler/algorithm/priorities/util/topologies.go +++ b/plugin/pkg/scheduler/algorithm/priorities/util/topologies.go @@ -75,7 +75,6 @@ type Topologies struct { } // NodesHaveSameTopologyKey checks if nodeA and nodeB have same label value with given topologyKey as label key. -// If the topologyKey is empty, check if the two nodes have any of the default topologyKeys, and have same corresponding label value. func (tps *Topologies) NodesHaveSameTopologyKey(nodeA, nodeB *v1.Node, topologyKey string) bool { return NodesHaveSameTopologyKey(nodeA, nodeB, topologyKey) }