Eliminate running paths of Predicates in scheduler #86133

Huang-Wei · 2019-12-10T18:28:14Z

What type of PR is this?

/kind cleanup
/sig scheduling

What this PR does / why we need it:

eliminate running paths of Predicates in scheduler; use Filter Plugins instead.
refactor all unit tests

Which issue(s) this PR fixes:

Part of #85918.

Special notes for your reviewer:

Some TODOs will be covered in the next PR.

Does this PR introduce a user-facing change?:

NONE

/cc @ahg-g @alculquicondor @damemi @ravisantoshgudimetla @draveness

ahg-g

This is great, thanks! a couple of nits.

ahg-g · 2019-12-10T18:56:47Z

pkg/scheduler/core/generic_scheduler.go

@@ -555,6 +554,7 @@ func (g *genericScheduler) findNodesThatFit(ctx context.Context, state *framewor
 			if !extender.IsInterested(pod) {
 				continue
 			}
+			// TODO(Huang-Wei): refactor the signature of extender Filter() and have it UTed.


why do we need to change the signature of filter?

It's extender's Filter(), not the generic framework.Filter():

kubernetes/pkg/scheduler/algorithm/scheduler_interface.go

Lines 35 to 37 in a8caae5

Filter(pod *v1.Pod,

nodes []*v1.Node, nodeNameToInfo map[string]*schedulernodeinfo.NodeInfo,

) (filteredNodes []*v1.Node, failedNodesMap extenderv1.FailedNodesMap, err error)

It should be adherent with:

kubernetes/pkg/scheduler/framework/v1alpha1/interface.go

Line 261 in a8caae5

Filter(ctx context.Context, state *CycleState, pod *v1.Pod, nodeInfo *schedulernodeinfo.NodeInfo) *Status

Am I correct?

I don't think so, we will call the extender with all filtered nodes (make a single call, not one per node), otherwise it will be very expensive. Also, this is a v1 API, good luck changing that :)

lol... Let me see how it goes.

ahg-g · 2019-12-10T19:05:07Z

pkg/scheduler/core/generic_scheduler_test.go

-					"1": []algorithmpredicates.PredicateFailureReason{algorithmpredicates.ErrFakePredicate, algorithmpredicates.ErrFakePredicate},
-				},
-				FilteredNodesStatuses: framework.NodeToStatusMap{},
+		// TODO(Huang-Wei): remove this when 'alwaysCheckAllPredicates' gets removed from API.


remove it now? alwaysCheckAllPredicates does not have an impact now anyways.

ahg-g · 2019-12-10T19:07:02Z

pkg/scheduler/framework/plugins/migration/utils.go

+type RegisterFilterPluginFunc func(reg *framework.Registry, plugins *schedulerapi.Plugins, pluginConfigs []schedulerapi.PluginConfig)
+
+// RegisterFilterPlugin returns a function to register a Filter Plugin to a given registry.
+func RegisterFilterPlugin(pluginName string, pluginNewFunc framework.PluginFactory) RegisterFilterPluginFunc {


I don't believe those two functions will be used anywhere other than unit tests, and they are not quite related to migration because they will live beyond migration, right? if yes, how about we put those in scheduler/testing/framework_helpers.go?

Correct. Sounds fair to move to testing package.

Huang-Wei · 2019-12-10T19:24:08Z

The integration test failure is a real one. Fixing...

alculquicondor · 2019-12-10T19:48:28Z

pkg/scheduler/core/generic_scheduler.go

 		stateToUse := state
 		nodeInfoToUse := info
 		if i == 0 {
 			var err error
-			podsAdded, metaToUse, stateToUse, nodeInfoToUse, err = g.addNominatedPods(ctx, pod, meta, state, info)
+			podsAdded, _, stateToUse, nodeInfoToUse, err = g.addNominatedPods(ctx, pod, meta, state, info)


It looks like this is the only usage of addNominatedPods. Remove the return value.

Let's get rid of it in the metadata cleanup PR.

sg, TODO maybe?

Yeap, added at L587:

// TODO(Huang-Wei): remove 'meta predicates.Metadata' from the signature.

k8s-ci-robot · 2019-12-11T00:11:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Huang-Wei

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

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

Huang-Wei · 2019-12-11T00:23:29Z

/hold
for squashing

draveness · 2019-12-11T00:45:16Z

pkg/scheduler/testing/framework_helpers.go

+		plugins.Filter.Enabled = append(plugins.Filter.Enabled, schedulerapi.Plugin{Name: pluginName})
+		//lint:ignore SA4006 this value of pluginConfigs is never used.
+		//lint:ignore SA4010 this result of append is never used.
+		pluginConfigs = append(pluginConfigs, schedulerapi.PluginConfig{Name: pluginName})


Why do we need these two comments to disable lints here?

As the single line reports two violations..

seems wierd...

Huang-Wei · 2019-12-11T01:05:27Z

/retest

ahg-g · 2019-12-11T01:50:52Z

Great! can you please squash.

- eliminate running paths of Predicates in scheduler; use Filter Plugins instead. - refactor all unit tests - adjust the TestPreemptWithPermitPlugin integration test

Huang-Wei · 2019-12-11T06:02:04Z

/hold cancel
@ahg-g Commits squashed.

ahg-g · 2019-12-11T12:06:18Z

/lgtm

k8s-ci-robot requested a review from ahg-g December 10, 2019 18:28

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Dec 10, 2019

k8s-ci-robot requested review from alculquicondor, damemi, draveness and ravisantoshgudimetla December 10, 2019 18:28

ahg-g reviewed Dec 10, 2019

View reviewed changes

alculquicondor reviewed Dec 10, 2019

View reviewed changes

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Dec 11, 2019

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

draveness reviewed Dec 11, 2019

View reviewed changes

Eliminate running paths of Predicates in scheduler

dc3d1bd

- eliminate running paths of Predicates in scheduler; use Filter Plugins instead. - refactor all unit tests - adjust the TestPreemptWithPermitPlugin integration test

Huang-Wei force-pushed the cleanup-predicate-path branch from 24b1eaa to dc3d1bd Compare December 11, 2019 03:14

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

k8s-ci-robot assigned ahg-g Dec 11, 2019

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

k8s-ci-robot merged commit cfdc365 into kubernetes:master Dec 11, 2019

k8s-ci-robot added this to the v1.18 milestone Dec 11, 2019

Huang-Wei deleted the cleanup-predicate-path branch December 11, 2019 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eliminate running paths of Predicates in scheduler #86133

Eliminate running paths of Predicates in scheduler #86133

Huang-Wei commented Dec 10, 2019

ahg-g left a comment

ahg-g Dec 10, 2019

Huang-Wei Dec 10, 2019

ahg-g Dec 10, 2019

Huang-Wei Dec 10, 2019

ahg-g Dec 10, 2019

Huang-Wei Dec 10, 2019

ahg-g Dec 10, 2019 •

edited

Huang-Wei Dec 10, 2019

Huang-Wei commented Dec 10, 2019

alculquicondor Dec 10, 2019

Huang-Wei Dec 10, 2019

alculquicondor Dec 11, 2019

Huang-Wei Dec 11, 2019

k8s-ci-robot commented Dec 11, 2019

Huang-Wei commented Dec 11, 2019

draveness Dec 11, 2019

Huang-Wei Dec 11, 2019

draveness Dec 11, 2019

Huang-Wei commented Dec 11, 2019

ahg-g commented Dec 11, 2019

Huang-Wei commented Dec 11, 2019

ahg-g commented Dec 11, 2019

	Filter(pod *v1.Pod,
	nodes []v1.Node, nodeNameToInfo map[string]schedulernodeinfo.NodeInfo,
	) (filteredNodes []*v1.Node, failedNodesMap extenderv1.FailedNodesMap, err error)

Eliminate running paths of Predicates in scheduler #86133

Eliminate running paths of Predicates in scheduler #86133

Conversation

Huang-Wei commented Dec 10, 2019

ahg-g 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

ahg-g Dec 10, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Huang-Wei commented Dec 10, 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

k8s-ci-robot commented Dec 11, 2019

Huang-Wei commented Dec 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Huang-Wei commented Dec 11, 2019

ahg-g commented Dec 11, 2019

Huang-Wei commented Dec 11, 2019

ahg-g commented Dec 11, 2019

ahg-g Dec 10, 2019 •

edited