Add defaultingType to PodTopologySpreadArgs

Change-Id: Ibf6a4fdb39a31fe9deed68de7e7cb24a9bf9d06a

pkg/scheduler/apis/config/scheme/BUILD

@@ -34,7 +34,6 @@ go_test(
     srcs = ["scheme_test.go"],
     embed = [":go_default_library"],
     deps = [
-        "//pkg/features:go_default_library",
         "//pkg/scheduler/apis/config:go_default_library",
         "//staging/src/k8s.io/api/core/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",

pkg/scheduler/apis/config/scheme/scheme_test.go

@@ -28,12 +28,13 @@ import (
 	"k8s.io/component-base/featuregate"
 	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	"k8s.io/kube-scheduler/config/v1beta1"
-	"k8s.io/kubernetes/pkg/features"
 	"k8s.io/kubernetes/pkg/scheduler/apis/config"
 	"k8s.io/utils/pointer"
 	"sigs.k8s.io/yaml"
 )
 
+// TestCodecsDecodePluginConfig tests that embedded plugin args get decoded
+// into their appropriate internal types and defaults are applied.
 func TestCodecsDecodePluginConfig(t *testing.T) {
 	testCases := []struct {
 		name         string
@@ -116,6 +117,7 @@ profiles:
 								DefaultConstraints: []corev1.TopologySpreadConstraint{
 									{MaxSkew: 1, TopologyKey: "zone", WhenUnsatisfiable: corev1.ScheduleAnyway},
 								},
+								DefaultingType: config.ListDefaulting,
 							},
 						},
 						{
@@ -297,71 +299,10 @@ profiles:
 								BindTimeoutSeconds: 600,
 							},
 						},
-						{
-							Name: "PodTopologySpread",
-							Args: &config.PodTopologySpreadArgs{},
-						},
-					},
-				},
-			},
-		},
-		{
-			name: "empty PodTopologySpread, feature DefaultPodTopologySpread enabled",
-			data: []byte(`
-apiVersion: kubescheduler.config.k8s.io/v1beta1
-kind: KubeSchedulerConfiguration
-profiles:
-- pluginConfig:
-  - name: PodTopologySpread
-    args:
-      defaultConstraints:
-`),
-			feature: features.DefaultPodTopologySpread,
-			wantProfiles: []config.KubeSchedulerProfile{
-				{
-					SchedulerName: "default-scheduler",
-					PluginConfig: []config.PluginConfig{
-						{
-							Name: "PodTopologySpread",
-							Args: &config.PodTopologySpreadArgs{
-								DefaultConstraints: []corev1.TopologySpreadConstraint{
-									{
-										MaxSkew:           3,
-										TopologyKey:       corev1.LabelHostname,
-										WhenUnsatisfiable: corev1.ScheduleAnyway,
-									},
-									{
-										MaxSkew:           5,
-										TopologyKey:       corev1.LabelZoneFailureDomainStable,
-										WhenUnsatisfiable: corev1.ScheduleAnyway,
-									},
-								},
-							},
-						},
-					},
-				},
-			},
-		},
-		{
-			name: "empty array PodTopologySpread, feature DefaultPodTopologySpread enabled",
-			data: []byte(`
-apiVersion: kubescheduler.config.k8s.io/v1beta1
-kind: KubeSchedulerConfiguration
-profiles:
-- pluginConfig:
-  - name: PodTopologySpread
-    args:
-      defaultConstraints: []
-`),
-			feature: features.DefaultPodTopologySpread,
-			wantProfiles: []config.KubeSchedulerProfile{
-				{
-					SchedulerName: "default-scheduler",
-					PluginConfig: []config.PluginConfig{
 						{
 							Name: "PodTopologySpread",
 							Args: &config.PodTopologySpreadArgs{
-								DefaultConstraints: []corev1.TopologySpreadConstraint{},
+								DefaultingType: config.ListDefaulting,
 							},
 						},
 					},
@@ -514,7 +455,6 @@ profiles:
     name: NodeResourcesLeastAllocated
   - args:
       apiVersion: kubescheduler.config.k8s.io/v1beta1
-      defaultConstraints: []
       kind: PodTopologySpreadArgs
     name: PodTopologySpread
   - args:
@@ -605,7 +545,6 @@ profiles:
     name: VolumeBinding
   - args:
       apiVersion: kubescheduler.config.k8s.io/v1beta1
-      defaultConstraints: null
       kind: PodTopologySpreadArgs
     name: PodTopologySpread
   - args:

pkg/scheduler/apis/config/testing/compatibility_test.go

@@ -1622,6 +1622,7 @@ func TestPluginsConfigurationCompatibility(t *testing.T) {
 								WhenUnsatisfiable: v1.ScheduleAnyway,
 							},
 						},
+						DefaultingType: config.ListDefaulting,
 					},
 				},
 				{
@@ -1686,6 +1687,7 @@ func TestPluginsConfigurationCompatibility(t *testing.T) {
 								WhenUnsatisfiable: v1.ScheduleAnyway,
 							},
 						},
