Add DefaultingType to PodTopologySpreadArgs #95048

alculquicondor · 2020-09-24T22:25:55Z

/sig scheduling

What type of PR is this?

/kind feature

What this PR does / why we need it:

This new enum (System, List) allows to distinguish between k8s defined and operator provided defaultConstraints for PodTopologySpread plugin.

Which issue(s) this PR fixes:

Ref #94863

Special notes for your reviewer:

This is compliant with best API practices of not distinguishing between nil and [].
The system default constant was moved to the plugin, as per API reviewer suggestion.
For backwards compatibility, the .defaultingType is set to List when the .defaultConstraints has a non empty value. This will be removed in component config v1beta2 Deprecate NodeLabel, ServiceAffinityand NodePreferAvoidPods from scheduler's CC api #94008.

Follow up to #94864

Does this PR introduce a user-facing change?:

New parameter `defaultingType` for `PodTopologySpread` plugin allows to use k8s defined or user provided default constraints

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://git.k8s.io/enhancements/keps/sig-scheduling/1258-default-pod-topology-spread
- [Usage]: https://kubernetes.io/docs/concepts/workloads/pods/pod-topology-spread-constraints/#cluster-level-default-constraints

alculquicondor · 2020-09-24T22:27:26Z

/assign @adtac

fejta-bot · 2020-09-24T22:46:06Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

Huang-Wei

A nit. LGTM otherwise.

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

adtac

minor nit, LGTM otherwise

pkg/scheduler/apis/config/types_pluginargs.go

staging/src/k8s.io/kube-scheduler/config/v1beta1/types_pluginargs.go

chendave · 2020-09-27T03:13:23Z

LGTM, seem like this change still needs review from API owner.

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

alculquicondor · 2020-09-28T19:05:19Z

/assign @liggitt
for API review

chendave · 2020-09-29T01:27:47Z

/retest

alculquicondor · 2020-09-30T17:45:24Z

/unassign @liggitt

/assign @lavalamp

for API review

lavalamp · 2020-10-06T20:27:50Z

staging/src/k8s.io/kube-scheduler/config/v1beta1/types_pluginargs.go

+	// `.defaultConstraints[*].labelSelectors` must be empty, as they are
+	// deduced from the Pod's membership to Services, ReplicationControllers,
+	// ReplicaSets or StatefulSets.
+	// If empty, the default constraints prefer to spread Pods across Nodes and Zones


And if you don't want that behavior either, you use the next field? (add comment)

lavalamp · 2020-10-06T20:28:07Z

staging/src/k8s.io/kube-scheduler/config/v1beta1/types_pluginargs.go

+	// Defaults to false. When set to true, DefaultConstraints must be empty
+	// or nil.
+	// +optional
+	DisableDefaultConstraints bool `json:"disableDefaultConstraints,omitempty"`


Can you flip the polarity, e.g. UseDefaultConstraints defaulted true?

After reading your implementation, I think if we were starting from scratch, you'd actually want a union/one-of?

E.g. the discriminator (this field) is "DefaultingType" and can be one of "SystemDefault", "List", or "None". The DefaultConstraints list may only be set if this field is "List".

Since there's existing files we want to not break (right?), I think we can default this discriminator to "List" if the list has items in it, or to "SystemDefault" otherwise. I hate to do this but I think it's OK since it's for backwards compatibility, and this is just a config file.

I like DefaultingType. Is this just a regular enum that you called union because of its interpretation?

Since there's existing files we want to not break (right?), I think we can default this discriminator to "List" if the list has items in it, or to "SystemDefault" otherwise.

It's alpha, so I assume it's fine to break (with the appropriate release note). However, is it normal to have to be this verbose?

Yeah, we're eventually going to get unions as first-class citizens of the API.

It's a little verbose but it makes it very clear what you want the system to do.

Actually, isn't "None" unnecessary? "List" + empty slice should be enough.

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

lavalamp · 2020-10-06T20:38:08Z

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

@@ -200,7 +200,7 @@ func SetDefaults_PodTopologySpreadArgs(obj *v1beta1.PodTopologySpreadArgs) {
 		// SelectorSpread plugin.
 		return
 	}
-	if obj.DefaultConstraints == nil {
+	if len(obj.DefaultConstraints) == 0 && !obj.DisableDefaultConstraints {


I guess people were providing an empty list to avoid getting the default? Yeah, that's bad.

This conditional defaulting is going to give you other problems though.

I actually recommend deleting this defaulting completely and making the consumer of the config do this logic.

In particular I think you should avoid mixing system defaults and user provided defaults in a single field in this layer.

I guess people were providing an empty list to avoid getting the default? Yeah, that's bad.

Yeah, my bad. OTOH, the system defaulting is an alpha feature.

I actually recommend deleting this defaulting completely and making the consumer of the config do this logic.

Even though this code is within pkg/scheduler, i.e. the consumer?

This conditional defaulting is going to give you other problems though.

Could you explain?

Also, having this in defaulting makes it easier to share what configuration is the plugin actually using, once we resolve #93718

Even though this code is within pkg/scheduler, i.e. the consumer?

The defaulting function is called during deserialization, which means original user intent is gone by the time the eventual actual consumer gets the go structures.

This conditional defaulting is going to give you other problems though.

Could you explain?

We're adding support for static defaulting, but it won't be able to replace conditional defaulting which depends on multiple fields. I don't know if I can make a great argument for this off the top of my head, but we've found that complicated (e.g. conditional or multi-field) defaulting logic is almost always regretted later.

It's less important because this is a config file, but I'd still try to avoid it.

In my mind there's two concepts that are mixed here, "what value does this field have if it's not specified", and "what does the application do given certain input". This distinction isn't imposed on us by the laws of the universe, but imposing it on ourselves makes the codebase easier to understand, e.g. less spooky action at a distance.

If the system were not ever going to evolve then I'd be fine with the first version of this I saw. But given that it will evolve, I think it's easier to evolve this logic when it's in the application layer and not the defaulting (deserialization) layer.

I hope that makes sense, sorry :)

easier to share what configuration is the plugin actually using

I'm not sure how? It sounds like stuff that'd go in a "status" section except we don't have those for config files.

alculquicondor · 2020-10-06T23:20:41Z

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

-		// SelectorSpread plugin.
+	if feature.DefaultFeatureGate.Enabled(features.DefaultPodTopologySpread) {
+		if obj.DefaultingType != "" {
+			obj.DefaultingType = v1beta1.SystemDefaulting


@lavalamp help me out here.

Even though "system defaulting" is alpha (graduating to beta in 1.20), "list defaulting", i.e. the slice, is a beta API.
Once we flip the feature gate, users of "list defaulting" would break because of this line.

Should I still have the "if slice is not empty, set to ListDefaulting" marked for removal in v1beta2, which we are indeed planing.

We shouldn't do something that would cause a beta feature to change behavior. It's OK for the next API version to do something different, but an existing config file shouldn't change behavior, I think. Does that help?

alculquicondor · 2020-10-06T23:23:36Z

staging/src/k8s.io/kube-scheduler/config/v1beta1/types_pluginargs.go

+	// Defaults to false. When set to true, DefaultConstraints must be empty
+	// or nil.
+	// +optional
+	DisableDefaultConstraints bool `json:"disableDefaultConstraints,omitempty"`


Actually, isn't "None" unnecessary? "List" + empty slice should be enough.

alculquicondor · 2020-10-08T14:19:42Z

@lavalamp ready for API review again :)

adtac

just a couple of minor nits

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

pkg/scheduler/framework/plugins/podtopologyspread/plugin.go

alculquicondor · 2020-10-08T19:45:45Z

/retest

Huang-Wei · 2020-10-08T22:04:45Z

/lgtm

alculquicondor · 2020-10-09T20:25:48Z

Ping @lavalamp

lavalamp · 2020-10-09T20:43:13Z

staging/src/k8s.io/kube-scheduler/config/v1beta1/types_pluginargs.go

+	// 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.


Mention that this only applies if .defaultingType is "List".

lavalamp · 2020-10-09T20:43:34Z

staging/src/k8s.io/kube-scheduler/config/v1beta1/types_pluginargs.go

+	// - "System": Use kubernetes defined constraints that spread Pods among
+	//   Nodes and Zones.
+	// - "List": Use constraints defined in .defaultConstraints.
+	// +optional


Mention the default of this field?

It depends on the feature gate. I guess that's fair to mention until it gets deleted?

Yeah, I think so.

lavalamp · 2020-10-09T20:44:29Z

/approve

... and so you can fix the comments:
/hold

k8s-ci-robot · 2020-10-09T20:47:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, lavalamp

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/scheduler/OWNERS~~ [alculquicondor,lavalamp]
~~staging/src/k8s.io/kube-scheduler/config/OWNERS~~ [lavalamp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Change-Id: Ibf6a4fdb39a31fe9deed68de7e7cb24a9bf9d06a

alculquicondor · 2020-10-09T21:13:03Z

/hold cancel

Huang-Wei · 2020-10-09T21:39:22Z

/lgtm

fejta-bot · 2020-10-10T00:38:32Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot requested review from ahg-g and chendave September 24, 2020 22:26

k8s-ci-robot assigned adtac Sep 24, 2020

Huang-Wei reviewed Sep 24, 2020

View reviewed changes

pkg/scheduler/apis/config/validation/validation_pluginargs.go Outdated Show resolved Hide resolved

adtac reviewed Sep 24, 2020

View reviewed changes

pkg/scheduler/apis/config/types_pluginargs.go Outdated Show resolved Hide resolved

chendave reviewed Sep 25, 2020

View reviewed changes

staging/src/k8s.io/kube-scheduler/config/v1beta1/types_pluginargs.go Show resolved Hide resolved

alculquicondor force-pushed the disable-default-spread branch from 0268d91 to bf8a9fc Compare September 25, 2020 14:49

soulxu reviewed Sep 28, 2020

View reviewed changes

pkg/scheduler/apis/config/validation/validation_pluginargs.go Show resolved Hide resolved

alculquicondor force-pushed the disable-default-spread branch from bf8a9fc to 71cb85d Compare September 28, 2020 19:04

k8s-ci-robot assigned liggitt Sep 28, 2020

k8s-ci-robot assigned lavalamp and unassigned liggitt Sep 30, 2020

alculquicondor force-pushed the disable-default-spread branch from 71cb85d to 6b7f7a1 Compare October 6, 2020 14:04

lavalamp reviewed Oct 6, 2020

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 6, 2020

alculquicondor commented Oct 6, 2020

View reviewed changes

alculquicondor force-pushed the disable-default-spread branch from beab504 to de20874 Compare October 8, 2020 14:14

adtac reviewed Oct 8, 2020

View reviewed changes

pkg/scheduler/apis/config/validation/validation_pluginargs.go Outdated Show resolved Hide resolved

pkg/scheduler/framework/plugins/podtopologyspread/plugin.go Show resolved Hide resolved

k8s-ci-robot assigned Huang-Wei Oct 8, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 8, 2020

This was referenced Oct 9, 2020

Umbrella: scheduler component config v1beta2 #95446

Closed

Map SelectorSpreadPriority to PodTopologySpread plugin #95448

Merged

Add v1beta2 for kube-scheduler #95308

Closed

lavalamp reviewed Oct 9, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 9, 2020

Add defaultingType to PodTopologySpreadArgs

c8a0b9e

Change-Id: Ibf6a4fdb39a31fe9deed68de7e7cb24a9bf9d06a

alculquicondor force-pushed the disable-default-spread branch from f9407a6 to c8a0b9e Compare October 9, 2020 21:12

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 9, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 9, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 9, 2020

k8s-ci-robot merged commit 78ece54 into kubernetes:master Oct 10, 2020

k8s-ci-robot added this to the v1.20 milestone Oct 10, 2020

alculquicondor mentioned this pull request Nov 30, 2020

Set schedulers default plugins via component config defaulting #92037

Closed

Huang-Wei mentioned this pull request Dec 3, 2020

Implement resource pre-check kubernetes-sigs/scheduler-plugins#109

Merged

adtac mentioned this pull request Feb 16, 2021

nominate adtac to sig-scheduling reviewers #99117

Merged

adtac mentioned this pull request May 4, 2021

scheduler CC: add v1beta2 API, deprecate plugins #99597

Merged

6 tasks

Add DefaultingType to PodTopologySpreadArgs #95048

Add DefaultingType to PodTopologySpreadArgs #95048

Conversation

alculquicondor commented Sep 24, 2020 • edited Loading

alculquicondor commented Sep 24, 2020

fejta-bot commented Sep 24, 2020

Huang-Wei left a comment

Choose a reason for hiding this comment

adtac left a comment

Choose a reason for hiding this comment

chendave commented Sep 27, 2020

alculquicondor commented Sep 28, 2020

chendave commented Sep 29, 2020

alculquicondor commented Sep 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented Oct 8, 2020

adtac left a comment

Choose a reason for hiding this comment

alculquicondor commented Oct 8, 2020

Huang-Wei commented Oct 8, 2020

alculquicondor commented Oct 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Oct 9, 2020

k8s-ci-robot commented Oct 9, 2020

alculquicondor commented Oct 9, 2020

Huang-Wei commented Oct 9, 2020

fejta-bot commented Oct 10, 2020

alculquicondor commented Sep 24, 2020 •

edited

Loading