♻️🐛 docker engine cleans up volumes when removing dynamic-sidecars #2915

GitHK · 2022-03-22T14:50:46Z

What do these changes do?

Previously the docker volume had to be manually removed from the stack. This never worked on multiple node deployments.
Now the docker engine will take care of cleaning it up.

Note: @mguidon as already discussed with you, this has issues with the docker rclone plugin. Since anonymous volumes are created the docker rclone volume plugin spawns 2/3 times the same anonymous volume, but only one of the ends up being used in the end.

Related issue/s

How to test

Checklist

Unit tests for the changes exist
Runs in the swarm

…oval

codecov · 2022-03-22T14:53:43Z

Codecov Report

Merging #2915 (88c1da0) into master (8093b42) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff            @@
##           master   #2915     +/-   ##
========================================
- Coverage    79.6%   79.5%   -0.1%     
========================================
  Files         678     681      +3     
  Lines       28353   28385     +32     
  Branches     3655    3657      +2     
========================================
+ Hits        22578   22594     +16     
- Misses       5008    5022     +14     
- Partials      767     769      +2

Flag	Coverage Δ
integrationtests	`65.7% <100.0%> (-0.2%)`	⬇️
unittests	`75.2% <100.0%> (+<0.1%)`	⬆️

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

Impacted Files	Coverage Δ
...tor_v2/modules/dynamic_sidecar/volumes_resolver.py	`64.1% <ø> (-20.6%)`	⬇️
...service_director_v2/api/routes/dynamic_services.py	`88.2% <100.0%> (ø)`
...re_service_director_v2/models/schemas/constants.py	`100.0% <100.0%> (ø)`
...or_v2/modules/db/repositories/projects_networks.py	`65.2% <100.0%> (ø)`
...c/simcore_service_director_v2/modules/db/tables.py	`100.0% <100.0%> (ø)`
..._director_v2/modules/dynamic_sidecar/client_api.py	`78.4% <100.0%> (ø)`
...tor_v2/modules/dynamic_sidecar/scheduler/events.py	`93.8% <100.0%> (-1.7%)`	⬇️
...e_service_director_v2/modules/projects_networks.py	`74.1% <100.0%> (ø)`
.../simcore_service_dynamic_sidecar/api/containers.py	`88.2% <100.0%> (+0.2%)`	⬆️
...ervice_dynamic_sidecar/api/containers_extension.py	`89.1% <100.0%> (-0.6%)`	⬇️
... and 24 more

pcrespov · 2022-03-24T01:51:43Z

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/core/docker_utils.py

+
+
+@asynccontextmanager
+async def docker_client() -> AsyncGenerator[aiodocker.Docker, None]:


We have this function 5 times in this repository. I wonder if we could put it in some package?

Not sure about the others but this one is slightly different from the rest.
If the others have the exact same body and signature maybe we could go for such a thing, but in a different PR.

Not sure? I guess we both have access to the same codebase, right :-) Please verify and create this separate PR if necessary.

pcrespov · 2022-03-24T01:56:41Z

services/dynamic-sidecar/tests/unit/test_modules_mounted_fs.py

@@ -9,6 +9,8 @@
 import pytest
 from simcore_service_dynamic_sidecar.modules import mounted_fs

+pytestmark = pytest.mark.asyncio


@sanderegg, there was a different way to set this now, right?

well I think it depends in this case if the pytest-aiohttp library is installed. in which case I think you can just remove the mark.

@sanderegg pytest-aiohttp is NOT installed but is pytest-asyncio. I thought now it was also automatic .. perhaps i misunderstood some comment you did a while ago

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/mounted_fs.py

pcrespov · 2022-03-24T02:06:29Z

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/mounted_fs.py

        for volume_state_path, state_path in zip(
            self.volume_name_state_paths(), self.state_paths
        ):
-            yield f"{volume_state_path}:{state_path}"
+            bind_path: Path = await self._get_bind_path_from_label(volume_state_path)
+            yield f"{bind_path}:{state_path}"


 def setup_mounted_fs() -> MountedVolumes:


There are at least two conventions that are violated here:

_mounted_volumes: we discourage using global variables. Use app.state instead. Please read Data Sharing aka No Singleton

get_settings(): settings should be also in app.state and init at the same time as the app but a) is yet another singleton b) is created every-time you call this function (ie. does not "get" but "create" )

I've already checked and can confirm the the _mounted_volumes can be moved to the app instance. For get_settings() I am not sure. Will also check.

So after spending too much time on trying to move part of the get_settings() away. Will stop for now and leave it for a later PR. It's not that straight forward.

i am a bit surprised this read this since we have been refactoring settings for the whole year ... so i wonder what is the difference here. let's check together

...ices/director-v2/src/simcore_service_director_v2/modules/dynamic_sidecar/scheduler/events.py

...ices/director-v2/src/simcore_service_director_v2/modules/dynamic_sidecar/volumes_resolver.py

services/director-v2/tests/unit/test_modules_dynamic_sidecar_volumes_resolver.py

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/core/docker_utils.py

services/dynamic-sidecar/tests/unit/test_docker_utils.py

sanderegg · 2022-03-24T07:06:38Z

services/dynamic-sidecar/tests/unit/test_modules_mounted_fs.py

@@ -9,6 +9,8 @@
 import pytest
 from simcore_service_dynamic_sidecar.modules import mounted_fs

+pytestmark = pytest.mark.asyncio


well I think it depends in this case if the pytest-aiohttp library is installed. in which case I think you can just remove the mark.

…oval

pcrespov

le'ts give it a try a moment together about the settings

…oval

Andrei Neagu added 2 commits March 22, 2022 15:48

volume removal is handled by the docker engine

365217b

