Fix a bug that out-of-tree plugin is misplaced when using scheduler v1beta3 config #108613

Huang-Wei · 2022-03-09T19:18:32Z

What type of PR is this?

/kind bug
/sig scheduling

What this PR does / why we need it:

When custom plugins are specified via regular extension points in v1beta3, the current logic places them at the head,, and then appends internal default plugins (in expandMultiPointPlugins()).

However, the eventual order should be like this:

part 1: plugins that are explicitly re-configured in multipoint (i.e., specified in both regular extension point and multipoint). Note their order should be intact as how they're configured in the regular extension point.
part 2: plugins enabled in multipoint excluding the ones in part 1. These plugins can be:
- defaulted in-tree plugin
- out-of-tree plugin
part 3: other plugins (excluded by part 1 & 2) in regular extension point. The category contains:
- non-defaulted in-tree plugin
- out-of-tree plugin

This PR changes the logic of expandMultiPointPlugins() to order the plugins when necessary.

Which issue(s) this PR fixes:

Fixes #108083

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix a bug that out-of-tree plugin is misplaced when using scheduler v1beta3 config

k8s-ci-robot · 2022-03-09T19:18:39Z

@Huang-Wei: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

Huang-Wei · 2022-03-09T21:40:28Z

/retest

Huang-Wei · 2022-03-09T22:56:27Z

With this PR, any testing plugin will be appended (not prepend) to the default plugins, that caused an integration test failure cuz DefaultBind would fail (due to the testing context), that failed the pod's scheduling as well as further TestBind plugin's execution.

Huang-Wei · 2022-03-10T00:30:46Z

/retest

Huang-Wei · 2022-03-10T01:24:09Z

pkg/scheduler/framework/runtime/framework.go

+		newPlugins := reflect.New(reflect.TypeOf(e.slicePtr).Elem()).Elem()
+		// part 1
+		deletionCnt := 0
+		for i, name := range enabledSet.list {


In Golang, the iteration order of a map is uncertain, and thus this PR introduces an orderedSet.

Another alternative is to use slice to replace enabledSet, but in that case, we need extra logic to deal with duplicated plugins.

Huang-Wei · 2022-03-10T01:24:31Z

@damemi This PR is ready for review.

pkg/scheduler/framework/runtime/framework.go

kerthcet · 2022-03-10T14:39:02Z

pkg/scheduler/framework/runtime/framework.go


 			newPlugins := reflect.Append(plugins, reflect.ValueOf(pg))
 			plugins.Set(newPlugins)
 		}
+
+		// Reorder plugins if needed. Here is the expected order:


It seems like we rearrange all the plugin orders here. So when len(profile.Plugins.MultiPoint.Enabled) > 0, we no longer need above updatePluginList process anymore, right? If this makes sense, we should merge them together.

kubernetes/pkg/scheduler/framework/runtime/framework.go

Lines 334 to 338 in e9af399

for _, e := range f.getExtensionPoints(profile.Plugins) {

if err := updatePluginList(e.slicePtr, *e.plugins, pluginsMap); err != nil {

return nil, err

}

}

I still prefer to keep them separate. When len(profile.Plugins.MultiPoint.Enabled) > 0, it's not necessarily going to rearrange the plugins - e.g., all plugins defined in multipoint has been specified (say reconfigured) in the regular extension point.

damemi · 2022-03-10T14:52:52Z

Thanks for working on this @Huang-Wei, I will take add some review soon when I have a moment

pkg/scheduler/framework/runtime/framework.go

kerthcet · 2022-03-11T06:23:21Z

pkg/scheduler/framework/runtime/framework.go

+				enabledSet.delete(name)
+			}
+		}
+		// enabledSet.isEmpty() implies part 3 is empty, so don't need to continue


If I understood correctly, we can harmless remove https://github.com/kubernetes/kubernetes/blob/8ef5ccdad6bba8b664b51a8b29fd1a5ce396a59d/pkg/scheduler/framework/runtime/framework.go#L532-L535 and

kubernetes/pkg/scheduler/framework/runtime/framework.go

Lines 480 to 481 in 897f2f7

newPlugins := reflect.Append(plugins, reflect.ValueOf(pg))

plugins.Set(newPlugins)

After removal, program still behaves the same but more clearly to read. Separate the reading and writing process.

SG. It reads better.

Huang-Wei · 2022-03-12T00:39:18Z

/retest

kerthcet · 2022-03-12T11:13:36Z

