Add AddedAffinity to NodeAffinity Filter and Score plugin #96202

alculquicondor · 2020-11-03T23:40:33Z

What type of PR is this?

/kind feature

/kind api-change

What this PR does / why we need it:

/sig scheduling

Add the AddedAffinity to the NodeAffinityArgs and use it in Filter and Score.

This can be used to replace legacy NodeLabel plugin with a more rich API that is consistent with PodSpec.
It can be useful to associate a scheduling profile with a set of Nodes.

Which issue(s) this PR fixes:

Fixes #95738

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NodeAffinity plugin can be configured with AddedAffinity.

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

- [Usage]: https://kubernetes.io/docs/reference/scheduling/config/
- [KEP]: https://git.k8s.io/enhancements/keps/sig-scheduling/785-scheduler-component-config-api

alculquicondor · 2020-11-03T23:40:56Z

/triage accepted
/assign @damemi

fejta-bot · 2020-11-04T01:24:49Z

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.

dims · 2020-11-04T13:25:54Z

@alculquicondor can you please see the https://github.com/kubernetes/community/blob/master/sig-architecture/api-review-process.md#mechanics and request an API review?

pkg/scheduler/framework/plugins/helper/node_affinity.go

alculquicondor · 2020-11-04T14:54:31Z

/hold

I'm looking at the integration test failure.

alculquicondor · 2020-11-04T15:12:08Z

/hold cancel

Just needed to update the error message in the integration test check.

/label api-review

soulxu · 2020-11-05T10:18:57Z

pkg/scheduler/apis/config/types_pluginargs.go

+
+	// DefaultAffinity is applied to all Pods additionally to the NodeAffinity
+	// specified in the PodSpec. That is, Nodes need to satisfy DefaultAffinity
+	// AND .spec.NodeAffinity. DefaultAffinity is empty by default (all Nodes


Already using default as the value when pod doesn't specify one somewhere, I guess most people will think about that semantic in the beginning. Maybe call it 'AdditionalNodeAffinity', then can help people to aware that is different semantic.

That's fair. Although I find Additional confusing. What about ImplicitAffinity? @Huang-Wei @ahg-g any other suggestions?

So if a pod doesn't specify any affinity, we have a default affinity that's applied to it? If this DefaultAffinity is set, does that override the other default affinity? If so, then I think "Default" is fine

No, that's not the behavior. It's AND semantics.

Ahh, then yeah something besides Default is a good idea

Not an expert on naming... But yes, "default" is not concise.

Another option: somehow "SupplementalAffinity" jumps out of my mind :)

AppendedAffinity/Constraints

I am inclined to use additional/added/supplemental adjective

I am inclined to use additional/added/supplemental adjective

agree those are better than implicit/default

Went with AddedAffinity

soulxu · 2020-11-06T02:45:48Z

pkg/scheduler/framework/plugins/nodeaffinity/node_affinity.go

@@ -75,6 +88,9 @@ func (pl *NodeAffinity) Score(ctx context.Context, state *framework.CycleState,
 	affinity := pod.Spec.Affinity

 	var count int64
+	if pl.defPrefSchedTerms != nil {
+		count += pl.defPrefSchedTerms.Score(node)
+	}


If the pod specified a same prefered affinity with default one, then that will be double count. For example, the default one is 'in zoneA with weight=1', the pod specified one is 'in zoneA with weight=1 and in zoneB with wieght=2'. In the end, actually zoneA and zoneB have the same weight. That changed the original intent for the user. But this is indeed the AND behavior as the design, just thinking about whether this makes sense for the user. Although I see the major use-case for this feature is required affinity, so I may over thing about this.

You are correct, and I think that is working as intended.

However, in "AddedAffinity", you would probably use a label that Pods usually don't care about. I'll try to put some thoughts in kubernetes/website#24914

liggitt

mechanics lgtm, just a couple comments on names/comments

liggitt · 2020-11-06T02:52:28Z

pkg/scheduler/apis/config/types_pluginargs.go

+
+	// DefaultAffinity is applied to all Pods additionally to the NodeAffinity
+	// specified in the PodSpec. That is, Nodes need to satisfy DefaultAffinity
+	// AND .spec.NodeAffinity. DefaultAffinity is empty by default (all Nodes


I am inclined to use additional/added/supplemental adjective

agree those are better than implicit/default

liggitt · 2020-11-06T02:53:06Z

pkg/scheduler/apis/config/types_pluginargs.go

+	// Daemonset Pods might remain unschedulable in some Nodes when this option
+	// is used.


generalize this to "pods with affinity requirements that match specific nodes (such as daemonset pods)..."

comments also apply to the versioned types.go

pkg/scheduler/framework/plugins/nodeaffinity/node_affinity.go

soulxu

Only found some place forget to rename

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

pkg/scheduler/framework/plugins/nodeaffinity/node_affinity.go

pkg/scheduler/framework/plugins/nodeaffinity/node_affinity_test.go

alculquicondor · 2020-11-09T15:46:59Z

Only found some place forget to rename

Done

alculquicondor · 2020-11-09T16:44:12Z

/priority important-soon

liggitt · 2020-11-09T20:13:02Z

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

+			allErrs = append(allErrs, field.Invalid(f.Child("preferredDuringSchedulingIgnoredDuringExecution"), affinity.PreferredDuringSchedulingIgnoredDuringExecution, err.Error()))
+		}
+	}
+	return allErrs.ToAggregate()


add a TODO to validate RequiredDuringSchedulingRequiredDuringExecution once implemented

liggitt · 2020-11-09T20:14:09Z

/approve
API changes look good, I didn't review the scheduler changes

one TODO requested, will leave lgtm to scheduler reviewers

And use it in Filter and Score. Change-Id: I173d8f2d5578762e9873181d5b44ea30b6dbbbc2

Huang-Wei

Just a nit/question. LGTM otherwise.

Huang-Wei · 2020-11-09T20:35:44Z

pkg/scheduler/framework/plugins/nodeaffinity/node_affinity.go

+	if pl.addedNodeSelector != nil && !pl.addedNodeSelector.Match(node) {
+		return framework.NewStatus(framework.UnschedulableAndUnresolvable, errReasonEnforced)
+	}
 	if !pluginhelper.PodMatchesNodeSelectorAndAffinityTerms(pod, node) {
-		return framework.NewStatus(framework.UnschedulableAndUnresolvable, ErrReason)
+		return framework.NewStatus(framework.UnschedulableAndUnresolvable, ErrReasonPod)
 	}
 	return nil


Nit: is it more efficient to do a "smart merge" on addedAffinity and user-specified ones? so that we only run "match logic" once. For example, "foo in [bar]" and "foo in [bar]" gets de-duplicated to "foo in [bar]"; while "foo in [bar]" and "foo in [baz]" gets merged to one entry: "foo in [bar, baz]".

I don't think it's worth.
See my documentation PR https://github.com/kubernetes/website/pull/24914/files#diff-ba98b126fe5dd6bf0570b39d4928eb9143d1f2ed60fbaf9fa2051810acc2e8a6R192

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

/retest

damemi

/lgtm

k8s-ci-robot · 2020-11-09T22:09:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, damemi, liggitt

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/kubelet/OWNERS~~ [liggitt]
~~pkg/scheduler/OWNERS~~ [alculquicondor,liggitt]
~~staging/src/k8s.io/kube-scheduler/config/OWNERS~~ [liggitt]
~~test/OWNERS~~ [liggitt]

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

k8s-ci-robot assigned damemi Nov 3, 2020

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 3, 2020

k8s-ci-robot requested review from dims and dixudx November 3, 2020 23:41

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. labels Nov 3, 2020

alculquicondor force-pushed the default-node-affinity branch from 6c935d6 to 62f10b8 Compare November 4, 2020 00:40

