Skip to content

🐛Redis locks disappearing and fixup weird usage #7020

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

sanderegg
Copy link
Member

@sanderegg sanderegg commented Jan 9, 2025

What do these changes do?

In short

  • lock_context is gone
  • exclusive decorator shall be used instead
  • exclusive_periodic decorator is now the solution when one wants a background task that runs exclusively on one replica of a service, with automatic recovery in case the replica is stopped/crashed.

Details

  • lock_context context manager is gone as it has an inherent flaw if the lock disappears from the Redis database for whatever reason it will not raise
  • exclusive decorator was re-written without using lock_context and is now the de-factor way of protecting functions with a distributed lock. This is the way now
  • removed many duplications w.r.t. cancelling an asyncio.Task
  • fixed the infamous exclusive_task_starter and renamed that was leaking asyncio.Tasks! Thus proving difficult to cancel them.
  • reworked the background_task.py module that now also provides a decorator periodic which wraps tenacity own retry decorator with some standard parameters
  • moved and renamed async_delayed decorator to with_delay into async_utils.py
  • refactored redis tests to follow new file naming scheme
  • replaced all usage of lock_context context manager with exclusive decorator repo-wide
  • start_exclusive_task_starter... was removed and replaced by the exclusive_periodic decorator

Related issue/s

How to test

Dev-ops checklist

@sanderegg sanderegg added the a:services-library issues on packages/service-libs label Jan 9, 2025
@sanderegg sanderegg self-assigned this Jan 9, 2025
Copy link

codecov bot commented Jan 9, 2025

Codecov Report

Attention: Patch coverage is 94.24460% with 16 lines in your changes missing coverage. Please review.

Project coverage is 86.31%. Comparing base (adb1867) to head (b3c6b34).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7020      +/-   ##
==========================================
- Coverage   86.94%   86.31%   -0.63%     
==========================================
  Files        1651     1615      -36     
  Lines       64811    63147    -1664     
  Branches     2041     2043       +2     
==========================================
- Hits        56351    54507    -1844     
- Misses       8122     8305     +183     
+ Partials      338      335       -3     
Flag Coverage Δ
integrationtests 64.98% <61.11%> (+1.54%) ⬆️
unittests 84.54% <93.88%> (-0.77%) ⬇️
Components Coverage Δ
api ∅ <ø> (∅)
pkg_aws_library 93.49% <ø> (ø)
pkg_dask_task_models_library 97.09% <ø> (ø)
pkg_models_library 91.58% <ø> (ø)
pkg_notifications_library 84.57% <ø> (ø)
pkg_postgres_database 88.27% <ø> (ø)
pkg_service_integration 70.02% <ø> (ø)
pkg_service_library 73.88% <97.90%> (+<0.01%) ⬆️
pkg_settings_library 90.60% <ø> (ø)
pkg_simcore_sdk 85.38% <ø> (ø)
agent 96.45% <83.33%> (-0.37%) ⬇️
api_server 90.54% <75.00%> (-0.03%) ⬇️
autoscaling 96.09% <100.00%> (+<0.01%) ⬆️
catalog 90.66% <ø> (ø)
clusters_keeper 99.24% <93.75%> (-0.25%) ⬇️
dask_sidecar 91.26% <ø> (ø)
datcore_adapter 93.18% <ø> (ø)
director 76.42% <100.00%> (-0.07%) ⬇️
director_v2 91.42% <100.00%> (+0.04%) ⬆️
dynamic_scheduler 97.20% <100.00%> (+0.01%) ⬆️
dynamic_sidecar 89.78% <100.00%> (+0.04%) ⬆️
efs_guardian 90.46% <71.42%> (+0.34%) ⬆️
invitations 93.44% <ø> (ø)
osparc_gateway_server ∅ <ø> (∅)
payments 92.66% <ø> (ø)
resource_usage_tracker 89.52% <100.00%> (+0.24%) ⬆️
storage 89.57% <100.00%> (-0.03%) ⬇️
webclient ∅ <ø> (∅)
webserver 81.58% <50.00%> (-2.49%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adb1867...b3c6b34. Read the comment docs.

@sanderegg sanderegg force-pushed the bugfixes/redis-lock-failing2 branch 3 times, most recently from 1d6a299 to cd8df19 Compare January 13, 2025 17:30
@sanderegg sanderegg added this to the Singularity milestone Jan 13, 2025
@sanderegg sanderegg force-pushed the bugfixes/redis-lock-failing2 branch 2 times, most recently from 7563c97 to 9759786 Compare January 14, 2025 10:55
@sanderegg sanderegg marked this pull request as ready for review January 14, 2025 10:56
Copy link
Collaborator

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the effort 🥇

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! Thanks for looking into this annoying issue.

@sanderegg sanderegg force-pushed the bugfixes/redis-lock-failing2 branch from ececacd to 746fffb Compare January 14, 2025 12:36
Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic redesign! I left some suggestions and a warning regarding func.__name__.

Copy link

@sanderegg sanderegg merged commit 99746e4 into ITISFoundation:master Jan 14, 2025
85 of 93 checks passed
@sanderegg sanderegg deleted the bugfixes/redis-lock-failing2 branch January 14, 2025 15:38
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Jan 15, 2025
58 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:services-library issues on packages/service-libs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exclusive tasks running without a corresponding lock
4 participants