diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 9467ba1..78549ab 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -7,6 +7,13 @@ Change Log ---------- +4.2.0 +===== + +* Better error diagnostics for S3FS-mounted files that are glaciated + if the ``CGAP_S3FS_UPLOAD_BUCKETS`` and ``CGAP_S3FS_UPLOAD_DIR`` environment variables are set. + + 4.1.0 ===== diff --git a/poetry.lock b/poetry.lock index 50ca599..b489b1c 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1610,4 +1610,4 @@ testing = ["big-O", "flake8 (<5)", "jaraco.functools", "jaraco.itertools", "more [metadata] lock-version = "2.0" python-versions = ">=3.7.0,<3.10" -content-hash = "97f0846571152517d2ec02a4c7a3e4dcc415d23aa90a6c6f2759df286db25737" +content-hash = "1931a8f265530d2ca4de1671b3e767a0835a8991a28ad9af78f02bc1797d01c7" diff --git a/pyproject.toml b/pyproject.toml index 6bdb70c..fbf9d1b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [tool.poetry] name = "submit_cgap" -version = "4.1.0" +version = "4.1.0.2b3" description = "Support for uploading file submissions to the Clinical Genomics Analysis Platform (CGAP)." authors = ["4DN-DCIC Team "] license = "MIT" @@ -64,7 +64,7 @@ coverage = ">=6.5.0" # coveralls = ">=3.3.1" # Linting generally -flake8 = ">=3.9.2" +flake8 = ">=4.0.1" # pygments is used by PyCharm pygments = ">=2.14.0" @@ -101,6 +101,5 @@ exclude_lines = [ ] [build-system] - -requires = ["poetry_core>=1.0.0"] +requires = ["poetry-core>=1.0.0"] build-backend = "poetry.core.masonry.api" diff --git a/submit_cgap/submission.py b/submit_cgap/submission.py index 38f9841..95648b3 100644 --- a/submit_cgap/submission.py +++ b/submit_cgap/submission.py @@ -1,3 +1,4 @@ +import boto3 import glob import io import json @@ -1174,6 +1175,7 @@ def wrapper(*args, **kwargs): % (file_name, self.uuid) ) except Exception as e: + maybe_show_s3fs_warnings(file_name) show("%s: %s" % (e.__class__.__name__, e)) return result return wrapper @@ -1213,6 +1215,97 @@ def upload_extra_files( wrapped_execute_prearranged_upload(extra_file_path, extra_file_credentials, auth=auth) +# Refs: +# * https://docs.aws.amazon.com/AmazonS3/latest/userguide/storage-class-intro.html +# * https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3.html +# +# We single out 'available' storage classes as the ones someone would expect to be readily +# available in short time and not subject to catastrophe, so that an I/O error probably +# isn't related to the storage class. In practice that's the two standard storage classes +# plus the intelligent tiering. Most of the others have a latency issue or are otherwise +# fragile. In practice, we just want to not overly warn about normal kinds of storage. + +ALL_S3_STORAGE_CLASSES = [ + 'STANDARD', 'REDUCED_REDUNDANCY', 'STANDARD_IA', 'ONEZONE_IA', 'INTELLIGENT_TIERING', + 'GLACIER', 'DEEP_ARCHIVE', 'OUTPOSTS', 'GLACIER_IR', +] + +AVAILABLE_S3_STORAGE_CLASSES = [ + 'STANDARD', 'STANDARD_IA', 'INTELLIGENT_TIERING' +] + +CGAP_S3FS_MAPPING_PATTERN = re.compile(r"^([^:]+):(.+)$") + + +def check_s3fs_mapped_filename(filename, *, s3): + """ + Determines whether a given filename is believed to be mapped by s3fs. + + :param filename: the filename to check + :param s3: an s3 client. + :return: + """ + s3 = s3 or boto3.client('s3') + upload_buckets = os.environ.get("CGAP_S3FS_UPLOAD_BUCKETS") + upload_dir = os.environ.get("CGAP_S3FS_UPLOAD_DIR") + if not upload_buckets or not upload_dir: + # We're not using S3FS mapping, so we have no warnings to show. + return None + mapped_dir = upload_dir.rstrip('/') + pattern = f"^(?:{re.escape(mapped_dir)}|{re.escape(os.path.expanduser(mapped_dir))})/(.*)$" + m = re.match(pattern, filename) + if not m: + # Some files might not match our mapping. + return None + mapped_key = m.group(1) + candidates = bash_enumeration(upload_buckets) + for mapped_bucket in candidates: + try: + s3.head_object(Bucket=mapped_bucket, Key=mapped_key) # an error means we're failing + return mapped_bucket, mapped_key + except Exception: + pass + else: + # No suitable match found + return None + + +STRING_LIST_SEPARATORS = str.maketrans("\n\t,", " ") + + +def bash_enumeration(string_list): + """ + For use to parse a bash variable value that purports to contain a list. + The items in the list can be separated by commas, spaces, or newlines + (and so the elements may not contain those characters). + + :param string_list: a string representing a list of items separated by commas, spaces, or newlines. + :return: the items in the list, as strings + """ + return [x for x in string_list.translate(STRING_LIST_SEPARATORS).split(" ") if x] + + +def maybe_show_s3fs_warnings(filename, *, s3=None): + """ + If the indicated filename is mapped bucket by s3fs to an s3 bucket/key, + this checks whether warning about its storage class is warranted and, if it is, presents such a warning. + """ + s3 = s3 or boto3.client('s3') # noQA + mapped_bucket_and_key = check_s3fs_mapped_filename(filename, s3=s3) + if not mapped_bucket_and_key: + return False + # If we get to here, we have a plausibly mapped filename that we might want to warn about. + mapped_bucket, mapped_key = mapped_bucket_and_key + try: + metadata = s3.head_object(Bucket=mapped_bucket, Key=mapped_key) + storage_class = metadata['StorageClass'] + if storage_class not in AVAILABLE_S3_STORAGE_CLASSES: + show(f"The file {filename} is mapped via S3FS to {storage_class} storage.") + except Exception as e: + # Add some context for an error message we're about to see on the console. + show(f"An error occurred while trying to ask S3 about Bucket={mapped_bucket!r}, Key={mapped_key!r}: {e}") + + def upload_item_data(item_filename, uuid, server, env, no_query=False): """ Given a part_filename, uploads that filename to the Item specified by uuid on the given server. diff --git a/submit_cgap/tests/test_submission.py b/submit_cgap/tests/test_submission.py index ba895fb..97eaacd 100644 --- a/submit_cgap/tests/test_submission.py +++ b/submit_cgap/tests/test_submission.py @@ -8,7 +8,9 @@ from dcicutils.common import APP_CGAP, APP_FOURFRONT, APP_SMAHT from dcicutils.misc_utils import ignored, ignorable, local_attrs, override_environ, NamedObject -from dcicutils.qa_utils import ControlledTime, MockFileSystem, raises_regexp, printed_output +from dcicutils.qa_utils import ( + ControlledTime, MockFileSystem, raises_regexp, printed_output, MockBoto3, MockBotoS3Client +) from dcicutils.s3_utils import HealthPageKey from typing import List, Dict from unittest import mock @@ -26,6 +28,8 @@ resolve_server, resume_uploads, show_section, submit_any_ingestion, upload_file_to_uuid, upload_item_data, get_s3_encrypt_key_id, get_s3_encrypt_key_id_from_health_page, running_on_windows_native, + check_s3fs_mapped_filename, maybe_show_s3fs_warnings, ALL_S3_STORAGE_CLASSES, AVAILABLE_S3_STORAGE_CLASSES, + bash_enumeration, search_for_file, UploadMessageWrapper, upload_extra_files, _resolve_app_args, # noQA - yes, a protected member, but we still need to test it _post_files_data, # noQA - again, testing a protected member @@ -2561,6 +2565,178 @@ def mysterious_error(*args, **kwargs): assert shown.lines == Scenario.make_timeout_submission_lines() +def test_check_s3fs_mapped_filename(): + """ + This will test that the mapping-finder works under various situations of buckets present or missing. + The idea is that there might be a key1 that we expect to find in a bucket1, assuming bucket1 is mapped, + and a key2 in a bucket2 if that bucket is mapped. But the filename we'll see will contain only the key, + not the bucket, so the bucket finder has to call .head_object to know if the key is there. It can then + find the key no matter what bucket it's in. + """ + + raw_mapping_dir = "~/my-uploads" + expanded_mapping_dir = os.path.expanduser(raw_mapping_dir) + bucket1 = 'foo1' + key1 = "12345/bar.baz" + bucket2 = 'foo2' + key2 = '67890/bar.baz' + + class FakeS3ClientForTesting: + """ + The only operation our test client needs to support is head_object, and only for side-effect. See below. + If other operations get called, we want to know about it, so we use a one-off class. + Having done f = FakeS3CLientForTesting("foo/bar"), later doing f.head_object(Bucket="foo", Key="bar") will + return without error but other arguments will not. + """ + + def __init__(self, filenames=None): + """Create the class with a list of 'files' in our fake S3.""" + self.filenames = filenames or [] + + def head_object(self, Bucket, Key): # noQA + """We're only being called for side-effect. A non-error return means the file is there.""" + filename = f"{Bucket}/{Key}" + if filename in self.filenames: + return {'found': filename} + else: + raise RuntimeError("File not found.") + + fake_s3 = FakeS3ClientForTesting(filenames=[f"{bucket1}/{key1}", f"{bucket2}/{key2}"]) + + def do_testing(mapped_buckets): + """ + We expect a set of mapped buckets like ['bucket1', 'bucket2'] and will run the set of tests on that. + The expectation is that if 'bucket1' is anywhere in the list, then key1 will be found in it, + and if 'bucket2' is anywhere in the list, then key2 will be found in it. Otherwise None. + """ + expectations = {'decoy': None, + key1: (bucket1, key1) if bucket1 in mapped_buckets else None, + key2: (bucket2, key2) if bucket2 in mapped_buckets else None} + for mapping_dir in [raw_mapping_dir, expanded_mapping_dir]: + for suffix in ["", "/"]: + for upload_key in [key1, key2, 'decoy']: + upload_dir = f"{mapping_dir}{suffix}" + upload_buckets = ",".join(mapped_buckets) + with override_environ(CGAP_S3FS_UPLOAD_BUCKETS=upload_buckets, CGAP_S3FS_UPLOAD_DIR=upload_dir): + sample_filename = f'{mapping_dir}/{upload_key}' + print(f"\nCGAP_S3FS_UPLOAD_BUCKETS={upload_buckets} CGAP_S3FS_UPLOAD_DIR={upload_dir}" + f" sample_filename={sample_filename} key={upload_key}") + parsed = check_s3fs_mapped_filename(sample_filename, s3=fake_s3) + print(f"parsed={parsed}") + assert parsed == expectations[upload_key] + + with printed_output() as printed: + sample_unmapped_filename = 'something.else' + parsed = check_s3fs_mapped_filename(sample_unmapped_filename, s3=fake_s3) + assert not parsed + assert printed.lines == [] + + with override_environ(CGAP_S3FS_UPLOAD_BUCKETS=""): + with printed_output() as printed: + parsed = check_s3fs_mapped_filename(sample_filename, s3=fake_s3) + assert not parsed + assert printed.lines == [] + + with override_environ(CGAP_S3FS_UPLOAD_DIR=""): + with printed_output() as printed: + parsed = check_s3fs_mapped_filename(sample_filename, s3=fake_s3) + assert not parsed + assert printed.lines == [] + + with override_environ(CGAP_S3FS_UPLOAD_DIR=None): + with printed_output() as printed: + parsed = check_s3fs_mapped_filename(sample_filename, s3=fake_s3) + assert not parsed + assert printed.lines == [] + + for mapped_buckets in [[], + [bucket1], + [bucket2], + [bucket1, bucket2], + [bucket2, bucket1], + ['decoy', bucket1, bucket2]]: + do_testing(mapped_buckets) + + +def test_maybe_show_s3fs_warnings(): + + class MockBotoStandardS3Client(MockBotoS3Client): + DEFAULT_STORAGE_CLASS = 'STANDARD' + mock_boto3_with_standard_s3 = MockBoto3(s3=MockBotoStandardS3Client) + + with mock.patch.object(submission_module, "check_s3fs_mapped_filename") as mock_check: + + mock_boto3 = mock_boto3_with_standard_s3 + with mock.patch.object(submission_module, "boto3", mock_boto3): + + mock_check.return_value = None + + some_file = 'some_upload_dir/some.file' + + with printed_output() as printed: + # This just tests that explicitly passing the argument works. + maybe_show_s3fs_warnings(some_file, s3=mock_boto3) + assert printed.lines == [] + + with printed_output() as printed: + maybe_show_s3fs_warnings(some_file) + assert printed.lines == [] + + mock_check.return_value = ("some-bucket", "some-key") + + with printed_output() as printed: + maybe_show_s3fs_warnings(some_file) + assert printed.lines == [ + "An error occurred while trying to ask S3" + " about Bucket='some-bucket'," + " Key='some-key': Mock File Not Found:" + " some-bucket/some-key. Existing files: []" + ] + + some_bucket = 'some-bucket' + some_key = 'some-key' + + mock_check.return_value = (some_bucket, some_key) + + s3 = mock_boto3.client('s3') + + s3.upload_fileobj( + Fileobj=io.BytesIO(b'{"some": "object"}'), + Bucket=some_bucket, Key=some_key) + + with printed_output() as printed: + maybe_show_s3fs_warnings(some_file) + assert printed.lines == [] # some_file is allocated as StorageClass=STANDARD so no warning to offer. + + assert isinstance(s3, MockBotoS3Client) # We do this for side-effect so that PyCharm will know the type + + for storage_class in ALL_S3_STORAGE_CLASSES: + + s3._set_object_storage_class_for_testing(f'{some_bucket}/{some_key}', storage_class) + + with printed_output() as printed: + + maybe_show_s3fs_warnings(some_file) + + if storage_class in AVAILABLE_S3_STORAGE_CLASSES: + assert printed.lines == [] + else: + assert printed.lines == [ + f"The file {some_file} is mapped via S3FS to {storage_class} storage." + ] + + +def test_bash_enumeration(): + + # The bash_enumeration function is just testing that a user of bash, having made what they informally think + # of as a list, will have specified something coherent. They can use commas, spaces, or newlines as separators. + + assert bash_enumeration('foo bar baz') == ['foo', 'bar', 'baz'] + assert bash_enumeration('foo\nbar\nbaz') == ['foo', 'bar', 'baz'] + assert bash_enumeration('foo, bar,baz') == ['foo', 'bar', 'baz'] + assert bash_enumeration('foo,\n bar ,,,baz, \n') == ['foo', 'bar', 'baz'] + + def test_running_on_windows_native(): for pair in [("nt", True), ("posix", False)]: os_name, is_windows = pair