Tolerate a failed AMI polling attempt (#4727)

* Tolerate a failed AMI polling attempt

* Start marking Internet-relates tests to keep them out of the offline step

Author: adamnovak

Makefile

@@ -145,6 +145,7 @@ test_debug: check_venv check_build_reqs
 test_offline: check_venv check_build_reqs
 	@printf "$(cyan)All docker related tests will be skipped.$(normal)\n"
 	TOIL_SKIP_DOCKER=True \
+	TOIL_SKIP_ONLINE=True \
 	    python -m pytest -vv --timeout=600 --strict-markers --log-level DEBUG --log-cli-level INFO $(cov) -n $(threads) --dist loadscope $(tests) -m "$(marker)"
 
 # This target will run about 1 minute of tests, and stop at the first failure

common.mk

@@ -69,6 +69,7 @@ export TOIL_AWS_NODE_DEBUG?=False  # Don't shut down EC2 instances that fail so
 export TOIL_TEST_INTEGRATIVE?=False  # If ``True``, this allows the integration tests to run.
 export TOIL_TEST_QUICK?=False  # If ``True``, long running tests are skipped.
 export TOIL_SKIP_DOCKER?=False  # Skip docker dependent tests
+export TOIL_SKIP_ONLINE?=False  # Skip Internet-dependent tests
 
 export TOIL_AWS_KEYNAME?=id_rsa  # SSH key to use for tests in AWS.
 export TOIL_GOOGLE_KEYNAME?=id_rsa  # SSH key to use for tests in google.

setup.cfg

@@ -28,6 +28,7 @@ markers =
     local_cuda
     lsf
     mesos
+    online
     rsync
     server_mode
     slow

src/toil/lib/aws/ami.py

@@ -6,6 +6,7 @@
 from urllib.error import HTTPError, URLError
 
 from botocore.client import BaseClient
+from botocore.exceptions import ClientError
 
 from toil.lib.retry import retry
 
@@ -155,11 +156,18 @@ def feed_flatcar_ami_release(ec2_client: BaseClient, architecture: str = 'amd64'
 
     for ami in flatcar_release_feed_amis(region, architecture, source):
         # verify it exists on AWS
-        response = ec2_client.describe_images(Filters=[{'Name': 'image-id', 'Values': [ami]}])  # type: ignore
-        if len(response['Images']) == 1 and response['Images'][0]['State'] == 'available':
-            return ami 
-        else:
-            logger.warning(f'Flatcar release feed suggests image {ami} which does not exist on AWS in {region}')
+        try:
+            response = ec2_client.describe_images(Filters=[{'Name': 'image-id', 'Values': [ami]}])  # type: ignore
+            if len(response['Images']) == 1 and response['Images'][0]['State'] == 'available':
+                return ami
+            else:
+                logger.warning(f'Flatcar release feed suggests image {ami} which does not exist on AWS in {region}')
+        except ClientError:
+            # Sometimes we get back nonsense like:
+            # botocore.exceptions.ClientError: An error occurred (AuthFailure) when calling the DescribeImages operation: AWS was not able to validate the provided access credentials
+            # Don't hold that against the AMI.
+            logger.exception(f'Unable to check if AMI {ami} exists on AWS in {region}; assuming it does')
+            return ami
     # We didn't find it
     logger.warning(f'Flatcar release feed does not have an image for region {region} that exists on AWS')
     return None

src/toil/test/__init__.py

@@ -360,10 +360,17 @@ def needs_rsync3(test_item: MT) -> MT:
     return test_item
 
 
+def needs_online(test_item: MT) -> MT:
+    """Use as a decorator before test classes or methods to run only if we are meant to talk to the Internet."""
+    test_item = _mark_test('online', test_item)
+    if os.getenv('TOIL_SKIP_ONLINE', '').lower() == 'true':
+        return unittest.skip('Skipping online test.')(test_item)
+    return test_item
+
 def needs_aws_s3(test_item: MT) -> MT:
     """Use as a decorator before test classes or methods to run only if AWS S3 is usable."""
     # TODO: we just check for generic access to the AWS account
-    test_item = _mark_test('aws-s3', test_item)
+    test_item = _mark_test('aws-s3', needs_online(test_item))
     try:
         from boto import config
         boto_credentials = config.get('Credentials', 'aws_access_key_id')
@@ -416,7 +423,7 @@ def needs_google_storage(test_item: MT) -> MT:
     Cloud is installed and we ought to be able to access public Google Storage
     URIs.
     """
-    test_item = _mark_test('google-storage', test_item)
+    test_item = _mark_test('google-storage', needs_online(test_item))
     try:
         from google.cloud import storage  # noqa
     except ImportError:
@@ -428,7 +435,7 @@ def needs_google_project(test_item: MT) -> MT:
     """
     Use as a decorator before test classes or methods to run only if we have a Google Cloud project set.
     """
-    test_item = _mark_test('google-project', test_item)
+    test_item = _mark_test('google-project', needs_online(test_item))
     test_item = needs_env_var('TOIL_GOOGLE_PROJECTID', "a Google project ID")(test_item)
     return test_item
 
@@ -460,7 +467,7 @@ def needs_kubernetes_installed(test_item: MT) -> MT:
 
 def needs_kubernetes(test_item: MT) -> MT:
     """Use as a decorator before test classes or methods to run only if Kubernetes is installed and configured."""
-    test_item = needs_kubernetes_installed(test_item)
+    test_item = needs_kubernetes_installed(needs_online(test_item))
     try:
         import kubernetes
         try:
@@ -539,7 +546,7 @@ def needs_docker(test_item: MT) -> MT:
     Use as a decorator before test classes or methods to only run them if
     docker is installed and docker-based tests are enabled.
     """
-    test_item = _mark_test('docker', test_item)
+    test_item = _mark_test('docker', needs_online(test_item))
     if os.getenv('TOIL_SKIP_DOCKER', '').lower() == 'true':
         return unittest.skip('Skipping docker test.')(test_item)
     if which('docker'):
@@ -552,7 +559,7 @@ def needs_singularity(test_item: MT) -> MT:
     Use as a decorator before test classes or methods to only run them if
     singularity is installed.
     """
-    test_item = _mark_test('singularity', test_item)
+    test_item = _mark_test('singularity', needs_online(test_item))
     if which('singularity'):
         return test_item
     else:
@@ -589,7 +596,7 @@ def needs_docker_cuda(test_item: MT) -> MT:
     Use as a decorator before test classes or methods to only run them if
     a CUDA setup is available through Docker.
     """
-    test_item = _mark_test('docker_cuda', test_item)
+    test_item = _mark_test('docker_cuda', needs_online(test_item))
     if have_working_nvidia_docker_runtime():
         return test_item
     else:
@@ -645,7 +652,7 @@ def needs_celery_broker(test_item: MT) -> MT:
     """
     Use as a decorator before test classes or methods to run only if RabbitMQ is set up to take Celery jobs.
     """
-    test_item = _mark_test('celery', test_item)
+    test_item = _mark_test('celery', needs_online(test_item))
     test_item = needs_env_var('TOIL_WES_BROKER_URL', "a URL to a RabbitMQ broker for Celery")(test_item)
     return test_item
 
@@ -654,7 +661,7 @@ def needs_wes_server(test_item: MT) -> MT:
     Use as a decorator before test classes or methods to run only if a WES
     server is available to run against.
     """
-    test_item = _mark_test('wes_server', test_item)
+    test_item = _mark_test('wes_server', needs_online(test_item))
 
     wes_url = os.environ.get('TOIL_WES_ENDPOINT')
     if not wes_url:
@@ -712,7 +719,7 @@ def needs_fetchable_appliance(test_item: MT) -> MT:
     the Toil appliance Docker image is able to be downloaded from the Internet.
     """
 
-    test_item = _mark_test('fetchable_appliance', test_item)
+    test_item = _mark_test('fetchable_appliance', needs_online(test_item))
     if os.getenv('TOIL_SKIP_DOCKER', '').lower() == 'true':
         return unittest.skip('Skipping docker test.')(test_item)
     try:
@@ -733,9 +740,7 @@ def integrative(test_item: MT) -> MT:
     Use this to decorate integration tests so as to skip them during regular builds.
 
     We define integration tests as A) involving other, non-Toil software components
-    that we develop and/or B) having a higher cost (time or money). Note that brittleness
-    does not qualify a test for being integrative. Neither does involvement of external
-    services such as AWS, since that would cover most of Toil's test.
+    that we develop and/or B) having a higher cost (time or money).
     """
     test_item = _mark_test('integrative', test_item)
     if os.getenv('TOIL_TEST_INTEGRATIVE', '').lower() == 'true':

src/toil/test/cwl/cwlTest.py

@@ -57,6 +57,7 @@
                        needs_local_cuda,
                        needs_lsf,
                        needs_mesos,
+                       needs_online,
                        needs_slurm,
                        needs_torque,
                        needs_wes_server,
@@ -766,6 +767,7 @@ def _expected_streaming_output(self, outDir):
 
 
 @needs_cwl
+@needs_online
 class CWLv10Test(ToilTest):
     """
     Run the CWL 1.0 conformance tests in various environments.
@@ -896,6 +898,7 @@ def test_kubernetes_cwl_conformance_with_caching(self):
 
 
 @needs_cwl
+@needs_online
 class CWLv11Test(ToilTest):
     """
     Run the CWL 1.1 conformance tests in various environments.
@@ -950,6 +953,7 @@ def test_kubernetes_cwl_conformance_with_caching(self):
 
 
 @needs_cwl
+@needs_online
 class CWLv12Test(ToilTest):
     """
     Run the CWL 1.2 conformance tests in various environments.

src/toil/test/lib/test_ec2.py

@@ -21,11 +21,12 @@
                               flatcar_release_feed_amis,
                               get_flatcar_ami)
 from toil.lib.aws.session import establish_boto3_session
-from toil.test import ToilTest, needs_aws_ec2
+from toil.test import ToilTest, needs_aws_ec2, needs_online
 
 logger = logging.getLogger(__name__)
 logging.basicConfig(level=logging.DEBUG)
 
+@needs_online
 class FlatcarFeedTest(ToilTest):
     """Test accessing the Flatcar AMI release feed, independent of the AWS API"""