Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

sanposhiho
Copy link
Member

@sanposhiho sanposhiho commented Nov 9, 2022

What type of PR is this?

/priority critical-urgent
/kind bug
/sig scheduling

What this PR does / why we need it:

#112637 got merged yesterday and it supports Skip status in the Filter plugin.
But, in the current implementation, when any PreFilter returns skip status, it results in unschedulable Pod.
We don't experience this issue because we don't return Skip in any in-tree plugins. But, it'll be an issue when users see the release note, realize the Skip in PreFIlter, and then try it on their custom plugins.


The skip status should be treated as success in all other places than calculating SkipFilterPlugins in the cycle state.
And, I added the test code on schedulePod func, which I should've implemented in #112637 along with other unit tests, and realized the bug 馃檱 馃檱

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Should I revert #112637 instead of aiming to merge it in v1.26?

Does this PR introduce a user-facing change?

NONE

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


@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 9, 2022
@k8s-ci-robot
Copy link
Contributor

@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
Copy link
Member Author

sanposhiho commented Nov 9, 2022

@kubernetes/sig-scheduling-leads

Sorry for the bug.
It's only small changes for the implementation and big changes are for testing purpose. And, I want to merge it in v1.26 if possible.
But, it was a bit late to find out the bug; we are already under code-freeze from 8 hours ago. Should I revert #112637 instead of aiming to merge this PR in v1.26?

@sanposhiho
Copy link
Member Author

sanposhiho commented Nov 9, 2022

I created #113786 to revert.
Given we are under code-freeze, probably we should revert the commit instead of merging this PR to solve.
But, if we don't need to revert it, feel free to close #113786 and I'll work on this PR so that it can get merged as soon as possible. 馃檹

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

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sanposhiho
Once this PR has been reviewed and has the lgtm label, please assign huang-wei for approval by writing /assign @huang-wei in a comment. 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:

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

@k8s-ci-robot
Copy link
Contributor

@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-integration 2470b9c6b72291e1bc4434859b264a69d919f117 link true /test pull-kubernetes-integration
pull-kubernetes-unit 874f3d4 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.

@alculquicondor
Copy link
Member

/close

include this fix in a roll forward PR

@k8s-ci-robot
Copy link
Contributor

@alculquicondor: Closed this PR.

In response to this:

/close

include this fix in a roll forward 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.

@sanposhiho
Copy link
Member Author

Thanks for merging revert PR.
I'll create another one for PreFilter Skip again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note-none Denotes a PR that doesn't merit a release note. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants