cache: apply the configured back-off and Get timeout - #22408
Conversation
getWatchLoop built its own defaultConfig() instead of reading the Config the cache was constructed with, so WithInitialBackoff had no effect: an unreachable endpoint was retried every 50ms whatever the caller asked for. The delay was also constant, which left MaxBackoff -- documented as capping "the exponential back-off between successive upstream watch retries" -- with nothing to cap and no reader anywhere in the package. GetTimeout had no reader either. The bootstrap Get ran on the cache's lifetime context, so an endpoint that accepts a connection and then goes quiet parks it forever: the retry loop never gets another turn and the cache never becomes ready, which blocks every Get and Watch on WaitReady. Take all three from c.cfg, double the delay after each failed attempt up to MaxBackoff, and bound the bootstrap Get. The retry timer now comes from the injected Clock, like the progress-notify timer already does, so the schedule is testable without sleeping. Reject the values that would make the loop spin, alongside the existing HistoryWindowSize and BTreeDegree checks. The back-off is deliberately not reset after a watch that ran for a while; that is a separate policy question from the one the Config already documents. Signed-off-by: datrixlab <325650023+datrixlab@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: datrixlab The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @datrixlab. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
1 participant
Three
Configfields incacheare documented and settable but never read.InitialBackoffandMaxBackoffgetWatchLoopbuilds its owndefaultConfig()rather than reading theConfigthe cache was constructed with:
So
WithInitialBackoffhas no effect at all — an unreachable endpoint is retriedevery 50ms no matter what the caller asked for. The delay is also constant, which
leaves
MaxBackoffwith nothing to cap; its doc comment promises it bounds "theexponential back-off between successive upstream watch retries", and there is no
exponential back-off and no reader of the field anywhere in the package.
GetTimeoutAlso unread. The bootstrap
Getruns on the cache's lifetime context, so anendpoint that accepts the connection and then goes quiet parks it indefinitely:
the retry loop never gets another turn, the cache never becomes ready, and every
Get/Watchblocked onWaitReadystays blocked. This is the failure modeGetTimeoutexists to prevent.Change
Read all three from
c.cfg, double the delay after each failed attempt up toMaxBackoff, and bound the bootstrapGet. The retry timer comes from theinjected
Clock, the way the progress-notify timer already does, so the scheduleis testable without sleeping.
Newnow rejects values that would make the loop spin, alongside the existingHistoryWindowSizeandBTreeDegreechecks.The back-off is deliberately not reset after a watch that ran for a while.
That is a separate policy question from the one the
Configalready documents,and I did not want to decide it inside a fix for the fields not being read.
Tests
Three tests in
cache_test.go, all insidesynctest.Testso they use virtualtime and run in 0.00s:
TestCacheRetriesUpstreamWatchWithConfiguredBackoff— a KV whose watch alwaysfails; asserts the retry instants are the configured sequence and that it
flattens at
MaxBackoffrather than growing past it.TestCacheBoundsBootstrapGetByGetTimeout— a KV whoseGetblocks forever;asserts the cache still reaches a retry instead of parking on
WaitReady.TestNewCacheRejectsUnusableRetrySettings— table over the three rejectedconfigurations.
Each was checked by reverting
cache.goalone: the three new tests fail, theexisting ones pass both before and after.