Skip to content
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

perf: imporve end to end queue processing latency #3772

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

fengruotj
Copy link
Contributor

@fengruotj fengruotj commented Oct 15, 2024

The current implementation of queue controller handles queue state operations and queuecommand operations for sequentially which may come into a performance bottleneck in performance-critical clusters. For example the end user may increase queuecommand & queue creation speed, so this pr add an option to make queue & queuecommand creation parallelly to handle such case.

Here, queue-controller workqueue pending process object metrics.
image

After perf: imporve end to end queue processing latency, the queue-controller workqueue pending process object finally reduced.

image

@volcano-sh-bot
Copy link
Contributor

Welcome @fengruotj!

It looks like this is your first PR to volcano-sh/volcano.

Thank you, and welcome to Volcano. 😃

@volcano-sh-bot volcano-sh-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 15, 2024
@fengruotj
Copy link
Contributor Author

hi @kevin-wangzefeng, this is the perf: imporve end to end queue processing latency
/assign @kevin-wangzefeng

@Monokaix
Copy link
Member

Hi, please sign off your commit with git commit -s.

@Monokaix
Copy link
Member

/ok-to-test

@volcano-sh-bot volcano-sh-bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Oct 16, 2024
Signed-off-by: tanjie.master <tanjiemaster@gmail.com>
@fengruotj
Copy link
Contributor Author

fengruotj commented Oct 16, 2024

Hi, please sign off your commit with git commit -s.

Hi, I've already signed off my commit wiht git commit -s. Thanks you very much~

@fengruotj
Copy link
Contributor Author

/assign @Monokaix @kevin-wangzefeng

@Monokaix
Copy link
Member

/lgtm
cc @hwdef @lowang-bh

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 16, 2024
Copy link
Member

@hwdef hwdef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@fengruotj
Copy link
Contributor Author

@hwdef I've considered this as well. The main issue is that the state transition of the queue involves queuecommand, and commandWorker is primarily used to handle the queuecommand CRD, which is then passed to the worker for processing by workqueue. The main performance bottleneck lies in both the worker and the commandWorker. I believe concurrency should be applied to both.

@hwdef
Copy link
Member

hwdef commented Oct 16, 2024

@fengruotj
Yes, you are right.

@Monokaix
Copy link
Member

/approve

@volcano-sh-bot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@volcano-sh-bot volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2024
@volcano-sh-bot volcano-sh-bot merged commit d8162a5 into volcano-sh:master Oct 18, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants