An interface that allows pre-filter plugins to update their pre-calculated status #82912

ahg-g · 2019-09-20T04:46:37Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

This is needed to allow efficient preemption simulations: during preemption, we remove/add pods from each node before running the filter plugins again to evaluate whether removing/ad
ding specific pods will allow the incoming pod to be scheduled on the node. Instead of calling prefilter again, we should allow the plugin to do incremental update to its pre-computed state.

Which issue(s) this PR fixes:
Fixes #82897

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

New APIs to allow adding/removing pods from pre-calculated prefilter state in the scheduling framework

k8s-ci-robot · 2019-09-20T04:48:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g

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~~ [ahg-g]
~~test/integration/scheduler/OWNERS~~ [ahg-g]

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

draveness

/label api-review
/hold feel free to cancel

Did we have a thorough discussion of whether or not exposing these API? Is this a required interface (AddPod, RemovePod) which we must provide to the developer, or just an approach to help with the performance.

Besides, it requires an API review if we change the interface of the scheduler extension point.

k8s-ci-robot · 2019-09-20T09:44:08Z

@draveness: The label(s) /label api-review cannot be applied. These labels are supported: api-review, community/discussion, community/maintenance, community/question, cuj/build-train-deploy, cuj/multi-user, platform/aws, platform/azure, platform/gcp, platform/minikube, platform/other

In response to this:

/label api-review
/hold

Did we have a thorough discussion of whether or not exposing these API? Is this a required interface (AddPod, RemovePod) which we must provide to the developer, or just an approach to help with the performance.

Besides, it requires an API review if we change the interface of the scheduler extension point.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2019-09-20T09:47:52Z

@draveness: The label(s) /label api-review cannot be applied. These labels are supported: api-review, community/discussion, community/maintenance, community/question, cuj/build-train-deploy, cuj/multi-user, platform/aws, platform/azure, platform/gcp, platform/minikube, platform/other

In response to this:

/label api-review
/hold feel free to cancel

Did we have a thorough discussion of whether or not exposing these API? Is this a required interface (AddPod, RemovePod) which we must provide to the developer, or just an approach to help with the performance.

Besides, it requires an API review if we change the interface of the scheduler extension point.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ahg-g · 2019-09-20T11:12:58Z

/label api-review
/hold feel free to cancel

Did we have a thorough discussion of whether or not exposing these API? Is this a required interface (AddPod, RemovePod) which we must provide to the developer, or just an approach to help with the performance.

Certainly, I created the issue to discuss the motivation. Will keep the hold until the community is comfortable with this addition.

Besides, it requires an API review if we change the interface of the scheduler extension point.

This particular change does not actually.

ahg-g · 2019-09-20T15:43:16Z

/cc @alculquicondor

ahg-g · 2019-09-20T15:51:46Z

/test pull-kubernetes-bazel-build

Huang-Wei

Some comments below.

Huang-Wei · 2019-09-20T22:16:23Z

pkg/scheduler/framework/v1alpha1/framework.go

+	pluginNameToWeightMap      map[string]int
+	queueSortPlugins           []QueueSortPlugin
+	preFilterPlugins           []PreFilterPlugin
+	preFilterWithUpdatePlugins []PreFilterWithUpdatePlugin


I'm not sure I like the idea of separating the two plugins. In a user's perspective, what they know is just a phase called "PreFilter" which "PreFilterPlugins" should be responsible for. IMO it's better to keep update related logic into another interface.

I'd propose to keep preFilterPlugins only, and then add a MetaUpdater() method in PreFilterPlugin like this:

type PreFilterPlugin interface { Plugin PreFilter(pc *PluginContext, p *v1.Pod) *Status MetaUpdater() MetaUpdater }

And MetaUpdater is an interface with AddPod() and RemovePod():

type MetaUpdater interface { AddPod(pc *PluginContext, podToSchedule *v1.Pod, podToAdd *v1.Pod, nodeInfo *schedulernodeinfo.NodeInfo) *Status RemovePod(pc *PluginContext, podToSchedule *v1.Pod, podToRemove *v1.Pod, nodeInfo *schedulernodeinfo.NodeInfo) *Status }

So RunPreFilter[Add|Remove]Pod method can be updated:

