workaround for S3 in us-east-1 #3710

mr-c · 2021-07-15T14:50:41Z

Fixes #3709

Add a test

Changelog Entry

To be copied to the draft changelog by merger:

Workaround for Strange behavior when trying to create an S3 bucket in us-east-1 boto/boto3#125 when the job store is S3 based and the region is "us-east-1".

Reviewer Checklist

Make sure it is coming from issues/XXXX-fix-the-thing in the Toil repo, or from an external repo.
- If it is coming from an external repo, make sure to pull it in for CI with:
```
contrib/admin/test-pr otheruser theirbranchname issues/XXXX-fix-the-thing
```
- If there is no associated issue, create one.
Read through the code changes. Make sure that it doesn't have:
- Addition of trailing whitespace.
- New variable or member names in camelCase that want to be in snake_case.
- New functions without type hints.
- New functions or classes without informative docstrings.
- Changes to semantics not reflected in the relevant docstrings.
- New or changed command line options for Toil workflows that are not reflected in docs/running/cliOptions.rst
- New features without tests.
Comment on the lines of code where problems exist with a review comment. You can shift-click the line numbers in the diff to select multiple lines.
Finish the review with an overall description of your opinion.

Merger Checklist

Make sure the PR passes tests.
Make sure the PR has been reviewed since its last modification. If not, review it.
Merge with the Github "Squash and merge" feature.
- If there are multiple authors' commits, add Co-authored-by to give credit to all contributing authors.
Copy its recommended changelog entry to the Draft Changelog.
Append the issue number in parentheses to the changelog entry.

src/toil/jobStores/aws/jobStore.py

mr-c · 2021-07-16T08:34:32Z

Huh, just running the integration tests in us-east-1 is not sufficient to reproduce the bug. I'm guessing we need to force testing without creating the bucket first to trigger the code path in question.

mr-c · 2021-07-16T09:40:29Z

Huzzah, finally able to reproduce the error "InvalidLocationConstraint" (in causes all the AWSJobStore tests to fail)

https://ucsc-ci.com/databiosphere/toil/-/jobs/89756

adamnovak

This looks like it should mostly work. But I think we'd end up passing regions where we mean to pass zones, sometimes, as written.

Also, rather than running another copy of the AWS job store tests, can't we just add a single unit test to an existing CI job to test a single bucket creation utility function? We already have a slightly unmanageable number of distinct things that can go wrong in each CI run; it would be good to minimize the number of times AWS can fail on us for things that aren't our fault.

src/toil/test/__init__.py

src/toil/jobStores/aws/jobStore.py

.gitlab-ci.yml

src/toil/test/jobStores/jobStoreTest.py

mr-c · 2021-07-17T09:20:08Z

Also, rather than running another copy of the AWS job store tests, can't we just add a single unit test to an existing CI job to test a single bucket creation utility function?

Sure. I ran the entire test suite on us-east-1 to confirm that there were no other easily detected surprises with that region. It did uncover https://github.com/DataBiosphere/toil/pull/3710/files#diff-2a277145725ad8637547dd4492cc349b942cc67a4a2d003755bd2f6ddeb11ad9R444

The testing can be scaled back to just a single bucket creation utility function, yes

mr-c · 2021-07-17T14:32:16Z

@adamnovak Thanks for the review. I believe I have addressed all of your comments.

adamnovak

I think this looks good. I added a missing docstring and hooked the new tests file up to a CI job so that CI will run it.

mr-c · 2021-07-19T14:41:23Z

.gitlab-ci.yml

@@ -271,6 +271,7 @@ py37_main:
    - make test tests=src/toil/test/src
    - make test tests=src/toil/test/utils
    - make test tests=src/toil/test/lib/test_ec2.py
+    - make test tests=src/toil/test/lib/aws


The new test is already run as part of the quick_test_offline

https://ucsc-ci.com/databiosphere/toil/-/jobs/90265/raw

src/toil/test/lib/aws/test_s3.py::S3Test::test_create_bucket -------------------------------- live log call --------------------------------- INFO toil.test:__init__.py:91 Setting up toil.test.lib.aws.test_s3.S3Test.test_create_bucket ... INFO toil.test:__init__.py:96 Tore down toil.test.lib.aws.test_s3.S3Test.test_create_bucket PASSED

But isn't it an online test? Shouldn't it be pulled out of there? make test_offline isn't supposed to need Internet access according to the Makefile.

I agree, it is an online test. Lots of the needs_aws tests run in the quick_test_offline, including the entire AWSJobStoreTest suite!

