[Storage] Replace gsutil with an aio library #631

rzvoncek · 2023-08-31T17:33:43Z

There is a bit of duplicated code, but that will go into the AbstractStorage once we do #628.
The S3 ITs are failing, because I enforced KMS and there is a KMS test that is not working because #612 is still open.

Oh, and I forgot to remove unused code, will handle asap.

Fixes #627.
Fixes #637.

codecov · 2023-08-31T18:11:56Z

Codecov Report

Merging #631 (f799451) into master (22612d3) will increase coverage by 9.09%.
The diff coverage is 90.79%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #631      +/-   ##
==========================================
+ Coverage   71.89%   80.98%   +9.09%     
==========================================
  Files          56       55       -1     
  Lines        4639     4669      +30     
  Branches      675      671       -4     
==========================================
+ Hits         3335     3781     +446     
+ Misses       1250      860     -390     
+ Partials       54       28      -26

Files Changed	Coverage Δ
medusa/storage/abstract_storage.py	`89.37% <50.00%> (-3.10%)`	⬇️
medusa/storage/google_storage.py	`94.88% <94.44%> (+42.38%)`	⬆️
medusa/backup_node.py	`87.43% <100.00%> (+1.79%)`	⬆️
medusa/download.py	`88.88% <100.00%> (-0.59%)`	⬇️
medusa/storage/s3_base_storage.py	`90.80% <100.00%> (+2.03%)`	⬆️

... and 20 files with indirect coverage changes

adejanovski · 2023-09-04T09:00:41Z

medusa/storage/google_storage.py

-        with GSUtil(self.config) as gsutil:
-            for parent, src_paths in _group_by_parent(srcs):
-                yield self._upload_paths(gsutil, parent, src_paths, dest)
+    @retry(stop_max_attempt_number=MAX_UP_DOWN_LOAD_RETRIES, wait_fixed=5000)


Issue: we should probably retry individual blob uploads (_upload_blob()) instead of this one.
It will work nicely with the resumable transfer mode.

adejanovski · 2023-09-04T09:07:19Z

medusa/storage/google_storage.py

+            )
+            resp = resp['resource']
+        else:
+            resp = await self.gcs_storage.upload_from_filename(


Issue: upload_from_filename() will read the whole file into memory, which will obviously not work for us.
Let's use upload() instead and pass it a file object as file_data arg. Let's also set force_resumable_upload to true and disable the timeout for now.
Multipart uploads in this lib don't seem to work as we would want them to anyway (seems like all parts are read at once, they're not uploaded concurrently and there's no retry when a part fails).

adejanovski · 2023-09-04T09:15:38Z

medusa/storage/google_storage.py

-        with GSUtil(self.config) as gsutil:
-            for parent, src_paths in _group_by_parent(srcs):
-                yield self._download_paths(gsutil, parent, src_paths, dest)
+    @retry(stop_max_attempt_number=MAX_UP_DOWN_LOAD_RETRIES, wait_fixed=5000)


Issue: I think we should move the retries on individual downloads instead.
Since the retry swallows the exceptions, I'd also catch it, log it and then re-raise it for better observability of these failed attempts.

adejanovski · 2023-09-04T09:27:32Z

medusa/backup_node.py

@@ -31,7 +31,7 @@
 from medusa.index import add_backup_start_to_index, add_backup_finish_to_index, set_latest_backup_in_index
 from medusa.monitoring import Monitoring
 from medusa.storage import Storage, format_bytes_str, ManifestObject, divide_chunks
-from medusa.storage.google_storage import GSUTIL_MAX_FILES_PER_CHUNK
+from medusa.storage.google_storage import GOOGLE_MAX_FILES_PER_CHUNK


Question: Do we still need this? I have a feeling this was related to gsutil.

You're correct, we don't need this. The new storage driver implementations handle this themsleves (eventually we'll do this in the AbstractStorage anyway).

adejanovski

Large files downloads are failing due to the download method being invoked.

adejanovski · 2023-09-12T19:29:42Z

medusa/storage/google_storage.py

-        logging.debug("Blob {} last modification time is {}".format(blob.name, blob.extra["last_modified"]))
-        return parser.parse(blob.extra["last_modified"])
+        try:
+            await self.gcs_storage.download_to_filename(


Issue: This method reads the files at once and puts their content into memory. We need to use download_stream() instead which will return a BufferedStream which should be writeable to a file without loading all the data in memory.

Done. Pushed a new commit, together with a rebase on a recent master.

adejanovski · 2023-09-12T19:30:21Z

setup.py

@@ -70,7 +70,8 @@
        'dnspython>=2.2.1',
        'asyncio==3.4.3',
        'aiohttp==3.8.5',
-        'aiohttp-s3-client==0.8.17'
+        'aiohttp-s3-client==0.8.17',
+        'gcloud-aio-storage==8.3.0'


Note: I added this dependency which was missing from setup.py

adejanovski

Looks great but I'm wondering about the concurrency level of some operations.

adejanovski · 2023-09-14T06:19:30Z

medusa/storage/google_storage.py

+    async def _delete_objects(self, objects: t.List[AbstractBlob]):
+        coros = [self._delete_object(obj) for obj in objects]
+        await asyncio.gather(*coros)


Question: Is this going to delete all objects concurrently? Maybe our concurrency setting should apply here to avoid sending too many concurrent requests?

Yes, it's all at once. Chunking might be a good idea. Do I chunk it in chunks of config.max_concurrent_transfers size?

Do I chunk it in chunks of config.max_concurrent_transfers size?

Yes, sounds good

adejanovski · 2023-09-14T06:20:45Z

medusa/storage/google_storage.py

+    async def _download_blobs(self, srcs: t.List[t.Union[Path, str]], dest: t.Union[Path, str]):
+        coros = [self._download_blob(src, dest) for src in map(str, srcs)]
+        await asyncio.gather(*coros)


Question: is it downloading all files concurrently at the same time?

Yes, the same as with the deletes. Do we chunk by concucurrent transfers?

I think we should chunk, otherwise we may overwhelm the network which could have unforeseen impacts.

sonarcloud · 2023-09-14T08:09:35Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
13 Code Smells

No Coverage information
3.8% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

adejanovski

Awesome stuff!

* [Storage] Replace gsutil with an aio library * [Storage] Dont chunk files outside of storage drivers * [Storage/GCS] Move retries to uploads of individual blobs * [Storage/GCS] Use timeouts everywhere and force resumable where relevant * [Storage/GCS] Move retries for downloads to individual blobs * [GCS Storage] Download via stream instead of all into memory * [GCS Storage] Chunk deletes/downloads by config.concurrent_transfers

rzvoncek marked this pull request as ready for review August 31, 2023 18:49

rzvoncek requested a review from adejanovski August 31, 2023 18:51

rzvoncek force-pushed the radovan/aio-gcs branch from f0301e6 to 8b15606 Compare September 1, 2023 09:43

adejanovski requested changes Sep 11, 2023

View reviewed changes

adejanovski requested changes Sep 12, 2023

View reviewed changes

rzvoncek added 5 commits September 13, 2023 16:44

[Storage] Replace gsutil with an aio library

0da5a15

[Storage] Dont chunk files outside of storage drivers

eefa981

[Storage/GCS] Move retries to uploads of individual blobs

54f5e57

[Storage/GCS] Use timeouts everywhere and force resumable where relevant

e38fe51

[Storage/GCS] Move retries for downloads to individual blobs

9f35539

rzvoncek force-pushed the radovan/aio-gcs branch from d0ef407 to 0306927 Compare September 13, 2023 14:38

[GCS Storage] Download via stream instead of all into memory

5eb0166

rzvoncek force-pushed the radovan/aio-gcs branch from 0306927 to 5eb0166 Compare September 13, 2023 15:05

rzvoncek mentioned this pull request Sep 12, 2023

Retry individual blobs download instead of retrying the whole download list #637

Closed

4 tasks

adejanovski reviewed Sep 14, 2023

View reviewed changes

[GCS Storage] Chunk deletes/downloads by config.concurrent_transfers

f799451

rzvoncek force-pushed the radovan/aio-gcs branch from 34ec86d to f799451 Compare September 14, 2023 08:08

adejanovski approved these changes Sep 14, 2023

View reviewed changes

rzvoncek merged commit e02afe1 into master Sep 14, 2023
27 of 28 checks passed

rzvoncek deleted the radovan/aio-gcs branch September 22, 2023 11:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Storage] Replace gsutil with an aio library #631

[Storage] Replace gsutil with an aio library #631

rzvoncek commented Aug 31, 2023 •

edited

codecov bot commented Aug 31, 2023 •

edited

adejanovski Sep 4, 2023

rzvoncek Sep 11, 2023

adejanovski Sep 4, 2023

rzvoncek Sep 11, 2023

adejanovski Sep 4, 2023

rzvoncek Sep 11, 2023

adejanovski Sep 4, 2023

rzvoncek Sep 11, 2023

adejanovski left a comment

adejanovski Sep 12, 2023

rzvoncek Sep 13, 2023

adejanovski Sep 12, 2023

adejanovski left a comment

adejanovski Sep 14, 2023

rzvoncek Sep 14, 2023 •

edited

adejanovski Sep 14, 2023

adejanovski Sep 14, 2023

rzvoncek Sep 14, 2023

adejanovski Sep 14, 2023

sonarcloud bot commented Sep 14, 2023

adejanovski left a comment

[Storage] Replace gsutil with an aio library #631

[Storage] Replace gsutil with an aio library #631

Conversation

rzvoncek commented Aug 31, 2023 • edited

codecov bot commented Aug 31, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adejanovski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adejanovski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzvoncek Sep 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 14, 2023

adejanovski left a comment

Choose a reason for hiding this comment

rzvoncek commented Aug 31, 2023 •

edited

codecov bot commented Aug 31, 2023 •

edited

rzvoncek Sep 14, 2023 •

edited