bug-1886021: Implement GCS storage classes for GCP #6572

relud · 2024-04-03T21:28:34Z

test plan

run CI tests locally

make build
make lint
make test

check output to see that both aws and gcp tests are executed

check aws mode works

echo CLOUD_PROVIDER=AWS > my.env
make runservices
make setup
make run

separate shell

make shell
./bin/socorro_aws_s3.sh ls --recursive telemetry-bucket
./socorro-cmd fetch_crashids --num=3 | tee crashids | ./bin/process_crashes.sh
sleep 10
./bin/socorro_aws_s3.sh ls --recursive dev-bucket
./bin/socorro_aws_s3.sh ls --recursive telemetry-bucket
cat crashids | xargs -i bash -c 'curl -s webapp:8000/api/CrashVerify/?crash_id={}; echo'

check gcp mode works

echo CLOUD_PROVIDER=GCP > my.env
make runservices
make setup
make run

separate shell

make shell
./socorro-cmd gcs list_objects telemetry-bucket
./socorro-cmd fetch_crashids --num=3 | tee crashids | ./bin/process_crashes.sh
sleep 10
./socorro-cmd gcs list_objects dev-bucket
./socorro-cmd gcs list_objects telemetry-bucket
cat crashids | xargs -i bash -c 'curl -s webapp:8000/api/CrashVerify/?crash_id={}; echo'

expectation:

before sleep: 3 raw crashes and dumps in dev-bucket, nothing in telemetry-bucket
after sleep: 3 processed crashes added to dev-bucket and telemetry-bucket, CrashVerify api returns true for all locations

relud · 2024-04-16T17:15:36Z

socorro/external/gcs/crashstorage.py

+        return json.loads(processed_crash_as_string)
+
+    def get(self, **kwargs):
+        """Return JSON data of a crash report, given its uuid."""


this method is copied with no changes from socorro.external.boto.crash_data.SimplifiedCrashData to make this class is compatible with SimplifiedCrashData for use in webapp/crashstats/crashstats/models.py

This code duplication could be removed by converting SimplifiedCrashData to a mixin.

Suggested change

"""Return JSON data of a crash report, given its uuid."""

"""Return JSON data of a crash report, given its uuid."""

# FIXME(relud): This method is used by the webapp API middleware nonsense. It shouldn't

# exist here. We should move it to the webapp.

This get thing is bad architecture we've had for a while. It's explicitly a webapp API "middleware" thing for HTTP GET requests and we shouldn't have it in the socorro/ tree at all--it should be somewhere in webapp/.

I think we should go with what you've done here and fix it in the future.

However, we should mark it with a nasty comment because it's a nasty thing.

relud · 2024-04-16T17:16:00Z

socorro/external/gcs/crashstorage.py

+        return json.loads(crash_report_as_str)
+
+    def get(self, **kwargs):
+        """Return JSON data of a crash report, given its uuid."""


this method is copied with no changes from socorro.external.boto.crash_data.TelemetryCrashData to make this class is compatible with TelemetryCrashData for use in webapp/crashstats/crashstats/models.py

This code duplication could be removed by converting TelemetryCrashData to a mixin.

Like the get above, this shouldn't be in socorro/ at all. What you have is fine, but we should fix this some day. It might require removing the whole webapp API "middleware" nonsense.

relud · 2024-04-16T17:18:51Z

socorro/external/boto/crashstorage.py


-        :returns: generator of keys
-
+        :returns: generator of pages (lists) of object keys


edited the return type here so it was easier to write a compatible gcs equivalent

relud · 2024-04-16T17:25:07Z

socorro/external/gcs/crashstorage.py

+                bucket_or_name=self.bucket, prefix=prefix, page_size=page_size
+            ),
+            page_size,
+        ):


more_itertools.chunked is used to pull a page worths of blobs at a time from the iterable returned by the sdk, and doing that efficiently requires a known page size. I didn't see a default page size for gcs, but for s3 i found it was 1000, so i used that here.

worth noting that webapp/crashstats/management/commands/verifyprocessed.py is the only place using this, and it chunks the page contents by 100 for sending to gcs, so page size should be a mulitple of 100.

willkg

I haven't finished the review, but figured I'd submit everything I had so far before disappearing for a few days.

willkg · 2024-04-16T18:17:00Z

bin/gcs_cli.py

+
+    # remove protocol from destination if present
+    destination = destination.split("://", 1)[-1]
+    bucket_name, _, prefix = destination.partition("/")


I need to remember that partition exists. It solves so many problems.

bin/gcs_cli.py

willkg · 2024-04-16T19:46:10Z

bin/process_crashes.sh

+  ./socorro-cmd gcs upload "${DATADIR}" "${CRASHSTORAGE_GCS_BUCKET}"
+  ./socorro-cmd gcs list_objects "${CRASHSTORAGE_GCS_BUCKET}"
+else
+  ./bin/socorro_aws_s3.sh mb "s3://${CRASHSTORAGE_S3_BUCKET}/"


I hated socorro_aws_s3.sh so much. It'll be great to see it gone.

Re-upping this sentiment. socorro_aws_s3.sh is for the birds.

willkg · 2024-04-16T19:51:42Z

socorro/external/gcs/crashstorage.py

+from socorro.schemas import TELEMETRY_SOCORRO_CRASH_SCHEMA
+
+
+class GcsCrashStorage(CrashStorageBase):


I probably would have done GCSCrashStorage, but there's probably lots of variance in the code so there's nothing to be consistent with.

socorro/external/gcs/crashstorage.py

willkg · 2024-04-16T19:55:45Z

socorro/external/gcs/crashstorage.py

+
+        # We don't know what type of dumps mapping we have. We do know,
+        # however, that by calling the memory_dump_mapping method, we will get
+        # a MemoryDumpMapping which is exactly what we need.


As a side note, I have no idea what this means or why we have to do this with MemoryDumpMapping. It's possible I've removed all the bits that required us to do this.

willkg

Some minor changes--but generally this is good:

utility functions in boto probably shouldn't be imported into gcs code--that feels icky
build_keys should either get copied to the gcs code or moved to a neutral place
probably should add comments to the get methods that came from the crash_data classes
maybe rename upload_fileobj to upload and download_fileobj to download in the s3 helper
since we nixed the crash_data classes, we should update the corresponding tests
test function docstrings for fcs_cli are copied from the first function

Otherwise, this is good and everything I tested with CLOUD_PROVIDER=<unset> and CLOUD_PROVIDER=GCP worked fine as far as I could tell.

willkg · 2024-04-26T23:23:07Z

bin/gcs_cli.py

+@gcs_group.command("upload")
+@click.argument("source")
+@click.argument("destination")
+def upload(source, destination):


At some point, we'll want to implement a recursive download as well, but we don't have to do that in this PR. The recursive upload/download is the only reason I kept the aws cli awfulness around.

willkg · 2024-04-26T23:24:05Z

bin/process_crashes.sh

+  ./socorro-cmd gcs upload "${DATADIR}" "${CRASHSTORAGE_GCS_BUCKET}"
+  ./socorro-cmd gcs list_objects "${CRASHSTORAGE_GCS_BUCKET}"
+else
+  ./bin/socorro_aws_s3.sh mb "s3://${CRASHSTORAGE_S3_BUCKET}/"


Re-upping this sentiment. socorro_aws_s3.sh is for the birds.

socorro/external/gcs/crashstorage.py

willkg · 2024-04-26T23:42:17Z

socorro/external/gcs/crashstorage.py

+    list_to_str,
+    str_to_list,
+)
+from socorro.lib import external_common, MissingArgumentError, BadArgumentError, libooid


Some day, I'm going to get rid of the fact we call "crash id" by three different names depending on where in the codebase we are. Some day.

willkg · 2024-04-27T00:12:45Z

socorro/external/gcs/crashstorage.py

+        return json.loads(processed_crash_as_string)
+
+    def get(self, **kwargs):
+        """Return JSON data of a crash report, given its uuid."""


Suggested change

"""Return JSON data of a crash report, given its uuid."""

"""Return JSON data of a crash report, given its uuid."""

# FIXME(relud): This method is used by the webapp API middleware nonsense. It shouldn't

# exist here. We should move it to the webapp.

This get thing is bad architecture we've had for a while. It's explicitly a webapp API "middleware" thing for HTTP GET requests and we shouldn't have it in the socorro/ tree at all--it should be somewhere in webapp/.

I think we should go with what you've done here and fix it in the future.

However, we should mark it with a nasty comment because it's a nasty thing.

socorro/tests/external/gcs/test_crashstorage.py

socorro/tests/test_gcs_cli.py

webapp/crashstats/crashstats/management/commands/verifyprocessed.py

relud · 2024-05-07T21:20:53Z

@willkg ready for another pass

willkg

Looks good! The test plan helped a ton. Thank you for that!

willkg · 2024-05-08T19:59:03Z

socorro/external/crashstorage_base.py

+    return datestamp
+
+
+class JSONISOEncoder(json.JSONEncoder):


We don't have to make this change in this PR, but this might be better in socorro/lib/libdatetime.py because there's a duplicate definition in webapp/crashstats/crashstats/utils.py.

We might even be able to replace JsonDTEncoder. They do slightly different things (one adds a T), but maybe we don't really need both?

Python 3.10.14 (main, May 6 2024, 10:26:19) [GCC 13.2.0] on linux Type "help", "copyright", "credits" or "license" for more information. >>> import datetime >>> now = datetime.datetime.now() >>> now.strftime("%Y-%m-%d %H:%M:%S.%f") '2024-05-08 15:59:42.779732' >>> now.isoformat() '2024-05-08T15:59:42.779732'

filed https://bugzilla.mozilla.org/show_bug.cgi?id=1895802 to track this

relud force-pushed the gcs-storage branch 10 times, most recently from 82931c8 to 11f76be Compare April 9, 2024 21:40

relud force-pushed the gcs-storage branch 2 times, most recently from 551ca5f to 523bab1 Compare April 16, 2024 17:07

bug-1886021: Implement GCS storage classes for GCP

435853f

relud force-pushed the gcs-storage branch from 523bab1 to 435853f Compare April 16, 2024 18:02

relud marked this pull request as ready for review April 16, 2024 18:02

relud requested a review from a team as a code owner April 16, 2024 18:02

relud commented Apr 16, 2024

View reviewed changes

relud requested a review from willkg April 16, 2024 18:03

willkg requested changes Apr 16, 2024

View reviewed changes

fix gcs upload and add tests

a818bf0

relud force-pushed the gcs-storage branch from 1735cfc to a818bf0 Compare April 17, 2024 22:07

fix copypasta docstring

f17cf2e

relud requested a review from willkg April 17, 2024 22:12

willkg requested changes Apr 27, 2024

View reviewed changes

relud added 5 commits May 7, 2024 14:07

address review

bc25d37

move and copy utility functions

9e8edda

remove _fileobj suffix from s3_helper methods

61ebf61

merge test_crash_data into test_crashstorage

7e291ae

only require objectUser permissions

7792931

relud requested a review from willkg May 7, 2024 21:16

Merge branch 'main' into gcs-storage

dfb5ece

willkg approved these changes May 8, 2024

View reviewed changes

relud added 2 commits May 8, 2024 14:36

Merge branch 'main' into gcs-storage

ab4cca2

Merge branch 'main' into gcs-storage

ef3069b

relud merged commit 6dc30d2 into main May 8, 2024
2 checks passed

relud deleted the gcs-storage branch May 8, 2024 21:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bug-1886021: Implement GCS storage classes for GCP #6572

bug-1886021: Implement GCS storage classes for GCP #6572

relud commented Apr 3, 2024 •

edited

Loading

relud Apr 16, 2024

willkg Apr 27, 2024

relud Apr 16, 2024

willkg Apr 27, 2024

relud Apr 16, 2024

relud Apr 16, 2024 •

edited

Loading

willkg left a comment

willkg Apr 16, 2024

willkg Apr 16, 2024

willkg Apr 26, 2024

willkg Apr 16, 2024

willkg Apr 16, 2024

willkg left a comment

willkg Apr 26, 2024

willkg Apr 26, 2024

willkg Apr 26, 2024

willkg Apr 27, 2024

relud commented May 7, 2024

willkg left a comment

willkg May 8, 2024

relud May 8, 2024


		:returns: generator of keys

		:returns: generator of pages (lists) of object keys

		from socorro.schemas import TELEMETRY_SOCORRO_CRASH_SCHEMA


		class GcsCrashStorage(CrashStorageBase):

bug-1886021: Implement GCS storage classes for GCP #6572

bug-1886021: Implement GCS storage classes for GCP #6572

Conversation

relud commented Apr 3, 2024 • edited Loading

run CI tests locally

check aws mode works

check gcp mode works

expectation:

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

relud Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

willkg 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willkg 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

Choose a reason for hiding this comment

relud commented May 7, 2024

willkg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

relud commented Apr 3, 2024 •

edited

Loading

relud Apr 16, 2024 •

edited

Loading