fix: Increase Scan count and move Scan outside of mutex lock to avoid slow calls timing out (#17645) #21926
Conversation
… calls timing out Simply moving the Scan outside of the mutex lock would cause the temporary loss of what tokens are revoked because it clears the storage.revokedTokens variable before starting the scan. Moving the wipe until afterwards would cause any tokens that had been revoked during the scan to be lost. So I've added a second variable storage.recentRevokedTokens that keeps track of all revoked tokens since the last time it did a loadRevokedTokens call. This then gets merged into storage.revokedTokens and wiped at the end of loadRevokedTokens to avoid the loss of any revoked tokens. Signed-off-by: Seth Gupton <[email protected]>
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #21926 +/- ##
==========================================
- Coverage 55.80% 55.80% -0.01%
==========================================
Files 342 342
Lines 57159 57168 +9
==========================================
+ Hits 31898 31902 +4
- Misses 22623 22627 +4
- Partials 2638 2639 +1 ☔ View full report in Codecov by Sentry. |
|
Went to contributor meeting today. They did wonder if the default count could be changed in redis. Which would mean the Scan can be configured in Redis without needing to add more configurations to Argo. I am going to look into that. |
|
Well dangit. Initial Google results were promising but incorrect. There is no way to set the default scan count on the server. I don't see it in their example config; https://redis.io/docs/latest/operate/oss_and_stack/management/config-file/ I'm sad because that would have been a really nice solution. |
There was a problem hiding this comment.
Thank you for PR @sethgupton-mastery
I looked through Argo CD code and looks like this is the only redis scan call we have. Everywhere else Argo CD have exact key and only token related logic require scanning the list.
Given how we use Redis it was a mistake to introduce scan and we will avoid doing it in a future.
So it makes sense to me to have hardcoded page size in the only scan request we have.
LGTM
… slow calls timing out (argoproj#17645) (argoproj#21926) Signed-off-by: Seth Gupton <[email protected]> Signed-off-by: Hapshanko <[email protected]>
|
/cherry-pick release-3.0 |
1 similar comment
|
/cherry-pick release-3.0 |
3 participants
Closes #17645
Simply moving the Scan outside of the mutex lock would cause the temporary loss of what tokens are revoked because it clears the storage.revokedTokens variable before starting the scan. Moving the wipe until afterwards would cause any tokens that had been revoked during the scan to be lost.
So I've added a second variable storage.recentRevokedTokens that keeps track of all revoked tokens since the last time it did a loadRevokedTokens call. This then gets merged into storage.revokedTokens and wiped at the end of loadRevokedTokens to avoid the loss of any revoked tokens.
Checklist: