-
Notifications
You must be signed in to change notification settings - Fork 550
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
register custom resource events for efficient pod enqueuing #317
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Huang-Wei 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 |
/check-cla |
return []framework.ClusterEvent{ | ||
{Resource: framework.Pod, ActionType: framework.Delete}, | ||
// TODO: once bump the dependency to k8s 1.22, addd custom object events. | ||
{Resource: framework.GVK(eqGVK), ActionType: framework.Add | framework.Update}, |
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.
We also need to include the Delete event for elasticquota. Pod may be scheduled successfully if the eq is deleted.
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.
Can you elaborate in which circumstance a deleted eq would potentially make a pod (failed by this plugin) schedulable? My understanding is that the unschedulable pods needs to be get its own label updated, and it's irrelevant with EQ's change.
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 wonder if there is a scenario.
Scheduling of PodA in namespace A fails because the used in eq of namespace A will exceed the max rather than a lack of cluster resources. Then we remove the eq in namespace A so that it is no longer restricted to pod in namespace A. At this point we may need to have the pod in namespace A rescheduled.
But in fact we are not allowed to delete eq in a production environment if there are pods in the namespace A, unless the pods are deleted first.
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.
sg, I will update the event to All
.
9a9b458
to
0d7aa7a
Compare
return []framework.ClusterEvent{ | ||
{Resource: framework.Pod, ActionType: framework.Delete}, | ||
{Resource: framework.Node, ActionType: framework.Add | framework.UpdateNodeAllocatable}, | ||
{Resource: framework.GVK(nrtGVK), ActionType: framework.Add | framework.Update}, |
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.
FYI: @swatisehgal @AlexeyPerevalov: this change is an optimization to give a pod failed by NRT filter plugin higher chance to be retried immediately. Here I think only NRT's add and update events are correlated.
0d7aa7a
to
b61deb0
Compare
/lgtm Thanks @Huang-Wei |
Along with kubernetes/kubernetes#101394, the downstream plugins can register custom resource event(s) so as to trigger efficient pod queueing.