cmd/kube-scheduler/app/server_test.go

+		t.Fatal(err)
+	}
+
+	// plugin config


nit: replace with out-of-tree plugin config v1beta2

will update along with other comments.

kerthcet · 2022-03-12T11:22:57Z

pkg/scheduler/framework/runtime/framework.go

-			plugins.Set(newPlugins)
+		// Reorder plugins. Here is the expected order:
+		// - part 1: overridePlugins. Their order stay intact as how they're specified in regular extension point.
+		// - part 2: multiPointEnabled - i.e., plugin defined in multipoint but not overriding regular extension point.


s/not overriding/not overrided by/

not really, it's the multi-point plugin overriding others instead of the other way around.

Aren't both correct? This is the set of multipoint plugins that aren't overriding (ie, not in part 1) but also they aren't being overridden (otherwise, they would be in part 1 as their overridden config). Part 1 includes explicitly reconfigured multiPoint plugins right?

Correct, it's bi-directional. This may lead to less ambiguity?

// - part 2: multiPointEnabled - i.e., plugins defined in multipoint but not in regular extension point.

kerthcet · 2022-03-12T11:24:48Z

Some nits, then LGTM. Left for @damemi for a further review.

kerthcet · 2022-03-12T11:24:57Z

/retest

damemi

Looks good to me overall. I think breaking the logic out into 3 distinct sets makes it a bit clearer how the precedence works
/approve

damemi · 2022-03-14T13:19:28Z

pkg/scheduler/framework/runtime/framework.go

+}
+
+func newOrderedSet() *orderedSet {
+	return &orderedSet{set: make(map[string]int)}


do you need to initialize the list too?

A nil slice is functionally equivalent to a zero-length slice. If we won't gone to use Marshal and reflect. DeepEqual with this field, freely to us. So I think we can omit it here.

yup, we usually don't need to initialize a slice. And in terms of using a slice: var s []string are in favor of s := []string.

damemi · 2022-03-14T13:26:14Z

pkg/scheduler/framework/runtime/framework_test.go

@@ -656,6 +656,44 @@ func TestNewFrameworkMultiPointExpansion(t *testing.T) {
 				PostBind: config.PluginSet{Enabled: []config.Plugin{{Name: testPlugin}}},
 			},
 		},
+		{
+			name: "Reorder MultiPoint plugins (specified extension only takes precedence when it exists in MultiPoint)",


I think this test covers the mix of use cases well

damemi · 2022-03-14T13:32:39Z

pkg/scheduler/framework/runtime/framework.go

-			plugins.Set(newPlugins)
+		// Reorder plugins. Here is the expected order:
+		// - part 1: overridePlugins. Their order stay intact as how they're specified in regular extension point.
+		// - part 2: multiPointEnabled - i.e., plugin defined in multipoint but not overriding regular extension point.


Aren't both correct? This is the set of multipoint plugins that aren't overriding (ie, not in part 1) but also they aren't being overridden (otherwise, they would be in part 1 as their overridden config). Part 1 includes explicitly reconfigured multiPoint plugins right?

k8s-ci-robot · 2022-03-14T13:34:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, 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:

~~cmd/kube-scheduler/OWNERS~~ [Huang-Wei,damemi]
~~pkg/scheduler/OWNERS~~ [Huang-Wei,damemi]
~~test/integration/scheduler/OWNERS~~ [Huang-Wei,damemi]

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

Huang-Wei · 2022-03-14T18:44:28Z

Commits squashed.

@damemi please take a another look when you get a chance.

Huang-Wei · 2022-03-18T02:58:59Z

@damemi could you please take another look?

damemi · 2022-03-21T13:27:06Z

Sorry @Huang-Wei, I only had those 2 nits. The rest looks good, thanks for fixing this
/lgtm

kerthcet · 2022-03-21T13:52:19Z

/retest

kerthcet · 2022-03-21T14:04:41Z

We should rebase the code as we changed the PreFilterPlugin interface days before.

k8s-triage-robot · 2022-03-21T16:33:25Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

…1beta3 config

Huang-Wei · 2022-03-22T06:42:30Z

@kerthcet @damemi could you please re-/lgtm?

denkensk · 2022-03-22T06:44:54Z

/lgtm

Thanks. @Huang-Wei

kerthcet · 2022-03-22T07:25:27Z

/retest

…08613-upstream-release-1.23 Automated cherry pick of #108613: Fix a bug that out-of-tree plugin is misplaced when using

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 9, 2022

k8s-ci-robot requested review from damemi and denkensk March 9, 2022 19:19

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

miamipanther mentioned this pull request Mar 9, 2022

[WIP] fix: scheduler v1beta3 doesn't append out-of-tree plugins correctly #108434

Closed

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Mar 9, 2022

Huang-Wei commented Mar 10, 2022

View reviewed changes

denkensk reviewed Mar 10, 2022

View reviewed changes

pkg/scheduler/framework/runtime/framework.go Outdated Show resolved Hide resolved

kerthcet reviewed Mar 10, 2022

View reviewed changes

kerthcet reviewed Mar 11, 2022

View reviewed changes

pkg/scheduler/framework/runtime/framework.go Outdated Show resolved Hide resolved

kerthcet reviewed Mar 11, 2022

View reviewed changes

Huang-Wei force-pushed the fix-v1beta3-order branch 2 times, most recently from c856910 to 912f320 Compare March 11, 2022 20:26

kerthcet reviewed Mar 12, 2022

View reviewed changes

damemi reviewed Mar 14, 2022

View reviewed changes

Huang-Wei force-pushed the fix-v1beta3-order branch from 912f320 to b74574f Compare March 14, 2022 18:44

k8s-ci-robot assigned damemi Mar 21, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 21, 2022

Huang-Wei force-pushed the fix-v1beta3-order branch from b74574f to d82ff1d Compare March 21, 2022 17:09

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 21, 2022

Fix a bug that out-of-tree plugin is misplaced when using scheduler v…

d330f4d

…1beta3 config

Huang-Wei force-pushed the fix-v1beta3-order branch from d82ff1d to d330f4d Compare March 21, 2022 17:46

k8s-ci-robot assigned denkensk Mar 22, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 22, 2022

k8s-ci-robot merged commit 0b8a665 into kubernetes:master Mar 22, 2022

k8s-ci-robot added this to the v1.24 milestone Mar 22, 2022

Huang-Wei deleted the fix-v1beta3-order branch March 22, 2022 18:21

Huang-Wei mentioned this pull request Mar 22, 2022

Automated cherry pick of #108613: Fix a bug that out-of-tree plugin is misplaced when using #108890

Merged

k8s-ci-robot added a commit that referenced this pull request Apr 28, 2022

Merge pull request #108890 from Huang-Wei/automated-cherry-pick-of-#1…

dcce235

…08613-upstream-release-1.23 Automated cherry pick of #108613: Fix a bug that out-of-tree plugin is misplaced when using

kerthcet mentioned this pull request May 27, 2023

Add kerthcet and sanposhiho as SIG scheduling approvers #118294

Merged

lianghao208 mentioned this pull request Nov 28, 2023

fix(scheduler): fix incorrect loop logic in MultiPoint to avoid a plugin being loaded multiple times #122068

Merged

	for _, e := range f.getExtensionPoints(profile.Plugins) {
	if err := updatePluginList(e.slicePtr, *e.plugins, pluginsMap); err != nil {
	return nil, err
	}
	}

	newPlugins := reflect.Append(plugins, reflect.ValueOf(pg))
	plugins.Set(newPlugins)

Fix a bug that out-of-tree plugin is misplaced when using scheduler v1beta3 config #108613

Fix a bug that out-of-tree plugin is misplaced when using scheduler v1beta3 config #108613

Conversation

Huang-Wei commented Mar 9, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Mar 9, 2022

Huang-Wei commented Mar 9, 2022

Huang-Wei commented Mar 9, 2022

Huang-Wei commented Mar 10, 2022

Choose a reason for hiding this comment

Huang-Wei commented Mar 10, 2022

Choose a reason for hiding this comment

Huang-Wei Mar 10, 2022 • edited

Choose a reason for hiding this comment

damemi commented Mar 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Huang-Wei commented Mar 12, 2022

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

kerthcet commented Mar 12, 2022

kerthcet commented Mar 12, 2022

damemi 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

k8s-ci-robot commented Mar 14, 2022

Huang-Wei commented Mar 14, 2022

Huang-Wei commented Mar 18, 2022

damemi commented Mar 21, 2022

kerthcet commented Mar 21, 2022

kerthcet commented Mar 21, 2022

k8s-triage-robot commented Mar 21, 2022

Huang-Wei commented Mar 22, 2022

denkensk commented Mar 22, 2022

kerthcet commented Mar 22, 2022

Huang-Wei Mar 10, 2022 •

edited