Merge remote-tracking branch 'upstream/master' into proper-volume-rem…

823c813

…oval

GitHK self-assigned this Mar 22, 2022

GitHK added a:director-v2 issue related with the director-v2 service changelog:♻️refactor a:dynamic-sidecar dynamic-sidecar service labels Mar 22, 2022

GitHK added this to the E.Shackleton milestone Mar 22, 2022

Andrei Neagu added 4 commits March 22, 2022 16:33

refactor and fix test

6cd13a9

removed outdated

58c5861

fixed test

f62fb3a

fix tests

00b9e9a

GitHK marked this pull request as ready for review March 23, 2022 06:17

GitHK requested review from sanderegg and pcrespov as code owners March 23, 2022 06:17

Andrei Neagu added 3 commits March 23, 2022 07:24

retriggering CI

1ec1e45

fix test

1590a00

removed unused

26f7543

GitHK requested a review from mguidon March 23, 2022 07:04

fix broken test

7811853

pcrespov reviewed Mar 24, 2022

View reviewed changes

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/mounted_fs.py Outdated Show resolved Hide resolved

pcrespov reviewed Mar 24, 2022

View reviewed changes

sanderegg reviewed Mar 24, 2022

View reviewed changes

GitHK mentioned this pull request Mar 24, 2022

Fix rclone issues with docker volumes #2841

Closed

pcrespov approved these changes Mar 24, 2022

View reviewed changes

GitHK mentioned this pull request Mar 24, 2022

osparc-fs: Internals ITISFoundation/osparc-issues#580

Open

bringing back uuid

e981950

Andrei Neagu added 11 commits March 30, 2022 11:20

renaming

4f6d5f2

added better exception handling

b6afa89

moved to global space

02148d0

Merge remote-tracking branch 'upstream/master' into proper-volume-rem…

7d86b5d

…oval

fix imports

31fe56d

codestyle and fix missing imports

1d4a332

moved mounted_volumes to application from global

10db7dd

Merge remote-tracking branch 'upstream/master' into proper-volume-rem…

c60513b

…oval

disabled rclone test for now

6cd8a51

fix labels

58eb6f3

fixed test

82516e4

GitHK requested review from sanderegg and pcrespov March 31, 2022 11:42

sanderegg approved these changes Mar 31, 2022

View reviewed changes

pcrespov reviewed Mar 31, 2022

View reviewed changes

GitHK requested a review from pcrespov April 6, 2022 07:49

pcrespov approved these changes Apr 6, 2022

View reviewed changes

Andrei Neagu and others added 2 commits April 7, 2022 11:31

Merge remote-tracking branch 'upstream/master' into proper-volume-rem…

619add9

…oval

Merge branch 'master' into proper-volume-removal

88c1da0

GitHK merged commit 5c09b8f into ITISFoundation:master Apr 7, 2022

GitHK deleted the proper-volume-removal branch April 7, 2022 13:19

sanderegg mentioned this pull request Apr 28, 2022

maintenance/scaling of the platform ITISFoundation/osparc-issues#428

Closed

2 tasks

GitHK mentioned this pull request Jun 13, 2022

improving dynamic-sidecar design ITISFoundation/osparc-issues#638

Open

GitHK mentioned this pull request Jul 4, 2022

dy-sidecar reboot volume/data issues #3116

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

♻️🐛 docker engine cleans up volumes when removing dynamic-sidecars #2915

♻️🐛 docker engine cleans up volumes when removing dynamic-sidecars #2915

GitHK commented Mar 22, 2022 •

edited

codecov bot commented Mar 22, 2022 •

edited

pcrespov Mar 24, 2022 •

edited

GitHK Mar 30, 2022 •

edited

pcrespov Mar 31, 2022

pcrespov Mar 24, 2022

sanderegg Mar 24, 2022

pcrespov Mar 31, 2022 •

edited

pcrespov Mar 24, 2022 •

edited

GitHK Mar 30, 2022

GitHK Mar 31, 2022

pcrespov Mar 31, 2022

sanderegg Mar 24, 2022

pcrespov left a comment



		@asynccontextmanager
		async def docker_client() -> AsyncGenerator[aiodocker.Docker, None]:

♻️🐛 docker engine cleans up volumes when removing dynamic-sidecars #2915

♻️🐛 docker engine cleans up volumes when removing dynamic-sidecars #2915

Conversation

GitHK commented Mar 22, 2022 • edited

What do these changes do?

Related issue/s

How to test

Checklist

codecov bot commented Mar 22, 2022 • edited

Codecov Report

pcrespov Mar 24, 2022 • edited

Choose a reason for hiding this comment

GitHK Mar 30, 2022 • edited

Choose a reason for hiding this comment

pcrespov Mar 31, 2022

Choose a reason for hiding this comment

pcrespov Mar 24, 2022

Choose a reason for hiding this comment

sanderegg Mar 24, 2022

Choose a reason for hiding this comment

pcrespov Mar 31, 2022 • edited

Choose a reason for hiding this comment

pcrespov Mar 24, 2022 • edited

Choose a reason for hiding this comment

GitHK Mar 30, 2022

Choose a reason for hiding this comment

GitHK Mar 31, 2022

Choose a reason for hiding this comment

pcrespov Mar 31, 2022

Choose a reason for hiding this comment

sanderegg Mar 24, 2022

Choose a reason for hiding this comment

pcrespov left a comment

Choose a reason for hiding this comment

GitHK commented Mar 22, 2022 •

edited

codecov bot commented Mar 22, 2022 •

edited

pcrespov Mar 24, 2022 •

edited

GitHK Mar 30, 2022 •

edited

pcrespov Mar 31, 2022 •

edited

pcrespov Mar 24, 2022 •

edited