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
podtopologyspread: scheduler queueing hints #122195
Conversation
Skipping CI for Draft Pull Request. |
Please note that we're already in Test Freeze for the Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Tue Dec 5 22:11:44 UTC 2023. |
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/assign
/ok-to-test
@nayihz Let me know when it's ready to review.
afb8e23
to
75434ce
Compare
Pls help me to ensure the logic is correct. Then I will add some unit tests. @sanposhiho |
4da2907
to
f7059ec
Compare
f7059ec
to
6aa9721
Compare
Unit tests have been added. PTAL if you have time. Thanks. @sanposhiho |
6aa9721
to
b1e1795
Compare
b1e1795
to
df72067
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Thanks.
@kubernetes/sig-scheduling-approvers Anyone please take a look. The bot seems have a preference to assign me for /approve
😓
df72067
to
32d0a9a
Compare
32d0a9a
to
09a80df
Compare
// and make these pod in scheduling schedulable or unschedulable. | ||
{Event: framework.ClusterEvent{Resource: framework.Node, ActionType: framework.Add | framework.Delete | framework.UpdateNodeLabel}}, | ||
{Event: framework.ClusterEvent{Resource: framework.Node, ActionType: framework.Add | framework.Delete | framework.Update}, QueueingHintFn: pl.isSchedulableAfterNodeChange}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to add a TODO here to change Update
to UpdateNodeLabel
again, after #122306 fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, either is actually fine though, let's just keep it as it is, since we already have a comment in isSchedulableAfterNodeChange. If isSchedulableAfterNodeChange is re-implemented properly after #122306, an implementor or reviewer should just notice that they can change framework.Update to framework.UpdateNodeLabel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
LGTM label has been added. Git tree hash: 38da8281e7f1f8d9341986c254428bbfab013fc7
|
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, nayihz 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Part of #118893
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: