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

implement ForgetDelayed in delaying queue #113607

Closed
wants to merge 2 commits into from
Closed

implement ForgetDelayed in delaying queue #113607

wants to merge 2 commits into from

Conversation

atiratree
Copy link
Member

@atiratree atiratree commented Nov 3, 2022 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

#108266 issue is caused by inability of controllers to cancel scheduled insertions to the queue when using AddAfter. This PR proposes a new function called ForgetDelayed that would achieve this.

I am trying to showcase how this would be used in the fix #113605 of the mentioned bug.

This was also suggested way back in kubernetes/client-go#131 and it seems it would also help other people.

Which issue(s) this PR fixes:

Fixes kubernetes/client-go#131

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

new ForgetDelayed method for consumers of RateLimitingInterface

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. 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-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 3, 2022
@k8s-ci-robot k8s-ci-robot requested review from deads2k and sttts November 3, 2022 20:50
@atiratree atiratree mentioned this pull request Nov 3, 2022
Closed
2 tasks
@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 Nov 3, 2022
@atiratree atiratree mentioned this pull request Nov 3, 2022
Open
@atiratree atiratree force-pushed the delaying-queue-forget-delayed branch from 6abfde8 to 58ef226 Compare November 3, 2022 21:11
atiratree
atiratree commented Nov 3, 2022
View reviewed changes
staging/src/k8s.io/client-go/util/workqueue/delaying_queue.go
@@ -176,7 +180,22 @@ func (q *delayingType) AddAfter(item interface{}, duration time.Duration) {
select {
Copy link
Member Author

Choose a reason for hiding this comment

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

reagarding the q.metrics.retry() in AddAfter. It depends what we want to measure with it.

We advertize this:

kubernetes/staging/src/k8s.io/component-base/metrics/prometheus/workqueue/metrics.go

Line 93 in d62cc3d

Help: "Total number of retries handled by workqueue",

But currently, it only correctly measures calls to AddAfter. It might not correspond to the number of adds to the workqueue since it also measures retries of the same element that is still waiting (will be queued only once).

This metric might be even less precise when used together with ForgetDelayed, since it will also count items that were canceled.

We could also decide to trigger the metric just before each q.Add(item) statement to be sure what we track. Nevertheless it will change the original behaviour.

Thoughts?

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: atiratree
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval by writing /assign @deads2k in a comment. For more information see the Kubernetes Code Review Process.

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

@k8s-ci-robot k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Nov 3, 2022
@atiratree atiratree force-pushed the delaying-queue-forget-delayed branch from f1a457f to 78e7fd2 Compare November 4, 2022 11:45
@atiratree atiratree force-pushed the delaying-queue-forget-delayed branch from 78e7fd2 to ddc35e2 Compare November 4, 2022 11:48
@fedebongio
Copy link
Contributor

/cc @liggitt
/triage accepted

@k8s-ci-robot k8s-ci-robot requested a review from liggitt November 8, 2022 21:23
@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 Nov 8, 2022
@liggitt
Copy link
Member

liggitt commented Nov 16, 2022

Between the API this PR adds and the control of the delaying queue added in #112328, adding the ability to specify what should happen to already-queued, waiting-to-be-queued-earlier and waiting-to-be-queued-later items when you call AddAfter seems more general-purpose and easier/safer to use.

Looking at how this capability would be used in #113605, the intent there looks to be "add after and preempt any waiting-to-be-queued items". Doing that with a single AddAfterWithOptions call that indicates exactly what it wants to happen to any existing waiting-to-be-queued items seems better.

@atiratree atiratree mentioned this pull request Nov 17, 2022
Closed
@atiratree
Copy link
Member Author

@liggitt thanks for bringing this PR to my attention. It will indeed cover most of our needs and is much more wholesome solution. Closing this PR and starting a discussion in the other one

@atiratree atiratree closed this Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k Awaiting requested review from deads2k

@sttts sttts Awaiting requested review from sttts

@liggitt liggitt Awaiting requested review from liggitt

Assignees
No one assigned
Labels
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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. 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. sig/apps Categorizes an issue or PR as relevant to SIG Apps. 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
No milestone
Development

Successfully merging this pull request may close these issues.

AddAfter should be able to cancel previous enqueues of the same key
4 participants
@atiratree @k8s-ci-robot @fedebongio @liggitt