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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: requeue pods rejected by Extenders properly #122022

Merged
merged 1 commit into from Dec 14, 2023

Conversation

sanposhiho
Copy link
Member

What type of PR is this?

/kind bug

What this PR does / why we need it:

Extender doesn't support any kind of requeueing feature like EnqueueExtensions in the scheduling framework.

When Extender filters out some Nodes, we don't set any unschedulable plugins at all.
It means Extender is completely ignored during the requeueing process.

So, what's happening is:

  • If Extender filters out all Nodes during scheduling, this Pod is soon retried because this Pod doesn't have any plugin name in unschedulable plugins. The scheduling queue doesn't put this Pod into the unschedulable pod pool, but put into activeQ/backoffQ.
  • If Extender filters out some Nodes and plugins filter out all other Nodes, this Pod is retried based on plugins' QueueingHint. Even if some cluster events happen and they could change Extender's decision, this Pod won't be requeued to activeQ/backoffQ.

The latter case is serious because it could make Pods stuck in unschedulable pod pool in 5min in the worst case scenario.

This PR makes the scheduling queue aware of extenders' failures.
After this PR, when Extenders reject some Nodes and the pod ends up being unschedulable, this Pod will be requeued from unschedulable pod pool to activeQ/backoffQ by any kind of cluster events.

Which issue(s) this PR fixes:

Fixes #122019

Special notes for your reviewer:

Probably, we have to cherry-pick this PR into past versions?

Does this PR introduce a user-facing change?

The scheduling queue didn't notice any extenders' failures, it could miss some cluster events,
and it could end up Pods rejected by Extenders stuck in unschedulable pod pool in 5min in the worst-case scenario.
Now, the scheduling queue notices extenders' failures and requeue Pods rejected by Extenders appropriately.

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


@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 23, 2023
@k8s-ci-robot
Copy link
Contributor

Please note that we're already in Test Freeze for the release-1.29 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.29.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Thu Nov 23 09:57:18 UTC 2023.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 23, 2023
@k8s-ci-robot
Copy link
Contributor

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.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Nov 23, 2023
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. 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 Nov 23, 2023
@sanposhiho
Copy link
Member Author

/cc @alculquicondor

I think it's worth cherry-picking this PR in older versions.

@sanposhiho
Copy link
Member Author

/hold

To involve other approvers

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 23, 2023
@alculquicondor
Copy link
Member

/lgtm
/approve
how far do we need to cherry-pick?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 23, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 33b5a3ba2a251a8b9a749a9185978a2e9ae1d34b

@k8s-ci-robot
Copy link
Contributor

[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:

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

@sanposhiho
Copy link
Member Author

how far do we need to cherry-pick?

Well, it's better to cherry-pick this for all supported versions because this event-based requeueing has been existing for a long time.

@alculquicondor
Copy link
Member

sgtm

@sanposhiho
Copy link
Member Author

/unhold

Got approval, not merged anyway though

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 26, 2023
@k8s-ci-robot k8s-ci-robot merged commit 5322af7 into kubernetes:master Dec 14, 2023
14 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.30 milestone Dec 14, 2023
k8s-ci-robot added a commit that referenced this pull request Dec 14, 2023
…122022-upstream-release-1.28

Automated cherry pick of #122022: fix: requeue pods rejected by Extenders properly
k8s-ci-robot added a commit that referenced this pull request Dec 18, 2023
…122022-upstream-release-1.26

Automated cherry pick of #122022: fix: requeue pods rejected by Extenders properly
k8s-ci-robot added a commit that referenced this pull request Dec 19, 2023
…122022-upstream-release-1.27

Automated cherry pick of #122022: fix: requeue pods rejected by Extenders properly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. 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.

No proper scheduling retries could be made when Extender filters out some Nodes
3 participants