fix: hierarchical queue webhook validation use listing podgroups instead #3913
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
volcano-sh-bot
added
the
size/M
JesseStutler
force-pushed
the
czc_dev
branch
from
b3efb1f to
10ccc99
Compare
volcano-sh-bot
requested review from
archlitchi,
googs1025,
lowang-bh and
Monokaix
|
/area webhooks |
|
Please make the CI happy :) |
| return fmt.Errorf("queue %s cannot be the parent queue of queue %s because it has PodGroups (pending: %d, running: %d, unknown: %d, inqueue: %d)", | ||
| parentQueue.Name, queue.Name, parentQueue.Status.Pending, | ||
| parentQueue.Status.Running, parentQueue.Status.Unknown, parentQueue.Status.Inqueue) | ||
| pgList, err := config.VolcanoClient.SchedulingV1beta1().PodGroups("").List(context.Background(), metav1.ListOptions{}) |
There was a problem hiding this comment.
Informer will list/watch all podgroups, which increases webhook memory consumption. I don’t really want to sacrifice memory for a verification.
There was a problem hiding this comment.
But not using the informer will put more pressure on the apiserver.
There was a problem hiding this comment.
Just a suggestion, we can look at other people's opinions
There was a problem hiding this comment.
We can check queue.status.allocated.
There was a problem hiding this comment.
updated, I have verified, it works
JesseStutler
force-pushed
the
czc_dev
branch
2 times, most recently
from
37f0dd4 to
9ecf734
Compare
volcano-sh-bot
added
size/S
Signed-off-by: JesseStutler <[email protected]>
|
/lgtm |
volcano-sh-bot
added
ok-to-test
fix #3911
validation
Steps:
parent-queueparent-queuechild-queueas the child ofparent-queueThe screenshot shows that create a child queue under parent queue which contains podgroups will be rejected:
