Auto-refresh zoom tokens via celery task + implement task locks#1652
Auto-refresh zoom tokens via celery task + implement task locks#1652devmount wants to merge 19 commits into
Conversation
davinotdavid
force-pushed
the
enhancements/1580-auto-refresh-zoom-tokens
branch
from
9715027 to
91dbaec
Compare
MelissaAutumn
left a comment
MelissaAutumn
left a comment
There was a problem hiding this comment.
Looks good, I have a few suggestions though. I can't test this locally right now, but I'm sure someone else can 😄
MelissaAutumn
left a comment
MelissaAutumn
left a comment
There was a problem hiding this comment.
Thanks for the change, just wanted to suggest a better flow for it though.
| if lock_token is None: | ||
| log.info('Zoom token check is already running, skipping.') | ||
| return | ||
| else: |
There was a problem hiding this comment.
This logic should probably be wrapped in a context manager so we can just use
try:
with task_lock('refresh_zoom_tokens'):
run()
except TaskLockFailed:
print('ah shoot it failed to acquire the lock')
... etc ...With task_lock pulling redis instance, locking the task, and has a finally that unlocks the task. Default values can include the default lock ttl seconds.
There was a problem hiding this comment.
Ah nice, indeed much better to do that! Changed, thanks!
| return 0 | ||
| end | ||
| """ | ||
| redis_instance.eval(release_script, 1, lock_key, lock_token) |
There was a problem hiding this comment.
I hope this can be done without an eval 🤔
There was a problem hiding this comment.
Hmm I've replaced this with Redis' Transactions, what do you think?
There was a problem hiding this comment.
Ohhh I should have seen this before commenting. That works for me!
davinotdavid
changed the title
MelissaAutumn
left a comment
MelissaAutumn
left a comment
There was a problem hiding this comment.
Just have some questions, no real changes. Looks good! Once they're answered I can approve.
|
|
||
| try: | ||
| yield | ||
| finally: |
There was a problem hiding this comment.
We might want to log all uncaught errors as well.
There was a problem hiding this comment.
Ah yes, added an except there, thanks!
| release_task_lock(redis_instance, task_name, lock_token) | ||
| except Exception as e: | ||
| logging.error(f'Failed to release {task_name} lock: {e}') | ||
| sentry_sdk.capture_exception(e) |
There was a problem hiding this comment.
I noticed sentry creates "issues" for logging.error as well as capture_exception. I'm not sure what would be the best solution here, maybe drop the log down to warning?
There was a problem hiding this comment.
Hmmm I dropped the logging down to warning in this case and in the uncaught errors from the comment above too! Thanks!
|
I'm out today due to cat stuff, but will test tonight or tmw morning. Thanks! |
3 participants
What changed?
This change implements a celery task that triggers a command to check all Zoom token expiry dates. If a token is expired or under a given time threshold, a token refresh is triggered by initializing the OAuth2 Session and making a lightweight
get_me()call.A new env var
ZOOM_TOKEN_TTL_IN_SECONDSgot introduced to specify, how long we assume an auth token to be valid before checking and renewing it.If there is an error during the process, the external connection is marked with
status = errorso that at least it is shown up in the UI. Otherwise, it is marked asstatus = ok.Also, we've added some repository functions for external connection token and status handling.
Why?
This improvement should help users reducing manual effort to keep a Zoom connection active.
Limitations and Notes
Added a comment with a TODO that we should proactively send an email to the Subscriber in case there's a problem with renewing the token: #1662
How to test
Run
docker compose exec backend run-command main refresh-zoom-tokensand thendocker compose logs celery-worker --tail 30to check the celery worker logs.Applicable Issues
Closes #1580