-
Notifications
You must be signed in to change notification settings - Fork 0
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
POC: Preemption Toleration Plugin Implementation and Tests #2
Conversation
fcc90e3
to
3465f9a
Compare
755bf93
to
8b4b6b8
Compare
1820505
to
599614e
Compare
8b4b6b8
to
245aa60
Compare
599614e
to
63b20e7
Compare
245aa60
to
4a029e0
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.
The overall design looks good. I left some comments on implementation details.
} | ||
|
||
// check it can tolerate the preemption in terms of priority value | ||
canTolerateOnPriorityValue := preemptorPriority < *pt.MinimumPreemptablePriority |
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.
pt
can be nil when the priority class does not have the toleration policy annotation.
// check it can tolerate the preemption in terms of toleration seconds | ||
_, scheduledCondition := podutil.GetPodCondition(&victimCandidate.Status, v1.PodScheduled) | ||
if scheduledCondition == nil { | ||
// if victim is not scheduled, skip evaluating tolerationSeconds |
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.
In what case a pod becomes a victim candidate even though it has not been scheduled yet?
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 misunderstood that nominated pods would be passed here. It should not happen here. But I would like to keep this guard. So, I will just delete the comment. Thanks for the catch.
if tt.canTolerate { | ||
// - the victim candidate pod keeps being scheduled, and | ||
// - the preemptor pod is not scheduled | ||
if err := wait.Poll(1*time.Second, 30*time.Second, func() (bool, error) { |
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.
wait.Poll()
returns immediately once the given condition function returns true, but here we should assert that the function continuously returns true for some seconds instead.
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.
Correct. I will fix this gomega's Consistently
like assertion.
@ordovicia @shioshiota I fixed things commented. PTAL. |
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
c1d51fa
to
a5cf119
Compare
squashed. |
4582a67
to
b97edf6
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.
Sorry, but I found an issue with handling a preemption policy.
Could you check it?
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.
My comment was wrong. I approve of this again.
f8042eb
to
cd1f519
Compare
4fae905
to
aa22f0c
Compare
…ration-kep KEP: Preemption Toleration Plugin
because most of identical code is needed but they're not exposed.
b97edf6
to
66a6703
Compare
I close this PR and delete the |
This PR is a PoC implementation of "Preemption Toleration" plugin proposed in (kubernetes-sigs#205).
Many code is borrowed from default preemption plugin in kube-scheduler because this plugin requires almost identical logics with this but most of them are not exposed. I believe kubernetes/kubernetes#95131 resolves it.