func (f *framework) RunPreFilterAddPod(pc *PluginContext, podToSchedule *v1.Pod, podToAdd *v1.Pod, nodeInfo *schedulernodeinfo.NodeInfo) *Status { for _, pl := range f.preFilterPlugins { status := pl.MetaUpdater().AddPod(pc, podToSchedule, podToAdd, nodeInfo)

The idea of the separate interfaces is that the extra methods are optional. And then we only iterate once to identify the plugins that implement the interface.
These private slices are not user (developer) facing.
And we have done the same for scorePlugins and scoreWithNormalizePlugins

Right, the idea is to make the methods optional, those slices are implementation details only.

Sry to confuse you. I should have commented it on the interface definitions.

So basically, instead of defining two interfaces (PreFilterPlugin and PreFilterWithUpdatePlugin), I'm leaning towards to keep one interface PreFilterPlugin.

With the above extra interface funtion MetaUpdater(), implementations that returns nil will get simply bypassed without invoking its AddPod() and RemovePod() upon preemption. Right now, we're relying on typecasting to tell whether it's a PreFilterPlugin or PreFilterWithUpdatePlugin implementation, which I'm not sure I like the way.

The above suggestion is similar like what we did in GVK:

kubernetes/staging/src/k8s.io/apimachinery/pkg/runtime/schema/interfaces.go

Line 28 in d62ebe3

GroupVersionKind() GroupVersionKind

Basically an API object can choose not to omit GVK by implementing GroupVersionKind() to simply return nil, so that API machinery won't call further GVK related methods upon this object.

this sounds good to me, it will also solve the confusion when a plugin implements only AddPod but not RemovePod for example, which will result in a failure in the type cast, and so AddPod will not be called contrary to what the developer may think.

Would it be worth making the optional normalize for score plugins consistent with this suggestion?

I think so, but we can have it in another PR.

Done. I named the interface "Updater" only, I think it is sufficient.

With the above extra interface funtion MetaUpdater(), implementations that returns nil will get simply bypassed without invoking its AddPod() and RemovePod() upon preemption. Right now, we're relying on typecasting to tell whether it's a PreFilterPlugin or PreFilterWithUpdatePlugin implementation, which I'm not sure I like the way.

I feel the same way, it's better to implement it as a noop. I sent #83042 to remove the scoreWithNormalize interface.

pkg/scheduler/framework/v1alpha1/framework.go

Huang-Wei · 2019-09-20T22:55:22Z

pkg/scheduler/framework/v1alpha1/framework.go

+// RunPreFilterAddPod calls the AddPod interface for the set of configured
+// PreFilter plugins. It returns directly if any of the plugins return any
+// status other than Success.
+func (f *framework) RunPreFilterAddPod(pc *PluginContext, podToSchedule *v1.Pod,


RunMetaAddPod sounds more clear to me so that it's possible in the future to enable other plugins to be called under the hood.

I don't think we make this generic, it should be specific to prefilter (i.e., the updater in the prefilter plugins). This is internal implementation in any case.

Huang-Wei · 2019-09-24T04:33:09Z

/retest

Huang-Wei · 2019-09-24T04:34:02Z

/lgtm

Thanks @ahg-g !

Huang-Wei · 2019-09-24T06:36:21Z

/retest

ahg-g · 2019-09-24T12:08:23Z

/lgtm

Thanks @ahg-g !

Thanks, now that the context PR is in, I added logic to clone the context during preemption, PTAL at the last commit, if all looks good I will squash.

alculquicondor · 2019-09-24T14:14:25Z

pkg/scheduler/core/generic_scheduler.go

@@ -1023,7 +1023,9 @@ func (g *genericScheduler) selectNodesForPreemption(
 		if meta != nil {
 			metaCopy = meta.ShallowCopy()
 		}
-		pods, numPDBViolations, fits := g.selectVictimsOnNode(pluginContext, pod, metaCopy, nodeNameToInfo[nodeName], fitPredicates, queue, pdbs)
+		pluginContextClone := pluginContext.Clone()


why not do the copy in selectVictimsNode, given that pluginContextClone isn't used outside of it?

Also, prefer shorter names for local variables. We could start using pCtx instead of pluginContext

why not do the copy in selectVictimsNode, given that pluginContextClone isn't used outside of it?

To be consistent with how "meta" is treated.

Also, prefer shorter names for local variables. We could start using pCtx instead of pluginContext

we already use pluginContext everywhere.

sg, they are just nits.

Huang-Wei · 2019-09-24T16:31:53Z

pkg/scheduler/framework/v1alpha1/context_test.go

@@ -64,3 +64,11 @@ func TestPluginContextClone(t *testing.T) {
 		t.Errorf("cloned copy should not change, got %q, expected %q", v.data, data1)
 	}
 }
+
+func TestPluginContextCloneNil(t *testing.T) {


Ideally, this can be merged into TestPluginContextClone in a Golang subtest manner.

I feel the boilerplate for doing that will outweigh its benefit.

Sure, it's just nits.

Huang-Wei · 2019-09-24T16:38:34Z

Thanks, now that the context PR is in, I added logic to clone the context during preemption, PTAL at the last commit, if all looks good I will squash.

Please go squashing the commits.

ahg-g · 2019-09-24T16:47:47Z

Thanks, now that the context PR is in, I added logic to clone the context during preemption, PTAL at the last commit, if all looks good I will squash.

Please go squashing the commits.

Done.

…lated. This is needed to allow efficient preemption simulations: during preemption, we remove/add pods from each node before running the filter plugins again to evaluate whether removing/adding specific pods will allow the incoming pod to be scheduled on the node. Instead of calling prefilter again, we should allow the plugin to do incremental update to its pre-computed state.

Huang-Wei · 2019-09-24T17:05:29Z

/lgtm

ahg-g · 2019-09-24T18:19:46Z

/test pull-kubernetes-e2e-gce-device-plugin-gpu

Huang-Wei · 2019-09-24T21:19:42Z

/hold cancel

tedyu · 2019-09-24T23:43:45Z

pkg/scheduler/framework/v1alpha1/framework.go

+	podToRemove *v1.Pod, nodeInfo *schedulernodeinfo.NodeInfo) *Status {
+	for _, pl := range f.preFilterPlugins {
+		if updater := pl.Updater(); updater != nil {
+			status := updater.RemovePod(pc, podToSchedule, podToRemove, nodeInfo)


Since this has same signature as that of updater.AddPod , it seems this func (RunPreFilterUpdaterRemovePod) can be unified with RunPreFilterUpdaterAddPod by passing AddPod / RemovePod as parameter.

ahg-g · 2019-09-25T05:36:12Z

/priority important-soon

k8s-ci-robot requested review from Huang-Wei and wgliang September 20, 2019 04:48

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 20, 2019

ahg-g force-pushed the ahg-prefilter-update branch 2 times, most recently from efc0728 to e5ad022 Compare September 20, 2019 05:15

draveness reviewed Sep 20, 2019

View reviewed changes

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

k8s-ci-robot requested a review from alculquicondor September 20, 2019 15:43

ahg-g force-pushed the ahg-prefilter-update branch from e5ad022 to b5be2b8 Compare September 20, 2019 18:05

Huang-Wei reviewed Sep 20, 2019

View reviewed changes

ahg-g force-pushed the ahg-prefilter-update branch from b5be2b8 to 65974b7 Compare September 24, 2019 02:48

draveness mentioned this pull request Sep 24, 2019

feat(scheduler): consolidate ScoreWithNormalizePlugin into ScorePlugin #83042

Merged

k8s-ci-robot assigned Huang-Wei Sep 24, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 24, 2019

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 24, 2019

ahg-g force-pushed the ahg-prefilter-update branch 3 times, most recently from 5859dc4 to afdb7f9 Compare September 24, 2019 12:06

alculquicondor reviewed Sep 24, 2019

View reviewed changes

Huang-Wei reviewed Sep 24, 2019

View reviewed changes

ahg-g force-pushed the ahg-prefilter-update branch from afdb7f9 to 8063ab3 Compare September 24, 2019 16:39

ahg-g force-pushed the ahg-prefilter-update branch from 8063ab3 to 37b9e6d Compare September 24, 2019 17:02

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 24, 2019

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

tedyu reviewed Sep 24, 2019

View reviewed changes

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 Sep 25, 2019

k8s-ci-robot merged commit 0b4cccc into kubernetes:master Sep 25, 2019

k8s-ci-robot added this to the v1.17 milestone Sep 25, 2019

ahg-g deleted the ahg-prefilter-update branch January 10, 2020 15:38

An interface that allows pre-filter plugins to update their pre-calculated status #82912

An interface that allows pre-filter plugins to update their pre-calculated status #82912

Conversation

ahg-g commented Sep 20, 2019

k8s-ci-robot commented Sep 20, 2019

draveness left a comment • edited

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 20, 2019

k8s-ci-robot commented Sep 20, 2019

ahg-g commented Sep 20, 2019 • edited

ahg-g commented Sep 20, 2019

ahg-g commented Sep 20, 2019

Huang-Wei 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

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

Huang-Wei commented Sep 24, 2019

Huang-Wei commented Sep 24, 2019

Huang-Wei commented Sep 24, 2019

ahg-g commented Sep 24, 2019

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

ahg-g Sep 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Huang-Wei commented Sep 24, 2019

ahg-g commented Sep 24, 2019

Huang-Wei commented Sep 24, 2019

ahg-g commented Sep 24, 2019

Huang-Wei commented Sep 24, 2019

Choose a reason for hiding this comment

ahg-g commented Sep 25, 2019

draveness left a comment •

edited

ahg-g commented Sep 20, 2019 •

edited

ahg-g Sep 24, 2019 •

edited