✨ Use S3 links in internal computational backend cluster, prepare for temporary tokens (⚠️ devops) #3006

sanderegg · 2022-04-26T16:13:04Z

What do these changes do?

Until this PR the data upload/download through the computational services was done the following way:

Download:
1. director-v2 asks for the presigned links to node_ports, which asks storage, which asks minio (with a TTL of X where X is big)
2. passes the presigned links to the dask-sidecar, which effectively downloads it
Upload:
1. director-v2 asks for the presigned links to node_ports, which asks storage, which asks minio (with a TTL of X where X is big)
2. passes the presigned links to the dask-sidecar, which uses them to upload service outputs and the log file

New env variables (⚠️ devops):

COMPUTATIONAL_BACKEND_DEFAULT_CLUSTER_FILE_LINK_TYPE defines the file type to use with the internal cluster (defaults to S3, available=[s3, presigned])
COMPUTATIONAL_BACKEND_DEFAULT_FILE_LINK_TYPE defines the file type for the other clusters (defaults to presigned)

When using S3 links, it is necessary to have access to the underlying S3 backend (on the contrary presigned links embed this access in their encoding). Therefore director-v2 now also transmits the S3 access credentials to the dask-sidecar when S3 file type is enabled.
In this PR, the default credentials are passed. In the next iteration, a temporary S3 access shall be computed using AWS STS interface. Which should allow the usage of external clusters at the price of some security.
Nevertheless, this solution cannot scale very much, as STS caps to a 1000 of these temporary credentials.

storage new API:
-POST v0/simcore-s3:access returns the S3 credentials (currently returns the default)

NOTE

Maybe another solution would be instead to use the multipart upload options. At least for the frontend, and probably also for any external dask cluster.

Bonus:

~~removed all the pytest-docker infrastructure in storage and use pytest-simcore for all unit tests (moved to ♻️ Maintenance/remove pytest docker storage #3011 )~~
fixes the case where dask-sidecar would not unzip a zip input file that is not meant to be kept as a zip in the service (for example osparc-python-runner)
fixes download/upload progress percentage not increasing, and added also average transfer rate to show in the logs
when an error happened in the dask-sidecar task, it is now shown in the director-v2 logs (nevertheless it cannot always be shown because of missing packages, @pcrespov not sure I will keep it)
overall cleanup

Related issue/s

How to test

make build up-prod or make build-devel up-devel
create a pipeline with inputs, run the pipeline.

Checklist

codecov · 2022-04-26T16:14:36Z

Codecov Report

Merging #3006 (72b58ce) into master (122703f) will increase coverage by 0.0%.
The diff coverage is 90.9%.

@@           Coverage Diff           @@
##           master   #3006    +/-   ##
=======================================
  Coverage    79.7%   79.7%            
=======================================
  Files         688     690     +2     
  Lines       28748   28872   +124     
  Branches     3707    3719    +12     
=======================================
+ Hits        22917   23038   +121     
+ Misses       5015    5010     -5     
- Partials      816     824     +8

Flag	Coverage Δ
integrationtests	`65.7% <84.4%> (+0.1%)`	⬆️
unittests	`75.5% <87.7%> (+0.1%)`	⬆️

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

Impacted Files	Coverage Δ
...ls-library/src/models_library/projects_nodes_io.py	`96.3% <ø> (ø)`
...rvice_dask_sidecar/computational_sidecar/errors.py	`100.0% <ø> (ø)`
...-sidecar/src/simcore_service_dask_sidecar/tasks.py	`80.5% <ø> (ø)`
...es/dynamic_sidecar/docker_service_specs/sidecar.py	`78.5% <ø> (ø)`
...-sdk/src/simcore_sdk/node_ports_v2/nodeports_v2.py	`94.3% <50.0%> (+<0.1%)`	⬆️
...rc/simcore_service_director_v2/core/application.py	`90.3% <50.0%> (-1.4%)`	⬇️
...r-v2/src/simcore_service_director_v2/utils/dask.py	`95.1% <60.0%> (-1.4%)`	⬇️
...k/src/simcore_sdk/node_ports_common/filemanager.py	`81.8% <71.4%> (ø)`
.../simcore-sdk/src/simcore_sdk/node_ports_v2/port.py	`84.9% <75.0%> (-0.5%)`	⬇️
...simcore_service_director_v2/modules/dask_client.py	`91.9% <88.2%> (-0.5%)`	⬇️
... and 24 more

pcrespov

🎉 Nice! Just minor comments

packages/dask-task-models-library/src/dask_task_models_library/container_tasks/io.py

services/storage/src/simcore_service_storage/sts.py

GitHK

Very nice. Please see my comments regarding the settings.

packages/simcore-sdk/tests/unit/test_storage_client.py

GitHK · 2022-04-29T14:07:32Z

services/dask-sidecar/tests/unit/test_file_utils.py

+    mocked_log_publishing_cb.assert_called()
+    mocked_log_publishing_cb.reset_mock()
+
+    # USE-CASE 3: if destination is a zip, but we pass a target mime type that is not, then we decompress


Is this description correct?
I would expect not to decompress.

it is correct. it uses the target mime type, which is what the service expects. Example:
my service has an input port that is defined as in:
type: data:*/*
Then it means it wants to have it unzipped.

If the service is defined as:
type: data:application/zip
then it means it wants a zip file

services/director-v2/src/simcore_service_director_v2/core/settings.py

GitHK · 2022-04-29T14:19:53Z

services/storage/src/simcore_service_storage/handlers.py

+        s3_settings: S3Settings = await sts.get_or_create_temporary_token_for_user(
+            request.app, user_id
+        )
+        return {"data": s3_settings.dict()}


I see the default values. Regarding my PR #2993 I would still not add this. Will enabled it once we decide on how to use STS

yes do not add it. my gut feeling is that probably that STS stuff is not the way to go.

sonarcloud · 2022-05-02T08:33:56Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

GitHK

👍

GitHK · 2022-05-02T10:55:20Z

services/director-v2/src/simcore_service_director_v2/core/settings.py

+    @cached_property
+    def endpoint(self) -> str:
+        return AnyHttpUrl.build(
+            scheme="http",
+            host=self.STORAGE_HOST,
+            port=f"{self.STORAGE_PORT}",
+            path=f"/{self.STORAGE_VTAG}",
+        )
+
+    @cached_property
+    def storage_endpoint(self) -> str:
+        """used to re-create STORAGE_ENDPOINT: used by node_ports and must be
+        in style host:port
+        without scheme or version tag"""
+        return f"{self.STORAGE_HOST}:{self.STORAGE_PORT}"


You might already be aware of this one, just wanted to point it out.

osparc-simcore/packages/settings-library/src/settings_library/utils_service.py

Lines 23 to 38 in 122703f

class MixinServiceSettings:

"""Mixin with common helpers based on validated fields with canonical name

Example:

- Subclass should define host, port and vtag fields as

class MyServiceSettings(BaseCustomSettings, MixinServiceSettings):

{prefix}_HOST: str

{prefix}_PORT: PortInt

{prefix}_VTAG: VersionTag [Optional]

# optional

{prefix}_SCHEME: str (urls default to http)

{prefix}_USER: str

{prefix}_PASSWORD: SecretStr

"""

nope, but good to know. anyway for this one it's a different one, and I expect it to disappear soonish

sanderegg added the changelog:🎨enhancement label Apr 26, 2022

sanderegg added this to the Macarons milestone Apr 26, 2022

sanderegg self-assigned this Apr 26, 2022

sanderegg force-pushed the enhancement/generate_temp_s3_access branch from da86799 to 6ffef00 Compare April 26, 2022 16:13

sanderegg force-pushed the enhancement/generate_temp_s3_access branch 2 times, most recently from 09b96b1 to ba5895a Compare April 28, 2022 20:36

sanderegg changed the title ~~✨ Allow generation of temporary S3 access~~ ✨ Use S3 links in internal computational backend cluster, prepare for temporary tokens Apr 29, 2022

sanderegg changed the title ~~✨ Use S3 links in internal computational backend cluster, prepare for temporary tokens~~ ✨ Use S3 links in internal computational backend cluster, prepare for temporary tokens (⚠️ devops) Apr 29, 2022

sanderegg mentioned this pull request Apr 29, 2022

♻️ Maintenance/remove pytest docker storage #3011

Merged

sanderegg force-pushed the enhancement/generate_temp_s3_access branch from 4b068f3 to 6b45df0 Compare April 29, 2022 09:59

sanderegg added 19 commits April 29, 2022 12:30

add new entrypoint to get user S3 access

bc38a4f

created test for new api

e17ed7e

test ready

cf837a2

storage implements API to return default S3 settings

ffe6696

created client for storage

808da7b

added test for storage client in director-v2

4e06cf9

added call to storage client

33905e0

test getting instance

4120024

cleanup

6e7719e

allow getting link type

10a6f0e

improve logging

4be8de6

refactor

b2d40e7

added error when connecting with storage

125a3f0

cleanup

ac460ec

show errors when getting status

7ea05fc

cleanup

eddadcb

pass S3 link to comp. backend

b3fb5fd

allow importing the FileLinkType

3a65d45

allow setting the file mime type

7b80910

sanderegg added 4 commits April 29, 2022 12:30

fast fail

9eab7fd

fixed test flakyness

69331b2

remove flakyness

5cdacca

correctly check for used file link type

95db602

sanderegg force-pushed the enhancement/generate_temp_s3_access branch from 6b45df0 to 95db602 Compare April 29, 2022 10:30

sanderegg marked this pull request as ready for review April 29, 2022 10:31

sanderegg requested review from mguidon and pcrespov as code owners April 29, 2022 10:31

sanderegg requested review from GitHK, mrnicegyu11 and odeimaiz April 29, 2022 10:31

sanderegg added 6 commits April 29, 2022 13:39

pylance

8f48906

fix compute_input_data test

8a75cb0

added test for compute_output_data_schema

f6c84e0

missing check on last argument

14c6dac

add avg

409af2f

fixed typo

f448c3c

pcrespov approved these changes Apr 29, 2022

View reviewed changes

packages/dask-task-models-library/src/dask_task_models_library/container_tasks/io.py Outdated Show resolved Hide resolved

services/storage/src/simcore_service_storage/sts.py Show resolved Hide resolved

GitHK reviewed Apr 29, 2022

View reviewed changes

sanderegg added 3 commits April 29, 2022 17:47

@pcrespov review: add a doc

7bfb072

@pcrespov review: move regex to models library

324be5a

@GitHK review: duplicated ENV variable

111b983

sanderegg mentioned this pull request Apr 29, 2022

5Gb limit ITISFoundation/osparc-issues#617

Open

storage endpoint needs no scheme and no version tag

72b58ce

sanderegg requested a review from GitHK May 2, 2022 10:12

GitHK approved these changes May 2, 2022

View reviewed changes

sanderegg merged commit 85c998f into ITISFoundation:master May 2, 2022

sanderegg deleted the enhancement/generate_temp_s3_access branch May 2, 2022 11:48

sanderegg mentioned this pull request Jun 2, 2022

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

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Use S3 links in internal computational backend cluster, prepare for temporary tokens (⚠️ devops) #3006

✨ Use S3 links in internal computational backend cluster, prepare for temporary tokens (⚠️ devops) #3006

sanderegg commented Apr 26, 2022 •

edited

Loading

codecov bot commented Apr 26, 2022 •

edited

Loading

pcrespov left a comment

GitHK left a comment •

edited

Loading

GitHK Apr 29, 2022

sanderegg Apr 29, 2022

GitHK Apr 29, 2022

sanderegg Apr 29, 2022

sonarcloud bot commented May 2, 2022

GitHK left a comment

GitHK May 2, 2022

sanderegg May 2, 2022

	class MixinServiceSettings:
	"""Mixin with common helpers based on validated fields with canonical name

	Example:
	- Subclass should define host, port and vtag fields as

	class MyServiceSettings(BaseCustomSettings, MixinServiceSettings):
	{prefix}_HOST: str
	{prefix}_PORT: PortInt
	{prefix}_VTAG: VersionTag [Optional]

	# optional
	{prefix}_SCHEME: str (urls default to http)
	{prefix}_USER: str
	{prefix}_PASSWORD: SecretStr
	"""

✨ Use S3 links in internal computational backend cluster, prepare for temporary tokens (⚠️ devops) #3006

✨ Use S3 links in internal computational backend cluster, prepare for temporary tokens (⚠️ devops) #3006

Conversation

sanderegg commented Apr 26, 2022 • edited Loading

What do these changes do?

NOTE

Related issue/s

How to test

Checklist

codecov bot commented Apr 26, 2022 • edited Loading

Codecov Report

pcrespov left a comment

Choose a reason for hiding this comment

GitHK left a comment • edited Loading

Choose a reason for hiding this comment

GitHK Apr 29, 2022

Choose a reason for hiding this comment

sanderegg Apr 29, 2022

Choose a reason for hiding this comment

GitHK Apr 29, 2022

Choose a reason for hiding this comment

sanderegg Apr 29, 2022

Choose a reason for hiding this comment

sonarcloud bot commented May 2, 2022

GitHK left a comment

Choose a reason for hiding this comment

GitHK May 2, 2022

Choose a reason for hiding this comment

sanderegg May 2, 2022

Choose a reason for hiding this comment

sanderegg commented Apr 26, 2022 •

edited

Loading

codecov bot commented Apr 26, 2022 •

edited

Loading

GitHK left a comment •

edited

Loading