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.

Sign up for GitHub

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

Jump to bottom

workqueue: make queue as configurable #123347

Merged
merged 1 commit into from
Apr 18, 2024
Merged

workqueue: make queue as configurable #123347

merged 1 commit into from
Apr 18, 2024
+100 −14

Conversation

zhouhaibing089
Copy link
Contributor

The default queue implementation is mostly FIFO and it is not exchangeable unless we implement the whole workqueue.Interface which is less desirable as we have to duplicate a lot of code. There was one attempt done in kubernetes/kubernetes#109349 which tried to implement a priority queue. That is really useful and knative/pkg implemented something called two-lane-queue. While two lane queue is great, but isn't perfect since a full slow queue can still slow down items in fast queue.

This change proposes a swappable queue implementation while not adding extra maintenance effort in kubernetes community. We are happy to maintain our own queue implementation (similar to two-lane-queue) in downstream.

What type of PR is this?

/kind feature

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 16, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @zhouhaibing089. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Feb 16, 2024
@k8s-ci-robot k8s-ci-robot requested review from aojea and yliaog February 16, 2024 20:17
@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 16, 2024
@zhouhaibing089 zhouhaibing089 force-pushed the abstract-queue branch 4 times, most recently from fb4bae7 to c3f0d65 Compare February 16, 2024 22:23
@heqg
Copy link
Contributor

heqg commented Feb 18, 2024

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 18, 2024
heqg
heqg approved these changes Feb 18, 2024
View reviewed changes
staging/src/k8s.io/client-go/util/workqueue/queue.go

type queue []interface{}

func (q *queue) Touch(item interface{}) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 18, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: d1937022f322118f691473778cd8dfc907d1a500

@heqg
Copy link
Contributor

heqg commented Feb 18, 2024

/lgtm

@zhouhaibing089
Copy link
Contributor Author

/retest

@zhouhaibing089
Copy link
Contributor Author

/assign @aojea

@aojea
Copy link
Member

aojea commented Feb 18, 2024

why not forking the code?

@zhouhaibing089
Copy link
Contributor Author

why not forking the code?

Forking is always the last resort (but I made it available in https://github.com/zhouhaibing089/workqueue). It would be super nice to be part of client-go while leaving the different implementations in downstream, since the code structure is mostly (99%) the same. Do you see a good reason to not have this in client-go? I assume this is a pretty minimal change, but people may feel differently, I'm open to discuss.

@zhouhaibing089
Copy link
Contributor Author

zhouhaibing089 commented Feb 19, 2024 •
edited
Loading

All good questions.

Is there any use case in the current codebase for this change?

In fact, yes. For example, in resourcequota controller, there are primary_queue and priority_queue, and there are two workers dealing with each queue respectively. While it might be fine for the same key being processed in two workers in that scenario, it is certainly not a common pattern - a key should be exclusively handled in one worker.

Is this only going to be used by external projects?

Yeah, client-go is already used as dependencies in many other external projects. (though may not be "only")

having code just only to be consumed by external clients increase the surface area and the effort to support such code.

I completely agree and thus why I only proposed an interface change without changing the existing implementation (it is still FIFO queue implemented with []t). The whole point of this change is to make extension possible. There might be a fuzzy line between extending in workqueue.Interface vs the new proposed workqueue.Queue. I favor the later since there is no need to copy paste.

Now as I said earlier, there are indeed use cases in kubernetes as well. I'm also open to bring this step even further (for e.g, proposing two-lane-queue implementation in kubernetes by default).

@aojea
Copy link
Member

aojea commented Feb 19, 2024

You may run this through sig-apimachinery , this is a critical piece of the project and they are the final decision takers

/cc @jpbetz @deads2k @sttts

@jiahuif
Copy link
Member

jiahuif commented Mar 14, 2024

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 14, 2024
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 24, 2024
@k8s-ci-robot k8s-ci-robot requested a review from heqg March 24, 2024 06:13
sttts
sttts reviewed Mar 25, 2024
View reviewed changes
staging/src/k8s.io/client-go/util/workqueue/queue.go Outdated
@@ -33,6 +33,46 @@ type Interface interface {
ShuttingDown() bool
}

// Queue is the underlying storage for items.
type Queue interface {
// Touch marks an existing item when it is added again.
Copy link
Contributor

Choose a reason for hiding this comment

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

what does it means semantically? I assume it will keep the object in the queue, but might change the position in the queue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it basically says the same item while still in the queue is now being added again. For the implementations which can change the priority (or position in your term), this function can be hooked.

@zhouhaibing089
workqueue: make queue as configurable
87b4279 
The default queue implementation is mostly FIFO and it is not
exchangeable unless we implement the whole `workqueue.Interface` which
is less desirable as we have to duplicate a lot of code. There was one
attempt done in [kubernetes#109349][1] which tried to
implement a priority queue. That is really useful and [knative/pkg][2]
implemented something called two-lane-queue. While two lane queue is
great, but isn't perfect since a full slow queue can still slow down
items in fast queue.

This change proposes a swappable queue implementation while not adding
extra maintenance effort in kubernetes community. We are happy to
maintain our own queue implementation (similar to two-lane-queue) in
downstream.

[1]: kubernetes#109349
[2]: https://github.com/knative/pkg/blob/main/controller/two_lane_queue.go
@sttts
Copy link
Contributor

sttts commented Mar 26, 2024

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 26, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: heqg, sttts, zhouhaibing089

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

@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: fca36852221393a6479c1cf9688bcf0f36b44f8c

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 26, 2024
@k8s-ci-robot k8s-ci-robot merged commit 301eb8d into kubernetes:master Apr 18, 2024
14 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.31 milestone Apr 18, 2024
@alvaroaleman alvaroaleman mentioned this pull request Apr 19, 2024
Merged
@zhouhaibing089 zhouhaibing089 deleted the abstract-queue branch June 24, 2024 04:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

@aojea aojea Awaiting requested review from aojea

@yliaog yliaog Awaiting requested review from yliaog

@deads2k deads2k Awaiting requested review from deads2k

@jpbetz jpbetz Awaiting requested review from jpbetz

@heqg heqg Awaiting requested review from heqg

Assignees

@sttts sttts

@aojea aojea

@heqg heqg

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.31
Development

Successfully merging this pull request may close these issues.
6 participants
@zhouhaibing089 @k8s-ci-robot @heqg @aojea @jiahuif @sttts