dims reviewed Nov 4, 2020

View reviewed changes

pkg/scheduler/framework/plugins/helper/node_affinity.go Show resolved Hide resolved

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

alculquicondor mentioned this pull request Nov 4, 2020

Parse Pod's Node affinity once in PreFilter and PreScore #96164

Closed

alculquicondor force-pushed the default-node-affinity branch from 62f10b8 to 44ee4e8 Compare November 4, 2020 15:11

k8s-ci-robot added api-review Categorizes an issue or PR as actively needing an API review. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 4, 2020

SataQiu mentioned this pull request Nov 4, 2020

scheduler: parse Pod's Node affinity once in PreScore phase #96223

Merged

soulxu reviewed Nov 5, 2020

View reviewed changes

alculquicondor changed the title ~~Add DefaultAffinity to NodeAffinity Filter plugin~~ Add DefaultAffinity to NodeAffinity Filter and Score plugin Nov 5, 2020

alculquicondor mentioned this pull request Nov 5, 2020

Add usage for per-profile node affinity kubernetes/website#24914

Merged

soulxu reviewed Nov 6, 2020

View reviewed changes

liggitt reviewed Nov 6, 2020

View reviewed changes

alculquicondor force-pushed the default-node-affinity branch from af788d7 to e5d21dd Compare November 6, 2020 16:27

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2020

alculquicondor changed the title ~~Add DefaultAffinity to NodeAffinity Filter and Score plugin~~ Add AddedAffinity to NodeAffinity Filter and Score plugin Nov 6, 2020

alculquicondor force-pushed the default-node-affinity branch from e5d21dd to cfcff7e Compare November 6, 2020 16:29

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2020

soulxu reviewed Nov 7, 2020

View reviewed changes

alculquicondor force-pushed the default-node-affinity branch from cfcff7e to 4b16281 Compare November 9, 2020 15:42

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 9, 2020

liggitt reviewed Nov 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 Nov 9, 2020

Add AddedAffinity to the NodeAffinityArgs

3ce1457

And use it in Filter and Score. Change-Id: I173d8f2d5578762e9873181d5b44ea30b6dbbbc2

alculquicondor force-pushed the default-node-affinity branch from 4b16281 to 3ce1457 Compare November 9, 2020 20:19

Huang-Wei reviewed Nov 9, 2020

View reviewed changes

damemi approved these changes Nov 9, 2020

View reviewed changes

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

k8s-ci-robot merged commit e5194dd into kubernetes:master Nov 9, 2020

k8s-ci-robot added this to the v1.20 milestone Nov 9, 2020

github-actions bot mentioned this pull request Nov 18, 2020

Week Ending November 15, 2020 dev-obs/actus#273

Open

Huang-Wei mentioned this pull request Dec 3, 2020

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

Merged

		// Daemonset Pods might remain unschedulable in some Nodes when this option
		// is used.

Add AddedAffinity to NodeAffinity Filter and Score plugin #96202

Add AddedAffinity to NodeAffinity Filter and Score plugin #96202

Conversation

alculquicondor commented Nov 3, 2020 • edited Loading

alculquicondor commented Nov 3, 2020

fejta-bot commented Nov 4, 2020

dims commented Nov 4, 2020

alculquicondor commented Nov 4, 2020

alculquicondor commented Nov 4, 2020

Choose a reason for hiding this comment

alculquicondor Nov 5, 2020 • edited Loading

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

liggitt left a 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

soulxu left a comment

Choose a reason for hiding this comment

alculquicondor commented Nov 9, 2020

alculquicondor commented Nov 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Nov 9, 2020

Huang-Wei left a comment

Choose a reason for hiding this comment

Huang-Wei Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented Nov 9, 2020

damemi left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 9, 2020

alculquicondor commented Nov 3, 2020 •

edited

Loading

alculquicondor Nov 5, 2020 •

edited

Loading

Huang-Wei Nov 9, 2020 •

edited

Loading