Add wheel package to all containers

.hadolint.yaml

@@ -2,6 +2,7 @@ failure-threshold: warning
 format: tty
 
 # Ignored rules:
+# DL3013 - Specify package version because we don't want to do that
 # DL3037 - Specify package version because we don't want to do that
 # DL4006 - Use SHELL option `-o pipefail` because that's non OCI container option
-ignored: [DL3037, DL4006]
+ignored: [DL3013, DL3037, DL4006]

Dockerfile

@@ -10,8 +10,9 @@
 # * Install system requirements
 # * Install pip requirements
 # * Empty system cache to conserve some space
 RUN source /etc/os-release && zypper addrepo -G -cf "https://download.opensuse.org/repositories/SUSE:/CA/$VERSION_ID/SUSE:CA.repo" && \
-    zypper -n in ca-certificates-suse gcc libffi-devel && pip install --no-cache-dir -r /pcw/requirements.txt && zypper clean && rm -rf /var/cache
+    zypper -n in ca-certificates-suse gcc libffi-devel && \
+    pip install --no-cache-dir wheel && pip install --no-cache-dir -r /pcw/requirements.txt && zypper clean && rm -rf /var/cache
 
 # Copy program files only
 COPY ocw  /pcw/ocw/

Dockerfile_dev

@@ -8,7 +8,7 @@ ENV UWSGI_HTTP_AUTO_CHUNKED=1 UWSGI_HTTP_KEEPALIVE=1 UWSGI_LAZY_APPS=1 UWSGI_WSG
 RUN zypper -n in gcc libffi-devel && zypper clean && rm -rf /var/cache
 
 COPY requirements.txt requirements_test.txt requirements_k8s.txt /tmp/
-RUN pip install --no-cache-dir -r /tmp/requirements_test.txt
+RUN pip install --no-cache-dir wheel && pip install --no-cache-dir -r /tmp/requirements_test.txt
 
 WORKDIR /pcw
 

Dockerfile_k8s

@@ -8,7 +8,7 @@ RUN curl -sf https://dl.google.com/dl/cloudsdk/channels/rapid/downloads/google-c
 
 # Install python dependences
 COPY requirements_k8s.txt /pcw/
-RUN  pip3.11 install --no-cache-dir -r /pcw/requirements_k8s.txt
+RUN pip install --no-cache-dir wheel && pip install --no-cache-dir -r /pcw/requirements_k8s.txt
 
 # Copy program files only
 COPY ocw /pcw/ocw/

Dockerfile_k8s_dev

@@ -8,7 +8,7 @@ RUN curl -s https://dl.google.com/dl/cloudsdk/channels/rapid/downloads/google-cl
 
 # Install python dependences
 COPY requirements_k8s.txt requirements.txt requirements_test.txt /pcw/
-RUN  pip3.11 install --no-cache-dir -r /pcw/requirements_k8s.txt
+RUN pip install --no-cache-dir wheel && pip install --no-cache-dir -r /pcw/requirements_k8s.txt
 
 ENV PATH ${PATH}:/opt/google-cloud-sdk/bin/
 

ocw/lib/gce.py

@@ -1,3 +1,4 @@
+import contextlib
 import json
 from os.path import basename
 from datetime import timezone
@@ -59,7 +60,19 @@
             if GCE.get_error_reason(err) == 'resourceInUseByAnotherResource':
                 self.log_dbg(f"{resource_type.title()} '{resource_name}' can not be deleted because in use")
             elif GCE.get_error_reason(err) == 'badRequest':
-                self.log_err(f"{resource_type.title()} '{resource_name}' can not be deleted because of unknown reason")
+                # These are system generated routes when you create a network. These
+                # will be deleted by the deletion of the network and do not block the
+                # deletion of that network.
+                # There are no properties on the Route struct that indicate a route is a
+                # default one. Typically, the name will contain the word "default" or the
+                # description will contain the word "Default" but a property like Kind
+                # returns "compute#route" for all routes.
+                # All this creating false alarms in log which we want to prevent.
+                # Only way to prevent is mute error
+                if resource_type.title() == "Route" and "The local route cannot be deleted" in str(err):
+                    self.log_info("Skip deletion of local route")
+                else:
+                    self.log_err(f"{resource_type.title()} '{resource_name}' can not be deleted. Error : {err}")
             else:
                 raise err
 
@@ -112,13 +125,9 @@
 
     @staticmethod
     def get_error_reason(error: "googleapiclient.errors.HttpError") -> str:
-        reason = "unknown"
-        try:
-            error_content = json.loads(error.content)
-            return error_content['error']['errors'][0]['reason']
-        except (KeyError, ValueError, IndexError):
-            pass
-        return reason
+        with contextlib.suppress(KeyError, ValueError, IndexError):
+            return json.loads(error.content)['error']['errors'][0]['reason']
+        return "unknown"
 
     def cleanup_all(self) -> None:
         self.log_info("Call cleanup_all")

tests/test_gce.py

@@ -1,4 +1,6 @@
-from pytest import fixture
+import httplib2
+from googleapiclient.errors import HttpError
+from pytest import fixture, mark, raises
 from unittest.mock import MagicMock, patch
 import json
 from datetime import datetime, timezone, timedelta
@@ -8,19 +10,25 @@
 
 
 class MockRequest:
-    def __init__(self, response=None):
+    def __init__(self, response=None, error_reason=None):
         if response is None:
             response = {}
         self.response = response
+        self.error_reason = error_reason
 
     def execute(self):
+        if self.error_reason:
+            content = bytes(json.dumps({"error": {"errors": [{"reason": self.error_reason}]}}), 'utf-8')
+            raise HttpError(httplib2.Response({'status': 200}), content)
         return self.response
 
 
 class MockResource:
+
     def __init__(self, responses):
         self.deleted_resources = list()
         self.responses = responses
+        self.error_reason = None
 
     def __call__(self, *args, **kwargs):
         return self
@@ -34,6 +42,8 @@ def list_next(self, *args, **kwargs):
     def delete(self, *args, **kwargs):
         for resource in ('image', 'disk', 'instance', 'firewall', 'forwardingRule', 'route', 'network', 'subnetwork'):
             if resource in kwargs:
+                if self.error_reason:
+                    return MockRequest(error_reason=self.error_reason)
                 self.deleted_resources.append(kwargs[resource])
                 if len(self.responses) > 0:
                     return self.responses.pop(0)
@@ -71,6 +81,38 @@ def gce():
         yield gce
 
 
+@fixture
+def gce_dry_run_false(gce):
+    gce.dry_run = False
+    with (
+        patch.object(gce, 'list_regions', return_value=['region1']),
+        patch.object(gce, 'list_zones', return_value=['zone1'])
+    ):
+        yield gce
+
+
+@fixture
+def mocked_resource():
+    older_than_max_age = (datetime.now(timezone.utc) - timedelta(hours=max_age_hours+1)).strftime("%m/%d/%Y, %H:%M:%S")
+    now_age = datetime.now(timezone.utc).strftime("%m/%d/%Y, %H:%M:%S")
+    return MockResource([
+        MockRequest({   # on images().list()
+            'items': [
+                {'name': 'keep', 'creationTimestamp': now_age, 'network': 'mynetwork'},
+                {'name': 'delete1', 'creationTimestamp': older_than_max_age, 'network': 'mynetwork'}
+            ], 'id': "id"}),
+        MockRequest(),  # on images().delete()
+        MockRequest({   # on images().list_next()
+            'items': [
+                {'name': 'keep', 'creationTimestamp': now_age, 'network': 'mynetwork'},
+                {'name': 'delete2', 'creationTimestamp': older_than_max_age, 'network': 'mynetwork'}
+            ], 'id': "id"}),
+        MockRequest({'error': {'errors': [{'message': 'err message'}]},
+                    'warnings': [{'message': 'warning message'}]}),
+        None   # on images().list_next()
+    ])
+
+
 def test_delete_instance(gce):
     instances = MockResource([MockRequest({'items': ['instance1', 'instance2']}), None])
     gce.compute_client.instances = instances
@@ -101,68 +143,74 @@ def test_list_zones(gce):
     assert gce.list_zones('Oxfordshire') == ['RabbitHole']
 
 
-def _test_cleanup(gce, resource_type, cleanup_call):
-    older_than_max_age = (datetime.now(timezone.utc) - timedelta(hours=max_age_hours+1)).strftime("%m/%d/%Y, %H:%M:%S")
-    now_age = datetime.now(timezone.utc).strftime("%m/%d/%Y, %H:%M:%S")
+def _test_cleanup(gce, resource_type, cleanup_call, resources):
+
+    with (
+        patch.object(gce, 'list_regions', return_value=['region1']),
+        patch.object(gce, 'list_zones', return_value=['zone1']),
+    ):
+        setattr(gce.compute_client, resource_type, resources)
+        cleanup_call()
+        if gce.dry_run:
+            assert resources.deleted_resources == []
+        else:
+            assert resources.deleted_resources == ['delete1', 'delete2']
 
-    for _ in range(2):
-        resources = MockResource([
-            MockRequest({   # on images().list()
-                'items': [
-                    {'name': 'keep', 'creationTimestamp': now_age, 'network': 'mynetwork'},
-                    {'name': 'delete1', 'creationTimestamp': older_than_max_age, 'network': 'mynetwork'}
-                ], 'id': "id"}),
-            MockRequest(),  # on images().delete()
-            MockRequest({   # on images().list_next()
-                'items': [
-                    {'name': 'keep', 'creationTimestamp': now_age, 'network': 'mynetwork'},
-                    {'name': 'delete2', 'creationTimestamp': older_than_max_age, 'network': 'mynetwork'}
-                ], 'id': "id"}),
-            MockRequest({'error': {'errors': [{'message': 'err message'}]},
-                        'warnings': [{'message': 'warning message'}]}),
-            None   # on images().list_next()
-        ])
 
-        with (
-            patch.object(gce, 'list_regions', return_value=['region1']),
-            patch.object(gce, 'list_zones', return_value=['zone1']),
-        ):
-            setattr(gce.compute_client, resource_type, resources)
+@mark.parametrize("dry_run", [True, False])
+def test_cleanup_disks(gce, mocked_resource, dry_run):
+    gce.dry_run = dry_run
+    _test_cleanup(gce, "disks", gce.cleanup_disks, mocked_resource)
 
-            gce.dry_run = not gce.dry_run
-            cleanup_call()
-            if gce.dry_run:
-                assert resources.deleted_resources == []
-            else:
-                assert resources.deleted_resources == ['delete1', 'delete2']
 
+@mark.parametrize("dry_run", [True, False])
+def test_cleanup_images(gce, mocked_resource, dry_run):
+    gce.dry_run = dry_run
+    _test_cleanup(gce, "images", gce.cleanup_images, mocked_resource)
 
-def test_cleanup_disks(gce):
-    _test_cleanup(gce, "disks", gce.cleanup_disks)
 
+@mark.parametrize("dry_run", [True, False])
+def test_cleanup_firewalls(gce, mocked_resource, dry_run):
+    gce.dry_run = dry_run
+    _test_cleanup(gce, "firewalls", gce.cleanup_firewalls, mocked_resource)
 
-def test_cleanup_images(gce):
-    _test_cleanup(gce, "images", gce.cleanup_images)
 
+@mark.parametrize("dry_run", [True, False])
+def test_cleanup_forwarding_rules(gce, mocked_resource, dry_run):
+    gce.dry_run = dry_run
+    _test_cleanup(gce, "forwardingRules", gce.cleanup_forwarding_rules, mocked_resource)
 
-def test_cleanup_firewalls(gce):
-    _test_cleanup(gce, "firewalls", gce.cleanup_firewalls)
 
+@mark.parametrize("dry_run", [True, False])
+def test_cleanup_routes(gce, mocked_resource, dry_run):
+    gce.dry_run = dry_run
+    _test_cleanup(gce, "routes", gce.cleanup_routes, mocked_resource)
 
-def test_cleanup_forwarding_rules(gce):
-    _test_cleanup(gce, "forwardingRules", gce.cleanup_forwarding_rules)
 
+def test_cleanup_routes_delete_default_route_raise_exception(gce_dry_run_false, mocked_resource):
+    setattr(gce_dry_run_false.compute_client, "routes", mocked_resource)
+    with raises(HttpError):
+        mocked_resource.error_reason = "unknown"
+        gce_dry_run_false.cleanup_routes()
 
-def test_cleanup_routes(gce):
-    _test_cleanup(gce, "routes", gce.cleanup_routes)
 
+@mark.parametrize("mocked_resource_reason", ["resourceInUseByAnotherResource", "badRequest"])
+def test_cleanup_routes_delete_default_route_not_raise_exception(gce_dry_run_false, mocked_resource, mocked_resource_reason):
+    setattr(gce_dry_run_false.compute_client, "routes", mocked_resource)
+    mocked_resource.error_reason = mocked_resource_reason
+    gce_dry_run_false.cleanup_routes()
 
-def test_cleanup_subnetworks(gce):
-    _test_cleanup(gce, "subnetworks", gce.cleanup_subnetworks)
 
+@mark.parametrize("dry_run", [True, False])
+def test_cleanup_subnetworks(gce, mocked_resource, dry_run):
+    gce.dry_run = dry_run
+    _test_cleanup(gce, "subnetworks", gce.cleanup_subnetworks, mocked_resource)
 
-def test_cleanup_networks(gce):
-    _test_cleanup(gce, "networks", gce.cleanup_networks)
+
+@mark.parametrize("dry_run", [True, False])
+def test_cleanup_networks(gce, mocked_resource, dry_run):
+    gce.dry_run = dry_run
+    _test_cleanup(gce, "networks", gce.cleanup_networks, mocked_resource)
 
 
 def test_cleanup_all(gce):
@@ -184,12 +232,10 @@ def test_cleanup_all(gce):
 
 
 def test_get_error_reason():
+
     class MockHttpError:
         def __init__(self, content) -> None:
-            if content is not None:
-                self.content = json.dumps(content)
-            else:
-                self.content = ""
+            self.content = json.dumps(content) if content is not None else ""
 
     assert GCE.get_error_reason(MockHttpError(content=None)) == "unknown"
     assert GCE.get_error_reason(MockHttpError({})) == "unknown"