+						DefaultingType: config.ListDefaulting,
 					},
 				},
 				{

pkg/scheduler/apis/config/types_pluginargs.go

@@ -64,19 +64,42 @@ type NodeResourcesFitArgs struct {
 	IgnoredResourceGroups []string
 }
 
+// PodTopologySpreadConstraintsDefaulting defines how to set default constraints
+// for the PodTopologySpread plugin.
+type PodTopologySpreadConstraintsDefaulting string
+
+const (
+	// SystemDefaulting instructs to use the kubernetes defined default.
+	SystemDefaulting PodTopologySpreadConstraintsDefaulting = "System"
+	// ListDefaulting instructs to use the config provided default.
+	ListDefaulting PodTopologySpreadConstraintsDefaulting = "List"
+)
+
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 
 // PodTopologySpreadArgs holds arguments used to configure the PodTopologySpread plugin.
 type PodTopologySpreadArgs struct {
 	metav1.TypeMeta
 
 	// DefaultConstraints defines topology spread constraints to be applied to
-	// pods that don't define any in `pod.spec.topologySpreadConstraints`.
-	// `topologySpreadConstraint.labelSelectors` must be empty, as they are
-	// deduced the pods' membership to Services, Replication Controllers, Replica
-	// Sets or Stateful Sets.
-	// Empty by default.
+	// Pods that don't define any in `pod.spec.topologySpreadConstraints`.
+	// `.defaultConstraints[*].labelSelectors` must be empty, as they are
+	// deduced from the Pod's membership to Services, ReplicationControllers,
+	// ReplicaSets or StatefulSets.
+	// When not empty, .defaultingType must be "List".
 	DefaultConstraints []v1.TopologySpreadConstraint
+
+	// DefaultingType determines how .defaultConstraints are deduced. Can be one
+	// of "System" or "List".
+	//
+	// - "System": Use kubernetes defined constraints that spread Pods among
+	//   Nodes and Zones.
+	// - "List": Use constraints defined in .defaultConstraints.
+	//
+	// Defaults to "List" if feature gate DefaultPodTopologySpread is disabled
+	// and to "System" if enabled.
+	// +optional
+	DefaultingType PodTopologySpreadConstraintsDefaulting
 }
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

pkg/scheduler/apis/config/v1beta1/defaults.go

@@ -195,23 +195,18 @@ func SetDefaults_VolumeBindingArgs(obj *v1beta1.VolumeBindingArgs) {
 }
 
 func SetDefaults_PodTopologySpreadArgs(obj *v1beta1.PodTopologySpreadArgs) {
-	if !feature.DefaultFeatureGate.Enabled(features.DefaultPodTopologySpread) {
-		// When feature is disabled, the default spreading is done by legacy
-		// SelectorSpread plugin.
+	if feature.DefaultFeatureGate.Enabled(features.DefaultPodTopologySpread) {
+		if obj.DefaultingType == "" {
+			// TODO(#94008): Always default to System in v1beta2.
+			if len(obj.DefaultConstraints) != 0 {
+				obj.DefaultingType = v1beta1.ListDefaulting
+			} else {
+				obj.DefaultingType = v1beta1.SystemDefaulting
+			}
+		}
 		return
 	}
-	if obj.DefaultConstraints == nil {
-		obj.DefaultConstraints = []corev1.TopologySpreadConstraint{
-			{
-				TopologyKey:       corev1.LabelHostname,
-				WhenUnsatisfiable: corev1.ScheduleAnyway,
-				MaxSkew:           3,
-			},
-			{
-				TopologyKey:       corev1.LabelZoneFailureDomainStable,
-				WhenUnsatisfiable: corev1.ScheduleAnyway,
-				MaxSkew:           5,
-			},
-		}
+	if obj.DefaultingType == "" {
+		obj.DefaultingType = v1beta1.ListDefaulting
 	}
 }

pkg/scheduler/apis/config/v1beta1/defaults_test.go

@@ -367,7 +367,9 @@ func TestPluginArgsDefaults(t *testing.T) {
 		{
 			name: "PodTopologySpreadArgs resources empty",
 			in:   &v1beta1.PodTopologySpreadArgs{},
-			want: &v1beta1.PodTopologySpreadArgs{},
+			want: &v1beta1.PodTopologySpreadArgs{
+				DefaultingType: v1beta1.ListDefaulting,
+			},
 		},
 		{
 			name: "PodTopologySpreadArgs resources with value",
@@ -388,35 +390,39 @@ func TestPluginArgsDefaults(t *testing.T) {
 						MaxSkew:           2,
 					},
 				},
+				DefaultingType: v1beta1.ListDefaulting,
 			},
 		},
 		{
-			name:    "PodTopologySpreadArgs resources empty, DefaultPodTopologySpread feature enabled",
+			name:    "PodTopologySpreadArgs empty, DefaultPodTopologySpread feature enabled",
 			feature: features.DefaultPodTopologySpread,
 			in:      &v1beta1.PodTopologySpreadArgs{},
 			want: &v1beta1.PodTopologySpreadArgs{
+				DefaultingType: v1beta1.SystemDefaulting,
+			},
+		},
+		{
+			name:    "PodTopologySpreadArgs with constraints, DefaultPodTopologySpread feature enabled",
+			feature: features.DefaultPodTopologySpread,
+			in: &v1beta1.PodTopologySpreadArgs{
 				DefaultConstraints: []v1.TopologySpreadConstraint{
 					{
 						TopologyKey:       v1.LabelHostname,
 						WhenUnsatisfiable: v1.ScheduleAnyway,
 						MaxSkew:           3,
 					},
+				},
+			},
+			want: &v1beta1.PodTopologySpreadArgs{
+				DefaultConstraints: []v1.TopologySpreadConstraint{
 					{
-						TopologyKey:       v1.LabelZoneFailureDomainStable,
+						TopologyKey:       v1.LabelHostname,
 						WhenUnsatisfiable: v1.ScheduleAnyway,
-						MaxSkew:           5,
+						MaxSkew:           3,
 					},
 				},
-			},
-		},
-		{
-			name:    "PodTopologySpreadArgs empty array, DefaultPodTopologySpread feature enabled",
-			feature: features.DefaultPodTopologySpread,
-			in: &v1beta1.PodTopologySpreadArgs{
-				DefaultConstraints: []v1.TopologySpreadConstraint{},
-			},
-			want: &v1beta1.PodTopologySpreadArgs{
-				DefaultConstraints: []v1.TopologySpreadConstraint{},
+				// TODO(#94008): Make SystemDefaulting in v1beta2.
+				DefaultingType: v1beta1.ListDefaulting,
 			},
 		},
 	}

pkg/scheduler/apis/config/validation/validation_pluginargs.go

@@ -75,6 +75,9 @@ func validateNoConflict(presentLabels []string, absentLabels []string) error {
 // with an additional check for .labelSelector to be nil.
 func ValidatePodTopologySpreadArgs(args *config.PodTopologySpreadArgs) error {
 	var allErrs field.ErrorList
+	if err := validateDefaultingType(field.NewPath("defaultingType"), args.DefaultingType, args.DefaultConstraints); err != nil {
+		allErrs = append(allErrs, err)
+	}
 	path := field.NewPath("defaultConstraints")
 
 	for i, c := range args.DefaultConstraints {
@@ -101,6 +104,16 @@ func ValidatePodTopologySpreadArgs(args *config.PodTopologySpreadArgs) error {
 	return allErrs.ToAggregate()
 }
 
+func validateDefaultingType(p *field.Path, v config.PodTopologySpreadConstraintsDefaulting, constraints []v1.TopologySpreadConstraint) *field.Error {
+	if v != config.SystemDefaulting && v != config.ListDefaulting {
+		return field.Invalid(p, v, fmt.Sprintf("must be one of {%q, %q}", config.SystemDefaulting, config.ListDefaulting))
+	}
+	if v == config.SystemDefaulting && len(constraints) > 0 {
+		return field.Invalid(p, v, "when .defaultConstraints are not empty")
+	}
+	return nil
+}
+
 func validateTopologyKey(p *field.Path, v string) field.ErrorList {
 	var allErrs field.ErrorList
 	if len(v) == 0 {