Can we merge this and fix the test distribution in a separate PR?

I opened #3717

We might have already had an issue for that; I guess we need a new decorator or something. We probably don't need to fix it here.

w-gao reviewed Jul 15, 2021

View reviewed changes

src/toil/jobStores/aws/jobStore.py Outdated Show resolved Hide resolved

mr-c force-pushed the issues/3709-workaround-boto3-us-east-1-bucket branch from 2f7eb14 to 2e090d9 Compare July 16, 2021 08:05

mr-c force-pushed the issues/3709-workaround-boto3-us-east-1-bucket branch 3 times, most recently from c54dda1 to d76754f Compare July 16, 2021 09:32

mr-c force-pushed the issues/3709-workaround-boto3-us-east-1-bucket branch 5 times, most recently from b4e3f38 to 72ce1c5 Compare July 16, 2021 12:35

mr-c requested a review from w-gao July 16, 2021 12:53

mr-c force-pushed the issues/3709-workaround-boto3-us-east-1-bucket branch 2 times, most recently from 9bc36f6 to 1401c8f Compare July 16, 2021 16:59

adamnovak requested changes Jul 16, 2021

View reviewed changes

src/toil/test/__init__.py Outdated Show resolved Hide resolved

src/toil/jobStores/aws/jobStore.py Outdated Show resolved Hide resolved

.gitlab-ci.yml Outdated Show resolved Hide resolved

src/toil/test/jobStores/jobStoreTest.py Show resolved Hide resolved

mr-c force-pushed the issues/3709-workaround-boto3-us-east-1-bucket branch 4 times, most recently from 4fe9454 to dde4479 Compare July 17, 2021 14:31

mr-c requested a review from adamnovak July 17, 2021 14:32

mr-c force-pushed the issues/3709-workaround-boto3-us-east-1-bucket branch from dde4479 to 6556a7f Compare July 17, 2021 14:42

workaround for boto/boto3#125

177e3f8

mr-c force-pushed the issues/3709-workaround-boto3-us-east-1-bucket branch from 6556a7f to 177e3f8 Compare July 17, 2021 15:06

mr-c changed the title ~~workaround for https://github.com/boto/boto3/issues/125~~ workaround for S3 in us-east-1 Jul 17, 2021

mr-c and others added 3 commits July 19, 2021 10:13

Merge branch 'master' into issues/3709-workaround-boto3-us-east-1-bucket

acbe720

Add a docstring to the new function

8716d04

Add the new lib/aws tests to a CI job so they run

570cfb3

adamnovak approved these changes Jul 19, 2021

View reviewed changes

mr-c commented Jul 19, 2021

View reviewed changes

mr-c enabled auto-merge (squash) July 20, 2021 14:15

Merge branch 'master' into issues/3709-workaround-boto3-us-east-1-bucket

6122d89

mr-c mentioned this pull request Jul 20, 2021

online tests being run during quick_test_offline #3717

Closed

mr-c merged commit 88db778 into master Jul 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

workaround for S3 in us-east-1 #3710

workaround for S3 in us-east-1 #3710

mr-c commented Jul 15, 2021 •

edited by adamnovak

Loading

mr-c commented Jul 16, 2021

mr-c commented Jul 16, 2021

adamnovak left a comment

mr-c commented Jul 17, 2021

mr-c commented Jul 17, 2021

adamnovak left a comment

mr-c Jul 19, 2021 •

edited

Loading

adamnovak Jul 20, 2021

mr-c Jul 20, 2021

mr-c Jul 20, 2021

adamnovak Jul 20, 2021

workaround for S3 in us-east-1 #3710

workaround for S3 in us-east-1 #3710

Conversation

mr-c commented Jul 15, 2021 • edited by adamnovak Loading

Changelog Entry

Reviewer Checklist

Merger Checklist

mr-c commented Jul 16, 2021

mr-c commented Jul 16, 2021

adamnovak left a comment

Choose a reason for hiding this comment

mr-c commented Jul 17, 2021

mr-c commented Jul 17, 2021

adamnovak left a comment

Choose a reason for hiding this comment

mr-c Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

adamnovak Jul 20, 2021

Choose a reason for hiding this comment

mr-c Jul 20, 2021

Choose a reason for hiding this comment

mr-c Jul 20, 2021

Choose a reason for hiding this comment

adamnovak Jul 20, 2021

Choose a reason for hiding this comment

mr-c commented Jul 15, 2021 •

edited by adamnovak

Loading

mr-c Jul 19, 2021 •

edited

Loading