fix: remove resources in correct order (#414)

Signed-off-by: Dario Faccin <dario.faccin@canonical.com>

Author: dariofaccin

charms/kserve-controller/poetry.lock

@@ -57,6 +57,7 @@ ops = "^2.22.0"
 serialized-data-interface = "^0.7.0"
 charmed-service-mesh-helpers = "^0.3.0"
 lightkube-extensions = "^0.3.0"
+tenacity = "^9.1.2"
 
 [tool.poetry.group.fmt]
 optional = true

charms/kserve-controller/pyproject.toml

@@ -18,12 +18,14 @@
 from pathlib import Path
 from typing import Dict
 
+import tenacity
 import yaml
 from charmed_kubeflow_chisme.exceptions import ErrorWithStatus, GenericCharmRuntimeError
 from charmed_kubeflow_chisme.kubernetes import KubernetesResourceHandler
 from charmed_kubeflow_chisme.lightkube.batch import delete_many
 from charmed_kubeflow_chisme.pebble import update_layer
 from charmed_kubeflow_chisme.service_mesh import generate_allow_all_authorization_policy
+from charmed_kubeflow_chisme.types import LightkubeResourcesList
 from charmed_service_mesh_helpers.interfaces import GatewayMetadataRequirer
 from charms.istio_beacon_k8s.v0.service_mesh import MeshType, PolicyResourceManager
 from charms.istio_pilot.v0.istio_gateway_info import (
@@ -40,7 +42,8 @@
 )
 from jinja2 import Template
 from jsonschema import ValidationError
-from lightkube import ApiError
+from lightkube import ApiError, Client
+from lightkube.generic_resource import load_in_cluster_generic_resources
 from lightkube.models.core_v1 import ServicePort
 from lightkube_extensions.batch import create_charm_default_labels
 from ops import main
@@ -104,6 +107,14 @@
 METRICS_PORT = 8080
 
 
+# For errors when a K8s object exists while it shouldn't
+class ObjectStillExistsError(Exception):
+    """Exception for when a K8s object exists, while it should have been removed."""
+
+    def __init__(self, resource_name: str):
+        self.resource_name = resource_name
+
+
 def parse_images_config(config: str) -> Dict:
     """
     Parse a YAML config-defined images list.
@@ -140,6 +151,7 @@ def __init__(self, *args):
         super().__init__(*args)
         self.custom_images = []
         self.images_context = {}
+        self.inference_service_context = {}
         self._ingress_gateway_requirer = GatewayRequirer(
             self, relation_name=SDI_INGRESS_GATEWAY_RELATION
         )
@@ -169,7 +181,6 @@ def __init__(self, *args):
             self.framework.observe(event, self._on_event)
 
         self._k8s_resource_handler = None
-        self._crd_resource_handler = None
         self._cm_resource_handler = None
         self._cluster_runtimes_resource_handler = None
         self._secrets_manifests_wrapper = None
@@ -236,8 +247,7 @@ def _context(self):
             "no_proxy": self.model.config["no-proxy"],
         }
 
-    @property
-    def _inference_service_context(self):
+    def generate_inference_service_context(self):
         """Context for rendering the inferenceservive-config ConfigMap."""
         # Ensure any input is valid for deployment mode
         deployment_mode = self._deployment_mode
@@ -283,7 +293,7 @@ def cm_resource_handler(self):
             self._cm_resource_handler = KubernetesResourceHandler(
                 field_manager=self._lightkube_field_manager,
                 template_files=CONFIG_FILES,
-                context={**self._inference_service_context, **self.images_context},
+                context={**self.inference_service_context, **self.images_context},
                 logger=log,
             )
         return self._cm_resource_handler
@@ -298,7 +308,7 @@ def cluster_runtimes_resource_handler(self):
                 context={**self.images_context},
                 logger=log,
             )
-
+        load_in_cluster_generic_resources(self._cluster_runtimes_resource_handler.lightkube_client)
         return self._cluster_runtimes_resource_handler
 
     @property
@@ -584,6 +594,7 @@ def _on_event(self, event):
         try:
             self.custom_images = parse_images_config(self.model.config["custom_images"])
             self.images_context = self.get_images(DEFAULT_IMAGES, self.custom_images)
+            self.inference_service_context = self.generate_inference_service_context()
             self.unit.status = MaintenanceStatus("Creating k8s resources")
             self.reconcile_authorization_policies()
             self.k8s_resource_handler.apply()
@@ -659,14 +670,7 @@ def _on_event(self, event):
             log.error(api_err)
             raise
 
-    def _on_remove(self, event):
-        try:
-            self.custom_images = parse_images_config(self.model.config["custom_images"])
-            self.images_context = self.get_images(DEFAULT_IMAGES, self.custom_images)
-        except ErrorWithStatus as err:
-            self.model.unit.status = err.status
-            log.error(f"Failed to handle {event} with error: {err}")
-            return
+    def _on_remove(self, _):
         self.unit.status = MaintenanceStatus("Removing k8s resources")
 
         # remove AuthorizationPolicies
@@ -675,20 +679,63 @@ def _on_remove(self, event):
         handlers = [
             self.k8s_resource_handler,
             self.cm_resource_handler,
-            self.cluster_runtimes_resource_handler,
         ]
-
         try:
+            runtimes_manifests = self.cluster_runtimes_resource_handler.render_manifests()
+            delete_many(
+                self.cluster_runtimes_resource_handler.lightkube_client,
+                runtimes_manifests,
+            )
+            for runtime_name, runtime_kind in _extract_runtimes_names(runtimes_manifests).items():
+                self.ensure_resource_is_deleted(
+                    client=self.cluster_runtimes_resource_handler.lightkube_client,
+                    resource_kind=runtime_kind,
+                    resource_name=runtime_name,
+                )
             for handler in handlers:
                 delete_many(
                     handler.lightkube_client,
                     handler.render_manifests(),
                 )
         except ApiError as e:
-            log.warning(f"Failed to delete resources, with error: {e}")
+            if e.status.code != 404:
+                log.warning(f"Failed to delete resources, with error: {e}")
+                raise e
+        except ObjectStillExistsError as e:
+            log.warning(
+                "Failed to remove resource: %s. Manual intervention for cleanup might be required",
+                e.resource_name,
+            )
             raise e
         self.unit.status = MaintenanceStatus("K8s resources removed")
 
+    @tenacity.retry(stop=tenacity.stop_after_delay(300), wait=tenacity.wait_fixed(5), reraise=True)
+    def ensure_resource_is_deleted(self, client: Client, resource_kind, resource_name: str):
+        """Check if the CRD doesn't exist with retries.
+
+        The function will keep retrying until the CRD is deleted, and handle the
+        404 error once it gets deleted.
+
+        Args:
+            crd_name: The CRD to be checked if it is deleted.
+            client: The lightkube client to use for talking to K8s.
+
+        Raises:
+            ApiError: From lightkube, if there was an error aside from 404.
+            ObjectStillExistsError: If the Profile's namespace was not deleted after retries.
+        """
+        log.info("Checking if resource exists: %s", resource_name)
+        try:
+            client.get(resource_kind, name=resource_name)
+            log.info('Resource "%s" exists, retrying...', resource_name)
+            raise ObjectStillExistsError(resource_name)
+        except ApiError as e:
+            if e.status.code == 404:
+                log.info('Resource "%s" does not exist!', resource_name)
+                return
+            # Raise any other error
+            raise
+
     def _check_container_connection(self, container: Container) -> None:
         """Check if connection can be made with container.
 
@@ -873,5 +920,22 @@ def _restart_controller_service(self) -> None:
             ) from err
 
 
