cleanup(framework): return earlier if all Score plugins are skipped

[sanposhiho]

What type of PR is this?

/kind bug
/kind cleanup

What this PR does / why we need it:

do an earlier return when all Score plugins returned Skip in PreScore.

This PR originally had the fix for panic bug

The scheduler panics when all these conditions are satisfied:

• some PreScore plugins which returned Skip, but are disabled in the Score extension point
• "the number of PreScore plugins that returned Skip and disabled in the Score" is more than the number of all Score plugins enabled in the scheduler

You can reproduce the panic with newly added UT like the following.

Running tool: /home/gitpod/go/bin/go test -timeout 30s -run ^TestRunScorePlugins$ k8s.io/kubernetes/pkg/scheduler/framework/runtime

I0527 12:20:50.858222  101963 framework.go:509] "MultiPoint plugin is explicitly re-configured; overriding" plugin="score-plugin-1"
--- FAIL: TestRunScorePlugins (0.00s)
    --- FAIL: TestRunScorePlugins/PreScore_returns_Skip_but_isn't_registered_in_Score (0.00s)
panic: runtime error: makeslice: cap out of range [recovered]
	panic: runtime error: makeslice: cap out of range

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

This PR originally had the fix for panic bug

=

@kubernetes/sig-scheduling-leads
In my opinion, we don't need to cherry-pick this into v1.27, but please anyone double-check.

This shouldn't break the default Kubernetes scheduler because any in-tree Score plugins don't return Skip in v1.27 in the first place. (ref)
It might break the custom scheduler, for example, the scheduler with some custom PreScore plugins which may return Skip, but are disabled in the Score extension point. Also, to see this bug, "the number of PreScore plugins that returned Skip and disabled in the Score" should be more than the number of all Score plugins enabled in the scheduler, which is an extremely minor case.

Does this PR introduce a user-facing change?

NONE

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

[sanposhiho]

/priority important-soon
/triage accepted

As described, it's not a critical bug. But, it's a blocker to implement Skip in Score plugins because it's breaking the e2e test in the PR of Skip implementation (e.g., #117024).

[sanposhiho]

The trigger of this bug is that numPlugins becomes less than 0.

[AxeZhan]

I ran into this before in some tests, but is there really a scenario that a plugin is enabled for prescore and disabled for score in reality?

[sanposhiho]

I ran into this before in some tests, but is there really a scenario that a plugin is enabled for prescore and disabled for score in reality?

I'd say 100% No if no one made mistakes in the scheduler component config. This fix is for mistakers.

/remove-priority important-soon

I'll fix the e2e test side to unblock the Skip PR.

[sanposhiho]

But, on second thought, we may just want to accept this bug because this PR will remove pre-allocation for pluginToNodeScores and plugins. 🤔

[sanposhiho]

But, on second thought, we may just want to accept this bug because this PR will remove pre-allocation for pluginToNodeScores and plugins. 🤔

Let's go with this thought unless other people want this bug fix.
I'll convert this PR to the cleanup one. Feel free to give your thought if you think that was worthwhile to have.

/remove-kind bug
/retitle cleanup(framework): return earlier if all Score plugins are skipped

[sanposhiho]

/retest

[k8s-ci-robot]

@sanposhiho: The following test 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-unit	6b6ab1b	link	true	/test pull-kubernetes-unit

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.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sanposhiho, Tusenka
Once this PR has been reviewed and has the lgtm label, please assign ahg-g for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• pkg/scheduler/OWNERS

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

[kerthcet]

/assign

[kerthcet]

Can you fix the test?

[k8s-ci-robot]

PR needs rebase.

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.

[alculquicondor]

Are you still planning this?

[sanposhiho]

It's in icebox state in my todo list. I don't think I can take time to take care of it for now. Anyone feel free to raise a similar PR. (or maybe future me will)

/close

[k8s-ci-robot]

@sanposhiho: Closed this PR.

In response to this:

It's in icebox state in my todo list. I don't think I can take time to take care of it for now. Anyone feel free to raise a similar PR. (or maybe future me will)

/close

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.