[Proposal] Add podgroup statistics doc #3750
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
volcano-sh-bot
added
the
size/S
|
/cc @hwdef @Monokaix @lowang-bh Please take a look~ I will push the other relating pr lately. |
|
Should also add that queue's pg statistics are still maintained in queue cache because these data is needed when close a queue. |
hwdef
reviewed
Sep 30, 2024
docs/design/podgroup-statistics.md
Outdated
|
|
||
| ## Backgrounds | ||
|
|
||
| Each time when podgroups states changed, the controller will update the statistics of podgroup of each state in the queue's status. And at the end of each scheduling session, the volcano scheduler will also update the allocated filed in queue's status to recored the amount of the amount of resources allocated. Both components use `UpdateStatus` api to update the queue status, which will cause conflict errors. When the controller encounter such an error, it will trigger `AddRateLimited` to push back the podgroup into work queue, resulting in accumulation of memory leak. See in issue #3597. |
docs/design/podgroup-statistics.md
Outdated
| Each time when podgroups states changed, the controller will update the statistics of podgroup of each state in the queue's status. And at the end of each scheduling session, the volcano scheduler will also update the allocated filed in queue's status to recored the amount of the amount of resources allocated. Both components use `UpdateStatus` api to update the queue status, which will cause conflict errors. When the controller encounter such an error, it will trigger `AddRateLimited` to push back the podgroup into work queue, resulting in accumulation of memory leak. See in issue #3597. | ||
|
|
||
| ## Alternative | ||
| Currently the statistics of podgroup of eatch state are only used for display by vcctl, there is no need to be persisted in queue's status. So when users need to use `vcctl queue get -n [name]` or `vcctl list` to display queues and each state of podgroups in queue, vcctl should calculate podgroup statistics in client side and then display them. And we can export these statistics of podgroups in each state as metrics. |
There was a problem hiding this comment.
each state means podgroups in pending/running/unknown/inqueue/completed status
There was a problem hiding this comment.
Oh sorry, didn't notice that there was a typo, now I have fixed it.
JesseStutler
force-pushed
the
czc_dev_doc
branch
2 times, most recently
from
64a1556 to
8944bfd
Compare
I added a notice item to record this, please review it again. |
JesseStutler
force-pushed
the
czc_dev_doc
branch
from
8944bfd to
1fcfa91
Compare
volcano-sh-bot
added
ok-to-test
Monokaix
reviewed
Oct 24, 2024
docs/design/podgroup-statistics.md
Outdated
| Currently the statistics of podgroups of each state are only used for display by vcctl, there is no need to be persisted in queue's status. So when users need to use `vcctl queue get -n [name]` or `vcctl list` to display queues and each state of podgroups in queue, vcctl should calculate podgroup statistics in client side and then display them. And we can export these statistics of podgroups in each state as metrics. | ||
|
|
||
| ## Implementation | ||
| - In `syncQueue` of queue controller, we should not stat counts of podgroups in each state here, instead these statistics should be recorded as metrics and then be exported outside: https://github.com/volcano-sh/volcano/blob/release-1.10/pkg/controllers/queue/queue_controller_action.go#L41-L61. And `UpdateStatus` should not be used here: https://github.com/volcano-sh/volcano/blob/release-1.10/pkg/controllers/queue/queue_controller_action.go#L84-L87, the `UpdateStatus` interface will verify the resourceVersion in the apiserver, which may cause concurrent update conflicts. Instead, `ApplyStatus` should be used here to avoid this situation, because we only need to update the status of the queue. It should be noted that the controller currently does not have patch queue status permissions, so we should add a patch queue/status permission to the clusterrole. |
There was a problem hiding this comment.
Please paste a permanent code link.
Signed-off-by: jessestutler <[email protected]>
JesseStutler
force-pushed
the
czc_dev_doc
branch
from
1fcfa91 to
655a3f4
Compare
volcano-sh-bot
added
size/M
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Monokaix 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 |
volcano-sh-bot
added
the
approved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fix #3597
Backgrounds
Each time when podgroups states changed, the controller will update the statistics of podgroup of each state in the queue's status. And at the end of each scheduling session, the volcano scheduler will also update the allocated filed in queue's status to recored the amount of the amount of resources allocated. Both components use
UpdateStatusapi to update the queue status, which will cause conflict errors. When the controller encounter such an error, it will triggerAddRateLimitedto push back the podgroup into work queue, resulting in accumulation of memory leak. See in issue #3597.Alternative
Currently the statistics of podgroup of eatch state are only used for display by vcctl, there is no need to be persisted in queue's status. So when users need to use
vcctl queue get -n [name]orvcctl listto display queues and each state of podgroups in queue, vcctl should calculate podgroup statistics in client side and then display them. And we can export these statistics of podgroups in each state as metrics.Implementation
syncQueueof queue controller, we should not stat counts of podgroups in each state here, instead these statistics should recorded as metrics and then be exported outside: https://github.com/volcano-sh/volcano/blob/release-1.10/pkg/controllers/queue/queue_controller_action.go#L41-L61. AndUpdateStatusshould not be used here: https://github.com/volcano-sh/volcano/blob/release-1.10/pkg/controllers/queue/queue_controller_action.go#L84-L87, theUpdateStatusinterface will verify the resourceVersion in the apiserver, which may cause concurrent update conflicts. Instead,ApplyStatusshould be used here to avoid this situation, because we only need to update the status of the queue. It should be noted that the controller currently does not have patch queue status permissions, so we should add a patch queue/status permission to the clusterrole.vcctl get -n [name]andvcctl listdisplay the statistics of podgroups in each state from queue's status directly, instead we should do one more step, query the podgroups owend in the queue, stat the counts of podgroups in each state at thevcctlside, and then display them.