From 8f9540d39d2f7802b6326de8df1bc6f85da44150 Mon Sep 17 00:00:00 2001 From: IbraAoad Date: Tue, 9 Jul 2024 21:49:43 +0300 Subject: [PATCH 1/8] switch stop hook --- .../observability_libs/v1/kubernetes_service_patch.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/charms/observability_libs/v1/kubernetes_service_patch.py b/lib/charms/observability_libs/v1/kubernetes_service_patch.py index a292837..f15790d 100644 --- a/lib/charms/observability_libs/v1/kubernetes_service_patch.py +++ b/lib/charms/observability_libs/v1/kubernetes_service_patch.py @@ -166,7 +166,7 @@ def setUp(self, *unused): # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 11 +LIBPATCH = 12 ServiceType = Literal["ClusterIP", "LoadBalancer"] @@ -227,7 +227,7 @@ def __init__( self.framework.observe(charm.on.install, self._patch) self.framework.observe(charm.on.upgrade_charm, self._patch) self.framework.observe(charm.on.update_status, self._patch) - self.framework.observe(charm.on.stop, self._remove_service) + self.framework.observe(charm.on.remove, self._remove_service) # apply user defined events if refresh_event: @@ -372,10 +372,11 @@ def _remove_service(self, _): try: client.delete(Service, self.service_name, namespace=self._namespace) + logger.info(f'Service {self.service_name} deleted successfully.') except ApiError as e: if e.status.code == 404: # Service not found, so no action needed - pass + return else: # Re-raise for other statuses raise From 541ffa3b23a69f5c783502590b1e58191bd04839 Mon Sep 17 00:00:00 2001 From: IbraAoad Date: Tue, 9 Jul 2024 23:11:20 +0300 Subject: [PATCH 2/8] handle removal on upgrade --- .../v1/kubernetes_service_patch.py | 33 ++++++++++++++++--- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/lib/charms/observability_libs/v1/kubernetes_service_patch.py b/lib/charms/observability_libs/v1/kubernetes_service_patch.py index f15790d..896e2fb 100644 --- a/lib/charms/observability_libs/v1/kubernetes_service_patch.py +++ b/lib/charms/observability_libs/v1/kubernetes_service_patch.py @@ -153,6 +153,7 @@ def setUp(self, *unused): from lightkube.models.meta_v1 import ObjectMeta from lightkube.resources.core_v1 import Service from lightkube.types import PatchType +from ops import UpgradeCharmEvent from ops.charm import CharmBase from ops.framework import BoundEvent, Object @@ -225,7 +226,7 @@ def __init__( assert isinstance(self._patch, MethodType) # Ensure this patch is applied during the 'install' and 'upgrade-charm' events self.framework.observe(charm.on.install, self._patch) - self.framework.observe(charm.on.upgrade_charm, self._patch) + self.framework.observe(charm.on.upgrade_charm, self._on_upgrade_charm) self.framework.observe(charm.on.update_status, self._patch) self.framework.observe(charm.on.remove, self._remove_service) @@ -356,6 +357,31 @@ def _is_patched(self, client: Client) -> bool: ] # noqa: E501 return expected_ports == fetched_ports + def _on_upgrade_charm(self, event: UpgradeCharmEvent): + """Handle the upgrade charm event.""" + # Handling the case when a user changes the service type from LB to ClusterIP in an upgrade, previous LB should be deleted. + if self.service_type == "ClusterIP": + + client = Client() # pyright: ignore + + # Define a label selector to find services related to the app + selector = {"app.kubernetes.io/name": self._app} + + # Check if any service of type LoadBalancer exists + services = client.list(Service, namespace=self._namespace, labels=selector) + for service in services: + if not service.metadata or not service.metadata.name: + logger.warning("Skipping resource with incomplete metadata.") + continue + if service.spec.type == "LoadBalancer": + client.delete(Service, service.metadata.name, namespace=self._namespace) + logger.info( + f"LoadBalancer service {service.metadata.name} deleted successfully." + ) + + # CContinue the upgrade flow normally + self._patch(event) + def _remove_service(self, _): """Remove a Kubernetes service associated with this charm. @@ -372,14 +398,13 @@ def _remove_service(self, _): try: client.delete(Service, self.service_name, namespace=self._namespace) - logger.info(f'Service {self.service_name} deleted successfully.') + logger.info(f"Service {self.service_name} deleted successfully.") except ApiError as e: if e.status.code == 404: # Service not found, so no action needed return - else: # Re-raise for other statuses - raise + raise @property def _app(self) -> str: From d3ea056feb39f90d760933a7b704dd048ccee78a Mon Sep 17 00:00:00 2001 From: IbraAoad Date: Tue, 9 Jul 2024 23:22:22 +0300 Subject: [PATCH 3/8] fix static checks --- .../v1/kubernetes_service_patch.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/charms/observability_libs/v1/kubernetes_service_patch.py b/lib/charms/observability_libs/v1/kubernetes_service_patch.py index 896e2fb..5745ad5 100644 --- a/lib/charms/observability_libs/v1/kubernetes_service_patch.py +++ b/lib/charms/observability_libs/v1/kubernetes_service_patch.py @@ -150,7 +150,7 @@ def setUp(self, *unused): from lightkube import ApiError, Client # pyright: ignore from lightkube.core import exceptions from lightkube.models.core_v1 import ServicePort, ServiceSpec -from lightkube.models.meta_v1 import ObjectMeta +from lightkube.models.meta_v1 import LabelSelector, ObjectMeta from lightkube.resources.core_v1 import Service from lightkube.types import PatchType from ops import UpgradeCharmEvent @@ -365,12 +365,17 @@ def _on_upgrade_charm(self, event: UpgradeCharmEvent): client = Client() # pyright: ignore # Define a label selector to find services related to the app - selector = {"app.kubernetes.io/name": self._app} + selector = LabelSelector(matchLabels={"app.kubernetes.io/name": self._app}) # Check if any service of type LoadBalancer exists - services = client.list(Service, namespace=self._namespace, labels=selector) + services = client.list(Service, namespace=self._namespace, labels=selector.to_dict()) for service in services: - if not service.metadata or not service.metadata.name: + if ( + not service.metadata + or not service.metadata.name + or not service.spec + or not service.spec.type + ): logger.warning("Skipping resource with incomplete metadata.") continue if service.spec.type == "LoadBalancer": From 7c0d4fe06d6f0494201c5434a104705467502554 Mon Sep 17 00:00:00 2001 From: IbraAoad Date: Tue, 9 Jul 2024 23:38:16 +0300 Subject: [PATCH 4/8] fix unit tests --- .../observability_libs/v1/kubernetes_service_patch.py | 8 ++++---- tests/unit/test_kubernetes_service.py | 1 + 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/charms/observability_libs/v1/kubernetes_service_patch.py b/lib/charms/observability_libs/v1/kubernetes_service_patch.py index 5745ad5..f796e7e 100644 --- a/lib/charms/observability_libs/v1/kubernetes_service_patch.py +++ b/lib/charms/observability_libs/v1/kubernetes_service_patch.py @@ -145,12 +145,12 @@ def setUp(self, *unused): import logging from types import MethodType -from typing import List, Literal, Optional, Union +from typing import Any, List, Literal, Optional, Union from lightkube import ApiError, Client # pyright: ignore from lightkube.core import exceptions from lightkube.models.core_v1 import ServicePort, ServiceSpec -from lightkube.models.meta_v1 import LabelSelector, ObjectMeta +from lightkube.models.meta_v1 import ObjectMeta from lightkube.resources.core_v1 import Service from lightkube.types import PatchType from ops import UpgradeCharmEvent @@ -365,10 +365,10 @@ def _on_upgrade_charm(self, event: UpgradeCharmEvent): client = Client() # pyright: ignore # Define a label selector to find services related to the app - selector = LabelSelector(matchLabels={"app.kubernetes.io/name": self._app}) + selector: dict[str, Any] = {"app.kubernetes.io/name": self._app} # Check if any service of type LoadBalancer exists - services = client.list(Service, namespace=self._namespace, labels=selector.to_dict()) + services = client.list(Service, namespace=self._namespace, labels=selector) for service in services: if ( not service.metadata diff --git a/tests/unit/test_kubernetes_service.py b/tests/unit/test_kubernetes_service.py index 6f6d674..f3782fb 100644 --- a/tests/unit/test_kubernetes_service.py +++ b/tests/unit/test_kubernetes_service.py @@ -374,6 +374,7 @@ def test_given_initialized_charm_when_install_event_then_event_listener_is_attac charm.on.install.emit() self.assertEqual(patch.call_count, 1) + @patch(f"{CL_PATH}._namespace", "test") def test_given_initialized_charm_when_upgrade_event_then_event_listener_is_attached(self): charm = self.harness.charm with mock.patch(f"{CL_PATH}._patch") as patch: From cb646d64c53786f1a21d29859954ec995ba0ebf7 Mon Sep 17 00:00:00 2001 From: IbraAoad Date: Tue, 9 Jul 2024 23:53:22 +0300 Subject: [PATCH 5/8] fix unit test iter 2 --- tests/unit/test_kubernetes_service.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_kubernetes_service.py b/tests/unit/test_kubernetes_service.py index f3782fb..3562be1 100644 --- a/tests/unit/test_kubernetes_service.py +++ b/tests/unit/test_kubernetes_service.py @@ -375,8 +375,11 @@ def test_given_initialized_charm_when_install_event_then_event_listener_is_attac self.assertEqual(patch.call_count, 1) @patch(f"{CL_PATH}._namespace", "test") - def test_given_initialized_charm_when_upgrade_event_then_event_listener_is_attached(self): + @patch(f"{MOD_PATH}.Client") + def test_given_initialized_charm_when_upgrade_event_then_event_listener_is_attached(self,client): charm = self.harness.charm + client.return_value = client + client.list.return_value = [] with mock.patch(f"{CL_PATH}._patch") as patch: charm.on.upgrade_charm.emit() self.assertEqual(patch.call_count, 1) From ae48c34dfd13440f1ac0b556e2054a6712d5b184 Mon Sep 17 00:00:00 2001 From: IbraAoad Date: Tue, 9 Jul 2024 23:54:46 +0300 Subject: [PATCH 6/8] now the linter --- tests/unit/test_kubernetes_service.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_kubernetes_service.py b/tests/unit/test_kubernetes_service.py index 3562be1..1ce8ee4 100644 --- a/tests/unit/test_kubernetes_service.py +++ b/tests/unit/test_kubernetes_service.py @@ -376,7 +376,9 @@ def test_given_initialized_charm_when_install_event_then_event_listener_is_attac @patch(f"{CL_PATH}._namespace", "test") @patch(f"{MOD_PATH}.Client") - def test_given_initialized_charm_when_upgrade_event_then_event_listener_is_attached(self,client): + def test_given_initialized_charm_when_upgrade_event_then_event_listener_is_attached( + self, client + ): charm = self.harness.charm client.return_value = client client.list.return_value = [] From 9d90bbda69cab97b2bdada3dca3a840a58b71c74 Mon Sep 17 00:00:00 2001 From: Leon <82407168+sed-i@users.noreply.github.com> Date: Wed, 10 Jul 2024 14:52:30 -0400 Subject: [PATCH 7/8] Apply suggestions from code review --- .../v1/kubernetes_service_patch.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/charms/observability_libs/v1/kubernetes_service_patch.py b/lib/charms/observability_libs/v1/kubernetes_service_patch.py index f796e7e..62e90fe 100644 --- a/lib/charms/observability_libs/v1/kubernetes_service_patch.py +++ b/lib/charms/observability_libs/v1/kubernetes_service_patch.py @@ -228,6 +228,8 @@ def __init__( self.framework.observe(charm.on.install, self._patch) self.framework.observe(charm.on.upgrade_charm, self._on_upgrade_charm) self.framework.observe(charm.on.update_status, self._patch) + # Sometimes Juju doesn't clean-up a manually created LB service, + # so we clean it up ourselves just in case. self.framework.observe(charm.on.remove, self._remove_service) # apply user defined events @@ -359,7 +361,7 @@ def _is_patched(self, client: Client) -> bool: def _on_upgrade_charm(self, event: UpgradeCharmEvent): """Handle the upgrade charm event.""" - # Handling the case when a user changes the service type from LB to ClusterIP in an upgrade, previous LB should be deleted. + # If a charm author changed the service type from LB to ClusterIP across an upgrade, we need to delete the previous LB. if self.service_type == "ClusterIP": client = Client() # pyright: ignore @@ -376,15 +378,15 @@ def _on_upgrade_charm(self, event: UpgradeCharmEvent): or not service.spec or not service.spec.type ): - logger.warning("Skipping resource with incomplete metadata.") + logger.warning("Service patch: skipping resource with incomplete metadata: %s.", service) continue if service.spec.type == "LoadBalancer": client.delete(Service, service.metadata.name, namespace=self._namespace) logger.info( - f"LoadBalancer service {service.metadata.name} deleted successfully." + f"LoadBalancer service {service.metadata.name} deleted." ) - # CContinue the upgrade flow normally + # Continue the upgrade flow normally self._patch(event) def _remove_service(self, _): @@ -403,12 +405,12 @@ def _remove_service(self, _): try: client.delete(Service, self.service_name, namespace=self._namespace) - logger.info(f"Service {self.service_name} deleted successfully.") + logger.info("The patched k8s service '%s' was deleted.", self.service_name) except ApiError as e: if e.status.code == 404: # Service not found, so no action needed return - # Re-raise for other statuses + # Re-raise for other statuses raise @property From cb7e81e8c4a22a8abe749058cb509a18d6ecaa5f Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Wed, 10 Jul 2024 15:09:06 -0400 Subject: [PATCH 8/8] Lint --- .../observability_libs/v1/kubernetes_service_patch.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/charms/observability_libs/v1/kubernetes_service_patch.py b/lib/charms/observability_libs/v1/kubernetes_service_patch.py index 62e90fe..e85834b 100644 --- a/lib/charms/observability_libs/v1/kubernetes_service_patch.py +++ b/lib/charms/observability_libs/v1/kubernetes_service_patch.py @@ -378,13 +378,13 @@ def _on_upgrade_charm(self, event: UpgradeCharmEvent): or not service.spec or not service.spec.type ): - logger.warning("Service patch: skipping resource with incomplete metadata: %s.", service) + logger.warning( + "Service patch: skipping resource with incomplete metadata: %s.", service + ) continue if service.spec.type == "LoadBalancer": client.delete(Service, service.metadata.name, namespace=self._namespace) - logger.info( - f"LoadBalancer service {service.metadata.name} deleted." - ) + logger.info(f"LoadBalancer service {service.metadata.name} deleted.") # Continue the upgrade flow normally self._patch(event)