diff --git a/fiaas_deploy_daemon/crd/watcher.py b/fiaas_deploy_daemon/crd/watcher.py index 6dbb9ca4..b0632f62 100644 --- a/fiaas_deploy_daemon/crd/watcher.py +++ b/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: diff --git a/fiaas_deploy_daemon/deployer/deploy.py b/fiaas_deploy_daemon/deployer/deploy.py index 325d18c0..753535c2 100644 --- a/fiaas_deploy_daemon/deployer/deploy.py +++ b/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: diff --git a/fiaas_deploy_daemon/deployer/kubernetes/adapter.py b/fiaas_deploy_daemon/deployer/kubernetes/adapter.py index c388fe9d..414efb11 100644 --- a/fiaas_deploy_daemon/deployer/kubernetes/adapter.py +++ b/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, diff --git a/fiaas_deploy_daemon/deployer/kubernetes/autoscaler.py b/fiaas_deploy_daemon/deployer/kubernetes/autoscaler.py index b922fce6..0135e8e9 100644 --- a/fiaas_deploy_daemon/deployer/kubernetes/autoscaler.py +++ b/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) diff --git a/fiaas_deploy_daemon/deployer/kubernetes/deployment/deployer.py b/fiaas_deploy_daemon/deployer/kubernetes/deployment/deployer.py index 24208286..da62ba16 100644 --- a/fiaas_deploy_daemon/deployer/kubernetes/deployment/deployer.py +++ b/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( diff --git a/fiaas_deploy_daemon/deployer/kubernetes/ingress.py b/fiaas_deploy_daemon/deployer/kubernetes/ingress.py index 5a3256ad..f4cece88 100644 --- a/fiaas_deploy_daemon/deployer/kubernetes/ingress.py +++ b/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) diff --git a/fiaas_deploy_daemon/deployer/kubernetes/service.py b/fiaas_deploy_daemon/deployer/kubernetes/service.py index bb4c7c37..95b63c67 100644 --- a/fiaas_deploy_daemon/deployer/kubernetes/service.py +++ b/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) diff --git a/tests/fiaas_deploy_daemon/crd/test_crd_watcher.py b/tests/fiaas_deploy_daemon/crd/test_crd_watcher.py index 0d109d52..9213ccb6 100644 --- a/tests/fiaas_deploy_daemon/crd/test_crd_watcher.py +++ b/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