+def _extract_runtimes_names(manifests: LightkubeResourcesList) -> dict:
+    """
+    Extracts a mapping of runtime resource names to their kinds.
+
+    Args:
+        manifests (LightkubeResourcesList): List of runtime manifest objects,
+        each with metadata and kind.
+
+    Returns:
+        dict: Dictionary mapping resource names (str) to their kind.
+    """
+    runtimes_kind_name_mapping = {}
+    for runtime in manifests:
+        runtimes_kind_name_mapping.update({runtime.metadata.name: runtime.__class__})
+    return runtimes_kind_name_mapping
+
+
 if __name__ == "__main__":
     main(KServeControllerCharm)

charms/kserve-controller/src/charm.py

@@ -20,7 +20,6 @@
 # ConfigMap
 CONFIGMAP_TEMPLATE_PATH = Path("./src/templates/configmap_manifests.yaml.j2")
 CONFIGMAP_DATA_INGRESS_DOMAIN = "example.com"
-CONFIGMAP_DATA_INGRESS_GATEWAY_NAMESPACE = "kubeflow"
 
 # Lightkube resources
 POD_DEFAULT = lightkube.generic_resource.create_namespaced_resource(

charms/kserve-controller/tests/integration/constants.py

@@ -23,6 +23,7 @@
     get_pod_names,
 )
 from lightkube import Client
+from lightkube.resources.apiextensions_v1 import CustomResourceDefinition
 from lightkube.resources.core_v1 import (
     ConfigMap,
     Pod,
@@ -42,7 +43,6 @@
 from tests.integration.constants import (
     APP_NAME,
     CONFIGMAP_DATA_INGRESS_DOMAIN,
-    CONFIGMAP_DATA_INGRESS_GATEWAY_NAMESPACE,
     CONFIGMAP_NAME,
     CONFIGMAP_TEMPLATE_PATH,
     CONTAINERS_SECURITY_CONTEXT_MAP,
@@ -77,19 +77,22 @@
 custom_images = json.loads(Path(CUSTOM_IMAGES_PATH).read_text())
 
 explainer_image, explainer_version = custom_images["configmap__explainers__art"].split(":")
-configmap_context = {
-    **custom_images,
-    "configmap__explainers__art__image": explainer_image,
-    "configmap__explainers__art__version": explainer_version,
-    "deployment_mode": "Serverless",
-    "enable_gateway_api": "false",
-    "ingress_domain": CONFIGMAP_DATA_INGRESS_DOMAIN,
-    "local_gateway_namespace": CONFIGMAP_DATA_LOCAL_GATEWAY_NAMESPACE,
-    "local_gateway_name": CONFIGMAP_DATA_LOCAL_GATEWAY_NAME,
-    "local_gateway_service_name": CONFIGMAP_DATA_LOCAL_GATEWAY_SERVICE_NAME,
-    "ingress_gateway_namespace": CONFIGMAP_DATA_INGRESS_GATEWAY_NAMESPACE,
-    "ingress_gateway_name": CONFIGMAP_DATA_INGRESS_GATEWAY_NAME_SERVERLESS,
-}
+
+
+def generate_configmap_context(ingress_gateway_namespace: str) -> dict:
+    return {
+        **custom_images,
+        "configmap__explainers__art__image": explainer_image,
+        "configmap__explainers__art__version": explainer_version,
+        "deployment_mode": "Serverless",
+        "enable_gateway_api": "false",
+        "ingress_domain": CONFIGMAP_DATA_INGRESS_DOMAIN,
+        "local_gateway_namespace": CONFIGMAP_DATA_LOCAL_GATEWAY_NAMESPACE,
+        "local_gateway_name": CONFIGMAP_DATA_LOCAL_GATEWAY_NAME,
+        "local_gateway_service_name": CONFIGMAP_DATA_LOCAL_GATEWAY_SERVICE_NAME,
+        "ingress_gateway_namespace": ingress_gateway_namespace,
+        "ingress_gateway_name": CONFIGMAP_DATA_INGRESS_GATEWAY_NAME_SERVERLESS,
+    }
 
 
 @pytest.mark.skip_if_deployed
@@ -265,7 +268,9 @@ async def test_configmap_created(lightkube_client: lightkube.Client, ops_test: O
         ConfigMap, CONFIGMAP_NAME, namespace=ops_test.model_name
     )
 
-    expected_configmap = populate_template(CONFIGMAP_TEMPLATE_PATH, configmap_context)
+    expected_configmap = populate_template(
+        CONFIGMAP_TEMPLATE_PATH, generate_configmap_context(ops_test.model_name)
+    )
     assert inferenceservice_config.data == expected_configmap["data"]
 
 
@@ -290,6 +295,7 @@ async def test_configmap_changes_with_config(
         ConfigMap, CONFIGMAP_NAME, namespace=ops_test.model_name
     )
 
+    configmap_context = generate_configmap_context(ops_test.model_name)
     configmap_context["configmap__batcher"] = "custom:1.0"
 
     expected_configmap = populate_template(CONFIGMAP_TEMPLATE_PATH, configmap_context)
@@ -487,3 +493,41 @@ async def test_container_security_context(
         CONTAINERS_SECURITY_CONTEXT_MAP,
         ops_test.model.name,
     )
+
+
+@pytest.mark.abort_on_fail
+async def test_remove_with_resources_present(ops_test: OpsTest):
+    """Test remove with all resources deployed.
+
+    Verify that all deployed resources that need to be removed are removed.
+
+    This test should be next after test_upgrade(), because it removes deployed charm.
+    """
+
+    @tenacity.retry(
+        wait=tenacity.wait_exponential(multiplier=1, min=1, max=15),
+        stop=tenacity.stop_after_delay(5 * 60),
+        reraise=True,
+    )
+    def assert_resources_removed():
+        """Asserts on the resource removal.
+
+        Retries multiple times using tenacity to allow time for the resources to be deleted.
+        """
+        lightkube_client = lightkube.Client()
+        crd_list = iter(
+            lightkube_client.list(
+                CustomResourceDefinition,
+                labels=[("app.juju.is/created-by", APP_NAME)],
+                namespace=ops_test.model_name,
+            )
+        )
+        # testing for empty list (iterator)
+        _last = object()
+        assert next(crd_list, _last) is _last
+
+    # remove deployed charm and verify that it is removed alongside resources it created
+    await ops_test.model.remove_application(app_name=APP_NAME, block_until_done=True)
+    assert APP_NAME not in ops_test.model.applications
+
+    assert_resources_removed()

charms/kserve-controller/tests/integration/test_charm.py

@@ -37,7 +37,6 @@
 from tests.integration.constants import (
     APP_NAME,
     CONFIGMAP_DATA_INGRESS_DOMAIN,
-    CONFIGMAP_DATA_INGRESS_GATEWAY_NAMESPACE,
     CONFIGMAP_NAME,
     CONFIGMAP_TEMPLATE_PATH,
     CONTAINERS_SECURITY_CONTEXT_MAP,
@@ -71,16 +70,19 @@
 
 
 explainer_image, explainer_version = custom_images["configmap__explainers__art"].split(":")
-configmap_context = {
-    **custom_images,
-    "configmap__explainers__art__image": explainer_image,
-    "configmap__explainers__art__version": explainer_version,
-    "deployment_mode": "RawDeployment",
-    "enable_gateway_api": "true",
-    "ingress_domain": CONFIGMAP_DATA_INGRESS_DOMAIN,
-    "ingress_gateway_namespace": CONFIGMAP_DATA_INGRESS_GATEWAY_NAMESPACE,
-    "ingress_gateway_name": "istio-ingress-k8s",
-}
+
+
+def generate_configmap_context(ingress_gateway_namespace: str) -> dict:
+    return {
+        **custom_images,
+        "configmap__explainers__art__image": explainer_image,
+        "configmap__explainers__art__version": explainer_version,
+        "deployment_mode": "RawDeployment",
+        "enable_gateway_api": "true",
+        "ingress_domain": CONFIGMAP_DATA_INGRESS_DOMAIN,
+        "ingress_gateway_namespace": ingress_gateway_namespace,
+        "ingress_gateway_name": "istio-ingress-k8s",
+    }
 
 
 @pytest.mark.skip_if_deployed
@@ -210,7 +212,9 @@ async def test_configmap_created(lightkube_client: lightkube.Client, ops_test: O
         ConfigMap, CONFIGMAP_NAME, namespace=ops_test.model_name
     )
 
-    expected_configmap = populate_template(CONFIGMAP_TEMPLATE_PATH, configmap_context)
+    expected_configmap = populate_template(
+        CONFIGMAP_TEMPLATE_PATH, generate_configmap_context(ops_test.model_name)
+    )
     assert inferenceservice_config.data == expected_configmap["data"]
 
 
@@ -235,6 +239,7 @@ async def test_configmap_changes_with_config(
         ConfigMap, CONFIGMAP_NAME, namespace=ops_test.model_name
     )
 
+    configmap_context = generate_configmap_context(ops_test.model_name)
     configmap_context["configmap__batcher"] = "custom:1.0"
 
     expected_configmap = populate_template(CONFIGMAP_TEMPLATE_PATH, configmap_context)