Merge pull request #262 from adevinta/remove-delete-event

Remove Delete event

fiaas_deploy_daemon/crd/watcher.py

@@ -14,7 +14,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-
+import datetime
 import logging
 from queue import Queue
 from typing import Union
@@ -90,6 +90,20 @@ def _skip_status_event(self, application: FiaasApplication):
             return True
         return False
 
+    def _skip_update_of_deleted_application(self, application: FiaasApplication):
+        # metadata.deletionTimestamp is set when a resource is deleted with propagationPolicy: orphan or
+        # propagationPolicy: foreground. The resource may be updated after this, for example finalizers may be set or
+        # removed. If the application resource is in the process of being deleted, deploy should be skipped otherwise
+        # it may interfere with the deletion process done by the garbage collector.
+        deletion_timestamp = application.metadata.deletionTimestamp
+        if isinstance(deletion_timestamp, datetime.datetime):
+            LOG.warning(
+                "Skipping update watch event for app %s; it was marked for deletion at %s",
+                application.spec.application,
+                deletion_timestamp,
+            )
+            return True
+
     def _deploy(self, application: FiaasApplication):
         app_name = application.spec.application
         LOG.debug("Deploying %s", app_name)
@@ -100,6 +114,8 @@ def _deploy(self, application: FiaasApplication):
             raise ValueError("The Application {} is missing the 'fiaas/deployment_id' label".format(app_name))
         if self._skip_status_event(application):
             return
+        if self._skip_update_of_deleted_application(application):
+            return
         if self._already_deployed(app_name, application.metadata.namespace, deployment_id):
             LOG.debug("Have already deployed %s for app %s", deployment_id, app_name)
             return
@@ -134,21 +150,8 @@ def _deploy(self, application: FiaasApplication):
             self._lifecycle.failed(lifecycle_subject)
 
     def _delete(self, application: FiaasApplication):
-        app_spec = self._spec_factory(
-            uid=application.metadata.uid,
-            name=application.spec.application,
-            image=application.spec.image,
-            app_config=application.spec.config,
-            teams=[],
-            tags=[],
-            deployment_id="deletion",
-            namespace=application.metadata.namespace,
-            additional_labels=application.spec.additional_labels,
-            additional_annotations=application.spec.additional_annotations,
-        )
-        set_extras(app_spec)
-        self._deploy_queue.put(DeployerEvent("DELETE", app_spec, lifecycle_subject=None))
-        LOG.debug("Queued delete for %s", application.spec.application)
+        app_name = application.spec.application
+        LOG.info("Deleting %s. No specific action, we leave automatic garbage collection to Kubernetes", app_name)
 
     def _already_deployed(self, app_name, namespace, deployment_id):
         try:

fiaas_deploy_daemon/deployer/deploy.py

@@ -61,8 +61,6 @@ def __call__(self):
             LOG.info("Received %r for %s", event.app_spec, event.action)
             if event.action == "UPDATE":
                 self._update(event.app_spec, event.lifecycle_subject)
-            elif event.action == "DELETE":
-                self._delete(event.app_spec)
             else:
                 raise ValueError("Unknown DeployerEvent action {}".format(event.action))
 
@@ -97,10 +95,6 @@ def _update(self, app_spec: AppSpec, lifecycle_subject: Subject):
                 LOG.exception("Error while saving status for %s: ", app_spec.name)
             self._bookkeeper.failed(app_spec)
 
-    def _delete(self, app_spec: AppSpec):
-        self._adapter.delete(app_spec)
-        LOG.info("Completed removal of %r", app_spec)
-
 
 def _make_gen(func):
     while True:

fiaas_deploy_daemon/deployer/kubernetes/adapter.py

@@ -65,12 +65,6 @@ def deploy(self, app_spec: AppSpec):
         self._autoscaler_deployer.deploy(app_spec, labels)
         self._pod_disruption_budget_deployer.deploy(app_spec, selector, labels)
 
