fix: use an all-catch main event handler (#197)

* feat: refactor to main event handler
* feat: unit test no relation to knative
* fix: write missing controller ready active unit test

charms/kserve-controller/src/charm.py

@@ -123,30 +123,22 @@ def __init__(self, *args):
         self._ingress_gateway_requirer = GatewayRequirer(self, relation_name="ingress-gateway")
         self._local_gateway_requirer = GatewayRequirer(self, relation_name="local-gateway")
 
-        self.framework.observe(self.on.install, self._on_install)
-        self.framework.observe(self.on.config_changed, self._on_config_changed)
         self.framework.observe(self.on.remove, self._on_remove)
-        self.framework.observe(
-            self.on.kserve_controller_pebble_ready, self._on_kserve_controller_ready
-        )
-        self.framework.observe(
-            self.on.kube_rbac_proxy_pebble_ready, self._on_kube_rbac_proxy_ready
-        )
-        self.framework.observe(
-            self.on["ingress-gateway"].relation_changed, self._on_ingress_gateway_relation_changed
-        )
-        # Observe if relation is removed by juju remove-application or juju remove-relation
-        self.framework.observe(
-            self.on["ingress-gateway"].relation_broken, self._on_ingress_gateway_relation_broken
-        )
-        self.framework.observe(
-            self.on["local-gateway"].relation_changed, self._on_local_gateway_relation_changed
-        )
-        self.framework.observe(
-            self.on["local-gateway"].relation_broken, self._on_local_gateway_relation_broken
-        )
-        for rel in ["object-storage", "secrets", "service-accounts"]:
-            self.framework.observe(self.on[rel].relation_changed, self._on_install)
+
+        for event in [
+            self.on.install,
+            self.on.config_changed,
+            self.on.kserve_controller_pebble_ready,
+            self.on.kube_rbac_proxy_pebble_ready,
+            self.on["local-gateway"].relation_changed,
+            self.on["ingress-gateway"].relation_changed,
+            self.on["object-storage"].relation_changed,
+            self.on["secrets"].relation_changed,
+            self.on["service-accounts"].relation_changed,
+            self.on["ingress-gateway"].relation_broken,
+            self.on["local-gateway"].relation_broken,
+        ]:
+            self.framework.observe(event, self._on_event)
 
         self._k8s_resource_handler = None
         self._crd_resource_handler = None
@@ -402,55 +394,6 @@ def get_images(
             ].rsplit(":", 1)
         return images
 
-    def _on_kserve_controller_ready(self, event):
-        """Define and start a workload using the Pebble API.
-
-        Learn more about Pebble layers at https://github.com/canonical/pebble
-        """
-        try:
-            self._upload_certs_to_container(
-                container=self.controller_container,
-                destination_path=CONTAINER_CERTS_DEST,
-                certs_store=self._stored,
-            )
-            update_layer(
-                self._controller_container_name,
-                self.controller_container,
-                self._controller_pebble_layer,
-                log,
-            )
-        except ErrorWithStatus as e:
-            self.model.unit.status = e.status
-            if isinstance(e.status, BlockedStatus):
-                log.error(str(e.msg))
-            else:
-                log.info(str(e.msg))
-
-        # TODO determine status checking if rbac proxy is also up
-        self.unit.status = ActiveStatus()
-
-    def _on_kube_rbac_proxy_ready(self, event):
-        """Define and start a workload using the Pebble API.
-
-        Learn more about Pebble layers at https://github.com/canonical/pebble
-        """
-        try:
-            update_layer(
-                self._rbac_proxy_container_name,
-                self.rbac_proxy_container,
-                self._rbac_proxy_pebble_layer,
-                log,
-            )
-        except ErrorWithStatus as e:
-            self.model.unit.status = e.status
-            if isinstance(e.status, BlockedStatus):
-                log.error(str(e.msg))
-            else:
-                log.info(str(e.msg))
-
-        # TODO determine status checking if controller is also up
-        self.unit.status = ActiveStatus()
-
     def send_object_storage_manifests(self):
         """Send object storage related manifests in case the object storage relation exists"""
         interfaces = self._get_interfaces()
@@ -480,14 +423,33 @@ def send_object_storage_manifests(self):
             interfaces, service_accounts_context, SERVICE_ACCOUNTS_FILES, "service-accounts"
         )
 
-    def _on_install(self, event):
+    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.unit.status = MaintenanceStatus("Creating k8s resources")
             self.k8s_resource_handler.apply()
             self.cm_resource_handler.apply()
             self.send_object_storage_manifests()
+            self._upload_certs_to_container(
+                container=self.controller_container,
+                destination_path=CONTAINER_CERTS_DEST,
+                certs_store=self._stored,
+            )
+            # update kserve-controller layer
+            update_layer(
+                self._controller_container_name,
+                self.controller_container,
+                self._controller_pebble_layer,
+                log,
+            )
+            # update kube-rbac-proxy layer
+            update_layer(
+                self._rbac_proxy_container_name,
+                self.rbac_proxy_container,
+                self._rbac_proxy_pebble_layer,
+                log,
+            )
 
             # The kserve-controller service must be restarted whenever the
             # configuration is changed, otherwise the service will remain
@@ -502,10 +464,6 @@ def _on_install(self, event):
             raise
         self.model.unit.status = ActiveStatus()
 
-    def _on_config_changed(self, event):
-        """Handle the config changed event."""
-        self._on_install(event)
-
     def _on_remove(self, event):
         try:
             self.custom_images = parse_images_config(self.model.config["custom_images"])
@@ -532,28 +490,6 @@ def _on_remove(self, event):
             raise e
         self.unit.status = MaintenanceStatus("K8s resources removed")
 
-    def _on_ingress_gateway_relation_changed(self, event) -> None:
-        """Handle the ingress-gateway relation changed event."""
-        # Just call the event handler that applies manifest files
-        self._on_install(event)
-
-    def _on_local_gateway_relation_changed(self, event) -> None:
-        """Handle the local-gateway relation changed event."""
-        # Just call the event handler that applies manifest files
-        self._on_install(event)
-
-    def _on_ingress_gateway_relation_broken(self, _) -> None:
-        """Handle the ingress-gateway relation broken event."""
-        # Ingress is always needed, so immediately go into BlockedStatus
-        self.unit.status = BlockedStatus("Please relate to istio-pilot:gateway-info")
-        return
-
-    def _on_local_gateway_relation_broken(self, _) -> None:
-        """Handle the local-gateway relation broken event."""
-        if self.model.config["deployment-mode"].lower() == "serverless":
-            self.unit.status = BlockedStatus("Please relate to knative-serving:local-gateway")
-        return
-
     def _check_container_connection(self, container: Container) -> None:
         """Check if connection can be made with container.
 

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

@@ -122,6 +122,7 @@ def lightkube_client() -> lightkube.Client:
     return client
 
 
+@pytest.mark.skip_if_deployed
 @pytest.mark.abort_on_fail
 async def test_build_and_deploy(ops_test: OpsTest):
     """Build the charm-under-test and deploy it together with related charms.

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

@@ -29,6 +29,18 @@
     }
 }
 
+KSERVE_CONTROLLER_EXPECTED_LAYER = {
+    "services": {
+        "kserve-controller": {
+            "command": "/manager --metrics-addr=:8080",
+            "environment": {"POD_NAMESPACE": None, "SECRET_NAME": "kserve-webhook-server-cert"},
+            "override": "replace",
+            "startup": "enabled",
+            "summary": "KServe Controller",
+        }
+    }
+}
+
 SECRETS_TEST_FILES = ["tests/test_data/secret.yaml.j2"]
 SERVICE_ACCOUNTS_TEST_FILES = ["tests/test_data/service-account-yaml.j2"]
 
@@ -83,26 +95,22 @@ def mocked_lightkube_client(mocker, mocked_resource_handler):
 
 def test_events(harness, mocked_resource_handler, mocker):
     harness.begin()
-    on_install = mocker.patch("charm.KServeControllerCharm._on_install")
+    on_event = mocker.patch("charm.KServeControllerCharm._on_event")
     on_remove = mocker.patch("charm.KServeControllerCharm._on_remove")
-    on_kserve_controller_pebble_ready = mocker.patch(
-        "charm.KServeControllerCharm._on_kserve_controller_ready"
-    )
-    on_kube_rbac_proxy_ready = mocker.patch(
-        "charm.KServeControllerCharm._on_kube_rbac_proxy_ready"
-    )
 
     harness.charm.on.install.emit()
-    on_install.assert_called_once()
+    on_event.assert_called_once()
 
     harness.charm.on.remove.emit()
     on_remove.assert_called_once()
 
+    on_event.reset_mock()
     harness.charm.on.kserve_controller_pebble_ready.emit("kserve-controller")
-    on_kserve_controller_pebble_ready.assert_called_once()
+    on_event.assert_called_once()
 
+    on_event.reset_mock()
     harness.charm.on.kube_rbac_proxy_pebble_ready.emit("kube-rbac-proxy")
-    on_kube_rbac_proxy_ready.assert_called_once()
+    on_event.assert_called_once()
 
 
 def test_on_install_active(harness, mocked_resource_handler):
@@ -125,13 +133,28 @@ def test_on_install_exception(harness, mocked_resource_handler, mocker):
     mocked_logger.error.assert_called()
 
 
-def test_on_kube_rbac_proxy_ready_active(harness, mocker):
+def test_on_kube_rbac_proxy_ready_active(harness, mocked_resource_handler, mocker):
     harness.begin()
-
     # Check initial plan is empty
     initial_plan = harness.get_container_pebble_plan("kube-rbac-proxy")
     assert initial_plan.to_yaml() == "{}\n"
 
+    # Add relation with ingress-gateway providers
+    relation_id_ingress = harness.add_relation("ingress-gateway", "test-istio-pilot")
+    relation_id_local = harness.add_relation("local-gateway", "test-knative-serving")
+
+    # Updated the data bag with ingress-gateway
+    remote_ingress_data = {
+        "gateway_name": "test-ingress-name",
+        "gateway_namespace": "test-ingress-namespace",
+    }
+    remote_local_data = {
+        "gateway_name": "test-local-name",
+        "gateway_namespace": "test-local-namespace",
+    }
+    harness.update_relation_data(relation_id_ingress, "test-istio-pilot", remote_ingress_data)
+    harness.update_relation_data(relation_id_local, "test-knative-serving", remote_local_data)
+
     # Check layer gets created
     harness.charm.on.kube_rbac_proxy_pebble_ready.emit("kube-rbac-proxy")
     assert harness.get_container_pebble_plan("kube-rbac-proxy")._services != {}
@@ -145,7 +168,7 @@ def test_on_kube_rbac_proxy_ready_active(harness, mocker):
     assert harness.model.unit.status == ActiveStatus()
 
 
-def test_on_kube_rbac_proxy_ready_exception_blocked(harness, mocker):
+def test_on_kube_rbac_proxy_ready_exception_blocked(harness, mocked_resource_handler, mocker):
     harness.begin()
 
     mocked_update_layer = mocker.patch("charm.update_layer")
@@ -157,7 +180,7 @@ def test_on_kube_rbac_proxy_ready_exception_blocked(harness, mocker):
     mocked_logger.info.assert_not_called()
 
 
-def test_on_kube_rbac_proxy_ready_exception_other(harness, mocker):
+def test_on_kube_rbac_proxy_ready_exception_other(harness, mocked_resource_handler, mocker):
     harness.begin()
 
     mocked_update_layer = mocker.patch("charm.update_layer")
@@ -166,21 +189,64 @@ def test_on_kube_rbac_proxy_ready_exception_other(harness, mocker):
 
     harness.charm.on.kube_rbac_proxy_pebble_ready.emit("kube-rbac-proxy")
 
-    mocked_logger.info.assert_called_once()
-    mocked_logger.error.assert_not_called()
+    mocked_logger.error.assert_called_once()
 
 
-def test_on_kserve_controller_ready_active(harness, mocker):
+def test_on_kserve_controller_ready_active(harness, mocked_resource_handler, mocker):
     harness.begin()
 
     # Check initial plan is empty
     initial_plan = harness.get_container_pebble_plan("kserve-controller")
     assert initial_plan.to_yaml() == "{}\n"
 
-    # FIXME: missing mocked ops.model.Container.push
+    # Add relation with ingress-gateway providers
+    relation_id_ingress = harness.add_relation("ingress-gateway", "test-istio-pilot")
+    relation_id_local = harness.add_relation("local-gateway", "test-knative-serving")
+
+    # Updated the data bag with ingress-gateway
+    remote_ingress_data = {
+        "gateway_name": "test-ingress-name",
+        "gateway_namespace": "test-ingress-namespace",
+    }
+    remote_local_data = {
+        "gateway_name": "test-local-name",
+        "gateway_namespace": "test-local-namespace",
+    }
+    harness.update_relation_data(relation_id_ingress, "test-istio-pilot", remote_ingress_data)
+    harness.update_relation_data(relation_id_local, "test-knative-serving", remote_local_data)
+
     # Check layer gets created
-    # harness.charm.on.kserve_controller_pebble_ready.emit("kserve-controller")
-    # assert harness.get_container_pebble_plan("kserve-controller")._services != {}
+    harness.charm.on.kube_rbac_proxy_pebble_ready.emit("kserve-controller-ready")
+    assert harness.get_container_pebble_plan("kserve-controller")._services != {}
+
+    updated_plan = harness.get_container_pebble_plan("kserve-controller").to_dict()
+    assert KSERVE_CONTROLLER_EXPECTED_LAYER == updated_plan
+
+    service = harness.model.unit.get_container("kserve-controller").get_service(
+        "kserve-controller"
+    )
+    assert service.is_running() is True
+
+    assert harness.model.unit.status == ActiveStatus()
+
+
+def test_on_kserve_controller_ready_no_relation_blocked(harness, mocked_resource_handler, mocker):
+    """Tests that charm goes to blocked when it has no relation to knative-serving."""
+    harness.begin()
+
+    # Add relation with ingress-gateway providers
+    relation_id_ingress = harness.add_relation("ingress-gateway", "test-istio-pilot")
+
+    # Updated the data bag with ingress-gateway
+    remote_ingress_data = {
+        "gateway_name": "test-ingress-name",
+        "gateway_namespace": "test-ingress-namespace",
+    }
+    harness.update_relation_data(relation_id_ingress, "test-istio-pilot", remote_ingress_data)
+
+    assert harness.model.unit.status == BlockedStatus(
+        "Please relate to knative-serving:local-gateway"
+    )
 
 
 def test_on_remove_success(harness, mocker, mocked_resource_handler):
@@ -447,7 +513,7 @@ def test_gen_certs_if_missing(cert_data_dict, should_certs_refresh, harness: Har
     assert mocked_gen_certs.called == should_certs_refresh
 
 
-def test_restart_controller_service(harness, mocker):
+def test_restart_controller_service(harness, mocked_resource_handler, mocker):
     """Checks the controller service is restarted correctly."""
     harness.begin()
 
@@ -462,6 +528,22 @@ def test_restart_controller_service(harness, mocker):
     )
     assert controller_service is None
 
+    # Add relation with ingress-gateway providers
+    relation_id_ingress = harness.add_relation("ingress-gateway", "test-istio-pilot")
+    relation_id_local = harness.add_relation("local-gateway", "test-knative-serving")
+
+    # Updated the data bag with ingress-gateway
+    remote_ingress_data = {
+        "gateway_name": "test-ingress-name",
+        "gateway_namespace": "test-ingress-namespace",
+    }
+    remote_local_data = {
+        "gateway_name": "test-local-name",
+        "gateway_namespace": "test-local-namespace",
+    }
+    harness.update_relation_data(relation_id_ingress, "test-istio-pilot", remote_ingress_data)
+    harness.update_relation_data(relation_id_local, "test-knative-serving", remote_local_data)
+
     # Simulate what happens after the pebble ready event
     harness.container_pebble_ready(harness.charm._controller_container_name)
     mocked_container_restart = mocker.patch.object(harness.charm.controller_container, "restart")