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
nodeaffinity: scheduler queueing hints #122309
Conversation
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. |
/assign @sanposhiho |
@@ -107,25 +107,14 @@ func (pl *NodeAffinity) isSchedulableAfterNodeChange(logger klog.Logger, pod *v1 | |||
if err != nil { | |||
return framework.Queue, err | |||
} | |||
if !isMatched { |
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.
Maybe it's also worth commenting this code rather than just delete it?
We can just uncomment this code after removing preCheck
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.
cc @sanposhiho what do you think?
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.
I don't like commented code generally speaking. We're not sure we'd be able to use commented code as it is when we uncomment them. Plus, we can just revisit the previous implementation when we want -especially this case, we actually committed it into master, we can just find it in history.
/unhold Now the revert PR is merged. |
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.
Thanks for a prompt action, LGTM overall. Just have a small suggestion. Also, I guess you have to rebase on the master (since it's reverted.)
pkg/scheduler/framework/plugins/nodeaffinity/node_affinity_test.go
Outdated
Show resolved
Hide resolved
@carlory Can you put a bit more context in the PR description? (example: we previously implemented NodeAffinity QHint in PR-A, but we reverted that in PR-B because of Issue-A, this PR implements QHint again with taking the problematic scenario into consideration) |
5dbb1f5
to
4d7c5d9
Compare
@sanposhiho updated. |
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
@kubernetes/sig-scheduling-approvers Anyone please take a look for /approve
LGTM label has been added. Git tree hash: d83addf48cfc2d9392a9d1085973552051a3b834
|
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, carlory 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:
we previously implemented NodeAffinity QHint in #119155, but we reverted that in #122285 because of #122284, this PR implements QHint again with taking the problematic scenario into consideration
Which issue(s) this PR 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.: