From 08dfa41f176502a0f220f6ca92ec95583f0b039a Mon Sep 17 00:00:00 2001 From: Alex Wang Date: Mon, 13 Mar 2023 19:24:50 +0800 Subject: [PATCH 1/3] fix: key in matchLabelKeys needs to be ignored when LabelSelector is nil Signed-off-by: Alex Wang --- .../plugins/podtopologyspread/common.go | 12 ++- .../podtopologyspread/filtering_test.go | 77 +++++++++++++++++++ 2 files changed, 85 insertions(+), 4 deletions(-) diff --git a/pkg/scheduler/framework/plugins/podtopologyspread/common.go b/pkg/scheduler/framework/plugins/podtopologyspread/common.go index 55f8d1e42b3..3cd929f9984 100644 --- a/pkg/scheduler/framework/plugins/podtopologyspread/common.go +++ b/pkg/scheduler/framework/plugins/podtopologyspread/common.go @@ -135,10 +135,14 @@ func (pl *PodTopologySpread) filterTopologySpreadConstraints(constraints []v1.To func mergeLabelSetWithSelector(matchLabels labels.Set, s labels.Selector) labels.Selector { mergedSelector := labels.SelectorFromSet(matchLabels) - if requirements, ok := s.Requirements(); ok { - for _, r := range requirements { - mergedSelector = mergedSelector.Add(r) - } + + requirements, ok := s.Requirements() + if !ok { + return s + } + + for _, r := range requirements { + mergedSelector = mergedSelector.Add(r) } return mergedSelector diff --git a/pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go b/pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go index c4ce832494d..801b538891d 100644 --- a/pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go +++ b/pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go @@ -1299,6 +1299,83 @@ func TestPreFilterState(t *testing.T) { }, enableMatchLabelKeys: true, }, + { + name: "key in matchLabelKeys is ignored when LabelSelector is nil when feature gate enabled", + pod: st.MakePod().Name("p").Label("foo", ""). + SpreadConstraint(1, "zone", v1.DoNotSchedule, nil, nil, nil, nil, []string{"bar"}). + Obj(), + nodes: []*v1.Node{ + st.MakeNode().Name("node-a").Label("zone", "zone1").Label("node", "node-a").Obj(), + st.MakeNode().Name("node-b").Label("zone", "zone1").Label("node", "node-b").Obj(), + st.MakeNode().Name("node-x").Label("zone", "zone2").Label("node", "node-x").Obj(), + st.MakeNode().Name("node-y").Label("zone", "zone2").Label("node", "node-y").Obj(), + }, + existingPods: []*v1.Pod{ + st.MakePod().Name("p-a1").Node("node-a").Label("foo", "a").Obj(), + st.MakePod().Name("p-a2").Node("node-a").Label("foo", "a").Obj(), + st.MakePod().Name("p-b1").Node("node-b").Label("foo", "a").Obj(), + st.MakePod().Name("p-y1").Node("node-y").Label("foo", "a").Obj(), + st.MakePod().Name("p-y2").Node("node-y").Label("foo", "a").Obj(), + }, + want: &preFilterState{ + Constraints: []topologySpreadConstraint{ + { + MaxSkew: 1, + TopologyKey: "zone", + Selector: mustConvertLabelSelectorAsSelector(t, nil), + MinDomains: 1, + NodeAffinityPolicy: v1.NodeInclusionPolicyHonor, + NodeTaintsPolicy: v1.NodeInclusionPolicyIgnore, + }, + }, + TpKeyToCriticalPaths: map[string]*criticalPaths{ + "zone": {{"zone2", 0}, {"zone1", 0}}, + }, + TpPairToMatchNum: map[topologyPair]int{ + {key: "zone", value: "zone1"}: 0, + {key: "zone", value: "zone2"}: 0, + }, + }, + enableMatchLabelKeys: true, + }, + { + name: "no pod is matched when LabelSelector is nil when feature gate disabled", + pod: st.MakePod().Name("p").Label("foo", ""). + SpreadConstraint(1, "zone", v1.DoNotSchedule, nil, nil, nil, nil, []string{"bar"}). + Obj(), + nodes: []*v1.Node{ + st.MakeNode().Name("node-a").Label("zone", "zone1").Label("node", "node-a").Obj(), + st.MakeNode().Name("node-b").Label("zone", "zone1").Label("node", "node-b").Obj(), + st.MakeNode().Name("node-x").Label("zone", "zone2").Label("node", "node-x").Obj(), + st.MakeNode().Name("node-y").Label("zone", "zone2").Label("node", "node-y").Obj(), + }, + existingPods: []*v1.Pod{ + st.MakePod().Name("p-a1").Node("node-a").Label("foo", "a").Obj(), + st.MakePod().Name("p-a2").Node("node-a").Label("foo", "a").Obj(), + st.MakePod().Name("p-b1").Node("node-b").Label("foo", "a").Obj(), + st.MakePod().Name("p-y1").Node("node-y").Label("foo", "a").Obj(), + st.MakePod().Name("p-y2").Node("node-y").Label("foo", "a").Obj(), + }, + want: &preFilterState{ + Constraints: []topologySpreadConstraint{ + { + MaxSkew: 1, + TopologyKey: "zone", + Selector: mustConvertLabelSelectorAsSelector(t, nil), + MinDomains: 1, + NodeAffinityPolicy: v1.NodeInclusionPolicyHonor, + NodeTaintsPolicy: v1.NodeInclusionPolicyIgnore, + }, + }, + TpKeyToCriticalPaths: map[string]*criticalPaths{ + "zone": {{"zone2", 0}, {"zone1", 0}}, + }, + TpPairToMatchNum: map[topologyPair]int{ + {key: "zone", value: "zone1"}: 0, + {key: "zone", value: "zone2"}: 0, + }, + }, + }, { name: "key in matchLabelKeys is ignored when it isn't exist in pod.labels", pod: st.MakePod().Name("p").Label("foo", ""). From 8a1f9f43e63401ab45e023ea6d258dd2a7a1a783 Mon Sep 17 00:00:00 2001 From: Alex Wang Date: Tue, 14 Mar 2023 11:29:12 +0800 Subject: [PATCH 2/3] feat: validate matchLabelKeys when labelSelector isn't set Signed-off-by: Alex Wang --- pkg/apis/core/validation/validation.go | 5 ++++- pkg/apis/core/validation/validation_test.go | 17 +++++++++++++++-- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 61d0fb3af42..4d910e9c5b9 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -6964,7 +6964,9 @@ func validateMatchLabelKeys(fldPath *field.Path, matchLabelKeys []string, labelS return nil } + var allErrs field.ErrorList labelSelectorKeys := sets.String{} + if labelSelector != nil { for key := range labelSelector.MatchLabels { labelSelectorKeys.Insert(key) @@ -6972,9 +6974,10 @@ func validateMatchLabelKeys(fldPath *field.Path, matchLabelKeys []string, labelS for _, matchExpression := range labelSelector.MatchExpressions { labelSelectorKeys.Insert(matchExpression.Key) } + } else { + allErrs = append(allErrs, field.Forbidden(fldPath, "must not be specified when labelSelector is not set")) } - allErrs := field.ErrorList{} for i, key := range matchLabelKeys { allErrs = append(allErrs, unversionedvalidation.ValidateLabelName(key, fldPath.Index(i))...) if labelSelectorKeys.Has(key) { diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index d20f0ffa532..793b067a2ad 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -22091,11 +22091,12 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { { MaxSkew: 1, TopologyKey: "k8s.io/zone", + LabelSelector: &metav1.LabelSelector{}, WhenUnsatisfiable: core.DoNotSchedule, MatchLabelKeys: []string{"/simple"}, }, }, - wantFieldErrors: []*field.Error{field.Invalid(fieldPathMatchLabelKeys.Index(0), "/simple", "prefix part must be non-empty")}, + wantFieldErrors: field.ErrorList{field.Invalid(fieldPathMatchLabelKeys.Index(0), "/simple", "prefix part must be non-empty")}, }, { name: "key exists in both matchLabelKeys and labelSelector", @@ -22116,7 +22117,19 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { }, }, }, - wantFieldErrors: []*field.Error{field.Invalid(fieldPathMatchLabelKeys.Index(0), "foo", "exists in both matchLabelKeys and labelSelector")}, + wantFieldErrors: field.ErrorList{field.Invalid(fieldPathMatchLabelKeys.Index(0), "foo", "exists in both matchLabelKeys and labelSelector")}, + }, + { + name: "key in MatchLabelKeys is forbidden to be specified when labelSelector is not set", + constraints: []core.TopologySpreadConstraint{ + { + MaxSkew: 1, + TopologyKey: "k8s.io/zone", + WhenUnsatisfiable: core.DoNotSchedule, + MatchLabelKeys: []string{"foo"}, + }, + }, + wantFieldErrors: field.ErrorList{field.Forbidden(fieldPathMatchLabelKeys, "must not be specified when labelSelector is not set")}, }, { name: "invalid matchLabels set on labelSelector when AllowInvalidTopologySpreadConstraintLabelSelector is false", From 199c37acefa092e2a46acf9827c36fcb2430036e Mon Sep 17 00:00:00 2001 From: Alex Wang Date: Tue, 14 Mar 2023 23:12:16 +0800 Subject: [PATCH 3/3] feat: update matchLabelKeys comment and code auto-generate Signed-off-by: Alex Wang --- api/openapi-spec/swagger.json | 2 +- api/openapi-spec/v3/api__v1_openapi.json | 2 +- api/openapi-spec/v3/apis__apps__v1_openapi.json | 2 +- api/openapi-spec/v3/apis__batch__v1_openapi.json | 2 +- pkg/apis/core/types.go | 6 +++++- pkg/generated/openapi/zz_generated.openapi.go | 2 +- staging/src/k8s.io/api/core/v1/generated.proto | 6 +++++- staging/src/k8s.io/api/core/v1/types.go | 6 +++++- .../src/k8s.io/api/core/v1/types_swagger_doc_generated.go | 2 +- 9 files changed, 21 insertions(+), 9 deletions(-) diff --git a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json index aea68010aab..c9f3d05879a 100644 --- a/api/openapi-spec/swagger.json +++ b/api/openapi-spec/swagger.json @@ -9746,7 +9746,7 @@ "description": "LabelSelector is used to find matching pods. Pods that match this label selector are counted to determine the number of pods in their corresponding topology domain." }, "matchLabelKeys": { - "description": "MatchLabelKeys is a set of pod label keys to select the pods over which spreading will be calculated. The keys are used to lookup values from the incoming pod labels, those key-value labels are ANDed with labelSelector to select the group of existing pods over which spreading will be calculated for the incoming pod. Keys that don't exist in the incoming pod labels will be ignored. A null or empty list means only match against labelSelector.", + "description": "MatchLabelKeys is a set of pod label keys to select the pods over which spreading will be calculated. The keys are used to lookup values from the incoming pod labels, those key-value labels are ANDed with labelSelector to select the group of existing pods over which spreading will be calculated for the incoming pod. The same key is forbidden to exist in both MatchLabelKeys and LabelSelector. MatchLabelKeys cannot be set when LabelSelector isn't set. Keys that don't exist in the incoming pod labels will be ignored. A null or empty list means only match against labelSelector.\n\nThis is a beta field and requires the MatchLabelKeysInPodTopologySpread feature gate to be enabled (enabled by default).", "items": { "type": "string" }, diff --git a/api/openapi-spec/v3/api__v1_openapi.json b/api/openapi-spec/v3/api__v1_openapi.json index 3ac5b22171a..68963f0da65 100644 --- a/api/openapi-spec/v3/api__v1_openapi.json +++ b/api/openapi-spec/v3/api__v1_openapi.json @@ -7444,7 +7444,7 @@ "description": "LabelSelector is used to find matching pods. Pods that match this label selector are counted to determine the number of pods in their corresponding topology domain." }, "matchLabelKeys": { - "description": "MatchLabelKeys is a set of pod label keys to select the pods over which spreading will be calculated. The keys are used to lookup values from the incoming pod labels, those key-value labels are ANDed with labelSelector to select the group of existing pods over which spreading will be calculated for the incoming pod. Keys that don't exist in the incoming pod labels will be ignored. A null or empty list means only match against labelSelector.", + "description": "MatchLabelKeys is a set of pod label keys to select the pods over which spreading will be calculated. The keys are used to lookup values from the incoming pod labels, those key-value labels are ANDed with labelSelector to select the group of existing pods over which spreading will be calculated for the incoming pod. The same key is forbidden to exist in both MatchLabelKeys and LabelSelector. MatchLabelKeys cannot be set when LabelSelector isn't set. Keys that don't exist in the incoming pod labels will be ignored. A null or empty list means only match against labelSelector.\n\nThis is a beta field and requires the MatchLabelKeysInPodTopologySpread feature gate to be enabled (enabled by default).", "items": { "default": "", "type": "string" diff --git a/api/openapi-spec/v3/apis__apps__v1_openapi.json b/api/openapi-spec/v3/apis__apps__v1_openapi.json index 967dbe46271..13daa09db15 100644 --- a/api/openapi-spec/v3/apis__apps__v1_openapi.json +++ b/api/openapi-spec/v3/apis__apps__v1_openapi.json @@ -4536,7 +4536,7 @@ "description": "LabelSelector is used to find matching pods. Pods that match this label selector are counted to determine the number of pods in their corresponding topology domain." }, "matchLabelKeys": { - "description": "MatchLabelKeys is a set of pod label keys to select the pods over which spreading will be calculated. The keys are used to lookup values from the incoming pod labels, those key-value labels are ANDed with labelSelector to select the group of existing pods over which spreading will be calculated for the incoming pod. Keys that don't exist in the incoming pod labels will be ignored. A null or empty list means only match against labelSelector.", + "description": "MatchLabelKeys is a set of pod label keys to select the pods over which spreading will be calculated. The keys are used to lookup values from the incoming pod labels, those key-value labels are ANDed with labelSelector to select the group of existing pods over which spreading will be calculated for the incoming pod. The same key is forbidden to exist in both MatchLabelKeys and LabelSelector. MatchLabelKeys cannot be set when LabelSelector isn't set. Keys that don't exist in the incoming pod labels will be ignored. A null or empty list means only match against labelSelector.\n\nThis is a beta field and requires the MatchLabelKeysInPodTopologySpread feature gate to be enabled (enabled by default).", "items": { "default": "", "type": "string" diff --git a/api/openapi-spec/v3/apis__batch__v1_openapi.json b/api/openapi-spec/v3/apis__batch__v1_openapi.json index 70e2916df57..325daf6a656 100644 --- a/api/openapi-spec/v3/apis__batch__v1_openapi.json +++ b/api/openapi-spec/v3/apis__batch__v1_openapi.json @@ -3710,7 +3710,7 @@ "description": "LabelSelector is used to find matching pods. Pods that match this label selector are counted to determine the number of pods in their corresponding topology domain." }, "matchLabelKeys": { - "description": "MatchLabelKeys is a set of pod label keys to select the pods over which spreading will be calculated. The keys are used to lookup values from the incoming pod labels, those key-value labels are ANDed with labelSelector to select the group of existing pods over which spreading will be calculated for the incoming pod. Keys that don't exist in the incoming pod labels will be ignored. A null or empty list means only match against labelSelector.", + "description": "MatchLabelKeys is a set of pod label keys to select the pods over which spreading will be calculated. The keys are used to lookup values from the incoming pod labels, those key-value labels are ANDed with labelSelector to select the group of existing pods over which spreading will be calculated for the incoming pod. The same key is forbidden to exist in both MatchLabelKeys and LabelSelector. MatchLabelKeys cannot be set when LabelSelector isn't set. Keys that don't exist in the incoming pod labels will be ignored. A null or empty list means only match against labelSelector.\n\nThis is a beta field and requires the MatchLabelKeysInPodTopologySpread feature gate to be enabled (enabled by default).", "items": { "default": "", "type": "string" diff --git a/pkg/apis/core/types.go b/pkg/apis/core/types.go index c3df021ded3..c782e4f5e13 100644 --- a/pkg/apis/core/types.go +++ b/pkg/apis/core/types.go @@ -5972,8 +5972,12 @@ type TopologySpreadConstraint struct { // spreading will be calculated. The keys are used to lookup values from the // incoming pod labels, those key-value labels are ANDed with labelSelector // to select the group of existing pods over which spreading will be calculated - // for the incoming pod. Keys that don't exist in the incoming pod labels will + // for the incoming pod. The same key is forbidden to exist in both MatchLabelKeys and LabelSelector. + // MatchLabelKeys cannot be set when LabelSelector isn't set. + // Keys that don't exist in the incoming pod labels will // be ignored. A null or empty list means only match against labelSelector. + // + // This is a beta field and requires the MatchLabelKeysInPodTopologySpread feature gate to be enabled (enabled by default). // +listType=atomic // +optional MatchLabelKeys []string diff --git a/pkg/generated/openapi/zz_generated.openapi.go b/pkg/generated/openapi/zz_generated.openapi.go index 3678e0ac83a..58a1c20e606 100644 --- a/pkg/generated/openapi/zz_generated.openapi.go +++ b/pkg/generated/openapi/zz_generated.openapi.go @@ -27161,7 +27161,7 @@ func schema_k8sio_api_core_v1_TopologySpreadConstraint(ref common.ReferenceCallb }, }, SchemaProps: spec.SchemaProps{ - Description: "MatchLabelKeys is a set of pod label keys to select the pods over which spreading will be calculated. The keys are used to lookup values from the incoming pod labels, those key-value labels are ANDed with labelSelector to select the group of existing pods over which spreading will be calculated for the incoming pod. Keys that don't exist in the incoming pod labels will be ignored. A null or empty list means only match against labelSelector.", + Description: "MatchLabelKeys is a set of pod label keys to select the pods over which spreading will be calculated. The keys are used to lookup values from the incoming pod labels, those key-value labels are ANDed with labelSelector to select the group of existing pods over which spreading will be calculated for the incoming pod. The same key is forbidden to exist in both MatchLabelKeys and LabelSelector. MatchLabelKeys cannot be set when LabelSelector isn't set. Keys that don't exist in the incoming pod labels will be ignored. A null or empty list means only match against labelSelector.\n\nThis is a beta field and requires the MatchLabelKeysInPodTopologySpread feature gate to be enabled (enabled by default).", Type: []string{"array"}, Items: &spec.SchemaOrArray{ Schema: &spec.Schema{ diff --git a/staging/src/k8s.io/api/core/v1/generated.proto b/staging/src/k8s.io/api/core/v1/generated.proto index 316f136b479..23345575c27 100644 --- a/staging/src/k8s.io/api/core/v1/generated.proto +++ b/staging/src/k8s.io/api/core/v1/generated.proto @@ -5697,8 +5697,12 @@ message TopologySpreadConstraint { // spreading will be calculated. The keys are used to lookup values from the // incoming pod labels, those key-value labels are ANDed with labelSelector // to select the group of existing pods over which spreading will be calculated - // for the incoming pod. Keys that don't exist in the incoming pod labels will + // for the incoming pod. The same key is forbidden to exist in both MatchLabelKeys and LabelSelector. + // MatchLabelKeys cannot be set when LabelSelector isn't set. + // Keys that don't exist in the incoming pod labels will // be ignored. A null or empty list means only match against labelSelector. + // + // This is a beta field and requires the MatchLabelKeysInPodTopologySpread feature gate to be enabled (enabled by default). // +listType=atomic // +optional repeated string matchLabelKeys = 8; diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go index b0f4116e43f..2e20466fc56 100644 --- a/staging/src/k8s.io/api/core/v1/types.go +++ b/staging/src/k8s.io/api/core/v1/types.go @@ -3696,8 +3696,12 @@ type TopologySpreadConstraint struct { // spreading will be calculated. The keys are used to lookup values from the // incoming pod labels, those key-value labels are ANDed with labelSelector // to select the group of existing pods over which spreading will be calculated - // for the incoming pod. Keys that don't exist in the incoming pod labels will + // for the incoming pod. The same key is forbidden to exist in both MatchLabelKeys and LabelSelector. + // MatchLabelKeys cannot be set when LabelSelector isn't set. + // Keys that don't exist in the incoming pod labels will // be ignored. A null or empty list means only match against labelSelector. + // + // This is a beta field and requires the MatchLabelKeysInPodTopologySpread feature gate to be enabled (enabled by default). // +listType=atomic // +optional MatchLabelKeys []string `json:"matchLabelKeys,omitempty" protobuf:"bytes,8,opt,name=matchLabelKeys"` diff --git a/staging/src/k8s.io/api/core/v1/types_swagger_doc_generated.go b/staging/src/k8s.io/api/core/v1/types_swagger_doc_generated.go index 89812f075c8..4a0d4b0f8b3 100644 --- a/staging/src/k8s.io/api/core/v1/types_swagger_doc_generated.go +++ b/staging/src/k8s.io/api/core/v1/types_swagger_doc_generated.go @@ -2465,7 +2465,7 @@ var map_TopologySpreadConstraint = map[string]string{ "minDomains": "MinDomains indicates a minimum number of eligible domains. When the number of eligible domains with matching topology keys is less than minDomains, Pod Topology Spread treats \"global minimum\" as 0, and then the calculation of Skew is performed. And when the number of eligible domains with matching topology keys equals or greater than minDomains, this value has no effect on scheduling. As a result, when the number of eligible domains is less than minDomains, scheduler won't schedule more than maxSkew Pods to those domains. If value is nil, the constraint behaves as if MinDomains is equal to 1. Valid values are integers greater than 0. When value is not nil, WhenUnsatisfiable must be DoNotSchedule.\n\nFor example, in a 3-zone cluster, MaxSkew is set to 2, MinDomains is set to 5 and pods with the same labelSelector spread as 2/2/2: ", "nodeAffinityPolicy": "NodeAffinityPolicy indicates how we will treat Pod's nodeAffinity/nodeSelector when calculating pod topology spread skew. Options are: - Honor: only nodes matching nodeAffinity/nodeSelector are included in the calculations. - Ignore: nodeAffinity/nodeSelector are ignored. All nodes are included in the calculations.\n\nIf this value is nil, the behavior is equivalent to the Honor policy. This is a beta-level feature default enabled by the NodeInclusionPolicyInPodTopologySpread feature flag.", "nodeTaintsPolicy": "NodeTaintsPolicy indicates how we will treat node taints when calculating pod topology spread skew. Options are: - Honor: nodes without taints, along with tainted nodes for which the incoming pod has a toleration, are included. - Ignore: node taints are ignored. All nodes are included.\n\nIf this value is nil, the behavior is equivalent to the Ignore policy. This is a beta-level feature default enabled by the NodeInclusionPolicyInPodTopologySpread feature flag.", - "matchLabelKeys": "MatchLabelKeys is a set of pod label keys to select the pods over which spreading will be calculated. The keys are used to lookup values from the incoming pod labels, those key-value labels are ANDed with labelSelector to select the group of existing pods over which spreading will be calculated for the incoming pod. Keys that don't exist in the incoming pod labels will be ignored. A null or empty list means only match against labelSelector.", + "matchLabelKeys": "MatchLabelKeys is a set of pod label keys to select the pods over which spreading will be calculated. The keys are used to lookup values from the incoming pod labels, those key-value labels are ANDed with labelSelector to select the group of existing pods over which spreading will be calculated for the incoming pod. The same key is forbidden to exist in both MatchLabelKeys and LabelSelector. MatchLabelKeys cannot be set when LabelSelector isn't set. Keys that don't exist in the incoming pod labels will be ignored. A null or empty list means only match against labelSelector.\n\nThis is a beta field and requires the MatchLabelKeysInPodTopologySpread feature gate to be enabled (enabled by default).", } func (TopologySpreadConstraint) SwaggerDoc() map[string]string {