Add the ability to dictate custom retries #962

sargun · 2021-04-09T08:57:10Z

Our current retry policy is naive and only does 20 retries. It is
also based off of the rate limiter. If the user is somewhat aggressive in
rate limiting, but they have a temporary outage on API server, they
may want to continue to delay.

In facts, K8s has a built-in function to suggest delays:
https://pkg.go.dev/k8s.io/apimachinery/pkg/api/errors#SuggestsClientDelay

Signed-off-by: Sargun Dhillon sargun@sargun.me

node/podcontroller.go

internal/queue/queue.go

cpuguy83 · 2021-04-09T17:51:45Z

internal/queue/queue.go

-		span.WithField(ctx, "delay", ratelimitDelay.String())
-		val.plannedToStartWorkAt = val.plannedToStartWorkAt.Add(ratelimitDelay)
-		val.delayedViaRateLimit = &ratelimitDelay
+		span.WithField(ctx, "delay", actualDelay.String())


We should special case stringers in the tracing so it only does the conversion when we are actually collecting the trace.

How do you think we should special case nil duration? or should nil always just be nil

We could say use -1 (or < 0) instead of nil to avoid all these pointer conversions.

There's already code (And tests for it), that allow you to pass a negative delay from the rate limiter back and result in it being processed immediately. That would regress / change this behaviour.

I don't understand why we did that since 0 should mean no delay. But it is what it is.

It's the way the kubernetes ratelimiters work, and we kept the contract the same.

cpuguy83 · 2021-04-13T21:29:24Z

node/queue.go

+//
+// If the work item should be is to be retried, a delay duration may be specified. If the value is nil, it will fall
+// back to the default behaviour of the queue, and use the ratelimiter that's configured to determine when to start work.
+type ShouldRetryFunc = queue.ShouldRetryFunc


What happens when the returned delay is a negative value?

Can you point me to what changed? I'm not noticing.

huh, no idea why it didn't update inline:

ShouldRetryFunc is a mechanism to have a custom retry policy

it is passed metadata about the work item when the handler returns an error. It returns the following:

The key

The number of attempts that this item has already had (and failed)

The (potentially wrapped) error from the queue handler.

The return value is an error, and optionally an amount to delay the work.
If an error is returned, the work will be aborted, and the returned error is bubbled up. It can be the error that
was passed in or that error can be wrapped.

If the work item should be is to be retried, a delay duration may be specified. The delay is used to schedule when
the item should begin processing relative to now, it does not necessarily dictate when the item will start work.
Items are processed in the order they are scheduled. If the delay is nil, it will fall back to the default behaviour
of the queue, and use the rate limiter that's configured to determine when to start work.

I'm not seeing where we are calling out what a negative value means, only nil.

It doesn't call out the negative value explicitly, but it says that the ordering is based on now + delay. If the delay is negative, your task will be scheduled in the past, thus pushing it to the front of the queue. Do you think we need to be explicit about it?

I think it is worth mentioning.

Added:

// If the delay is negative, the item will be scheduled "earlier" than now. This will result in the item being executed
// earlier than other items in the FIFO work order.

cpuguy83 · 2021-04-14T17:25:26Z

Looks like there's a test timeout.

This starts watching for events prior to the start of the controller. This smells like a bug in the fakeclient bits, but it seems to fix the problem. Signed-off-by: Sargun Dhillon <sargun@sargun.me>

Our current retry policy is naive and only does 20 retries. It is also based off of the rate limiter. If the user is somewhat aggressive in rate limiting, but they have a temporary outage on API server, they may want to continue to delay. In facts, K8s has a built-in function to suggest delays: https://pkg.go.dev/k8s.io/apimachinery/pkg/api/errors#SuggestsClientDelay Signed-off-by: Sargun Dhillon <sargun@sargun.me>

sargun · 2021-04-14T20:40:56Z

Huh, didn't happen again.

CC: @cpuguy83

sargun · 2021-04-15T05:14:48Z

I literally ran:
go test -run=TestPodLifecycle/Async/testDanglingPodScenarioWithDeletionTimestamp/mockProvider -v -count=1000 -parallel=8 -race ./node 2>&1|tee test.log

and no errors.

sargun · 2021-04-15T06:08:31Z

There's somehow a situation where "time="2021-04-14T20:50:38Z" level=warning msg="skipping sync of pod "default/my-pod" in "Succeeded" phase"" is happening, and this results in it not being put on the deletionQ.

I don't see how this is possible.

cpuguy83

LGTM

sargun requested a review from cpuguy83 April 9, 2021 08:57

cpuguy83 requested changes Apr 9, 2021

View reviewed changes

sargun force-pushed the expose-custom-retry branch from 87594c4 to 0eaede8 Compare April 12, 2021 02:52

sargun requested a review from cpuguy83 April 12, 2021 02:53

sargun force-pushed the expose-custom-retry branch 3 times, most recently from 80f2c01 to 85800f6 Compare April 13, 2021 06:03

cpuguy83 reviewed Apr 13, 2021

View reviewed changes

sargun force-pushed the expose-custom-retry branch 2 times, most recently from 79e61e5 to 27a8050 Compare April 13, 2021 21:40

sargun requested a review from cpuguy83 April 13, 2021 21:41

sargun force-pushed the expose-custom-retry branch from 27a8050 to 88606b1 Compare April 14, 2021 16:36

sargun added 2 commits April 14, 2021 10:52

Fix test

e95023b

This starts watching for events prior to the start of the controller. This smells like a bug in the fakeclient bits, but it seems to fix the problem. Signed-off-by: Sargun Dhillon <sargun@sargun.me>

sargun force-pushed the expose-custom-retry branch from 88606b1 to b259cb0 Compare April 14, 2021 17:52

cpuguy83 approved these changes Apr 15, 2021

View reviewed changes

cpuguy83 merged commit bf3a764 into virtual-kubelet:master Apr 15, 2021

sargun deleted the expose-custom-retry branch April 15, 2021 23:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to dictate custom retries #962

Add the ability to dictate custom retries #962

sargun commented Apr 9, 2021

cpuguy83 Apr 9, 2021

sargun Apr 9, 2021

cpuguy83 Apr 13, 2021

sargun Apr 13, 2021

cpuguy83 Apr 13, 2021

sargun Apr 13, 2021

cpuguy83 Apr 13, 2021

sargun Apr 13, 2021

cpuguy83 Apr 13, 2021

sargun Apr 13, 2021

cpuguy83 Apr 14, 2021

sargun Apr 14, 2021

cpuguy83 Apr 14, 2021

sargun Apr 14, 2021

cpuguy83 commented Apr 14, 2021

sargun commented Apr 14, 2021

sargun commented Apr 15, 2021

sargun commented Apr 15, 2021

cpuguy83 left a comment

Add the ability to dictate custom retries #962

Add the ability to dictate custom retries #962

Conversation

sargun commented Apr 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpuguy83 commented Apr 14, 2021

sargun commented Apr 14, 2021

sargun commented Apr 15, 2021

sargun commented Apr 15, 2021

cpuguy83 left a comment

Choose a reason for hiding this comment