Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

workaround for S3 in us-east-1 #3710

Merged
merged 5 commits into from
Jul 21, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitlab-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

@mr-c mr-c Jul 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #3717

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

- make test tests=src/toil/test/lib/test_conversions.py

py37_appliance_build:
Expand Down
3 changes: 1 addition & 2 deletions contrib/admin/mypy-with-ignore.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,7 @@ def main():
'src/toil/lib/encryption/_dummy.py',
'src/toil/lib/encryption/conftest.py',
'src/toil/lib/encryption/__init__.py',
'src/toil/lib/aws/utils.py',
'src/toil/lib/aws/__init__.py'
'src/toil/lib/aws/__init__.py',
]]

filtered_files_to_check = []
Expand Down
5 changes: 4 additions & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,13 @@ def run_setup():
addict,
pytz,
pyyaml,
enlighten]
enlighten,
"typing-extensions ; python_version < '3.8'"
]
aws_reqs = [
boto,
boto3,
"boto3-stubs[s3]>=1.17, <2",
futures,
pycryptodome]
cwl_reqs = [
Expand Down
2 changes: 1 addition & 1 deletion src/toil/jobStores/abstractJobStore.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
logger = logging.getLogger(__name__)

try:
from botocore.exceptions import ProxyConnectionError # type: ignore
from botocore.exceptions import ProxyConnectionError
except ImportError:
class ProxyConnectionError(BaseException): # type: ignore
pass
Expand Down
16 changes: 9 additions & 7 deletions src/toil/jobStores/aws/jobStore.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
from toil.jobStores.utils import (ReadablePipe,
ReadableTransformingPipe,
WritablePipe)
from toil.lib.aws.utils import create_s3_bucket
from toil.lib.compatibility import compat_bytes
from toil.lib.ec2 import establish_boto3_session
from toil.lib.ec2nodes import EC2Regions
Expand Down Expand Up @@ -744,11 +745,9 @@ def bucket_creation_pending(error):
bucketExisted = False
logger.debug("Bucket '%s' does not exist.", bucket_name)
if create:
logger.debug("Creating bucket '%s'.", bucket_name)
location = region_to_bucket_location(self.region)
bucket = self.s3_resource.create_bucket(
Bucket=bucket_name,
CreateBucketConfiguration={'LocationConstraint': location})
bucket = create_s3_bucket(
self.s3_resource, bucket_name, self.region
)
# Wait until the bucket exists before checking the region and adding tags
bucket.wait_until_exists()

Expand All @@ -757,7 +756,9 @@ def bucket_creation_pending(error):
# produce an S3ResponseError with code
# NoSuchBucket. We let that kick us back up to the
# main retry loop.
assert self.getBucketRegion(bucket_name) == self.region
assert (
self.getBucketRegion(bucket_name) == self.region
), f"bucket_name: {bucket_name}, {self.getBucketRegion(bucket_name)} != {self.region}"

owner_tag = os.environ.get('TOIL_OWNER_TAG')
if owner_tag:
Expand Down Expand Up @@ -1637,7 +1638,8 @@ def _delete_domain(self, domain):
if not no_such_sdb_domain(e):
raise

def _delete_bucket(self, bucket):
@staticmethod
def _delete_bucket(bucket):
"""
:param bucket: S3.Bucket
"""
Expand Down
2 changes: 1 addition & 1 deletion src/toil/jobStores/aws/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -441,4 +441,4 @@ def region_to_bucket_location(region):


def bucket_location_to_region(location):
return 'us-east-1' if location == '' else location
return "us-east-1" if location == "" or location is None else location
58 changes: 49 additions & 9 deletions src/toil/lib/aws/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,34 @@
# See the License for the specific language governing permissions and
# limitations under the License.
import logging
import sys
from typing import Optional, Union

from typing import Optional
from toil.lib import aws
from toil.lib.misc import printq
from toil.lib.retry import retry
from toil.lib import aws

if sys.version_info >= (3, 8):
from typing import Literal
else:
from typing_extensions import Literal

try:
from boto.exception import BotoServerError
from mypy_boto3_s3 import S3ServiceResource
from mypy_boto3_s3.literals import BucketLocationConstraintType
from mypy_boto3_s3.service_resource import Bucket
except ImportError:
BotoServerError = None # AWS/boto extra is not installed
BotoServerError = None # type: ignore
# AWS/boto extra is not installed

logger = logging.getLogger(__name__)


@retry(errors=[BotoServerError])
def delete_iam_role(role_name: str, region: Optional[str] = None, quiet: bool = True):
def delete_iam_role(
role_name: str, region: Optional[str] = None, quiet: bool = True
) -> None:
from boto.iam.connection import IAMConnection
iam_client = aws.client('iam', region_name=region)
iam_resource = aws.resource('iam', region_name=region)
Expand All @@ -47,8 +59,10 @@ def delete_iam_role(role_name: str, region: Optional[str] = None, quiet: bool =


@retry(errors=[BotoServerError])
def delete_iam_instance_profile(instance_profile_name: str, region: Optional[str] = None, quiet: bool = True):
iam_resource = aws.resource('iam', region_name=region)
def delete_iam_instance_profile(
instance_profile_name: str, region: Optional[str] = None, quiet: bool = True
) -> None:
iam_resource = aws.resource("iam", region_name=region)
instance_profile = iam_resource.InstanceProfile(instance_profile_name)
for role in instance_profile.roles:
printq(f'Now dissociating role: {role.name} from instance profile {instance_profile_name}', quiet)
Expand All @@ -58,14 +72,16 @@ def delete_iam_instance_profile(instance_profile_name: str, region: Optional[str


@retry(errors=[BotoServerError])
def delete_sdb_domain(sdb_domain_name: str, region: Optional[str] = None, quiet: bool = True):
sdb_client = aws.client('sdb', region_name=region)
def delete_sdb_domain(
sdb_domain_name: str, region: Optional[str] = None, quiet: bool = True
) -> None:
sdb_client = aws.client("sdb", region_name=region)
sdb_client.delete_domain(DomainName=sdb_domain_name)
printq(f'SBD Domain: "{sdb_domain_name}" successfully deleted.', quiet)


@retry(errors=[BotoServerError])
def delete_s3_bucket(bucket: str, region: Optional[str], quiet: bool = True):
def delete_s3_bucket(bucket: str, region: Optional[str], quiet: bool = True) -> None:
printq(f'Deleting s3 bucket in region "{region}": {bucket}', quiet)
s3_client = aws.client('s3', region_name=region)
s3_resource = aws.resource('s3', region_name=region)
Expand All @@ -78,3 +94,27 @@ def delete_s3_bucket(bucket: str, region: Optional[str], quiet: bool = True):
s3_client.delete_object(Bucket=bucket, Key=version['Key'], VersionId=version['VersionId'])
s3_resource.Bucket(bucket).delete()
printq(f'\n * Deleted s3 bucket successfully: {bucket}\n\n', quiet)


def create_s3_bucket(
s3_session: "S3ServiceResource",
bucket_name: str,
region: Union["BucketLocationConstraintType", Literal["us-east-1"]],
) -> "Bucket":
"""
Create an AWS S3 bucket, using the given Boto3 S3 session, with the
given name, in the given region.

Supports the us-east-1 region, where bucket creation is special.

*ALL* S3 bucket creation should use this function.
"""
logger.debug("Creating bucket '%s' in region %s.", bucket_name, region)
if region == "us-east-1": # see https://github.com/boto/boto3/issues/125
bucket = s3_session.create_bucket(Bucket=bucket_name)
else:
bucket = s3_session.create_bucket(
Bucket=bucket_name,
CreateBucketConfiguration={"LocationConstraint": region},
)
return bucket
12 changes: 8 additions & 4 deletions src/toil/test/jobStores/jobStoreTest.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
NoSuchJobException)
from toil.jobStores.fileJobStore import FileJobStore
from toil.lib.memoize import memoize
from toil.lib.aws.utils import create_s3_bucket
from toil.statsAndLogging import StatsAndLogging
from toil.test import (ToilTest,
make_tests,
Expand Down Expand Up @@ -1408,14 +1409,17 @@ def _hashTestFile(self, url: str) -> str:
def _createExternalStore(self):
"""A S3.Bucket instance is returned"""
from toil.jobStores.aws.utils import retry_s3
from toil.jobStores.aws.utils import region_to_bucket_location
from toil.jobStores.aws.jobStore import establish_boto3_session
resource = establish_boto3_session().resource('s3', region_name=self.awsRegion())
bucket = resource.Bucket('import-export-test-%s' % uuid.uuid4())

resource = establish_boto3_session().resource(
"s3", region_name=self.awsRegion()
)
bucket_name = f"import-export-test-{uuid.uuid4()}"
location = self.awsRegion()

for attempt in retry_s3():
with attempt:
bucket.create(CreateBucketConfiguration={'LocationConstraint': region_to_bucket_location(self.awsRegion())})
mr-c marked this conversation as resolved.
Show resolved Hide resolved
bucket = create_s3_bucket(resource, bucket_name, location)
bucket.wait_until_exists()
return bucket

Expand Down
Empty file.
62 changes: 62 additions & 0 deletions src/toil/test/lib/aws/test_s3.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
# Copyright (C) 2021 Michael R. Crusoe
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
import logging
import os
import uuid
from typing import Optional

from toil.jobStores.aws.jobStore import AWSJobStore
from toil.lib.aws.utils import create_s3_bucket
from toil.lib.ec2 import establish_boto3_session
from toil.test import ToilTest, needs_aws_s3

logger = logging.getLogger(__name__)
logging.basicConfig(level=logging.DEBUG)


@needs_aws_s3
class S3Test(ToilTest):
"""Confirm the workarounds for us-east-1."""

from mypy_boto3_s3 import S3ServiceResource
from mypy_boto3_s3.service_resource import Bucket

s3_resource: Optional[S3ServiceResource]
bucket: Optional[Bucket]

@classmethod
def setUpClass(cls) -> None:
session = establish_boto3_session(region_name="us-east-1")
cls.s3_resource = session.resource("s3", region_name="us-east-1")
cls.bucket = None

def test_create_bucket(self) -> None:
"""Test bucket creation for us-east-1."""
bucket_name = f"toil-s3test-{uuid.uuid4()}"
assert self.s3_resource
self.bucket = create_s3_bucket(self.s3_resource, bucket_name, "us-east-1")
self.bucket.wait_until_exists()
owner_tag = os.environ.get("TOIL_OWNER_TAG")
if owner_tag:
bucket_tagging = self.s3_resource.BucketTagging(bucket_name)
bucket_tagging.put(
Tagging={"TagSet": [{"Key": "Owner", "Value": owner_tag}]}
)
self.assertTrue(AWSJobStore.getBucketRegion, "us-east-1")

@classmethod
def tearDownClass(cls) -> None:
if cls.bucket:
AWSJobStore._delete_bucket(cls.bucket)
super().tearDownClass()