Rework the Lock API so that locks can be auto-renewed.#482
Rework the Lock API so that locks can be auto-renewed.#482Julian Gutierrez Oschmann (juli4n) wants to merge 1 commit into
Conversation
Instead of requiring callers to specify a fixed deadline for the lock, let the store return a `Lock` object that performs auto-renewal in background. This fixes an issue when a workflow takes longer than the hard-coded TTL, most commonly when atelet takes long times to initialize an actor (e.g. pulling / untaring large OCI images). Leases are taken for 30 seconds, but renewed every 10 seconds (TTL/3). Removing that implicit TTL-based limit means a caller that sets no deadline of its own (e.g. kubectl-ate) could otherwise hang a Resume/Suspend/Pause indefinitely. MaxDeadlineUnaryInterceptor adds a 5-minute ceiling on every RPC as a hang-prevention backstop.
| case <-ctx.Done(): | ||
| return | ||
| case <-timer.C: | ||
| callCtx, cancel := context.WithTimeout(ctx, interval) |
There was a problem hiding this comment.
🟡 🤖 If Redis is unreachable, the workflow can keep running past lease expiry. A retry's callCtx allows a full extra interval, and the elapsed-TTL check runs only after the attempt returns. So the lease context may not cancel until ~8s after the key expired, when another replica can already hold the lock. The old design cancelled the workflow 2s before the TTL.
Fix: declare the lease lost no later than lastRenewed.Add(ttl), e.g. cap the retry callCtx at time.Until(lastRenewed.Add(ttl)).
There was a problem hiding this comment.
+1. Also provided more context on the comment below, maybe also with some buffer time.
| ) | ||
|
|
||
| // maxRPCDeadline is the max deadline for all RPC methods exposed by this server. | ||
| const maxRPCDeadline = 10 * time.Minute |
There was a problem hiding this comment.
🟢 🤖 The PR description says the backstop is 5 minutes; this sets 10. Worth aligning one or the other.
Jeffrey Ying (Jefftree)
left a comment
Jeffrey Ying (Jefftree)
left a comment
There was a problem hiding this comment.
👋 Ben asked me to take a look at this, I work on the leader election library in k8s core. Providing a couple suggestions based on my experience with k8s
| slog.WarnContext(ctx, "lock renewal found lease no longer owned", "key", key) | ||
| return | ||
|
|
||
| case time.Since(lastRenewed) >= ttl: |
There was a problem hiding this comment.
k8s has precedence in that we explicitly accept that lease is lost well ahead of the ttl duration if renew is deemed unhealthy. Inflight operations generally take time stop (eg: I see that there was an explicit stop at 28s instead of stopping at the full 30s). Getting here means the renew failed multiple times already and another instance is taking over. I understand substate has a different set of requirements so it doesn't need to exactly follow k8s shape, just providing it as reference. K8s uses a separate Deadline parameter kept below the lease duration (defaults to 2/3 of actual deadline) to account for things like clock skew so the holder gives up while there's still time left on a lease lock. This gives the renewal 2 tries before accepting that the renewal isn't working.
Suggestion: keep a padding here
| case <-ctx.Done(): | ||
| return | ||
| case <-timer.C: | ||
| callCtx, cancel := context.WithTimeout(ctx, interval) |
There was a problem hiding this comment.
+1. Also provided more context on the comment below, maybe also with some buffer time.
| value1 := "token-1" | ||
| value2 := "token-2" | ||
| ttl := 10 * time.Second | ||
| ttl := 300 * time.Millisecond // renewed every ~100ms; overriding s.lockTTL lets the test avoid waiting out the real lockTTL. |
There was a problem hiding this comment.
nit: using a real clock has been sometimes brittle with CI and race conditions. Would recommend eventually transitioning to inject a fake clock that you can deterministically control to test for edge cases.
There was a problem hiding this comment.
we might be able to use https://go.dev/blog/testing-time
| } | ||
|
|
||
| // 2. Fast-forward time past TTL | ||
| mr.FastForward(6 * time.Second) |
There was a problem hiding this comment.
nit: I'd imagine you want to keep testing the path of actual TTL expiry (crash/give up) resulting in allowing another client to grab the lock? With renewals, maybe that involves two clients now.
3 participants
Instead of requiring callers to specify a fixed deadline for the lock, let the store return a
Lockobject that performs auto-renewal in background.This fixes an issue when a workflow takes longer than the hard-coded TTL, most commonly when atelet takes long times to initialize an actor (e.g. pulling / untaring large OCI images).
Leases are taken for 30 seconds, but renewed every 10 seconds (TTL/3).
Removing that implicit TTL-based limit means a caller that sets no deadline of its own (e.g. kubectl-ate) could otherwise hang a Resume/Suspend/Pause indefinitely. MaxDeadlineUnaryInterceptor adds a 10-minute ceiling on every RPC as a hang-prevention backstop.