From 740ebeb298783567352675ba5135c28b724e1cbc Mon Sep 17 00:00:00 2001 From: "philippe.dacosta" Date: Wed, 27 Nov 2024 11:36:09 +0100 Subject: [PATCH 1/5] Remove Delete event --- fiaas_deploy_daemon/crd/watcher.py | 19 ------------------- fiaas_deploy_daemon/deployer/deploy.py | 6 ------ .../deployer/kubernetes/adapter.py | 6 ------ .../deployer/kubernetes/autoscaler.py | 8 ++------ .../kubernetes/deployment/deployer.py | 8 -------- .../deployer/kubernetes/ingress.py | 4 ---- .../deployer/kubernetes/service.py | 4 ++-- .../crd/test_crd_watcher.py | 6 ------ 8 files changed, 4 insertions(+), 57 deletions(-) diff --git a/fiaas_deploy_daemon/crd/watcher.py b/fiaas_deploy_daemon/crd/watcher.py index 6dbb9ca4..3577ba31 100644 --- a/fiaas_deploy_daemon/crd/watcher.py +++ b/fiaas_deploy_daemon/crd/watcher.py @@ -72,8 +72,6 @@ def _watch(self, namespace): def _handle_watch_event(self, event: WatchEvent): if event.type in (WatchEvent.ADDED, WatchEvent.MODIFIED): self._deploy(event.object) - elif event.type == WatchEvent.DELETED: - self._delete(event.object) else: raise ValueError("Unknown WatchEvent type {}".format(event.type)) @@ -133,23 +131,6 @@ def _deploy(self, application: FiaasApplication): LOG.exception("Failed to create app spec from fiaas config file") 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) - def _already_deployed(self, app_name, namespace, deployment_id): try: name = create_name(app_name, deployment_id) 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..9eb4808d 100644 --- a/tests/fiaas_deploy_daemon/crd/test_crd_watcher.py +++ b/tests/fiaas_deploy_daemon/crd/test_crd_watcher.py @@ -55,11 +55,6 @@ "type": WatchEvent.MODIFIED, } -DELETED_EVENT = { - "object": ADD_EVENT["object"], - "type": WatchEvent.DELETED, -} - STATUS_EVENT = { "object": { "metadata": { @@ -156,7 +151,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( From 3fa3ba54342988c6ae1053a8c152083ff7913e47 Mon Sep 17 00:00:00 2001 From: "philippe.dacosta" Date: Wed, 27 Nov 2024 15:45:42 +0100 Subject: [PATCH 2/5] Fix watcher.py + create test_delete --- fiaas_deploy_daemon/crd/watcher.py | 6 ++++++ .../fiaas_deploy_daemon/crd/test_crd_watcher.py | 16 +++++++++++----- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/fiaas_deploy_daemon/crd/watcher.py b/fiaas_deploy_daemon/crd/watcher.py index 3577ba31..565139bf 100644 --- a/fiaas_deploy_daemon/crd/watcher.py +++ b/fiaas_deploy_daemon/crd/watcher.py @@ -72,6 +72,8 @@ def _watch(self, namespace): def _handle_watch_event(self, event: WatchEvent): if event.type in (WatchEvent.ADDED, WatchEvent.MODIFIED): self._deploy(event.object) + elif event.type == WatchEvent.DELETED: + self._delete(event.object) else: raise ValueError("Unknown WatchEvent type {}".format(event.type)) @@ -131,6 +133,10 @@ def _deploy(self, application: FiaasApplication): LOG.exception("Failed to create app spec from fiaas config file") self._lifecycle.failed(lifecycle_subject) + def _delete(self, application: FiaasApplication): + 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: name = create_name(app_name, deployment_id) diff --git a/tests/fiaas_deploy_daemon/crd/test_crd_watcher.py b/tests/fiaas_deploy_daemon/crd/test_crd_watcher.py index 9eb4808d..3b25dcc6 100644 --- a/tests/fiaas_deploy_daemon/crd/test_crd_watcher.py +++ b/tests/fiaas_deploy_daemon/crd/test_crd_watcher.py @@ -55,6 +55,11 @@ "type": WatchEvent.MODIFIED, } +DELETED_EVENT = { + "object": ADD_EVENT["object"], + "type": WatchEvent.DELETED, +} + STATUS_EVENT = { "object": { "metadata": { @@ -173,11 +178,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 @@ -223,6 +224,11 @@ def test_deploy( assert deployer_event == DeployerEvent(deployer_event_type, app_spec, None) assert deploy_queue.empty() + def test_delete(self, deploy_queue, watcher): + watcher.watch.return_value = [WatchEvent(DELETED_EVENT, FiaasApplication)] + + assert deploy_queue.empty() + @pytest.mark.parametrize("namespace", [None, "default"]) def test_watch_namespace(self, crd_watcher, watcher, namespace): crd_watcher._watch(namespace) From 85b5494dce1e5708c103f1f3a6856393551e1c35 Mon Sep 17 00:00:00 2001 From: "philippe.dacosta" Date: Thu, 28 Nov 2024 09:36:40 +0100 Subject: [PATCH 3/5] Fixup! --- tests/fiaas_deploy_daemon/crd/test_crd_watcher.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/fiaas_deploy_daemon/crd/test_crd_watcher.py b/tests/fiaas_deploy_daemon/crd/test_crd_watcher.py index 3b25dcc6..5b8dbf56 100644 --- a/tests/fiaas_deploy_daemon/crd/test_crd_watcher.py +++ b/tests/fiaas_deploy_daemon/crd/test_crd_watcher.py @@ -224,9 +224,11 @@ def test_deploy( assert deployer_event == DeployerEvent(deployer_event_type, app_spec, None) assert deploy_queue.empty() - def test_delete(self, deploy_queue, watcher): + 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"]) From dcb2b6f89755562177262666c836214cd8606090 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=98yvind=20Ingebrigtsen=20=C3=98vergaard?= Date: Thu, 28 Nov 2024 10:31:25 +0100 Subject: [PATCH 4/5] Skip updates on Application with deletionTimestamp in the past 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 (i.e. has deletionTimestamp set and in the past), skip deploy deploy to avoid interfering with the deletion process done by the garbage collector. --- fiaas_deploy_daemon/crd/watcher.py | 20 ++++++++++++++++++- .../crd/test_crd_watcher.py | 10 ++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/fiaas_deploy_daemon/crd/watcher.py b/fiaas_deploy_daemon/crd/watcher.py index 565139bf..15d16b21 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,22 @@ 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) and deletion_timestamp <= datetime.datetime.now( + tz=datetime.timezone.utc + ): + 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 +116,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 diff --git a/tests/fiaas_deploy_daemon/crd/test_crd_watcher.py b/tests/fiaas_deploy_daemon/crd/test_crd_watcher.py index 5b8dbf56..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 @@ -311,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 From 88d6206ff6d4678e931bca6edf545a638fc92ec4 Mon Sep 17 00:00:00 2001 From: "philippe.dacosta" Date: Fri, 29 Nov 2024 11:24:05 +0100 Subject: [PATCH 5/5] Remove test about deletionTimestamp <= now --- fiaas_deploy_daemon/crd/watcher.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/fiaas_deploy_daemon/crd/watcher.py b/fiaas_deploy_daemon/crd/watcher.py index 15d16b21..b0632f62 100644 --- a/fiaas_deploy_daemon/crd/watcher.py +++ b/fiaas_deploy_daemon/crd/watcher.py @@ -96,9 +96,7 @@ def _skip_update_of_deleted_application(self, application: FiaasApplication): # 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) and deletion_timestamp <= datetime.datetime.now( - tz=datetime.timezone.utc - ): + 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,