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.

Sign up for GitHub

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

Jump to bottom

Revert "Optimization on running prePreEnqueuePlugins before adding pods into activeQ" #117194

Merged
merged 2 commits into from
Apr 13, 2023
Merged

Revert "Optimization on running prePreEnqueuePlugins before adding pods into activeQ" #117194

merged 2 commits into from
Apr 13, 2023

Conversation

sanposhiho
Copy link
Member

@sanposhiho sanposhiho commented Apr 11, 2023 •
edited by liggitt
Loading

What type of PR is this?

/kind bug

What this PR does / why we need it:

This reverts commit c01fa82.

The discussion: #115583 (comment)

This PR fixes that the PreEnqueue plugins aren't executed if Pods proceed to activeQ through backoffQ.

It doesn't affect the vanilla kubernetes because if the scheduling gate plugin once allow Pods to go to activeQ, it always allows them afterward. But, it may break custom PreEnqueue plugins.

This PR also adds the description of this issue as the known issue on the release note of v1.27.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix a 1.27 regression that custom PreEnqueue plugins aren't executed for Pods proceeding to activeQ through backoffQ.

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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. labels Apr 11, 2023
@k8s-ci-robot
Copy link
Contributor

Please note that we're already in Test Freeze for the release-1.27 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.27.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Tue Apr 11 02:03:33 UTC 2023.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 11, 2023
@k8s-ci-robot k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 11, 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 Apr 11, 2023
@sanposhiho
Copy link
Member Author

/cc @ahg-g @Huang-Wei

@k8s-ci-robot k8s-ci-robot requested review from ahg-g and Huang-Wei April 11, 2023 05:15
@sanposhiho sanposhiho mentioned this pull request Apr 11, 2023
Merged
Huang-Wei
Huang-Wei reviewed Apr 11, 2023
View reviewed changes
Copy link
Member

@Huang-Wei Huang-Wei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

We will see if we can make it to 1.27, or we can wait for the first 1.27 patch release.

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2023
@sanposhiho
Copy link
Member Author

/retest

@liggitt
Copy link
Member

liggitt commented Apr 11, 2023

That seems like a good candidate for inclusion in a 1.27.1 patch release, and noting the issue as a known issue for someone using custom scheduler plug-ins in 1.27.0.

@liggitt
Copy link
Member

liggitt commented Apr 11, 2023

as a follow-up, I'd recommend scheduling folks open an issue to track improving the testing of external plugins in a way that would have prevented this in the first place

@sanposhiho
Copy link
Member Author

ok, I'll include the change to add the description as the known issue to the release note once v1.27 is released.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 11, 2023
@alculquicondor
Copy link
Member

as a follow-up, I'd recommend scheduling folks open an issue to track improving the testing of external plugins in a way that would have prevented this in the first place

@sanposhiho can you add an integration test?

@Huang-Wei Huang-Wei mentioned this pull request Apr 11, 2023
Closed
@Huang-Wei
Copy link
Member

as a follow-up, I'd recommend scheduling folks open an issue to track improving the testing of external plugins in a way that would have prevented this in the first place

Creating #117215 to track follow-ups.

@Huang-Wei
Copy link
Member

@sanposhiho can you add an integration test?

We can have a separate test. And have this PR easy to back-port.

@sanposhiho
Copy link
Member Author

sanposhiho commented Apr 11, 2023 •
edited
Loading

added the known issue section on the release note v1.26. (or should I create a separate PR to update the release note?

@k8s-ci-robot k8s-ci-robot added area/release-eng Issues or PRs related to the Release Engineering subproject sig/release Categorizes an issue or PR as relevant to SIG Release. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 11, 2023
@sanposhiho
Copy link
Member Author

can you add an integration test?

We can have a separate test. And have this PR easy to back-port.

Sure. I'll follow up this in another PR.

@sanposhiho sanposhiho mentioned this pull request Apr 11, 2023
Closed
@sanposhiho
Copy link
Member Author

sanposhiho commented Apr 13, 2023 •
edited
Loading

@alculquicondor @Huang-Wei @ahg-g Could anyone take a look? 🙏
I'd like to get this on the release note as soon as possible.

@alculquicondor
Copy link
Member

You already have Wei's approval, you are missing one for CHANGELOG

@Verolop
Copy link

Verolop commented Apr 13, 2023

/approve

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Apr 13, 2023
@cici37
Copy link
Contributor

cici37 commented Apr 13, 2023

/unhold
Sorry accidentally did it in the wrong pr.

@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 Apr 13, 2023
@alculquicondor alculquicondor mentioned this pull request Apr 13, 2023
Merged
@alculquicondor
Copy link
Member

cherry-pick for 1.27 #117308

@dims
Copy link
Member

dims commented Apr 13, 2023

/approve
/lgtm

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

LGTM label has been added.

Git tree hash: 320579c4fa054169c4ca9538b78cf28495a68c2c

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, Huang-Wei, sanposhiho, Verolop

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

alculquicondor
alculquicondor reviewed Apr 13, 2023
View reviewed changes
CHANGELOG/CHANGELOG-1.27.md

### The PreEnqueue extension point doesn't work for Pods going to activeQ through backoffQ

In v1.26.0, we've found the bug that the PreEnqueue extension point doesn't work for Pods going to activeQ through backoffQ.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.27.0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, I'll fix in another PR 🙏

CHANGELOG/CHANGELOG-1.27.md
In v1.26.0, we've found the bug that the PreEnqueue extension point doesn't work for Pods going to activeQ through backoffQ.
It doesn't affect any of the vanilla Kubernetes behavior, but, may break custom PreEnqueue plugins.

The cause PR is [reverted](https://github.com/kubernetes/kubernetes/pull/117194) by v1.26.1.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.27.1

@xmudrii
Copy link
Member

xmudrii commented Apr 13, 2023

Seems unrelated.
/retest

@k8s-ci-robot k8s-ci-robot merged commit 29c8fb6 into kubernetes:master Apr 13, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.28 milestone Apr 13, 2023
k8s-ci-robot added a commit that referenced this pull request Apr 13, 2023
@k8s-ci-robot
Merge pull request #117308 from alculquicondor/automated-cherry-pick-…
1f69edd 
…of-#117194-upstream-release-1.27

Automated cherry pick of #117194: Revert "Optimization on running prePreEnqueuePlugins
@liggitt liggitt added the kind/regression Categorizes issue or PR as related to a regression from a prior release. label Sep 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Huang-Wei Huang-Wei Huang-Wei left review comments

@alculquicondor alculquicondor alculquicondor left review comments

@chendave chendave Awaiting requested review from chendave

@denkensk denkensk Awaiting requested review from denkensk

@ahg-g ahg-g Awaiting requested review from ahg-g

Assignees

@dims dims

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/release-eng Issues or PRs related to the Release Engineering subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/regression Categorizes issue or PR as related to a regression from a prior release. 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/release Categorizes an issue or PR as relevant to SIG Release. 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
Milestone
v1.28
Development

Successfully merging this pull request may close these issues.
9 participants
@sanposhiho @k8s-ci-robot @liggitt @alculquicondor @Huang-Wei @Verolop @cici37 @dims @xmudrii