Reintroduce renew lease optimization #10462
Conversation
|
@RobbieMcKinstry WDYT? I'm considering maybe lifting the refresh logic out to spell out some unit tests as this is getting more and more tricky. |
|
Ah looks like our integration tests are flagging this as invalid, working it out. |
t0yv0
force-pushed
the
t0yv0/reintroduce-renew-lease-optimization
branch
from
dd2f1d2
to
397bfbd
Compare
| // If ticker has not kept up, block on | ||
| // renewing rather than risking returning a | ||
| // stale token. | ||
| renewUpdateLeaseIfStale() |
There was a problem hiding this comment.
Oof - i've tried adding a test for this case, but it seems like it's not possible to reach here. The problem is that we need an active token to renew a lease. If we're detecting we've not kept up with refreshing tokens, we have a stale token, then we won't be able to review it either. There is some grace here perhaps that the staleness check indicates a need to renew slightly before actual expiration. So if the ticker fails completely, this safeguard might be beneficial to keep. How to test this though..
|
@t0yv0 looks good to me! |
Description
Re-introduce the optimization that avoids calling renewLease on critical startup path. Previous attempt caused issues and was reverted (see Postmortem). The underlying backend issue with a short expiration on initial token has been patched, so this is safe to reintroduce. Besides reintroducing the original code, the PR addresses @RobbieMcKinstry concern that we cannot solely rely on background ticker to keep the token fresh (up-to-date). To address this, we chose to block the caller of GetToken and refresh the token right then rather than risking returning a stale token.
Fixes https://github.com/pulumi/home/issues/2200
Checklist