Better error message for s3fs-mapped files that are in glacier (C4-747), second try

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
 =====
 

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 <support@4dnucleome.org>"]
 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"

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.

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