From cc464fd6bdc8eb7402d68a1acd66c3d5a02f4c02 Mon Sep 17 00:00:00 2001 From: Daniela Plascencia Date: Tue, 13 Feb 2024 15:47:45 +0100 Subject: [PATCH] fix: set telemetry config value, patch service, update tests (#185) (#186) * fix: set telemetry config value, patch service, update tests This commit ensures the configuration value for the telemetry setting is correctly passed to the workload configuration value. With this we ensure the workload is correctly exposing metrics in the desired endpoint so they can be scraped by prometheus. With this change we also must ensure that the K8s Service correctly exposes the ports of the endpoints that this workload has (for metrics and the actual dex service). Finally, a bit of refactoring work had to be made in the unit test to match the recent changes and the kubernetes_service_patch library this charm uses has been bumped v0 -> v1. Part of canonical/bundle-kubeflow#563 * tests: add an assertion for checking unit is available The test_prometheus_grafana_integration test case was doing queries to prometheus and checking the request returned successfully and that the application name and model was listed correctly. To make this test case more accurately, we can add an assertion that also checks that the unit is available, this way we avoid issues like the one described in canonical/bundle-kubeflow#564. Part of canonical/bundle-kubeflow#564 --- .../{v0 => v1}/kubernetes_service_patch.py | 167 ++++++++++++------ src/charm.py | 18 +- tests/integration/test_charm.py | 6 + tests/unit/test_charm.py | 16 +- 4 files changed, 141 insertions(+), 66 deletions(-) rename lib/charms/observability_libs/{v0 => v1}/kubernetes_service_patch.py (64%) diff --git a/lib/charms/observability_libs/v0/kubernetes_service_patch.py b/lib/charms/observability_libs/v1/kubernetes_service_patch.py similarity index 64% rename from lib/charms/observability_libs/v0/kubernetes_service_patch.py rename to lib/charms/observability_libs/v1/kubernetes_service_patch.py index a3fb9109..2cce729e 100644 --- a/lib/charms/observability_libs/v0/kubernetes_service_patch.py +++ b/lib/charms/observability_libs/v1/kubernetes_service_patch.py @@ -9,21 +9,20 @@ default contains a "placeholder" port, which is 65536/TCP. When modifying the default set of resources managed by Juju, one must consider the lifecycle of the -charm. In this case, any modifications to the default service (created during deployment), will -be overwritten during a charm upgrade. +charm. In this case, any modifications to the default service (created during deployment), will be +overwritten during a charm upgrade. When initialised, this library binds a handler to the parent charm's `install` and `upgrade_charm` events which applies the patch to the cluster. This should ensure that the service ports are correct throughout the charm's life. -The constructor simply takes a reference to the parent charm, and a list of tuples that each define -a port for the service, where each tuple contains: +The constructor simply takes a reference to the parent charm, and a list of +[`lightkube`](https://github.com/gtsystem/lightkube) ServicePorts that each define a port for the +service. For information regarding the `lightkube` `ServicePort` model, please visit the +`lightkube` [docs](https://gtsystem.github.io/lightkube-models/1.23/models/core_v1/#serviceport). -- a name for the port -- port for the service to listen on -- optionally: a targetPort for the service (the port in the container!) -- optionally: a nodePort for the service (for NodePort or LoadBalancer services only!) -- optionally: a name of the service (in case service name needs to be patched as well) +Optionally, a name of the service (in case service name needs to be patched as well), labels, +selectors, and annotations can be provided as keyword arguments. ## Getting Started @@ -32,8 +31,8 @@ ```shell cd some-charm -charmcraft fetch-lib charms.observability_libs.v0.kubernetes_service_patch -echo <<-EOF >> requirements.txt +charmcraft fetch-lib charms.observability_libs.v1.kubernetes_service_patch +cat << EOF >> requirements.txt lightkube lightkube-models EOF @@ -41,28 +40,71 @@ Then, to initialise the library: -For ClusterIP services: +For `ClusterIP` services: + ```python # ... -from charms.observability_libs.v0.kubernetes_service_patch import KubernetesServicePatch +from charms.observability_libs.v1.kubernetes_service_patch import KubernetesServicePatch +from lightkube.models.core_v1 import ServicePort class SomeCharm(CharmBase): def __init__(self, *args): # ... - self.service_patcher = KubernetesServicePatch(self, [(f"{self.app.name}", 8080)]) + port = ServicePort(443, name=f"{self.app.name}") + self.service_patcher = KubernetesServicePatch(self, [port]) # ... ``` -For LoadBalancer/NodePort services: +For `LoadBalancer`/`NodePort` services: + ```python # ... -from charms.observability_libs.v0.kubernetes_service_patch import KubernetesServicePatch +from charms.observability_libs.v1.kubernetes_service_patch import KubernetesServicePatch +from lightkube.models.core_v1 import ServicePort class SomeCharm(CharmBase): def __init__(self, *args): # ... + port = ServicePort(443, name=f"{self.app.name}", targetPort=443, nodePort=30666) self.service_patcher = KubernetesServicePatch( - self, [(f"{self.app.name}", 443, 443, 30666)], "LoadBalancer" + self, [port], "LoadBalancer" + ) + # ... +``` + +Port protocols can also be specified. Valid protocols are `"TCP"`, `"UDP"`, and `"SCTP"` + +```python +# ... +from charms.observability_libs.v1.kubernetes_service_patch import KubernetesServicePatch +from lightkube.models.core_v1 import ServicePort + +class SomeCharm(CharmBase): + def __init__(self, *args): + # ... + tcp = ServicePort(443, name=f"{self.app.name}-tcp", protocol="TCP") + udp = ServicePort(443, name=f"{self.app.name}-udp", protocol="UDP") + sctp = ServicePort(443, name=f"{self.app.name}-sctp", protocol="SCTP") + self.service_patcher = KubernetesServicePatch(self, [tcp, udp, sctp]) + # ... +``` + +Bound with custom events by providing `refresh_event` argument: +For example, you would like to have a configurable port in your charm and want to apply +service patch every time charm config is changed. + +```python +from charms.observability_libs.v1.kubernetes_service_patch import KubernetesServicePatch +from lightkube.models.core_v1 import ServicePort + +class SomeCharm(CharmBase): + def __init__(self, *args): + # ... + port = ServicePort(int(self.config["charm-config-port"]), name=f"{self.app.name}") + self.service_patcher = KubernetesServicePatch( + self, + [port], + refresh_event=self.on.config_changed ) # ... ``` @@ -83,15 +125,16 @@ def setUp(self, *unused): import logging from types import MethodType -from typing import Literal, Sequence, Tuple, Union +from typing import List, Literal, Optional, Union -from lightkube import ApiError, Client +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.resources.core_v1 import Service from lightkube.types import PatchType from ops.charm import CharmBase -from ops.framework import Object +from ops.framework import BoundEvent, Object logger = logging.getLogger(__name__) @@ -99,13 +142,12 @@ def setUp(self, *unused): LIBID = "0042f86d0a874435adef581806cddbbb" # Increment this major API version when introducing breaking changes -LIBAPI = 0 +LIBAPI = 1 # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 6 +LIBPATCH = 9 -PortDefinition = Union[Tuple[str, int], Tuple[str, int, int], Tuple[str, int, int, int]] ServiceType = Literal["ClusterIP", "LoadBalancer"] @@ -115,18 +157,20 @@ class KubernetesServicePatch(Object): def __init__( self, charm: CharmBase, - ports: Sequence[PortDefinition], - service_name: str = None, + ports: List[ServicePort], + service_name: Optional[str] = None, service_type: ServiceType = "ClusterIP", - additional_labels: dict = None, - additional_selectors: dict = None, - additional_annotations: dict = None, + additional_labels: Optional[dict] = None, + additional_selectors: Optional[dict] = None, + additional_annotations: Optional[dict] = None, + *, + refresh_event: Optional[Union[BoundEvent, List[BoundEvent]]] = None, ): """Constructor for KubernetesServicePatch. Args: charm: the charm that is instantiating the library. - ports: a list of tuples (name, port, targetPort, nodePort) for every service port. + ports: a list of ServicePorts service_name: allows setting custom name to the patched service. If none given, application name will be used. service_type: desired type of K8s service. Default value is in line with ServiceSpec's @@ -136,6 +180,9 @@ def __init__( additional_selectors: Selectors to be added to the kubernetes service (by default only "app.kubernetes.io/name" is set to the service name) additional_annotations: Annotations to be added to the kubernetes service. + refresh_event: an optional bound event or list of bound events which + will be observed to re-apply the patch (e.g. on port change). + The `install` and `upgrade-charm` events would be observed regardless. """ super().__init__(charm, "kubernetes-service-patch") self.charm = charm @@ -154,23 +201,29 @@ def __init__( # 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.update_status, self._patch) + + # apply user defined events + if refresh_event: + if not isinstance(refresh_event, list): + refresh_event = [refresh_event] + + for evt in refresh_event: + self.framework.observe(evt, self._patch) def _service_object( self, - ports: Sequence[PortDefinition], - service_name: str = None, + ports: List[ServicePort], + service_name: Optional[str] = None, service_type: ServiceType = "ClusterIP", - additional_labels: dict = None, - additional_selectors: dict = None, - additional_annotations: dict = None, + additional_labels: Optional[dict] = None, + additional_selectors: Optional[dict] = None, + additional_annotations: Optional[dict] = None, ) -> Service: """Creates a valid Service representation. Args: - ports: a list of tuples of the form (name, port) or (name, port, targetPort) - or (name, port, targetPort, nodePort) for every service port. If the 'targetPort' - is omitted, it is assumed to be equal to 'port', with the exception of NodePort - and LoadBalancer services, where all port numbers have to be specified. + ports: a list of ServicePorts service_name: allows setting custom name to the patched service. If none given, application name will be used. service_type: desired type of K8s service. Default value is in line with ServiceSpec's @@ -203,15 +256,7 @@ def _service_object( ), spec=ServiceSpec( selector=selector, - ports=[ - ServicePort( - name=p[0], - port=p[1], - targetPort=p[2] if len(p) > 2 else p[1], # type: ignore[misc] - nodePort=p[3] if len(p) > 3 else None, # type: ignore[arg-type, misc] - ) - for p in ports - ], + ports=ports, type=service_type, ), ) @@ -222,11 +267,15 @@ def _patch(self, _) -> None: Raises: PatchFailed: if patching fails due to lack of permissions, or otherwise. """ - if not self.charm.unit.is_leader(): + try: + client = Client() # pyright: ignore + except exceptions.ConfigError as e: + logger.warning("Error creating k8s client: %s", e) return - client = Client() try: + if self._is_patched(client): + return if self.service_name != self._app: self._delete_and_create_service(client) client.patch(Service, self.service_name, self.service, patch_type=PatchType.MERGE) @@ -251,13 +300,25 @@ def is_patched(self) -> bool: Returns: bool: A boolean indicating if the service patch has been applied. """ - client = Client() + client = Client() # pyright: ignore + return self._is_patched(client) + + def _is_patched(self, client: Client) -> bool: # Get the relevant service from the cluster - service = client.get(Service, name=self.service_name, namespace=self._namespace) + try: + service = client.get(Service, name=self.service_name, namespace=self._namespace) + except ApiError as e: + if e.status.code == 404 and self.service_name != self._app: + return False + logger.error("Kubernetes service get failed: %s", str(e)) + raise + # Construct a list of expected ports, should the patch be applied - expected_ports = [(p.port, p.targetPort) for p in self.service.spec.ports] + expected_ports = [(p.port, p.targetPort) for p in self.service.spec.ports] # type: ignore[attr-defined] # Construct a list in the same manner, using the fetched service - fetched_ports = [(p.port, p.targetPort) for p in service.spec.ports] # type: ignore[attr-defined] # noqa: E501 + fetched_ports = [ + (p.port, p.targetPort) for p in service.spec.ports # type: ignore[attr-defined] + ] # noqa: E501 return expected_ports == fetched_ports @property diff --git a/src/charm.py b/src/charm.py index aaa23c06..e2ce4eee 100755 --- a/src/charm.py +++ b/src/charm.py @@ -11,8 +11,9 @@ import yaml from charmed_kubeflow_chisme.exceptions import ErrorWithStatus from charms.grafana_k8s.v0.grafana_dashboard import GrafanaDashboardProvider -from charms.observability_libs.v0.kubernetes_service_patch import KubernetesServicePatch +from charms.observability_libs.v1.kubernetes_service_patch import KubernetesServicePatch from charms.prometheus_k8s.v0.prometheus_scrape import MetricsEndpointProvider +from lightkube.models.core_v1 import ServicePort from ops.charm import CharmBase from ops.framework import StoredState from ops.main import main @@ -32,6 +33,16 @@ def __init__(self, *args): self.logger: logging.Logger = logging.getLogger(__name__) + # Patch the service to correctly expose the ports to be used + dex_port = ServicePort(int(self.model.config["port"]), name="dex") + metrics_port = ServicePort(int(METRICS_PORT), name="metrics-port") + + self.service_patcher = KubernetesServicePatch( + self, + [dex_port, metrics_port], + service_name=f"{self.model.app.name}", + ) + self.prometheus_provider = MetricsEndpointProvider( charm=self, relation_name="metrics-endpoint", @@ -51,10 +62,6 @@ def __init__(self, *args): self._entrypoint = "/usr/local/bin/docker-entrypoint" self._dex_config_path = "/etc/dex/config.docker.yaml" - self.service_patcher = KubernetesServicePatch( - self, [(self._container_name, self.model.config["port"])] - ) - for event in [ self.on.install, self.on.leader_elected, @@ -222,6 +229,7 @@ def _generate_dex_auth_config(self) -> str: "issuer": f"{public_url}/dex", "storage": {"type": "kubernetes", "config": {"inCluster": True}}, "web": {"http": f"0.0.0.0:{port}"}, + "telemetry": {"http": "0.0.0.0:5558"}, "logger": {"level": "debug", "format": "text"}, "oauth2": {"skipApprovalScreen": True}, "staticClients": oidc_client_info, diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 22da0e28..3fd35727 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -241,6 +241,12 @@ async def test_prometheus_grafana_integration(ops_test: OpsTest): assert response_metric["juju_application"] == APP_NAME assert response_metric["juju_model"] == ops_test.model_name + # Assert the unit is available by checking the query result + # The data is presented as a list [1707357912.349, '1'], where the + # first value is a timestamp and the second value is the state of the unit + # 1 means available, 0 means unavailable + assert response["data"]["result"][0]["value"][1] == "1" + # Helper to retry calling a function over 30 seconds or 5 attempts retry_for_5_attempts = Retrying( diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index ba44398b..cd8e1999 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -17,7 +17,7 @@ def harness(): return Harness(Operator) -@patch("charm.KubernetesServicePatch", lambda x, y: None) +@patch("charm.KubernetesServicePatch", lambda *_, **__: None) def test_not_leader(harness): harness.begin_with_initial_hooks() assert isinstance(harness.charm.model.unit.status, WaitingStatus) @@ -36,7 +36,7 @@ def ensure_state(self): self.state.user_id = "123" -@patch("charm.KubernetesServicePatch", lambda x, y: None) +@patch("charm.KubernetesServicePatch", lambda *_, **__: None) @patch("charm.Operator._update_layer") def test_install_event(update, harness): harness.set_leader(True) @@ -55,7 +55,7 @@ def test_install_event(update, harness): assert isinstance(harness.charm.model.unit.status, ActiveStatus) -@patch("charm.KubernetesServicePatch", lambda x, y: None) +@patch("charm.KubernetesServicePatch", lambda *_, **__: None) def test_generate_dex_auth_config_raises(harness): """Check the method raises when static login is disabled and no connectors are provided.""" harness.begin() @@ -96,7 +96,7 @@ def test_generate_dex_auth_config_raises(harness): ) @patch("charm.Operator._update_layer") @patch.object(Operator, "ensure_state", ensure_state) -@patch("charm.KubernetesServicePatch", lambda x, y: None) +@patch("charm.KubernetesServicePatch", lambda *_, **__: None) def test_generate_dex_auth_config_returns(update_layer, dex_config, harness): """Check the method returns dex-auth configuration when different settings are provided.""" harness.set_leader(True) @@ -125,7 +125,7 @@ def test_generate_dex_auth_config_returns(update_layer, dex_config, harness): assert harness.model.config["static-username"] == static_passwords[0].get("username") -@patch("charm.KubernetesServicePatch", lambda x, y: None) +@patch("charm.KubernetesServicePatch", lambda *_, **__: None) def test_disable_static_login_no_connector_blocked_status(harness): harness.set_leader(True) harness.begin() @@ -141,7 +141,7 @@ def test_disable_static_login_no_connector_blocked_status(harness): assert isinstance(harness.charm.model.unit.status, BlockedStatus) -@patch("charm.KubernetesServicePatch", lambda x, y: None) +@patch("charm.KubernetesServicePatch", lambda *_, **__: None) @patch("charm.Operator._update_layer") def test_config_changed(update, harness): harness.set_leader(True) @@ -165,7 +165,7 @@ def test_config_changed(update, harness): assert new_config == config_updates -@patch("charm.KubernetesServicePatch", lambda x, y: None) +@patch("charm.KubernetesServicePatch", lambda *_, **__: None) @patch("charm.Operator._update_layer") @patch.object(Operator, "ensure_state", ensure_state) def test_main_oidc(update, harness): @@ -188,7 +188,7 @@ def test_main_oidc(update, harness): assert isinstance(harness.charm.model.unit.status, ActiveStatus) -@patch("charm.KubernetesServicePatch", lambda x, y: None) +@patch("charm.KubernetesServicePatch", lambda *_, **__: None) @patch("charm.Operator._update_layer") @patch.object(Operator, "ensure_state", ensure_state) def test_main_ingress(update, harness):