Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰Introduce timeout in project lock (a la aioredlock) #3675

Conversation

sanderegg
Copy link
Member

@sanderegg sanderegg commented Dec 13, 2022

What do these changes do?

Previously we were using aioredlock to lock projects. When we migrated to redis-py, we lost the automatic timeout extension of distributed locks. This probably led to the following issues:
ITISFoundation/osparc-issues#724
#3145

in case of a crash in the webserver, these locks would never timeout.

This PR aims to fix this by mimicking the behavior of aioredlock (default timeout of 10 seconds, with auto-extension every 6 seconds). In case the webserver would restart/crash without having the time to release the lock, this would ensure the lock is released at some point.
This PR just fixes the ProjectLock, all the other locks are not concerned with this change.

just looking at master's redis-commander shows how this is a problem, most of these locks are lost:
image

Related issue/s

How to test

Checklist

@codecov
Copy link

codecov bot commented Dec 13, 2022

Codecov Report

Merging #3675 (4198e81) into master (e1f6496) will increase coverage by 0.3%.
The diff coverage is 84.2%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #3675     +/-   ##
========================================
+ Coverage    84.3%   84.7%   +0.3%     
========================================
  Files         883     734    -149     
  Lines       37382   31939   -5443     
  Branches      785     415    -370     
========================================
- Hits        31545   27073   -4472     
+ Misses       5629    4742    -887     
+ Partials      208     124     -84     
Flag Coverage 螖
integrationtests 67.7% <80.0%> (+<0.1%) 猬嗭笍
unittests 81.5% <84.2%> (-0.1%) 猬囷笍

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage 螖
...simcore_service_webserver/projects/project_lock.py 90.2% <80.0%> (-3.7%) 猬囷笍
.../service-library/src/servicelib/background_task.py 96.8% <88.8%> (-3.2%) 猬囷笍
...rary/src/servicelib/fastapi/requests_decorators.py 80.3% <0.0%> (-4.0%) 猬囷笍
...imcore_service_webserver/garbage_collector_core.py 62.6% <0.0%> (-1.9%) 猬囷笍
.../server/src/simcore_service_webserver/users_api.py 95.1% <0.0%> (-1.4%) 猬囷笍
.../simcore_postgres_database/models/confirmations.py
...dels-library/src/models_library/projects_access.py
...abase/src/simcore_postgres_database/models/tags.py
...ages/models-library/src/models_library/clusters.py
...tion/src/service_integration/compose_spec_model.py
... and 146 more

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 addressing this!

services/web/server/tests/unit/with_dbs/03/test_redis.py Outdated Show resolved Hide resolved
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.

馃憤

@sanderegg sanderegg force-pushed the enhancement/autoscaling/handle_app_restart branch from 96c53cd to 4198e81 Compare December 14, 2022 17:48
@sonarcloud
Copy link

sonarcloud bot commented Dec 14, 2022

Kudos, SonarCloud Quality Gate passed!聽 聽 Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sanderegg sanderegg merged commit 4744774 into ITISFoundation:master Dec 14, 2022
@sanderegg sanderegg deleted the enhancement/autoscaling/handle_app_restart branch December 14, 2022 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants