From 3e6e48de7f68caa1b824078cb9c80165c7748854 Mon Sep 17 00:00:00 2001 From: dushu Date: Fri, 6 Oct 2023 14:58:40 -0600 Subject: [PATCH] address the comments --- pyproject.toml | 8 +-- src/charm.py | 72 +++++++++---------------- src/constants.py | 9 ++-- src/grafana_dashboards/glauth.json.tmpl | 2 +- src/kubernetes_resource.py | 23 ++++---- src/validators.py | 51 +++++++++--------- tests/unit/conftest.py | 4 +- tests/unit/test_charm.py | 39 +++++++------- tests/unit/test_validators.py | 22 ++++---- 9 files changed, 100 insertions(+), 130 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index dafcf6f2..0694585a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -24,17 +24,17 @@ log_cli_level = "INFO" # Formatting tools configuration [tool.black] -line-length = 80 +line-length = 99 target-version = ["py38"] [tool.isort] -line_length = 80 +line_length = 99 profile = "black" # Linting tools configuration [tool.flake8] -max-line-length = 80 -max-doc-length = 80 +max-line-length = 99 +max-doc-length = 99 max-complexity = 10 exclude = [".git", "__pycache__", ".tox", "build", "dist", "*.egg_info", "venv"] select = ["E", "W", "F", "C", "N", "R", "D", "H"] diff --git a/src/charm.py b/src/charm.py index 35454069..19391571 100755 --- a/src/charm.py +++ b/src/charm.py @@ -14,13 +14,8 @@ DatabaseRequires, ) from charms.grafana_k8s.v0.grafana_dashboard import GrafanaDashboardProvider -from charms.loki_k8s.v0.loki_push_api import ( - LogProxyConsumer, - PromtailDigestError, -) -from charms.observability_libs.v0.kubernetes_service_patch import ( - KubernetesServicePatch, -) +from charms.loki_k8s.v0.loki_push_api import LogProxyConsumer, PromtailDigestError +from charms.observability_libs.v0.kubernetes_service_patch import KubernetesServicePatch from charms.prometheus_k8s.v0.prometheus_scrape import MetricsEndpointProvider from jinja2 import Template from lightkube import Client @@ -37,23 +32,24 @@ from ops.pebble import ChangeError, Layer from constants import ( - DATABASE_RELATION_NAME, + DATABASE_INTEGRATION_NAME, GLAUTH_COMMANDS, GLAUTH_CONFIG_DIR, GLAUTH_LDAP_PORT, - GRAFANA_DASHBOARD_RELATION_NAME, + GRAFANA_DASHBOARD_INTEGRATION_NAME, LOG_DIR, LOG_FILE, - LOKI_API_PUSH_RELATION_NAME, - PROMETHEUS_SCRAPE_RELATION_NAME, + LOKI_API_PUSH_INTEGRATION_NAME, + PROMETHEUS_SCRAPE_INTEGRATION_NAME, WORKLOAD_CONTAINER, + WORKLOAD_SERVICE, ) from kubernetes_resource import ConfigMapResource, StatefulSetResource from validators import ( leader_unit, validate_container_connectivity, - validate_database_relation, validate_database_resource, + validate_integration_exists, ) logger = logging.getLogger(__name__) @@ -66,53 +62,39 @@ def __init__(self, *args): super().__init__(*args) self._container = self.unit.get_container(WORKLOAD_CONTAINER) - self._k8s_client = Client( - field_manager=self.app.name, namespace=self.model.name - ) - self._configmap = ConfigMapResource( - client=self._k8s_client, name=self.app.name - ) - self._statefulset = StatefulSetResource( - client=self._k8s_client, name=self.app.name - ) + self._k8s_client = Client(field_manager=self.app.name, namespace=self.model.name) + self._configmap = ConfigMapResource(client=self._k8s_client, name=self.app.name) + self._statefulset = StatefulSetResource(client=self._k8s_client, name=self.app.name) self._db_name = f"{self.model.name}_{self.app.name}" self.database = DatabaseRequires( self, - relation_name=DATABASE_RELATION_NAME, + relation_name=DATABASE_INTEGRATION_NAME, database_name=self._db_name, extra_user_roles="SUPERUSER", ) - self.service_patcher = KubernetesServicePatch( - self, [("ldap", GLAUTH_LDAP_PORT)] - ) + self.service_patcher = KubernetesServicePatch(self, [("ldap", GLAUTH_LDAP_PORT)]) self.loki_consumer = LogProxyConsumer( self, log_files=[str(LOG_FILE)], - relation_name=LOKI_API_PUSH_RELATION_NAME, + relation_name=LOKI_API_PUSH_INTEGRATION_NAME, container_name=WORKLOAD_CONTAINER, ) self.metrics_endpoint = MetricsEndpointProvider( - self, relation_name=PROMETHEUS_SCRAPE_RELATION_NAME + self, relation_name=PROMETHEUS_SCRAPE_INTEGRATION_NAME ) self._grafana_dashboards = GrafanaDashboardProvider( - self, relation_name=GRAFANA_DASHBOARD_RELATION_NAME + self, relation_name=GRAFANA_DASHBOARD_INTEGRATION_NAME ) 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.glauth_pebble_ready, self._on_pebble_ready - ) - self.framework.observe( - self.database.on.database_created, self._on_database_created - ) - self.framework.observe( - self.database.on.endpoints_changed, self._on_database_changed - ) + self.framework.observe(self.on.glauth_pebble_ready, self._on_pebble_ready) + self.framework.observe(self.database.on.database_created, self._on_database_created) + self.framework.observe(self.database.on.endpoints_changed, self._on_database_changed) self.framework.observe( self.loki_consumer.on.promtail_digest_error, self._on_promtail_error, @@ -124,7 +106,7 @@ def _pebble_layer(self) -> Layer: "summary": "GLAuth layer", "description": "pebble layer for GLAuth service", "services": { - WORKLOAD_CONTAINER: { + WORKLOAD_SERVICE: { "override": "replace", "summary": "GLAuth Operator layer", "startup": "disabled", @@ -147,13 +129,11 @@ def _restart_glauth_service(self): ) @validate_container_connectivity - @validate_database_relation + @validate_integration_exists(DATABASE_INTEGRATION_NAME) @validate_database_resource def _handle_event_update(self, event: HookEvent) -> None: self._update_glauth_config() - self._container.add_layer( - WORKLOAD_CONTAINER, self._pebble_layer, combine=True - ) + self._container.add_layer(WORKLOAD_CONTAINER, self._pebble_layer, combine=True) self._restart_glauth_service() self.unit.status = ActiveStatus() @@ -221,15 +201,11 @@ def _on_remove(self, event: RemoveEvent): def _on_database_created(self, event: DatabaseCreatedEvent): self._update_glauth_config() self._mount_glauth_config() - self._container.add_layer( - WORKLOAD_CONTAINER, self._pebble_layer, combine=True - ) + self._container.add_layer(WORKLOAD_CONTAINER, self._pebble_layer, combine=True) self._restart_glauth_service() self.unit.status = ActiveStatus() - def _on_database_changed( - self, event: DatabaseEndpointsChangedEvent - ) -> None: + def _on_database_changed(self, event: DatabaseEndpointsChangedEvent) -> None: self._handle_event_update(event) def _on_config_changed(self, event: ConfigChangedEvent): diff --git a/src/constants.py b/src/constants.py index b1d8accd..c43604e3 100644 --- a/src/constants.py +++ b/src/constants.py @@ -3,10 +3,10 @@ from pathlib import PurePath -DATABASE_RELATION_NAME = "pg-database" -LOKI_API_PUSH_RELATION_NAME = "logging" -PROMETHEUS_SCRAPE_RELATION_NAME = "metrics-endpoint" -GRAFANA_DASHBOARD_RELATION_NAME = "grafana-dashboard" +DATABASE_INTEGRATION_NAME = "pg-database" +LOKI_API_PUSH_INTEGRATION_NAME = "logging" +PROMETHEUS_SCRAPE_INTEGRATION_NAME = "metrics-endpoint" +GRAFANA_DASHBOARD_INTEGRATION_NAME = "grafana-dashboard" GLAUTH_CONFIG_DIR = PurePath("/etc/config") GLAUTH_CONFIG_FILE = GLAUTH_CONFIG_DIR / "glauth.cfg" @@ -17,3 +17,4 @@ LOG_FILE = LOG_DIR / "glauth.log" WORKLOAD_CONTAINER = "glauth" +WORKLOAD_SERVICE = "glauth" diff --git a/src/grafana_dashboards/glauth.json.tmpl b/src/grafana_dashboards/glauth.json.tmpl index ddc7c01f..e2a7947c 100644 --- a/src/grafana_dashboards/glauth.json.tmpl +++ b/src/grafana_dashboards/glauth.json.tmpl @@ -51,7 +51,7 @@ }, "id": 27, "panels": [], - "title": "Avaliability", + "title": "Availability", "type": "row" }, { diff --git a/src/kubernetes_resource.py b/src/kubernetes_resource.py index 5b91463f..63f99e6d 100644 --- a/src/kubernetes_resource.py +++ b/src/kubernetes_resource.py @@ -21,6 +21,11 @@ logger = logging.getLogger(__name__) +class KubernetesResourceError(Exception): + def __init__(self, message: str): + self.message = message + + def watch_for_update(func: Callable): def wrapper(self, *args, **kwargs): resource = self.get() @@ -36,10 +41,7 @@ def wrapper(self, *args, **kwargs): ): with attempt: resource = self.get() - if ( - not resource - or resource.metadata.resourceVersion == version - ): + if not resource or resource.metadata.resourceVersion == version: raise TryAgain except RetryError: logger.debug("No changes in the watched k8s resource") @@ -58,9 +60,7 @@ def name(self): def get(self): try: - cm = self._client.get( - ConfigMap, self._name, namespace=self._client.namespace - ) + cm = self._client.get(ConfigMap, self._name, namespace=self._client.namespace) return cm except ApiError as e: logging.error(f"Error fetching ConfigMap: {e}") @@ -81,6 +81,7 @@ def create(self): self._client.create(cm) except ApiError as e: logging.error(f"Error creating ConfigMap: {e}") + raise KubernetesResourceError(f"Failed to create ConfigMap {self._name}") @watch_for_update def patch(self, data: dict): @@ -98,9 +99,7 @@ def patch(self, data: dict): def delete(self): try: - self._client.delete( - ConfigMap, self._name, namespace=self._client.namespace - ) + self._client.delete(ConfigMap, self._name, namespace=self._client.namespace) except ApiError as e: logging.error(f"Error deleting ConfigMap: {e}") @@ -116,9 +115,7 @@ def name(self): def get(self): try: - ss = self._client.get( - StatefulSet, self._name, namespace=self._client.namespace - ) + ss = self._client.get(StatefulSet, self._name, namespace=self._client.namespace) return ss except ApiError as e: logging.error(f"Error fetching ConfigMap: {e}") diff --git a/src/validators.py b/src/validators.py index 800d30e1..666fb21b 100644 --- a/src/validators.py +++ b/src/validators.py @@ -5,17 +5,15 @@ from functools import wraps from typing import Any, Callable -from ops.charm import EventBase +from ops.charm import CharmBase, EventBase from ops.model import BlockedStatus, MaintenanceStatus, WaitingStatus -from constants import DATABASE_RELATION_NAME - logger = logging.getLogger(__name__) def leader_unit(func: Callable): @wraps(func) - def wrapper(self, *args: EventBase, **kwargs: Any): + def wrapper(self: CharmBase, *args: EventBase, **kwargs: Any): if not self.unit.is_leader(): return @@ -26,16 +24,14 @@ def wrapper(self, *args: EventBase, **kwargs: Any): def validate_container_connectivity(func: Callable): @wraps(func) - def wrapper(self, *args: EventBase, **kwargs: Any): + def wrapper(self: CharmBase, *args: EventBase, **kwargs: Any): event, *_ = args logger.debug(f"Handling event: {event}") if not self._container.can_connect(): logger.debug(f"Cannot connect to container, defer event {event}.") event.defer() - self.unit.status = WaitingStatus( - "Waiting to connect to " "container." - ) + self.unit.status = WaitingStatus("Waiting to connect to container.") return return func(self, *args, **kwargs) @@ -43,38 +39,39 @@ def wrapper(self, *args: EventBase, **kwargs: Any): return wrapper -def validate_database_relation(func: Callable): - @wraps(func) - def wrapper(self, *args: EventBase, **kwargs: Any): - event, *_ = args - logger.debug(f"Handling event: {event}") +def validate_integration_exists(integration_name: str): + def decorator(func: Callable): + @wraps(func) + def wrapper(self: CharmBase, *args: EventBase, **kwargs: Any): + event, *_ = args + logger.debug(f"Handling event: {event}") - self.unit.status = MaintenanceStatus("Configuring resources") - if not self.model.relations[DATABASE_RELATION_NAME]: - logger.debug(f"Database relation is missing, defer event {event}.") - event.defer() + self.unit.status = MaintenanceStatus("Configuring resources") + if not self.model.relations[integration_name]: + logger.debug(f"Integration {integration_name} is missing, defer event {event}.") + event.defer() - self.unit.status = BlockedStatus( - "Missing required relation with " "database" - ) - return + self.unit.status = BlockedStatus( + f"Missing required integration {integration_name}" + ) + return - return func(self, *args, **kwargs) + return func(self, *args, **kwargs) - return wrapper + return wrapper + + return decorator def validate_database_resource(func: Callable): @wraps(func) - def wrapper(self, *args: EventBase, **kwargs: Any): + def wrapper(self: CharmBase, *args: EventBase, **kwargs: Any): event, *_ = args logger.debug(f"Handling event: {event}") self.unit.status = MaintenanceStatus("Configuring resources") if not self.database.is_resource_created(): - logger.debug( - f"Database has not been created yet, defer event {event}" - ) + logger.debug(f"Database has not been created yet, defer event {event}") event.defer() self.unit.status = WaitingStatus("Waiting for database creation") diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 5737b897..e31c3b69 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -8,7 +8,7 @@ from pytest_mock import MockerFixture from charm import GLAuthCharm -from constants import DATABASE_RELATION_NAME +from constants import DATABASE_INTEGRATION_NAME DB_APP = "postgresql-k8s" DB_USERNAME = "relation_id" @@ -58,7 +58,7 @@ def mocked_statefulset(mocker: MockerFixture) -> MagicMock: @pytest.fixture() def database_relation(harness: Harness) -> int: - relation_id = harness.add_relation(DATABASE_RELATION_NAME, DB_APP) + relation_id = harness.add_relation(DATABASE_INTEGRATION_NAME, DB_APP) harness.add_relation_unit(relation_id, "postgresql-k8s/0") return relation_id diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index f0791b05..0e4f7052 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -1,17 +1,17 @@ # Copyright 2023 Canonical Ltd. # See LICENSE file for licensing details. -from ops.model import ActiveStatus, BlockedStatus, WaitingStatus +import pytest +from ops.model import ActiveStatus, BlockedStatus, MaintenanceStatus, WaitingStatus from ops.testing import Harness from pytest_mock import MockerFixture -from constants import WORKLOAD_CONTAINER +from constants import WORKLOAD_CONTAINER, WORKLOAD_SERVICE +from kubernetes_resource import KubernetesResourceError class TestInstallEvent: - def test_on_install_non_leader_unit( - self, harness: Harness, mocker: MockerFixture - ) -> None: + def test_on_install_non_leader_unit(self, harness: Harness, mocker: MockerFixture) -> None: mocked = mocker.patch("charm.ConfigMapResource.create") harness.set_leader(False) @@ -25,11 +25,18 @@ def test_on_install(self, harness: Harness, mocker: MockerFixture) -> None: mocked.assert_called_once() + def test_configmap_creation_failed(self, harness: Harness, mocker: MockerFixture) -> None: + mocked = mocker.patch("charm.ConfigMapResource.create") + mocked.side_effect = KubernetesResourceError("Some reason.") + + with pytest.raises(KubernetesResourceError): + harness.charm.on.install.emit() + + assert isinstance(harness.model.unit.status, MaintenanceStatus) + class TestRemoveEvent: - def test_on_remove_non_leader_unit( - self, harness: Harness, mocker: MockerFixture - ) -> None: + def test_on_remove_non_leader_unit(self, harness: Harness, mocker: MockerFixture) -> None: mocked = mocker.patch("charm.ConfigMapResource.delete") harness.set_leader(False) @@ -50,7 +57,7 @@ def test_when_container_not_connected(self, harness: Harness) -> None: container = harness.model.unit.get_container(WORKLOAD_CONTAINER) harness.charm.on.glauth_pebble_ready.emit(container) - assert isinstance(harness.charm.unit.status, WaitingStatus) + assert isinstance(harness.model.unit.status, WaitingStatus) def test_when_missing_database_relation(self, harness: Harness) -> None: container = harness.model.unit.get_container(WORKLOAD_CONTAINER) @@ -58,9 +65,7 @@ def test_when_missing_database_relation(self, harness: Harness) -> None: assert isinstance(harness.model.unit.status, BlockedStatus) - def test_when_database_not_created( - self, harness: Harness, database_relation: int - ) -> None: + def test_when_database_not_created(self, harness: Harness, database_relation: int) -> None: container = harness.model.unit.get_container(WORKLOAD_CONTAINER) harness.charm.on.glauth_pebble_ready.emit(container) @@ -74,7 +79,7 @@ def test_pebble_ready_event( harness.charm.on.glauth_pebble_ready.emit(container) - service = container.get_service(WORKLOAD_CONTAINER) + service = container.get_service(WORKLOAD_SERVICE) assert service.is_running() assert isinstance(harness.model.unit.status, ActiveStatus) @@ -85,7 +90,7 @@ def test_database_created_event( ) -> None: container = harness.model.unit.get_container(WORKLOAD_CONTAINER) - service = container.get_service(WORKLOAD_CONTAINER) + service = container.get_service(WORKLOAD_SERVICE) assert service.is_running() assert isinstance(harness.model.unit.status, ActiveStatus) @@ -102,9 +107,7 @@ def test_when_missing_database_relation(self, harness: Harness) -> None: assert isinstance(harness.model.unit.status, BlockedStatus) - def test_when_database_not_created( - self, harness: Harness, database_relation: int - ) -> None: + def test_when_database_not_created(self, harness: Harness, database_relation: int) -> None: harness.charm.on.config_changed.emit() assert isinstance(harness.model.unit.status, WaitingStatus) @@ -116,6 +119,6 @@ def test_on_config_changed_event( harness.charm.on.config_changed.emit() - service = container.get_service(WORKLOAD_CONTAINER) + service = container.get_service(WORKLOAD_SERVICE) assert service.is_running() assert isinstance(harness.model.unit.status, ActiveStatus) diff --git a/tests/unit/test_validators.py b/tests/unit/test_validators.py index 8d6999bf..a10ff38e 100644 --- a/tests/unit/test_validators.py +++ b/tests/unit/test_validators.py @@ -7,12 +7,12 @@ from ops.model import BlockedStatus, WaitingStatus from ops.testing import Harness -from constants import WORKLOAD_CONTAINER +from constants import DATABASE_INTEGRATION_NAME, WORKLOAD_CONTAINER from validators import ( leader_unit, validate_container_connectivity, - validate_database_relation, validate_database_resource, + validate_integration_exists, ) @@ -33,9 +33,7 @@ def wrapped(charm: CharmBase): assert wrapped(harness.charm) is None - def test_container_connected( - self, harness: Harness, mocked_hook_event: MagicMock - ) -> None: + def test_container_connected(self, harness: Harness, mocked_hook_event: MagicMock) -> None: @validate_container_connectivity def wrapped(charm: CharmBase, event: HookEvent): return sentinel @@ -44,9 +42,7 @@ def wrapped(charm: CharmBase, event: HookEvent): assert wrapped(harness.charm, mocked_hook_event) is sentinel - def test_container_not_connected( - self, harness: Harness, mocked_hook_event: MagicMock - ): + def test_container_not_connected(self, harness: Harness, mocked_hook_event: MagicMock): @validate_container_connectivity def wrapped(charm: CharmBase, event: HookEvent): return sentinel @@ -54,7 +50,7 @@ def wrapped(charm: CharmBase, event: HookEvent): harness.set_can_connect(WORKLOAD_CONTAINER, False) assert wrapped(harness.charm, mocked_hook_event) is None - assert isinstance(harness.charm.unit.status, WaitingStatus) + assert isinstance(harness.model.unit.status, WaitingStatus) def test_when_database_relation_integrated( self, @@ -62,7 +58,7 @@ def test_when_database_relation_integrated( database_relation: int, mocked_hook_event: MagicMock, ) -> None: - @validate_database_relation + @validate_integration_exists(DATABASE_INTEGRATION_NAME) def wrapped(charm: CharmBase, event: HookEvent): return sentinel @@ -71,12 +67,12 @@ def wrapped(charm: CharmBase, event: HookEvent): def test_when_database_relation_not_integrated( self, harness: Harness, mocked_hook_event: MagicMock ) -> None: - @validate_database_relation + @validate_integration_exists(DATABASE_INTEGRATION_NAME) def wrapped(charm: CharmBase, event: HookEvent): return sentinel assert wrapped(harness.charm, mocked_hook_event) is None - assert isinstance(harness.charm.unit.status, BlockedStatus) + assert isinstance(harness.model.unit.status, BlockedStatus) def test_database_resource_created( self, harness: Harness, database_resource, mocked_hook_event: MagicMock @@ -95,4 +91,4 @@ def wrapped(charm: CharmBase, event: HookEvent): return sentinel assert wrapped(harness.charm, mocked_hook_event) is None - assert isinstance(harness.charm.unit.status, WaitingStatus) + assert isinstance(harness.model.unit.status, WaitingStatus)