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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛✨ Ensure adding/deleting node is thread safe #3490

Conversation

sanderegg
Copy link
Member

@sanderegg sanderegg commented Oct 31, 2022

What do these changes do?

  • some findings from ✨ Allow selective start/stop of dynamic services (⚠️ devops) #3449 were splited in here:
    • adding nodes concurrently would fail to be correctly saved into the databsae, leading to inconsistencies (e.g. creating hundreds of new nodes in a project would only save some tens of them) --> this is fixed by patching the workbench instead of replacing the whole project with an outdated project (the process of patching is locking the project in the database while being updated, preventing any other reads.
    • deleting node was not updating the database, which was relying on the frontend to save the project. This is now done 100% in the backend and also by patching the workbench

This goes along the lines needed for ITISFoundation/osparc-issues#756

Next steps will be to:

  • change links
  • change node names
    through the backend as well.

Bonus:

  • added carryforward flag to codecov, in an attempt to not have reduced tests now that unit tests are skipped some times

Related issue/s

How to test

Checklist

@codecov
Copy link

codecov bot commented Oct 31, 2022

Codecov Report

Merging #3490 (dc963b7) into master (f6efa59) will decrease coverage by 4.8%.
The diff coverage is 100.0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3490      +/-   ##
=========================================
- Coverage    83.5%   78.6%    -4.9%     
=========================================
  Files         828     349     -479     
  Lines       35201   17756   -17445     
  Branches      744     133     -611     
=========================================
- Hits        29394   13967   -15427     
+ Misses       5617    3740    -1877     
+ Partials      190      49     -141     
Flag Coverage Δ
integrationtests 62.5% <55.5%> (-5.2%) ⬇️
unittests 82.1% <100.0%> (+1.7%) ⬆️

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

Impacted Files Coverage Δ
...simcore_service_webserver/projects/projects_api.py 92.6% <100.0%> (ø)
.../simcore_service_webserver/projects/projects_db.py 96.4% <100.0%> (+0.1%) ⬆️
...-v2/src/simcore_service_director_v2/cli/_client.py 0.0% <0.0%> (-100.0%) ⬇️
...v2/src/simcore_service_director_v2/cli/__init__.py 0.0% <0.0%> (-100.0%) ⬇️
...service_director_v2/cli/_close_and_save_service.py 0.0% <0.0%> (-100.0%) ⬇️
...or-v2/src/simcore_service_director_v2/cli/_core.py 0.0% <0.0%> (-87.5%) ⬇️
...rc/simcore_service_director_v2/utils/dict_utils.py 16.1% <0.0%> (-83.9%) ⬇️
...v2/modules/dynamic_sidecar/docker_compose_specs.py 22.2% <0.0%> (-77.8%) ⬇️
...ce_director_v2/modules/db/repositories/clusters.py 23.0% <0.0%> (-71.7%) ⬇️
...tor_v2/modules/dynamic_sidecar/docker_api/_core.py 25.0% <0.0%> (-70.5%) ⬇️
... and 557 more

@sanderegg sanderegg force-pushed the enhancement/prevent_non_thread_safe_change_of_project_workbench branch from 67bd50e to ca6672c Compare October 31, 2022 10:32
@sanderegg sanderegg marked this pull request as ready for review October 31, 2022 10:38
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
Please check some minor below

await storage_api.delete_data_folders_of_project_node(
request.app, project_uuid, node_uuid, user_id
request.app, f"{project_uuid}", node_uuid, user_id
Copy link
Member

Choose a reason for hiding this comment

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

THOUGHT: I find shame that we go back and forth from UUID to string and then back to uuid ...

When converted to the str we lose the guarantee of a valid UUID downstream ...

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, did you see I changed the syntax of delete_node?

@sonarcloud
Copy link

sonarcloud bot commented Oct 31, 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 23d5254 into ITISFoundation:master Oct 31, 2022
@sanderegg sanderegg deleted the enhancement/prevent_non_thread_safe_change_of_project_workbench branch October 31, 2022 12:41
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

2 participants