From e6c2bf85168be7b9b2c7127abea7621432c42692 Mon Sep 17 00:00:00 2001 From: Alex Wang Date: Sat, 30 Jul 2022 13:21:16 +0800 Subject: [PATCH] api defination for MatchLabelKeys in TopologySpreadConstraint Signed-off-by: Alex Wang --- pkg/api/pod/util.go | 26 ++++ pkg/api/pod/util_test.go | 135 ++++++++++++++++++++ pkg/apis/core/types.go | 9 ++ pkg/apis/core/validation/validation.go | 28 ++++ pkg/apis/core/validation/validation_test.go | 34 +++++ staging/src/k8s.io/api/core/v1/types.go | 9 ++ 6 files changed, 241 insertions(+) diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 3f485aad1ee..66dd9063f94 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -553,6 +553,7 @@ func dropDisabledFields( dropDisabledTopologySpreadConstraintsFields(podSpec, oldPodSpec) dropDisabledNodeInclusionPolicyFields(podSpec, oldPodSpec) + dropDisabledMatchLabelKeysField(podSpec, oldPodSpec) } // dropDisabledTopologySpreadConstraintsFields removes disabled fields from PodSpec related @@ -626,6 +627,31 @@ func dropDisabledNodeInclusionPolicyFields(podSpec, oldPodSpec *api.PodSpec) { } } +// dropDisabledMatchLabelKeysField removes disabled fields from PodSpec related +// to MatchLabelKeys only if it is not already used by the old spec. +func dropDisabledMatchLabelKeysField(podSpec, oldPodSpec *api.PodSpec) { + if !utilfeature.DefaultFeatureGate.Enabled(features.MatchLabelKeysInPodTopologySpread) && !matchLabelKeysInUse(oldPodSpec) { + for i := range podSpec.TopologySpreadConstraints { + podSpec.TopologySpreadConstraints[i].MatchLabelKeys = nil + } + } +} + +// matchLabelKeysInUse returns true if the pod spec is non-nil +// and has MatchLabelKeys field set in TopologySpreadConstraints. +func matchLabelKeysInUse(podSpec *api.PodSpec) bool { + if podSpec == nil { + return false + } + + for _, c := range podSpec.TopologySpreadConstraints { + if len(c.MatchLabelKeys) > 0 { + return true + } + } + return false +} + // nodeAffinityPolicyInUse returns true if the pod spec is non-nil and has NodeAffinityPolicy field set // in TopologySpreadConstraints func nodeAffinityPolicyInUse(podSpec *api.PodSpec) bool { diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index 4e95e70a0e5..ddeff7c5196 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -1930,3 +1930,138 @@ func TestDropNodeInclusionPolicyFields(t *testing.T) { }) } } + +func TestDropDisabledMatchLabelKeysField(t *testing.T) { + tests := []struct { + name string + enabled bool + podSpec *api.PodSpec + oldPodSpec *api.PodSpec + wantPodSpec *api.PodSpec + }{ + { + name: "feature disabled, both pods don't use MatchLabelKeys fields", + enabled: false, + oldPodSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{}, + }, + podSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{}, + }, + wantPodSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{}, + }, + }, + { + name: "feature disabled, only old pod uses MatchLabelKeys field", + enabled: false, + oldPodSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{ + {MatchLabelKeys: []string{"foo"}}, + }, + }, + podSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{}, + }, + wantPodSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{}, + }, + }, + { + name: "feature disabled, only current pod uses MatchLabelKeys field", + enabled: false, + oldPodSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{}, + }, + podSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{ + {MatchLabelKeys: []string{"foo"}}, + }, + }, + wantPodSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{{}}, + }, + }, + { + name: "feature disabled, both pods use MatchLabelKeys fields", + enabled: false, + oldPodSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{ + {MatchLabelKeys: []string{"foo"}}, + }, + }, + podSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{ + {MatchLabelKeys: []string{"foo"}}, + }, + }, + wantPodSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{ + {MatchLabelKeys: []string{"foo"}}, + }, + }, + }, + { + name: "feature enabled, only old pod uses MatchLabelKeys field", + enabled: true, + oldPodSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{ + {MatchLabelKeys: []string{"foo"}}, + }, + }, + podSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{}, + }, + wantPodSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{}, + }, + }, + { + name: "feature enabled, only current pod uses MatchLabelKeys field", + enabled: true, + oldPodSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{}, + }, + podSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{ + {MatchLabelKeys: []string{"foo"}}, + }, + }, + wantPodSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{ + {MatchLabelKeys: []string{"foo"}}, + }, + }, + }, + { + name: "feature enabled, both pods use MatchLabelKeys fields", + enabled: false, + oldPodSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{ + {MatchLabelKeys: []string{"foo"}}, + }, + }, + podSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{ + {MatchLabelKeys: []string{"foo"}}, + }, + }, + wantPodSpec: &api.PodSpec{ + TopologySpreadConstraints: []api.TopologySpreadConstraint{ + {MatchLabelKeys: []string{"foo"}}, + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.MatchLabelKeysInPodTopologySpread, test.enabled)() + + dropDisabledFields(test.podSpec, nil, test.oldPodSpec, nil) + if diff := cmp.Diff(test.wantPodSpec, test.podSpec); diff != "" { + t.Errorf("unexpected pod spec (-want, +got):\n%s", diff) + } + }) + } +} diff --git a/pkg/apis/core/types.go b/pkg/apis/core/types.go index 3e33e319013..b3537c16eec 100644 --- a/pkg/apis/core/types.go +++ b/pkg/apis/core/types.go @@ -5732,6 +5732,15 @@ type TopologySpreadConstraint struct { // This is a alpha-level feature enabled by the NodeInclusionPolicyInPodTopologySpread feature flag. // +optional NodeTaintsPolicy *NodeInclusionPolicy + // 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. + // +listType=atomic + // +optional + MatchLabelKeys []string } // These are the built-in errors for PortStatus. diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index b0ad047073f..de90250bbb2 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -6490,6 +6490,7 @@ func validateTopologySpreadConstraints(constraints []core.TopologySpreadConstrai if err := validateNodeInclusionPolicy(subFldPath.Child("nodeTaintsPolicy"), constraint.NodeTaintsPolicy); err != nil { allErrs = append(allErrs, err) } + allErrs = append(allErrs, validateMatchLabelKeys(subFldPath.Child("matchLabelKeys"), constraint.MatchLabelKeys, constraint.LabelSelector)...) } return allErrs @@ -6563,6 +6564,33 @@ func validateNodeInclusionPolicy(fldPath *field.Path, policy *core.NodeInclusion return nil } +// validateMatchLabelKeys tests that the elements are a valid label name and are not already included in labelSelector. +func validateMatchLabelKeys(fldPath *field.Path, matchLabelKeys []string, labelSelector *metav1.LabelSelector) field.ErrorList { + if len(matchLabelKeys) == 0 { + return nil + } + + labelSelectorKeys := sets.String{} + if labelSelector != nil { + for key := range labelSelector.MatchLabels { + labelSelectorKeys.Insert(key) + } + for _, matchExpression := range labelSelector.MatchExpressions { + labelSelectorKeys.Insert(matchExpression.Key) + } + } + + allErrs := field.ErrorList{} + for i, key := range matchLabelKeys { + allErrs = append(allErrs, unversionedvalidation.ValidateLabelName(key, fldPath.Index(i))...) + if labelSelectorKeys.Has(key) { + allErrs = append(allErrs, field.Invalid(fldPath.Index(i), key, "exists in both matchLabelKeys and labelSelector")) + } + } + + return allErrs +} + // ValidateServiceClusterIPsRelatedFields validates .spec.ClusterIPs,, // .spec.IPFamilies, .spec.ipFamilyPolicy. This is exported because it is used // during IP init and allocation. diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index ff7bc068445..9f8264fb427 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -18855,6 +18855,7 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { fieldPathTopologyKey := subFldPath0.Child("topologyKey") fieldPathWhenUnsatisfiable := subFldPath0.Child("whenUnsatisfiable") fieldPathTopologyKeyAndWhenUnsatisfiable := subFldPath0.Child("{topologyKey, whenUnsatisfiable}") + fieldPathMatchLabelKeys := subFldPath0.Child("matchLabelKeys") nodeAffinityField := subFldPath0.Child("nodeAffinityPolicy") nodeTaintsField := subFldPath0.Child("nodeTaintsPolicy") unknown := core.NodeInclusionPolicy("Unknown") @@ -19033,6 +19034,39 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { field.NotSupported(nodeTaintsField, &unknown, supportedPodTopologySpreadNodePolicies.List()), }, }, + { + name: "key in MatchLabelKeys isn't correctly defined", + constraints: []core.TopologySpreadConstraint{ + { + MaxSkew: 1, + TopologyKey: "k8s.io/zone", + WhenUnsatisfiable: core.DoNotSchedule, + MatchLabelKeys: []string{"/simple"}, + }, + }, + wantFieldErrors: []*field.Error{field.Invalid(fieldPathMatchLabelKeys.Index(0), "/simple", "prefix part must be non-empty")}, + }, + { + name: "key exists in both matchLabelKeys and labelSelector", + constraints: []core.TopologySpreadConstraint{ + { + MaxSkew: 1, + TopologyKey: "k8s.io/zone", + WhenUnsatisfiable: core.DoNotSchedule, + MatchLabelKeys: []string{"foo"}, + LabelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "foo", + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{"value1", "value2"}, + }, + }, + }, + }, + }, + wantFieldErrors: []*field.Error{field.Invalid(fieldPathMatchLabelKeys.Index(0), "foo", "exists in both matchLabelKeys and labelSelector")}, + }, } for _, tc := range testCases { diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go index b4b58a17d4a..ebc6e66e7a0 100644 --- a/staging/src/k8s.io/api/core/v1/types.go +++ b/staging/src/k8s.io/api/core/v1/types.go @@ -3455,6 +3455,15 @@ type TopologySpreadConstraint struct { // This is a alpha-level feature enabled by the NodeInclusionPolicyInPodTopologySpread feature flag. // +optional NodeTaintsPolicy *NodeInclusionPolicy `json:"nodeTaintsPolicy,omitempty" protobuf:"bytes,7,opt,name=nodeTaintsPolicy"` + // 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. + // +listType=atomic + // +optional + MatchLabelKeys []string `json:"matchLabelKeys,omitempty" protobuf:"bytes,8,opt,name=matchLabelKeys"` } const (