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
Activate unschedulable pods only if the node became more schedulable #70366
Conversation
@mlmhl please make the CI happy(seem like you need to gofmt the code). |
And I think this PR need a effective release note. |
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 so much for working on this so quickly. Given that nodes send updates every 10 seconds, in large clusters we receive hundreds of node updates per second. So, it is important that the checks be very quick. That's the main reason that I suggested a couple of changes to make the logic simpler. Of course, simpler logic is easier to maintain as well.
pkg/scheduler/factory/factory.go
Outdated
if reflect.DeepEqual(oldAllocatable, newAllocatable) { | ||
return false | ||
} | ||
for resource, newValue := range newAllocatable { |
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.
What you have done makes sense, but in order to be quicker in checking node changes and also to be conservative, I would return "true" as long allocatables are changed, no matter whether they are reduced or increased. In other words, remove this for loop and just return true.
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.
Done
pkg/scheduler/factory/factory.go
Outdated
return false | ||
} | ||
|
||
healthyConditions := []v1.NodeConditionType{v1.NodeReady} |
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.
Similar to my previous comment, I would return true as long as old and new conditions are not equal. This will help reduce chances of introducing bugs in the future when new node conditions are added.
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.
Done
f7033a4
to
c50f89d
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
/approve
Thanks, @mlmhl!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bsalamat, mlmhl 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 |
/test pull-kubernetes-e2e-gce-100-performance |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This is a performance optimization for scheduler:
Move unschedulable pods to active queue only if a node's scheduling related properties updated. This PR considers node allocatable, node conditions, node taints, node labels and
Node.Spec.Unschedulable
as scheduling related properties.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #70316
Does this PR introduce a user-facing change?: