feature(scheduler): won't run Filter if PreFilter returned a Skip status #112637

sanposhiho · 2022-09-21T14:15:35Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Change the framework so that it doesn't run plugins's Filter() if its PreFilter() returned a Skip status.
For example, nodeAffinity returns Skip in PreFilter if the pod doesn't specify any node selector or affinity.

In this PR, I only changed the Framework logic. I'm going to change each plugin's PreFilter in another PR.

Which issue(s) this PR fixes:

Part of #107556
Part of #110643

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

k8s-ci-robot · 2022-09-21T14:15:42Z

@sanposhiho: 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.

sanposhiho · 2022-09-21T14:16:38Z

/sig scheduling

sanposhiho · 2022-09-24T04:44:38Z

cc @x13n
(Just for your information, this PR is created before #112660 and no one from autoscaling team is assigned now so.)

x13n · 2022-10-06T14:26:29Z

Thanks and apologies for late response. The change LGTM from autoscaling point of view. Since it predates #112660, I guess there's nothing else for me to do here (i.e. my approval won't matter to k8s bot).

alculquicondor · 2022-10-13T15:22:23Z

/assign

alculquicondor · 2022-10-13T15:41:12Z

pkg/scheduler/framework/runtime/framework.go

 // anything but Success. If a non-success status is returned, then the scheduling
 // cycle is aborted.
-func (f *frameworkImpl) RunPreFilterPlugins(ctx context.Context, state *framework.CycleState, pod *v1.Pod) (_ *framework.PreFilterResult, status *framework.Status) {
+func (f *frameworkImpl) RunPreFilterPlugins(ctx context.Context, state *framework.CycleState, pod *v1.Pod) (_ *framework.PreFilterResult, _ sets.String, status *framework.Status) {


Maybe we should add a dedicated field in CycleState to hold the set of SkipFilterPlugins

Ah, makes sense. I didn't come up with that idea.
Let me change...

changed implementation as suggested. 🙏

pkg/scheduler/framework/cycle_state.go

alculquicondor · 2022-10-20T13:40:22Z

pkg/scheduler/framework/cycle_state.go

@@ -48,6 +50,8 @@ type CycleState struct {
 	storage sync.Map
 	// if recordPluginMetrics is true, PluginExecutionDuration will be recorded for this cycle.
 	recordPluginMetrics bool
+	// skipFilterPlugin is plugins that are skipped in the Filter extension point.
+	skipFilterPlugins sets.String


I would just export this field instead of adding getters and setters. Or you could have functions like SkipFilterPlugin to add elements and ShouldSkipFilterPlugin to check if the plugin is in the set.

Are there scenarios in the codebase where c==nil?

I would just export this field instead of adding getters and setters.

That seems simpler and better.
I just followed how we handle recordPluginMetrics, and no big reason to do this.

Are there scenarios in the codebase where c==nil?

I checked and seems No.

alculquicondor · 2022-10-20T13:51:33Z

pkg/scheduler/framework/runtime/framework_test.go

-		name: preFilterPluginName,
-		inj:  injectedResult{PreFilterStatus: int(framework.Error)},
+func TestRunPreFilterPlugins(t *testing.T) {
+	tests := []struct {


So much better, thanks!

pkg/scheduler/framework/runtime/framework_test.go

alculquicondor · 2022-10-20T13:53:54Z

pkg/scheduler/framework/runtime/framework_test.go

+			if status == nil && tt.wantStatus != nil || status != nil && tt.wantStatus == nil {
+				t.Errorf("wrong status. got: %v, want: %v", status, tt.wantStatus)
+			} else if status == nil && tt.wantStatus == nil {
+				// do nothing


doesn't cmp.Diff already handle all these cases?

If status and/or tt.wantStatus is nil, *status and/or *tt.wantStatus in cmp.Diff will be nil-pointer panic.

you can use cmp.Diff(status, tt.wantStatus)

cmp.Diff cannot handle pointer.
↓ is the result by using cmp.Diff(status, tt.wantStatus, cmpopts.IgnoreUnexported(framework.Status{})) here.
It was actually the same, but cmp.Diff thought they are different from each other since it's pointer.

(*framework.Status)( - &⟪0xc00016d780⟫{ - code: 1, - reasons: []string(⟪ptr:0xc0003d44a0, len:1, cap:1⟫{`running PreFilter plugin "error": injected status`}), - err: &⟪0xc0003a2060⟫"fmt".wrapError{ - msg: `running PreFilter plugin "error": injected status`, - err: &⟪0xc0003d4420⟫"errors".errorString{s: "injected status"}, - }, - failedPlugin: "error", - }, + &⟪0xc00016d2c0⟫{ + code: 1, + reasons: []string(⟪ptr:0xc0003d52f0, len:1, cap:1⟫{`running PreFilter plugin "error": injected status`}), + err: &⟪0xc0003a2580⟫"fmt".wrapError{ + msg: `running PreFilter plugin "error": injected status`, + err: &⟪0xc0003d4c80⟫"errors".errorString{s: "injected status"}, + }, + failedPlugin: "error", + }, )

I found ↑ is a lie. Actually, cmp.Diff can handle pointers.
The reason we cannot just use cmp.Diff is both errors are created from different error bases.

The status from TestPlugin contains the error created in framework.NewStatus.

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

Line 181 in 3efd107

return nil, framework.NewStatus(framework.Code(pl.inj.PreFilterStatus), injectReason)

But, the status in each test case contains the error errInjectedStatus.

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

Line 382 in 3efd107

errInjectedStatus = errors.New(injectReason)

I fixed here to use cmp.Comparer and deal with the ↑ problem.

pkg/scheduler/framework/runtime/framework_test.go

sanposhiho · 2022-11-05T00:10:42Z

Sure, I will do it in this weekend

sanposhiho · 2022-11-05T01:55:01Z

@alculquicondor @kerthcet Fixed and squashed. 🙏

sanposhiho · 2022-11-06T16:36:15Z

/retest

sanposhiho · 2022-11-06T17:20:37Z

/retest

kerthcet

One nit.

pkg/scheduler/framework/cycle_state.go

alculquicondor · 2022-11-07T15:57:49Z

pkg/scheduler/framework/runtime/framework_test.go

+			if d := cmp.Diff(result, tt.wantPreFilterResult); d != "" {
+				t.Errorf("wrong status. got: %v, want: %v, diff: %s", result, tt.wantPreFilterResult, d)
+			}
+			if d := cmp.Diff(status, tt.wantStatus, cmpopts.EquateErrors(), cmp.Comparer(func(a, b *framework.Status) bool {


there should be no need for EquateErrors

Ah 🤦, fixed.

k8s-ci-robot · 2022-11-08T00:20:43Z

@sanposhiho: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-gce-csi-serial	f6ee0066ce8ff73798ab0e538a8baf4e67c096db	link	false	`/test pull-kubernetes-e2e-gce-csi-serial`
pull-kubernetes-e2e-gce-alpha-features	f6ee0066ce8ff73798ab0e538a8baf4e67c096db	link	false	`/test pull-kubernetes-e2e-gce-alpha-features`
pull-kubernetes-conformance-kind-ipv6-parallel	f6ee0066ce8ff73798ab0e538a8baf4e67c096db	link	false	`/test pull-kubernetes-conformance-kind-ipv6-parallel`
pull-kubernetes-e2e-gce-storage-slow	f6ee0066ce8ff73798ab0e538a8baf4e67c096db	link	false	`/test pull-kubernetes-e2e-gce-storage-slow`
pull-kubernetes-e2e-gce-storage-snapshot	f6ee0066ce8ff73798ab0e538a8baf4e67c096db	link	false	`/test pull-kubernetes-e2e-gce-storage-snapshot`
pull-kubernetes-e2e-capz-windows-containerd	f6ee0066ce8ff73798ab0e538a8baf4e67c096db	link	false	`/test pull-kubernetes-e2e-capz-windows-containerd`
check-dependency-stats	f6ee0066ce8ff73798ab0e538a8baf4e67c096db	link	false	`/test check-dependency-stats`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

sanposhiho · 2022-11-08T01:08:39Z

/retest

kerthcet · 2022-11-08T03:13:04Z

/lgtm

alculquicondor · 2022-11-08T13:11:01Z

/approve

k8s-ci-robot · 2022-11-08T13:11:34Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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

sanposhiho · 2022-11-10T00:14:57Z

remove the release note.

alculquicondor · 2022-11-24T16:55:20Z

@sanposhiho can you work on this now?

I think this is important for performance reasons too.

sanposhiho · 2022-11-24T22:50:23Z

yes, I think i can create a PR hopefully by this weekend

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 Sep 21, 2022

k8s-ci-robot requested review from chendave and denkensk September 21, 2022 14:16

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 21, 2022

sanposhiho force-pushed the pre-filter-skip branch 2 times, most recently from 2703fdc to ec85e91 Compare September 21, 2022 15:14

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

sanposhiho force-pushed the pre-filter-skip branch 2 times, most recently from 27d1069 to 746d8be Compare September 22, 2022 02:01

sanposhiho mentioned this pull request Sep 22, 2022

Add Framework interface detectors in scheduler #110717

Merged

k8s-ci-robot assigned alculquicondor Oct 13, 2022

alculquicondor reviewed Oct 13, 2022

View reviewed changes

sanposhiho force-pushed the pre-filter-skip branch from 746d8be to 35f971d Compare October 20, 2022 00:05

alculquicondor reviewed Oct 20, 2022

View reviewed changes

sanposhiho force-pushed the pre-filter-skip branch from 59983cf to f6ee006 Compare October 30, 2022 10:55

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 30, 2022

sanposhiho force-pushed the pre-filter-skip branch from df677ef to 041d0f7 Compare November 5, 2022 01:54

sanposhiho force-pushed the pre-filter-skip branch from fe9cbef to 519c9d9 Compare November 6, 2022 14:16

sanposhiho mentioned this pull request Nov 6, 2022

feature(nodeaffinity): support Skip in NodeAffinity PreFilter #113658

Closed

kerthcet reviewed Nov 7, 2022

View reviewed changes

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

sanposhiho force-pushed the pre-filter-skip branch from 519c9d9 to f131a03 Compare November 7, 2022 15:23

alculquicondor reviewed Nov 7, 2022

View reviewed changes

feature(scheduler): won't run Filter if PreFilter returned a Skip status

786be73

sanposhiho force-pushed the pre-filter-skip branch from f131a03 to 786be73 Compare November 7, 2022 23:48

k8s-ci-robot assigned kerthcet Nov 8, 2022

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

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

k8s-ci-robot merged commit 2b7b524 into kubernetes:master Nov 8, 2022

k8s-ci-robot added this to the v1.26 milestone Nov 8, 2022

This was referenced Nov 9, 2022

fix(framework): fix the bug on PreFilter skip #113779

Closed

Revert "feature(scheduler): won't run Filter if PreFilter returned a Skip status" #113786

Merged

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Nov 10, 2022

sanposhiho mentioned this pull request Nov 25, 2022

feature(scheduler): won't run Filter if PreFilter returned a Skip status #114125

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(scheduler): won't run Filter if PreFilter returned a Skip status #112637

feature(scheduler): won't run Filter if PreFilter returned a Skip status #112637

sanposhiho commented Sep 21, 2022 •

edited

k8s-ci-robot commented Sep 21, 2022

sanposhiho commented Sep 21, 2022

sanposhiho commented Sep 24, 2022

x13n commented Oct 6, 2022 •

edited

alculquicondor commented Oct 13, 2022

alculquicondor Oct 13, 2022

sanposhiho Oct 17, 2022

sanposhiho Oct 20, 2022

alculquicondor Oct 20, 2022

sanposhiho Oct 30, 2022 •

edited

sanposhiho Oct 30, 2022

alculquicondor Oct 20, 2022

alculquicondor Oct 20, 2022

sanposhiho Oct 30, 2022 •

edited

alculquicondor Oct 31, 2022

sanposhiho Nov 5, 2022 •

edited

sanposhiho Nov 6, 2022

sanposhiho commented Nov 5, 2022

sanposhiho commented Nov 5, 2022 •

edited

sanposhiho commented Nov 6, 2022

sanposhiho commented Nov 6, 2022

kerthcet left a comment

alculquicondor Nov 7, 2022

sanposhiho Nov 7, 2022

k8s-ci-robot commented Nov 8, 2022 •

edited

sanposhiho commented Nov 8, 2022

kerthcet commented Nov 8, 2022

alculquicondor commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

sanposhiho commented Nov 10, 2022

alculquicondor commented Nov 24, 2022

sanposhiho commented Nov 24, 2022

feature(scheduler): won't run Filter if PreFilter returned a Skip status #112637

feature(scheduler): won't run Filter if PreFilter returned a Skip status #112637

Conversation

sanposhiho commented Sep 21, 2022 • edited

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?

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

k8s-ci-robot commented Sep 21, 2022

sanposhiho commented Sep 21, 2022

sanposhiho commented Sep 24, 2022

x13n commented Oct 6, 2022 • edited

alculquicondor commented Oct 13, 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

sanposhiho Oct 30, 2022 • edited

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

sanposhiho Oct 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho Nov 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho commented Nov 5, 2022

sanposhiho commented Nov 5, 2022 • edited

sanposhiho commented Nov 6, 2022

sanposhiho commented Nov 6, 2022

kerthcet left a 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 Nov 8, 2022 • edited

sanposhiho commented Nov 8, 2022

kerthcet commented Nov 8, 2022

alculquicondor commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

sanposhiho commented Nov 10, 2022

alculquicondor commented Nov 24, 2022

sanposhiho commented Nov 24, 2022

sanposhiho commented Sep 21, 2022 •

edited

x13n commented Oct 6, 2022 •

edited

sanposhiho Oct 30, 2022 •

edited

sanposhiho Oct 30, 2022 •

edited

sanposhiho Nov 5, 2022 •

edited

sanposhiho commented Nov 5, 2022 •

edited

k8s-ci-robot commented Nov 8, 2022 •

edited