From c8a0b9e8e7d5d0535a03ffa94f87e504aac483ad Mon Sep 17 00:00:00 2001 From: Aldo Culquicondor Date: Thu, 24 Sep 2020 18:09:44 -0400 Subject: [PATCH] Add defaultingType to PodTopologySpreadArgs Change-Id: Ibf6a4fdb39a31fe9deed68de7e7cb24a9bf9d06a --- pkg/scheduler/apis/config/scheme/BUILD | 1 - .../apis/config/scheme/scheme_test.go | 69 +---------- .../apis/config/testing/compatibility_test.go | 2 + pkg/scheduler/apis/config/types_pluginargs.go | 33 ++++- pkg/scheduler/apis/config/v1beta1/defaults.go | 27 ++-- .../apis/config/v1beta1/defaults_test.go | 40 +++--- .../config/v1beta1/zz_generated.conversion.go | 2 + .../validation/validation_pluginargs.go | 13 ++ .../validation/validation_pluginargs_test.go | 30 +++++ .../framework/plugins/podtopologyspread/BUILD | 1 + .../plugins/podtopologyspread/common.go | 6 +- .../plugins/podtopologyspread/filtering.go | 2 +- .../podtopologyspread/filtering_test.go | 7 +- .../plugins/podtopologyspread/plugin.go | 35 ++++-- .../plugins/podtopologyspread/scoring.go | 2 +- .../plugins/podtopologyspread/scoring_test.go | 117 +++++++++++++----- .../framework/runtime/framework_test.go | 8 +- .../config/v1beta1/types_pluginargs.go | 35 +++++- 18 files changed, 267 insertions(+), 163 deletions(-) diff --git a/pkg/scheduler/apis/config/scheme/BUILD b/pkg/scheduler/apis/config/scheme/BUILD index df12007b9ef..1c7c14eb4e4 100644 --- a/pkg/scheduler/apis/config/scheme/BUILD +++ b/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", diff --git a/pkg/scheduler/apis/config/scheme/scheme_test.go b/pkg/scheduler/apis/config/scheme/scheme_test.go index 3756ae7c113..a494de5f85c 100644 --- a/pkg/scheduler/apis/config/scheme/scheme_test.go +++ b/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: diff --git a/pkg/scheduler/apis/config/testing/compatibility_test.go b/pkg/scheduler/apis/config/testing/compatibility_test.go index f22e5db6ba6..6e57ae08384 100644 --- a/pkg/scheduler/apis/config/testing/compatibility_test.go +++ b/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, }, }, { diff --git a/pkg/scheduler/apis/config/types_pluginargs.go b/pkg/scheduler/apis/config/types_pluginargs.go index 24dd2f6ffda..11bd5b6c9ef 100644 --- a/pkg/scheduler/apis/config/types_pluginargs.go +++ b/pkg/scheduler/apis/config/types_pluginargs.go @@ -64,6 +64,17 @@ 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. @@ -71,12 +82,24 @@ 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 diff --git a/pkg/scheduler/apis/config/v1beta1/defaults.go b/pkg/scheduler/apis/config/v1beta1/defaults.go index 0c30a35ca6c..c41c399cbed 100644 --- a/pkg/scheduler/apis/config/v1beta1/defaults.go +++ b/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 } } diff --git a/pkg/scheduler/apis/config/v1beta1/defaults_test.go b/pkg/scheduler/apis/config/v1beta1/defaults_test.go index 6212b0ce32f..f334da90baa 100644 --- a/pkg/scheduler/apis/config/v1beta1/defaults_test.go +++ b/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,12 +390,29 @@ 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{ { @@ -401,22 +420,9 @@ func TestPluginArgsDefaults(t *testing.T) { WhenUnsatisfiable: v1.ScheduleAnyway, MaxSkew: 3, }, - { - TopologyKey: v1.LabelZoneFailureDomainStable, - WhenUnsatisfiable: v1.ScheduleAnyway, - MaxSkew: 5, - }, }, - }, - }, - { - 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, }, }, } diff --git a/pkg/scheduler/apis/config/v1beta1/zz_generated.conversion.go b/pkg/scheduler/apis/config/v1beta1/zz_generated.conversion.go index b10fecad617..edd6a18a749 100644 --- a/pkg/scheduler/apis/config/v1beta1/zz_generated.conversion.go +++ b/pkg/scheduler/apis/config/v1beta1/zz_generated.conversion.go @@ -854,6 +854,7 @@ func Convert_config_Plugins_To_v1beta1_Plugins(in *config.Plugins, out *v1beta1. func autoConvert_v1beta1_PodTopologySpreadArgs_To_config_PodTopologySpreadArgs(in *v1beta1.PodTopologySpreadArgs, out *config.PodTopologySpreadArgs, s conversion.Scope) error { out.DefaultConstraints = *(*[]corev1.TopologySpreadConstraint)(unsafe.Pointer(&in.DefaultConstraints)) + out.DefaultingType = config.PodTopologySpreadConstraintsDefaulting(in.DefaultingType) return nil } @@ -864,6 +865,7 @@ func Convert_v1beta1_PodTopologySpreadArgs_To_config_PodTopologySpreadArgs(in *v func autoConvert_config_PodTopologySpreadArgs_To_v1beta1_PodTopologySpreadArgs(in *config.PodTopologySpreadArgs, out *v1beta1.PodTopologySpreadArgs, s conversion.Scope) error { out.DefaultConstraints = *(*[]corev1.TopologySpreadConstraint)(unsafe.Pointer(&in.DefaultConstraints)) + out.DefaultingType = v1beta1.PodTopologySpreadConstraintsDefaulting(in.DefaultingType) return nil } diff --git a/pkg/scheduler/apis/config/validation/validation_pluginargs.go b/pkg/scheduler/apis/config/validation/validation_pluginargs.go index 860c1abe535..47418ed330d 100644 --- a/pkg/scheduler/apis/config/validation/validation_pluginargs.go +++ b/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 { diff --git a/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go b/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go index 652ccf0d131..4cd158902ad 100644 --- a/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go +++ b/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go @@ -112,6 +112,7 @@ func TestValidatePodTopologySpreadArgs(t *testing.T) { WhenUnsatisfiable: v1.ScheduleAnyway, }, }, + DefaultingType: config.ListDefaulting, }, }, "maxSkew less than zero": { @@ -123,6 +124,7 @@ func TestValidatePodTopologySpreadArgs(t *testing.T) { WhenUnsatisfiable: v1.DoNotSchedule, }, }, + DefaultingType: config.ListDefaulting, }, wantErr: `defaultConstraints[0].maxSkew: Invalid value: -1: must be greater than zero`, }, @@ -135,6 +137,7 @@ func TestValidatePodTopologySpreadArgs(t *testing.T) { WhenUnsatisfiable: v1.DoNotSchedule, }, }, + DefaultingType: config.ListDefaulting, }, wantErr: `defaultConstraints[0].topologyKey: Required value: can not be empty`, }, @@ -147,6 +150,7 @@ func TestValidatePodTopologySpreadArgs(t *testing.T) { WhenUnsatisfiable: "", }, }, + DefaultingType: config.ListDefaulting, }, wantErr: `defaultConstraints[0].whenUnsatisfiable: Required value: can not be empty`, }, @@ -159,6 +163,7 @@ func TestValidatePodTopologySpreadArgs(t *testing.T) { WhenUnsatisfiable: "unknown action", }, }, + DefaultingType: config.ListDefaulting, }, wantErr: `defaultConstraints[0].whenUnsatisfiable: Unsupported value: "unknown action": supported values: "DoNotSchedule", "ScheduleAnyway"`, }, @@ -176,6 +181,7 @@ func TestValidatePodTopologySpreadArgs(t *testing.T) { WhenUnsatisfiable: v1.DoNotSchedule, }, }, + DefaultingType: config.ListDefaulting, }, wantErr: `defaultConstraints[1]: Duplicate value: "{node, DoNotSchedule}"`, }, @@ -193,9 +199,33 @@ func TestValidatePodTopologySpreadArgs(t *testing.T) { }, }, }, + DefaultingType: config.ListDefaulting, }, wantErr: `defaultConstraints[0].labelSelector: Forbidden: constraint must not define a selector, as they deduced for each pod`, }, + "list default constraints, no constraints": { + args: &config.PodTopologySpreadArgs{ + DefaultingType: config.ListDefaulting, + }, + }, + "system default constraints": { + args: &config.PodTopologySpreadArgs{ + DefaultingType: config.SystemDefaulting, + }, + }, + "system default constraints, but has constraints": { + args: &config.PodTopologySpreadArgs{ + DefaultConstraints: []v1.TopologySpreadConstraint{ + { + MaxSkew: 1, + TopologyKey: "key", + WhenUnsatisfiable: v1.DoNotSchedule, + }, + }, + DefaultingType: config.SystemDefaulting, + }, + wantErr: `defaultingType: Invalid value: "System": when .defaultConstraints are not empty`, + }, } for name, tc := range cases { diff --git a/pkg/scheduler/framework/plugins/podtopologyspread/BUILD b/pkg/scheduler/framework/plugins/podtopologyspread/BUILD index f696c10da00..933532dbf81 100644 --- a/pkg/scheduler/framework/plugins/podtopologyspread/BUILD +++ b/pkg/scheduler/framework/plugins/podtopologyspread/BUILD @@ -37,6 +37,7 @@ go_test( embed = [":go_default_library"], deps = [ "//pkg/scheduler/apis/config:go_default_library", + "//pkg/scheduler/framework/runtime:go_default_library", "//pkg/scheduler/framework/v1alpha1:go_default_library", "//pkg/scheduler/internal/cache:go_default_library", "//pkg/scheduler/internal/parallelize:go_default_library", diff --git a/pkg/scheduler/framework/plugins/podtopologyspread/common.go b/pkg/scheduler/framework/plugins/podtopologyspread/common.go index df95d4dd64c..6290d7a1980 100644 --- a/pkg/scheduler/framework/plugins/podtopologyspread/common.go +++ b/pkg/scheduler/framework/plugins/podtopologyspread/common.go @@ -38,11 +38,11 @@ type topologySpreadConstraint struct { Selector labels.Selector } -// defaultConstraints builds the constraints for a pod using +// buildDefaultConstraints builds the constraints for a pod using // .DefaultConstraints and the selectors from the services, replication // controllers, replica sets and stateful sets that match the pod. -func (pl *PodTopologySpread) defaultConstraints(p *v1.Pod, action v1.UnsatisfiableConstraintAction) ([]topologySpreadConstraint, error) { - constraints, err := filterTopologySpreadConstraints(pl.args.DefaultConstraints, action) +func (pl *PodTopologySpread) buildDefaultConstraints(p *v1.Pod, action v1.UnsatisfiableConstraintAction) ([]topologySpreadConstraint, error) { + constraints, err := filterTopologySpreadConstraints(pl.defaultConstraints, action) if err != nil || len(constraints) == 0 { return nil, err } diff --git a/pkg/scheduler/framework/plugins/podtopologyspread/filtering.go b/pkg/scheduler/framework/plugins/podtopologyspread/filtering.go index 55aef0ee5f4..b9f54c8ba76 100644 --- a/pkg/scheduler/framework/plugins/podtopologyspread/filtering.go +++ b/pkg/scheduler/framework/plugins/podtopologyspread/filtering.go @@ -209,7 +209,7 @@ func (pl *PodTopologySpread) calPreFilterState(pod *v1.Pod) (*preFilterState, er return nil, fmt.Errorf("obtaining pod's hard topology spread constraints: %v", err) } } else { - constraints, err = pl.defaultConstraints(pod, v1.DoNotSchedule) + constraints, err = pl.buildDefaultConstraints(pod, v1.DoNotSchedule) if err != nil { return nil, fmt.Errorf("setting default hard topology spread constraints: %v", err) } diff --git a/pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go b/pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go index 293a1520afc..bee72352adc 100644 --- a/pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go +++ b/pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go @@ -29,7 +29,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" - "k8s.io/kubernetes/pkg/scheduler/apis/config" framework "k8s.io/kubernetes/pkg/scheduler/framework/v1alpha1" "k8s.io/kubernetes/pkg/scheduler/internal/cache" "k8s.io/kubernetes/pkg/scheduler/internal/parallelize" @@ -518,10 +517,8 @@ func TestPreFilterState(t *testing.T) { ctx := context.Background() informerFactory := informers.NewSharedInformerFactory(fake.NewSimpleClientset(tt.objs...), 0) pl := PodTopologySpread{ - sharedLister: cache.NewSnapshot(tt.existingPods, tt.nodes), - args: config.PodTopologySpreadArgs{ - DefaultConstraints: tt.defaultConstraints, - }, + sharedLister: cache.NewSnapshot(tt.existingPods, tt.nodes), + defaultConstraints: tt.defaultConstraints, } pl.setListers(informerFactory) informerFactory.Start(ctx.Done()) diff --git a/pkg/scheduler/framework/plugins/podtopologyspread/plugin.go b/pkg/scheduler/framework/plugins/podtopologyspread/plugin.go index 3011e52cbe5..a03eb023754 100644 --- a/pkg/scheduler/framework/plugins/podtopologyspread/plugin.go +++ b/pkg/scheduler/framework/plugins/podtopologyspread/plugin.go @@ -19,6 +19,7 @@ package podtopologyspread import ( "fmt" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/informers" appslisters "k8s.io/client-go/listers/apps/v1" @@ -35,14 +36,27 @@ const ( ErrReasonNodeLabelNotMatch = ErrReasonConstraintsNotMatch + " (missing required label)" ) +var systemDefaultConstraints = []v1.TopologySpreadConstraint{ + { + TopologyKey: v1.LabelHostname, + WhenUnsatisfiable: v1.ScheduleAnyway, + MaxSkew: 3, + }, + { + TopologyKey: v1.LabelZoneFailureDomainStable, + WhenUnsatisfiable: v1.ScheduleAnyway, + MaxSkew: 5, + }, +} + // PodTopologySpread is a plugin that ensures pod's topologySpreadConstraints is satisfied. type PodTopologySpread struct { - args config.PodTopologySpreadArgs - sharedLister framework.SharedLister - services corelisters.ServiceLister - replicationCtrls corelisters.ReplicationControllerLister - replicaSets appslisters.ReplicaSetLister - statefulSets appslisters.StatefulSetLister + defaultConstraints []v1.TopologySpreadConstraint + sharedLister framework.SharedLister + services corelisters.ServiceLister + replicationCtrls corelisters.ReplicationControllerLister + replicaSets appslisters.ReplicaSetLister + statefulSets appslisters.StatefulSetLister } var _ framework.PreFilterPlugin = &PodTopologySpread{} @@ -73,10 +87,13 @@ func New(plArgs runtime.Object, h framework.FrameworkHandle) (framework.Plugin, return nil, err } pl := &PodTopologySpread{ - sharedLister: h.SnapshotSharedLister(), - args: args, + sharedLister: h.SnapshotSharedLister(), + defaultConstraints: args.DefaultConstraints, } - if len(pl.args.DefaultConstraints) != 0 { + if args.DefaultingType == config.SystemDefaulting { + pl.defaultConstraints = systemDefaultConstraints + } + if len(pl.defaultConstraints) != 0 { if h.SharedInformerFactory() == nil { return nil, fmt.Errorf("SharedInformerFactory is nil") } diff --git a/pkg/scheduler/framework/plugins/podtopologyspread/scoring.go b/pkg/scheduler/framework/plugins/podtopologyspread/scoring.go index 8b0c402b03d..85c92fb4d81 100644 --- a/pkg/scheduler/framework/plugins/podtopologyspread/scoring.go +++ b/pkg/scheduler/framework/plugins/podtopologyspread/scoring.go @@ -64,7 +64,7 @@ func (pl *PodTopologySpread) initPreScoreState(s *preScoreState, pod *v1.Pod, fi return fmt.Errorf("obtaining pod's soft topology spread constraints: %v", err) } } else { - s.Constraints, err = pl.defaultConstraints(pod, v1.ScheduleAnyway) + s.Constraints, err = pl.buildDefaultConstraints(pod, v1.ScheduleAnyway) if err != nil { return fmt.Errorf("setting default soft topology spread constraints: %v", err) } diff --git a/pkg/scheduler/framework/plugins/podtopologyspread/scoring_test.go b/pkg/scheduler/framework/plugins/podtopologyspread/scoring_test.go index f951d822d70..147b4d082c0 100644 --- a/pkg/scheduler/framework/plugins/podtopologyspread/scoring_test.go +++ b/pkg/scheduler/framework/plugins/podtopologyspread/scoring_test.go @@ -28,6 +28,7 @@ import ( "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" "k8s.io/kubernetes/pkg/scheduler/apis/config" + frameworkruntime "k8s.io/kubernetes/pkg/scheduler/framework/runtime" framework "k8s.io/kubernetes/pkg/scheduler/framework/v1alpha1" "k8s.io/kubernetes/pkg/scheduler/internal/cache" "k8s.io/kubernetes/pkg/scheduler/internal/parallelize" @@ -37,12 +38,12 @@ import ( func TestPreScoreStateEmptyNodes(t *testing.T) { tests := []struct { - name string - pod *v1.Pod - nodes []*v1.Node - objs []runtime.Object - defaultConstraints []v1.TopologySpreadConstraint - want *preScoreState + name string + pod *v1.Pod + nodes []*v1.Node + objs []runtime.Object + config config.PodTopologySpreadArgs + want *preScoreState }{ { name: "normal case", @@ -55,6 +56,9 @@ func TestPreScoreStateEmptyNodes(t *testing.T) { st.MakeNode().Name("node-b").Label("zone", "zone1").Label(v1.LabelHostname, "node-b").Obj(), st.MakeNode().Name("node-x").Label("zone", "zone2").Label(v1.LabelHostname, "node-x").Obj(), }, + config: config.PodTopologySpreadArgs{ + DefaultingType: config.ListDefaulting, + }, want: &preScoreState{ Constraints: []topologySpreadConstraint{ { @@ -87,6 +91,9 @@ func TestPreScoreStateEmptyNodes(t *testing.T) { st.MakeNode().Name("node-b").Label("zone", "zone1").Label(v1.LabelHostname, "node-b").Obj(), st.MakeNode().Name("node-x").Label(v1.LabelHostname, "node-x").Obj(), }, + config: config.PodTopologySpreadArgs{ + DefaultingType: config.ListDefaulting, + }, want: &preScoreState{ Constraints: []topologySpreadConstraint{ { @@ -107,13 +114,48 @@ func TestPreScoreStateEmptyNodes(t *testing.T) { TopologyNormalizingWeight: []float64{topologyNormalizingWeight(1), topologyNormalizingWeight(2)}, }, }, + { + name: "system defaults constraints and a replica set", + pod: st.MakePod().Name("p").Label("foo", "tar").Label("baz", "sup").Obj(), + config: config.PodTopologySpreadArgs{ + DefaultingType: config.SystemDefaulting, + }, + nodes: []*v1.Node{ + st.MakeNode().Name("node-a").Label(v1.LabelHostname, "node-a").Label(v1.LabelZoneFailureDomainStable, "mars").Obj(), + }, + objs: []runtime.Object{ + &appsv1.ReplicaSet{Spec: appsv1.ReplicaSetSpec{Selector: st.MakeLabelSelector().Exists("foo").Obj()}}, + }, + want: &preScoreState{ + Constraints: []topologySpreadConstraint{ + { + MaxSkew: 3, + TopologyKey: v1.LabelHostname, + Selector: mustConvertLabelSelectorAsSelector(t, st.MakeLabelSelector().Exists("foo").Obj()), + }, + { + MaxSkew: 5, + TopologyKey: v1.LabelZoneFailureDomainStable, + Selector: mustConvertLabelSelectorAsSelector(t, st.MakeLabelSelector().Exists("foo").Obj()), + }, + }, + IgnoredNodes: sets.NewString(), + TopologyPairToPodCounts: map[topologyPair]*int64{ + {key: v1.LabelZoneFailureDomainStable, value: "mars"}: pointer.Int64Ptr(0), + }, + TopologyNormalizingWeight: []float64{topologyNormalizingWeight(1), topologyNormalizingWeight(1)}, + }, + }, { name: "defaults constraints and a replica set", pod: st.MakePod().Name("p").Label("foo", "tar").Label("baz", "sup").Obj(), - defaultConstraints: []v1.TopologySpreadConstraint{ - {MaxSkew: 1, TopologyKey: v1.LabelHostname, WhenUnsatisfiable: v1.ScheduleAnyway}, - {MaxSkew: 2, TopologyKey: "rack", WhenUnsatisfiable: v1.DoNotSchedule}, - {MaxSkew: 2, TopologyKey: "planet", WhenUnsatisfiable: v1.ScheduleAnyway}, + config: config.PodTopologySpreadArgs{ + DefaultConstraints: []v1.TopologySpreadConstraint{ + {MaxSkew: 1, TopologyKey: v1.LabelHostname, WhenUnsatisfiable: v1.ScheduleAnyway}, + {MaxSkew: 2, TopologyKey: "rack", WhenUnsatisfiable: v1.DoNotSchedule}, + {MaxSkew: 2, TopologyKey: "planet", WhenUnsatisfiable: v1.ScheduleAnyway}, + }, + DefaultingType: config.ListDefaulting, }, nodes: []*v1.Node{ st.MakeNode().Name("node-a").Label("rack", "rack1").Label(v1.LabelHostname, "node-a").Label("planet", "mars").Obj(), @@ -144,8 +186,11 @@ func TestPreScoreStateEmptyNodes(t *testing.T) { { name: "defaults constraints and a replica set that doesn't match", pod: st.MakePod().Name("p").Label("foo", "bar").Label("baz", "sup").Obj(), - defaultConstraints: []v1.TopologySpreadConstraint{ - {MaxSkew: 2, TopologyKey: "planet", WhenUnsatisfiable: v1.ScheduleAnyway}, + config: config.PodTopologySpreadArgs{ + DefaultConstraints: []v1.TopologySpreadConstraint{ + {MaxSkew: 2, TopologyKey: "planet", WhenUnsatisfiable: v1.ScheduleAnyway}, + }, + DefaultingType: config.ListDefaulting, }, nodes: []*v1.Node{ st.MakeNode().Name("node-a").Label("planet", "mars").Obj(), @@ -162,8 +207,11 @@ func TestPreScoreStateEmptyNodes(t *testing.T) { pod: st.MakePod().Name("p").Label("foo", "bar").Label("baz", "sup"). SpreadConstraint(1, "zone", v1.DoNotSchedule, st.MakeLabelSelector().Label("foo", "bar").Obj()). SpreadConstraint(2, "planet", v1.ScheduleAnyway, st.MakeLabelSelector().Label("baz", "sup").Obj()).Obj(), - defaultConstraints: []v1.TopologySpreadConstraint{ - {MaxSkew: 2, TopologyKey: "galaxy", WhenUnsatisfiable: v1.ScheduleAnyway}, + config: config.PodTopologySpreadArgs{ + DefaultConstraints: []v1.TopologySpreadConstraint{ + {MaxSkew: 2, TopologyKey: "galaxy", WhenUnsatisfiable: v1.ScheduleAnyway}, + }, + DefaultingType: config.ListDefaulting, }, nodes: []*v1.Node{ st.MakeNode().Name("node-a").Label("planet", "mars").Label("galaxy", "andromeda").Obj(), @@ -191,17 +239,21 @@ func TestPreScoreStateEmptyNodes(t *testing.T) { t.Run(tt.name, func(t *testing.T) { ctx := context.Background() informerFactory := informers.NewSharedInformerFactory(fake.NewSimpleClientset(tt.objs...), 0) - pl := PodTopologySpread{ - sharedLister: cache.NewSnapshot(nil, tt.nodes), - args: config.PodTopologySpreadArgs{ - DefaultConstraints: tt.defaultConstraints, - }, + f, err := frameworkruntime.NewFramework(nil, nil, nil, + frameworkruntime.WithSnapshotSharedLister(cache.NewSnapshot(nil, tt.nodes)), + frameworkruntime.WithInformerFactory(informerFactory)) + if err != nil { + t.Fatalf("Failed creating framework runtime: %v", err) + } + pl, err := New(&tt.config, f) + if err != nil { + t.Fatalf("Failed creating plugin: %v", err) } - pl.setListers(informerFactory) informerFactory.Start(ctx.Done()) informerFactory.WaitForCacheSync(ctx.Done()) + p := pl.(*PodTopologySpread) cs := framework.NewCycleState() - if s := pl.PreScore(context.Background(), cs, tt.pod, tt.nodes); !s.IsSuccess() { + if s := p.PreScore(context.Background(), cs, tt.pod, tt.nodes); !s.IsSuccess() { t.Fatal(s.AsError()) } @@ -765,24 +817,25 @@ func BenchmarkTestDefaultEvenPodsSpreadPriority(b *testing.B) { existingPods, allNodes, filteredNodes := st.MakeNodesAndPodsForEvenPodsSpread(pod.Labels, tt.existingPodsNum, tt.allNodesNum, tt.allNodesNum) state := framework.NewCycleState() snapshot := cache.NewSnapshot(existingPods, allNodes) - p := &PodTopologySpread{ - sharedLister: snapshot, - args: config.PodTopologySpreadArgs{ - DefaultConstraints: []v1.TopologySpreadConstraint{ - {MaxSkew: 1, TopologyKey: v1.LabelHostname, WhenUnsatisfiable: v1.ScheduleAnyway}, - {MaxSkew: 1, TopologyKey: v1.LabelZoneFailureDomain, WhenUnsatisfiable: v1.ScheduleAnyway}, - }, - }, - } client := fake.NewSimpleClientset( &v1.Service{Spec: v1.ServiceSpec{Selector: map[string]string{"foo": ""}}}, ) ctx := context.Background() informerFactory := informers.NewSharedInformerFactory(client, 0) - p.setListers(informerFactory) + f, err := frameworkruntime.NewFramework(nil, nil, nil, + frameworkruntime.WithSnapshotSharedLister(snapshot), + frameworkruntime.WithInformerFactory(informerFactory)) + if err != nil { + b.Fatalf("Failed creating framework runtime: %v", err) + } + pl, err := New(&config.PodTopologySpreadArgs{DefaultingType: config.SystemDefaulting}, f) + if err != nil { + b.Fatalf("Failed creating plugin: %v", err) + } + p := pl.(*PodTopologySpread) + informerFactory.Start(ctx.Done()) informerFactory.WaitForCacheSync(ctx.Done()) - b.ResetTimer() for i := 0; i < b.N; i++ { diff --git a/pkg/scheduler/framework/runtime/framework_test.go b/pkg/scheduler/framework/runtime/framework_test.go index 62ac58a6ac8..cd3783d8cf1 100644 --- a/pkg/scheduler/framework/runtime/framework_test.go +++ b/pkg/scheduler/framework/runtime/framework_test.go @@ -530,7 +530,9 @@ func TestNewFrameworkPluginDefaults(t *testing.T) { "RequestedToCapacityRatio": &config.RequestedToCapacityRatioArgs{ Resources: []config.ResourceSpec{{Name: "cpu", Weight: 1}, {Name: "memory", Weight: 1}}, }, - "PodTopologySpread": &config.PodTopologySpreadArgs{}, + "PodTopologySpread": &config.PodTopologySpreadArgs{ + DefaultingType: config.ListDefaulting, + }, "VolumeBinding": &config.VolumeBindingArgs{ BindTimeoutSeconds: 600, }, @@ -590,7 +592,9 @@ func TestNewFrameworkPluginDefaults(t *testing.T) { "NodeResourcesMostAllocated": &config.NodeResourcesMostAllocatedArgs{ Resources: []config.ResourceSpec{{Name: "resource", Weight: 3}}, }, - "PodTopologySpread": &config.PodTopologySpreadArgs{}, + "PodTopologySpread": &config.PodTopologySpreadArgs{ + DefaultingType: config.ListDefaulting, + }, "RequestedToCapacityRatio": &config.RequestedToCapacityRatioArgs{ Resources: []config.ResourceSpec{{Name: "resource", Weight: 2}}, }, diff --git a/staging/src/k8s.io/kube-scheduler/config/v1beta1/types_pluginargs.go b/staging/src/k8s.io/kube-scheduler/config/v1beta1/types_pluginargs.go index 72088af2050..d9092b8277e 100644 --- a/staging/src/k8s.io/kube-scheduler/config/v1beta1/types_pluginargs.go +++ b/staging/src/k8s.io/kube-scheduler/config/v1beta1/types_pluginargs.go @@ -70,6 +70,17 @@ type NodeResourcesFitArgs struct { IgnoredResourceGroups []string `json:"ignoredResourceGroups,omitempty"` } +// 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. @@ -77,14 +88,26 @@ type PodTopologySpreadArgs struct { metav1.TypeMeta `json:",inline"` // 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". // +optional // +listType=atomic - DefaultConstraints []corev1.TopologySpreadConstraint `json:"defaultConstraints"` + DefaultConstraints []corev1.TopologySpreadConstraint `json:"defaultConstraints,omitempty"` + + // 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 `json:"defaultingType,omitempty"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object