-    def delete(self, app_spec: AppSpec):
-        self._ingress_deployer.delete(app_spec)
-        self._autoscaler_deployer.delete(app_spec)
-        self._service_deployer.delete(app_spec)
-        self._deployment_deployer.delete(app_spec)
-
     def _make_labels(self, app_spec: AppSpec):
         labels = {
             "app": app_spec.name,

fiaas_deploy_daemon/deployer/kubernetes/autoscaler.py

@@ -57,13 +57,9 @@ def deploy(self, app_spec, labels):
             self._extension_hook.apply(autoscaler, app_spec)
             autoscaler.save()
         else:
-            try:
-                LOG.info("Deleting any pre-existing autoscaler for %s", app_spec.name)
-                HorizontalPodAutoscaler.delete(app_spec.name, app_spec.namespace)
-            except NotFound:
-                pass
+            self._delete(app_spec)
 
-    def delete(self, app_spec):
+    def _delete(self, app_spec):
         LOG.info("Deleting autoscaler for %s", app_spec.name)
         try:
             HorizontalPodAutoscaler.delete(app_spec.name, app_spec.namespace)

fiaas_deploy_daemon/deployer/kubernetes/deployment/deployer.py

@@ -184,14 +184,6 @@ def deploy(self, app_spec, selector, labels, besteffort_qos_is_required):
         self._extension_hook.apply(deployment, app_spec)
         deployment.save()
 
-    def delete(self, app_spec):
-        LOG.info("Deleting deployment for %s", app_spec.name)
-        try:
-            body = {"kind": "DeleteOptions", "apiVersion": "v1", "propagationPolicy": "Foreground"}
-            Deployment.delete(app_spec.name, app_spec.namespace, body=body)
-        except NotFound:
-            pass
-
     def _make_volumes(self, app_spec):
         volumes = []
         volumes.append(

fiaas_deploy_daemon/deployer/kubernetes/ingress.py

@@ -79,10 +79,6 @@ def deploy(self, app_spec, labels):
         else:
             self._ingress_adapter.delete_unused(app_spec, labels)
 
-    def delete(self, app_spec):
-        LOG.info("Deleting ingresses for %s", app_spec.name)
-        self._ingress_adapter.delete_list(app_spec)
-
     def _create(self, app_spec, labels):
         LOG.info("Creating/updating ingresses for %s", app_spec.name)
         custom_labels = merge_dicts(app_spec.labels.ingress, labels)

fiaas_deploy_daemon/deployer/kubernetes/service.py

@@ -39,9 +39,9 @@ def deploy(self, app_spec, selector, labels):
         if self._should_have_service(app_spec):
             self._create(app_spec, selector, labels)
         else:
-            self.delete(app_spec)
+            self._delete(app_spec)
 
-    def delete(self, app_spec):
+    def _delete(self, app_spec):
         LOG.info("Deleting service for %s", app_spec.name)
         try:
             Service.delete(app_spec.name, app_spec.namespace)

tests/fiaas_deploy_daemon/crd/test_crd_watcher.py

@@ -16,6 +16,7 @@
 # limitations under the License.
 
 
+import copy
 from queue import Queue
 
 from unittest import mock
@@ -156,7 +157,6 @@ def test_is_able_to_watch_custom_resource_definition(self, crd_watcher, deploy_q
             (ADD_EVENT, "UPDATE", {"deployment": {"fiaas/source-repository": "xyz"}}, "xyz"),
             (MODIFIED_EVENT, "UPDATE", None, None),
             (MODIFIED_EVENT, "UPDATE", {"deployment": {"fiaas/source-repository": "xyz"}}, "xyz"),
-            (DELETED_EVENT, "DELETE", None, None),
         ],
     )
     def test_deploy(
@@ -179,11 +179,7 @@ def test_deploy(
         app_name = spec["application"]
         uid = event["object"]["metadata"]["uid"]
         namespace = event["object"]["metadata"]["namespace"]
-        deployment_id = (
-            event["object"]["metadata"]["labels"]["fiaas/deployment_id"]
-            if deployer_event_type != "DELETE"
-            else "deletion"
-        )
+        deployment_id = event["object"]["metadata"]["labels"]["fiaas/deployment_id"]
 
         app_spec = app_spec._replace(name=app_name, namespace=namespace, deployment_id=deployment_id)
         spec_factory.return_value = app_spec
@@ -229,6 +225,13 @@ def test_deploy(
             assert deployer_event == DeployerEvent(deployer_event_type, app_spec, None)
         assert deploy_queue.empty()
 
+    def test_delete(self, crd_watcher, deploy_queue, watcher):
+        watcher.watch.return_value = [WatchEvent(DELETED_EVENT, FiaasApplication)]
+
+        crd_watcher._watch(None)
+
+        assert deploy_queue.empty()
+
     @pytest.mark.parametrize("namespace", [None, "default"])
     def test_watch_namespace(self, crd_watcher, watcher, namespace):
         crd_watcher._watch(namespace)
@@ -309,3 +312,12 @@ def test_deploy_save_status(self, crd_watcher, deploy_queue, watcher, status_get
         assert deploy_queue.qsize() == 0
         crd_watcher._watch(None)
         assert deploy_queue.qsize() == 0
+
+    def test_deploy_skip_deleted_app(self, crd_watcher, deploy_queue, watcher, status_get):
+        event = copy.deepcopy(MODIFIED_EVENT)
+        event['object']['metadata']['deletionTimestamp'] = '2000-01-01T00:00:00Z'
+        watcher.watch.return_value = [WatchEvent(event, FiaasApplication)]
+
+        assert deploy_queue.qsize() == 0
+        crd_watcher._watch(None)
+        assert deploy_queue.qsize() == 0