From c2c9633f6da8cf7487a1bf51eea9b46f56b4ec92 Mon Sep 17 00:00:00 2001 From: Franco Forneron Date: Wed, 26 Jul 2023 11:07:28 -0300 Subject: [PATCH 1/8] test commit --- indico_rock/rockcraft.yaml | 4 ++ metadata.yaml | 21 ------ pyproject.toml | 2 +- src/charm.py | 113 +++++--------------------------- tests/integration/conftest.py | 20 ------ tests/integration/test_charm.py | 35 +--------- tests/unit/test_actions.py | 7 -- tests/unit/test_core.py | 15 ----- 8 files changed, 22 insertions(+), 195 deletions(-) diff --git a/indico_rock/rockcraft.yaml b/indico_rock/rockcraft.yaml index 1a12cdbb..afa5acfe 100644 --- a/indico_rock/rockcraft.yaml +++ b/indico_rock/rockcraft.yaml @@ -44,6 +44,10 @@ parts: - texlive-fonts-recommended - texlive-plain-generic - texlive-xetex + stage-snaps: + - celery-prometheus-exporter/latest/edge + - nginx-prometheus-exporter/latest/edge + - gtrkiller-statsd-prometheus-exporter/latest/edge source: plugins plugin: nil override-build: | diff --git a/metadata.yaml b/metadata.yaml index b72d6edd..c3a5d531 100644 --- a/metadata.yaml +++ b/metadata.yaml @@ -38,12 +38,6 @@ containers: resource: indico-nginx-image indico-celery: resource: indico-image - celery-prometheus-exporter: - resource: celery-prometheus-exporter-image - nginx-prometheus-exporter: - resource: nginx-prometheus-exporter-image - statsd-prometheus-exporter: - resource: statsd-prometheus-exporter-image resources: indico-image: @@ -54,21 +48,6 @@ resources: type: oci-image description: Docker image for nginx Indico auto-fetch: true - celery-prometheus-exporter-image: - type: oci-image - description: Prometheus exporter for celery - auto-fetch: true - upstream-source: danihodovic/celery-exporter:0.7.6 - nginx-prometheus-exporter-image: - type: oci-image - description: Prometheus exporter for nginx - auto-fetch: true - upstream-source: nginx/nginx-prometheus-exporter:0.11.0 - statsd-prometheus-exporter-image: - type: oci-image - description: Prometheus exporter for statsd - auto-fetch: true - upstream-source: prom/statsd-exporter:v0.22.8 provides: grafana-dashboard: diff --git a/pyproject.toml b/pyproject.toml index bd5175d6..fb9e1790 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -54,4 +54,4 @@ markers = [ ] [tool.pylint] -disable = "wrong-import-order" +disable = "wrong-import-order, duplicate-code" diff --git a/src/charm.py b/src/charm.py index 04da907f..af94d7ca 100755 --- a/src/charm.py +++ b/src/charm.py @@ -70,15 +70,6 @@ def __init__(self, *args): self.framework.observe(self.on.indico_pebble_ready, self._on_pebble_ready) self.framework.observe(self.on.indico_celery_pebble_ready, self._on_pebble_ready) self.framework.observe(self.on.indico_nginx_pebble_ready, self._on_pebble_ready) - self.framework.observe( - self.on.nginx_prometheus_exporter_pebble_ready, self._on_pebble_ready - ) - self.framework.observe( - self.on.statsd_prometheus_exporter_pebble_ready, self._on_pebble_ready - ) - self.framework.observe( - self.on.celery_prometheus_exporter_pebble_ready, self._on_pebble_ready - ) self.framework.observe( self.on.refresh_external_resources_action, self._refresh_external_resources_action ) @@ -308,7 +299,22 @@ def _get_indico_pebble_config(self, container: Container) -> Dict: "override": "replace", "level": "ready", "tcp": {"port": 8081}, - } + }, + "celery-exporter-up": { + "override": "replace", + "level": "alive", + "http": {"url": "http://localhost:9808/health"}, + }, + "nginx-exporter-up": { + "override": "replace", + "level": "alive", + "http": {"url": "http://localhost:9113/metrics"}, + }, + "statsd-exporter-up": { + "override": "replace", + "level": "alive", + "http": {"url": "http://localhost:9102/metrics"}, + }, }, } @@ -375,93 +381,6 @@ def _get_indico_nginx_pebble_config(self, _) -> Dict: }, } - def _get_celery_prometheus_exporter_pebble_config(self, _) -> Dict: - """Generate pebble config for the celery-prometheus-exporter container. - - Returns: - The pebble configuration for the container. - """ - return { - "summary": "Celery prometheus exporter", - "description": "Prometheus exporter for celery", - "services": { - "celery-exporter": { - "override": "replace", - "summary": "Celery Exporter", - "command": ( - "python" - " /app/cli.py" - f" --broker-url={self._get_celery_backend()}" - " --retry-interval=5" - ), - "environment": {"CE_ACCEPT_CONTENT": "json,pickle"}, - "startup": "enabled", - }, - }, - "checks": { - "celery-exporter-up": { - "override": "replace", - "level": "alive", - "http": {"url": "http://localhost:9808/health"}, - }, - }, - } - - def _get_nginx_prometheus_exporter_pebble_config(self, _) -> Dict: - """Generate pebble config for the nginx-prometheus-exporter container. - - Returns: - The pebble configuration for the container. - """ - return { - "summary": "Nginx prometheus exporter", - "description": "Prometheus exporter for nginx", - "services": { - "nginx-exporter": { - "override": "replace", - "summary": "Nginx Exporter", - "command": ( - "nginx-prometheus-exporter" - " -nginx.scrape-uri=http://localhost:9080/stub_status" - ), - "startup": "enabled", - }, - }, - "checks": { - "nginx-exporter-up": { - "override": "replace", - "level": "alive", - "http": {"url": "http://localhost:9113/metrics"}, - }, - }, - } - - def _get_statsd_prometheus_exporter_pebble_config(self, _) -> Dict: - """Generate pebble config for the statsd-prometheus-exporter container. - - Returns: - The pebble configuration for the container. - """ - return { - "summary": "Statsd prometheus exporter", - "description": "Prometheus exporter for statsd", - "services": { - "statsd-exporter": { - "override": "replace", - "summary": "Statsd Exporter", - "command": ("statsd_exporter"), - "startup": "enabled", - }, - }, - "checks": { - "statsd-exporter-up": { - "override": "replace", - "level": "alive", - "http": {"url": "http://localhost:9102/metrics"}, - }, - }, - } - def _get_redis_rel(self, name: str) -> Optional[Relation]: """Get Redis relation. diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 24c2be67..441b6bbd 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -5,7 +5,6 @@ import asyncio from pathlib import Path -from typing import Dict import pytest_asyncio import yaml @@ -44,23 +43,6 @@ def app_name_fixture(metadata): yield metadata["name"] -@fixture(scope="module", name="prometheus_exporter_images") -def prometheus_exporter_images_fixture(metadata): - """Provides Prometheus exporter images from the metadata.""" - prometheus_exporter_images = { - "nginx-prometheus-exporter-image": metadata["resources"][ - "nginx-prometheus-exporter-image" - ]["upstream-source"], - "statsd-prometheus-exporter-image": metadata["resources"][ - "statsd-prometheus-exporter-image" - ]["upstream-source"], - "celery-prometheus-exporter-image": metadata["resources"][ - "celery-prometheus-exporter-image" - ]["upstream-source"], - } - yield prometheus_exporter_images - - @fixture(scope="module") def requests_timeout(): """Provides a global default timeout for HTTP requests""" @@ -72,7 +54,6 @@ async def app( ops_test: OpsTest, app_name: str, pytestconfig: Config, - prometheus_exporter_images: Dict[str, str], ): """Indico charm used for integration testing. @@ -91,7 +72,6 @@ async def app( "indico-image": pytestconfig.getoption("--indico-image"), "indico-nginx-image": pytestconfig.getoption("--indico-nginx-image"), } - resources.update(prometheus_exporter_images) charm = pytestconfig.getoption("--charm-file") application = await ops_test.model.deploy( f"./{charm}", diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 98f1495e..a09fcae0 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -17,12 +17,7 @@ from ops.model import ActiveStatus, Application from pytest_operator.plugin import OpsTest -from charm import ( - CELERY_PROMEXP_PORT, - NGINX_PROMEXP_PORT, - STAGING_UBUNTU_SAML_URL, - STATSD_PROMEXP_PORT, -) +from charm import STAGING_UBUNTU_SAML_URL ADMIN_USER_EMAIL = "sample@email.com" ADMIN_USER_EMAIL_FAIL = "sample2@email.com" @@ -61,34 +56,6 @@ async def test_indico_is_up(ops_test: OpsTest, app: Application): assert response.status_code == 200 -@pytest.mark.asyncio -@pytest.mark.abort_on_fail -async def test_prom_exporters_are_up(app: Application): - """ - arrange: given charm in its initial state - act: when the metrics endpoints are scraped - assert: the response is 200 (HTTP OK) - """ - # Application actually does have units - indico_unit = app.units[0] # type: ignore - prometheus_targets = [ - f"localhost:{NGINX_PROMEXP_PORT}", - f"localhost:{STATSD_PROMEXP_PORT}", - f"localhost:{CELERY_PROMEXP_PORT}", - ] - # Send request to /metrics for each target and check the response - for target in prometheus_targets: - cmd = f"curl -m 10 http://{target}/metrics" - action = await indico_unit.run(cmd, timeout=15) - # Change this if upgrading Juju lib version to >= 3 - # See https://github.com/juju/python-libjuju/issues/707#issuecomment-1212296289 - result = action.data - code = result["results"].get("Code") - stdout = result["results"].get("Stdout") - stderr = result["results"].get("Stderr") - assert code == "0", f"{cmd} failed ({code}): {stderr or stdout}" - - @pytest.mark.asyncio @pytest.mark.abort_on_fail async def test_health_checks(app: Application): diff --git a/tests/unit/test_actions.py b/tests/unit/test_actions.py index 049f974c..0c679aac 100644 --- a/tests/unit/test_actions.py +++ b/tests/unit/test_actions.py @@ -34,7 +34,6 @@ def test_refresh_external_resources_when_customization_and_plugins_set(self, moc self.is_ready( [ - "nginx-prometheus-exporter", "indico", "indico-celery", "indico-nginx", @@ -75,9 +74,6 @@ def test_add_admin(self, mock_exec): self.is_ready( [ - "celery-prometheus-exporter", - "statsd-prometheus-exporter", - "nginx-prometheus-exporter", "indico", "indico-celery", "indico-nginx", @@ -182,9 +178,6 @@ def mock_exec_side_effect(*args, **kwargs): # pylint: disable=unused-argument self.is_ready( [ - "celery-prometheus-exporter", - "statsd-prometheus-exporter", - "nginx-prometheus-exporter", "indico", "indico-celery", "indico-nginx", diff --git a/tests/unit/test_core.py b/tests/unit/test_core.py index 1994aee2..2a82c8c2 100644 --- a/tests/unit/test_core.py +++ b/tests/unit/test_core.py @@ -74,12 +74,6 @@ def test_all_pebble_services_ready(self, mock_exec): self.set_up_all_relations() self.harness.set_leader(True) - self.harness.container_pebble_ready("celery-prometheus-exporter") - self.assertEqual(self.harness.model.unit.status, WaitingStatus("Waiting for pebble")) - self.harness.container_pebble_ready("statsd-prometheus-exporter") - self.assertEqual(self.harness.model.unit.status, WaitingStatus("Waiting for pebble")) - self.harness.container_pebble_ready("nginx-prometheus-exporter") - self.assertEqual(self.harness.model.unit.status, WaitingStatus("Waiting for pebble")) self.harness.container_pebble_ready("indico") self.assertEqual(self.harness.model.unit.status, WaitingStatus("Waiting for pebble")) self.harness.container_pebble_ready("indico-celery") @@ -247,9 +241,6 @@ def test_config_changed(self, mock_exec): # pylint: disable=R0915 # pylint: disable=duplicate-code self.is_ready( [ - "celery-prometheus-exporter", - "statsd-prometheus-exporter", - "nginx-prometheus-exporter", "indico", "indico-celery", "indico-nginx", @@ -389,9 +380,6 @@ def test_config_changed_when_config_invalid(self, mock_exec): # pylint: disable=duplicate-code self.is_ready( [ - "celery-prometheus-exporter", - "statsd-prometheus-exporter", - "nginx-prometheus-exporter", "indico", "indico-celery", "indico-nginx", @@ -418,9 +406,6 @@ def test_config_changed_with_external_resources(self, mock_exec): # pylint: disable=duplicate-code self.is_ready( [ - "celery-prometheus-exporter", - "statsd-prometheus-exporter", - "nginx-prometheus-exporter", "indico", "indico-celery", "indico-nginx", From 6536fe4a6c33afbf329e44a340e012c12708331d Mon Sep 17 00:00:00 2001 From: Franco Forneron Date: Mon, 31 Jul 2023 07:27:27 -0300 Subject: [PATCH 2/8] execute tests with terminal --- .github/workflows/integration_test.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/integration_test.yaml b/.github/workflows/integration_test.yaml index a220c82d..b57e0831 100644 --- a/.github/workflows/integration_test.yaml +++ b/.github/workflows/integration_test.yaml @@ -8,6 +8,7 @@ jobs: uses: canonical/operator-workflows/.github/workflows/integration_test.yaml@main secrets: inherit with: + tmate-debug: true chaos-app-label: app.kubernetes.io/name=indico chaos-enabled: true chaos-experiments: pod-delete From 69bc5d7dc8142a724d4bc798971558e5ae9bd3cf Mon Sep 17 00:00:00 2001 From: Franco Forneron Date: Mon, 31 Jul 2023 15:40:29 -0300 Subject: [PATCH 3/8] change snap, address nginx exporter race condition --- indico_rock/rockcraft.yaml | 2 +- install.sh | 11 ++++ src/charm.py | 120 ++++++++++++++++++++++++++++++++----- tests/unit/test_core.py | 4 +- 4 files changed, 120 insertions(+), 17 deletions(-) create mode 100755 install.sh diff --git a/indico_rock/rockcraft.yaml b/indico_rock/rockcraft.yaml index afa5acfe..aac58e7e 100644 --- a/indico_rock/rockcraft.yaml +++ b/indico_rock/rockcraft.yaml @@ -46,7 +46,7 @@ parts: - texlive-xetex stage-snaps: - celery-prometheus-exporter/latest/edge - - nginx-prometheus-exporter/latest/edge + - gtrkiller-nginx-prometheus-exporter/latest/edge - gtrkiller-statsd-prometheus-exporter/latest/edge source: plugins plugin: nil diff --git a/install.sh b/install.sh new file mode 100755 index 00000000..150ffb9b --- /dev/null +++ b/install.sh @@ -0,0 +1,11 @@ +juju destroy-model indicodepl --force -y --destroy-storage +charmcraft pack +juju add-model indicodepl +juju deploy ./indico_ubuntu-20.04-amd64.charm --resource indico-image=localhost:32000/indico3:latest --resource indico-nginx-image=localhost:32000/indico-nginx:latest +juju deploy postgresql-k8s --channel=latest/stable --series=focal +juju deploy redis-k8s redis-broker +juju deploy redis-k8s redis-cache +juju relate redis-broker indico +juju relate redis-cache indico +juju relate indico postgresql-k8s:db +juju debug-log --replay diff --git a/src/charm.py b/src/charm.py index af94d7ca..6b3e34e1 100755 --- a/src/charm.py +++ b/src/charm.py @@ -239,6 +239,15 @@ def _on_pebble_ready(self, event: PebbleReadyEvent) -> None: event.defer() return self._config_pebble(event.workload) + if event.workload.name == "indico-nginx": + self._config_nginx_exporter() + + def _config_nginx_exporter(self): + """Configure NGINX exporter to avoid race conditions with NGINX.""" + container = self.unit.get_container("indico") + pebble_config = self._get_nginx_prometheus_exporter_pebble_config(container) + container.add_layer("nginx", pebble_config, combine=True) + container.pebble.replan_services() def _config_pebble(self, container: Container) -> None: """Apply pebble configurations to a container. @@ -262,6 +271,12 @@ def _config_pebble(self, container: Container) -> None: pebble_config = pebble_config_func(container) container.add_layer(container.name, pebble_config, combine=True) if container.name == "indico": + for item in ["statsd", "celery"]: + pebble_config_func = getattr( + self, f"_get_{item}_prometheus_exporter_pebble_config" + ) + pebble_config = pebble_config_func(container) + container.add_layer(item, pebble_config, combine=True) self._download_customization_changes(container) self.unit.status = MaintenanceStatus(f"Starting {container.name} container") container.pebble.replan_services() @@ -300,21 +315,6 @@ def _get_indico_pebble_config(self, container: Container) -> Dict: "level": "ready", "tcp": {"port": 8081}, }, - "celery-exporter-up": { - "override": "replace", - "level": "alive", - "http": {"url": "http://localhost:9808/health"}, - }, - "nginx-exporter-up": { - "override": "replace", - "level": "alive", - "http": {"url": "http://localhost:9113/metrics"}, - }, - "statsd-exporter-up": { - "override": "replace", - "level": "alive", - "http": {"url": "http://localhost:9102/metrics"}, - }, }, } @@ -381,6 +381,96 @@ def _get_indico_nginx_pebble_config(self, _) -> Dict: }, } + def _get_celery_prometheus_exporter_pebble_config(self, container) -> Dict: + """Generate pebble config for the celery-prometheus-exporter container. + + Args: + container: Celery container that has the target configuration. + + Returns: + The pebble configuration for the container. + """ + indico_env_config = self._get_indico_env_config(container) + return { + "summary": "Celery prometheus exporter", + "description": "Prometheus exporter for celery", + "services": { + "celery-exporter": { + "override": "replace", + "summary": "Celery Exporter", + "command": ( + "celery-exporter" + f" --broker-url={self._get_celery_backend()}" + " --retry-interval=5" + ), + "environment": indico_env_config, + "startup": "enabled", + }, + }, + "checks": { + "celery-exporter-up": { + "override": "replace", + "level": "alive", + "http": {"url": "http://localhost:9808/health"}, + }, + }, + } + + def _get_nginx_prometheus_exporter_pebble_config(self, _) -> Dict: + """Generate pebble config for the nginx-prometheus-exporter container. + + Returns: + The pebble configuration for the container. + """ + return { + "summary": "Nginx prometheus exporter", + "description": "Prometheus exporter for nginx", + "services": { + "nginx-prometheus-exporter": { + "override": "replace", + "summary": "Nginx Exporter", + "command": ( + "nginx-prometheus-exporter" + " -nginx.scrape-uri=http://localhost:9080/stub_status" + ), + "startup": "enabled", + }, + }, + "checks": { + "nginx-exporter-up": { + "override": "replace", + "level": "alive", + "http": {"url": "http://localhost:9113/metrics"}, + }, + }, + } + + def _get_statsd_prometheus_exporter_pebble_config(self, _) -> Dict: + """Generate pebble config for the statsd-prometheus-exporter container. + + Returns: + The pebble configuration for the container. + """ + return { + "summary": "Statsd prometheus exporter", + "description": "Prometheus exporter for statsd", + "services": { + "statsd-exporter": { + "override": "replace", + "summary": "Statsd Exporter", + "command": ("statsd_exporter"), + "startup": "enabled", + }, + }, + "checks": { + "statsd-exporter-up": { + "override": "replace", + "level": "alive", + "http": {"url": "http://localhost:9102/metrics"}, + }, + }, + } + def _get_redis_rel(self, name: str) -> Optional[Relation]: """Get Redis relation. diff --git a/tests/unit/test_core.py b/tests/unit/test_core.py index 2a82c8c2..889566d2 100644 --- a/tests/unit/test_core.py +++ b/tests/unit/test_core.py @@ -11,6 +11,7 @@ from ops.jujuversion import JujuVersion from ops.model import ActiveStatus, BlockedStatus, Container, WaitingStatus +from charm import IndicoOperatorCharm from tests.unit.test_base import TestBase @@ -44,8 +45,9 @@ def test_missing_relations(self): self.harness.model.unit.status, WaitingStatus("Waiting for database availability") ) + @patch.object(IndicoOperatorCharm, "_config_nginx_exporter") @patch.object(Container, "exec") - def test_indico_nginx_pebble_ready(self, mock_exec): + def test_indico_nginx_pebble_ready(self, mock_exec, _): """ arrange: charm created act: trigger container pebble ready event for nginx container From 301dad9f5dc16d42f2fc389a79557caa3143cf37 Mon Sep 17 00:00:00 2001 From: Franco Forneron Date: Mon, 31 Jul 2023 15:42:13 -0300 Subject: [PATCH 4/8] change snap, address nginx exporter race condition --- install.sh | 11 ----------- 1 file changed, 11 deletions(-) delete mode 100755 install.sh diff --git a/install.sh b/install.sh deleted file mode 100755 index 150ffb9b..00000000 --- a/install.sh +++ /dev/null @@ -1,11 +0,0 @@ -juju destroy-model indicodepl --force -y --destroy-storage -charmcraft pack -juju add-model indicodepl -juju deploy ./indico_ubuntu-20.04-amd64.charm --resource indico-image=localhost:32000/indico3:latest --resource indico-nginx-image=localhost:32000/indico-nginx:latest -juju deploy postgresql-k8s --channel=latest/stable --series=focal -juju deploy redis-k8s redis-broker -juju deploy redis-k8s redis-cache -juju relate redis-broker indico -juju relate redis-cache indico -juju relate indico postgresql-k8s:db -juju debug-log --replay From 972c44b28ee56e45d2f8a49a55bd9907af3f83b7 Mon Sep 17 00:00:00 2001 From: Franco Forneron Date: Thu, 3 Aug 2023 16:48:57 -0300 Subject: [PATCH 5/8] add layers --- indico_rock/rockcraft.yaml | 1 - nginx_rock/rockcraft.yaml | 2 ++ src/charm.py | 12 +++------- tests/integration/test_charm.py | 39 ++++++++++++++++++++++++++++++--- tests/unit/test_core.py | 4 +--- 5 files changed, 42 insertions(+), 16 deletions(-) diff --git a/indico_rock/rockcraft.yaml b/indico_rock/rockcraft.yaml index aac58e7e..dc48c5d6 100644 --- a/indico_rock/rockcraft.yaml +++ b/indico_rock/rockcraft.yaml @@ -46,7 +46,6 @@ parts: - texlive-xetex stage-snaps: - celery-prometheus-exporter/latest/edge - - gtrkiller-nginx-prometheus-exporter/latest/edge - gtrkiller-statsd-prometheus-exporter/latest/edge source: plugins plugin: nil diff --git a/nginx_rock/rockcraft.yaml b/nginx_rock/rockcraft.yaml index a242a5fd..79fb1309 100644 --- a/nginx_rock/rockcraft.yaml +++ b/nginx_rock/rockcraft.yaml @@ -26,6 +26,8 @@ parts: nginx: stage-packages: - nginx + stage-snaps: + - gtrkiller-nginx-prometheus-exporter/latest/edge plugin: nil override-build: | craftctl default diff --git a/src/charm.py b/src/charm.py index 6b3e34e1..105a4cfe 100755 --- a/src/charm.py +++ b/src/charm.py @@ -239,15 +239,6 @@ def _on_pebble_ready(self, event: PebbleReadyEvent) -> None: event.defer() return self._config_pebble(event.workload) - if event.workload.name == "indico-nginx": - self._config_nginx_exporter() - - def _config_nginx_exporter(self): - """Configure NGINX exporter to avoid race conditions with NGINX.""" - container = self.unit.get_container("indico") - pebble_config = self._get_nginx_prometheus_exporter_pebble_config(container) - container.add_layer("nginx", pebble_config, combine=True) - container.pebble.replan_services() def _config_pebble(self, container: Container) -> None: """Apply pebble configurations to a container. @@ -278,6 +269,9 @@ def _config_pebble(self, container: Container) -> None: pebble_config = pebble_config_func(container) container.add_layer(item, pebble_config, combine=True) self._download_customization_changes(container) + if container.name == "indico-nginx": + pebble_config = self._get_nginx_prometheus_exporter_pebble_config(container) + container.add_layer("nginx", pebble_config, combine=True) self.unit.status = MaintenanceStatus(f"Starting {container.name} container") container.pebble.replan_services() if self._are_pebble_instances_ready(): diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index a09fcae0..9ad9f7fb 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -17,7 +17,12 @@ from ops.model import ActiveStatus, Application from pytest_operator.plugin import OpsTest -from charm import STAGING_UBUNTU_SAML_URL +from charm import ( + CELERY_PROMEXP_PORT, + NGINX_PROMEXP_PORT, + STAGING_UBUNTU_SAML_URL, + STATSD_PROMEXP_PORT, +) ADMIN_USER_EMAIL = "sample@email.com" ADMIN_USER_EMAIL_FAIL = "sample2@email.com" @@ -63,7 +68,7 @@ async def test_health_checks(app: Application): Assume that the charm has already been built and is running. """ - container_list = ["indico", "indico-nginx", "indico-celery"] + container_list = ["indico-celery", "indico-nginx", "indico"] # Application actually does have units indico_unit = app.units[0] # type: ignore for container in container_list: @@ -79,7 +84,7 @@ async def test_health_checks(app: Application): # When executing the checks, `0/3` means there are 0 errors of 3. # Each check has it's own `0/3`, so we will count `n` times, # where `n` is the number of checks for that container. - assert stdout.count("0/3") == 1 + assert stdout.count("0/3") == container_list.index(container) + 1 @pytest.mark.abort_on_fail @@ -116,6 +121,34 @@ async def add_admin(app: Application): assert f'Admin with email "{email_fail}" correctly created' in action2.results["output"] +@pytest.mark.asyncio +@pytest.mark.abort_on_fail +async def test_prom_exporters_are_up(app: Application): + """ + arrange: given charm in its initial state + act: when the metrics endpoints are scraped + assert: the response is 200 (HTTP OK) + """ + # Application actually does have units + indico_unit = app.units[0] # type: ignore + prometheus_targets = [ + f"localhost:{NGINX_PROMEXP_PORT}", + f"localhost:{STATSD_PROMEXP_PORT}", + f"localhost:{CELERY_PROMEXP_PORT}", + ] + # Send request to /metrics for each target and check the response + for target in prometheus_targets: + cmd = f"curl -m 10 http://{target}/metrics" + action = await indico_unit.run(cmd, timeout=15) + # Change this if upgrading Juju lib version to >= 3 + # See https://github.com/juju/python-libjuju/issues/707#issuecomment-1212296289 + result = action.data + code = result["results"].get("Code") + stdout = result["results"].get("Stdout") + stderr = result["results"].get("Stderr") + assert code == "0", f"{cmd} failed ({code}): {stderr or stdout}" + + @pytest.mark.asyncio @pytest.mark.abort_on_fail @pytest.mark.usefixtures("add_admin") diff --git a/tests/unit/test_core.py b/tests/unit/test_core.py index 889566d2..2a82c8c2 100644 --- a/tests/unit/test_core.py +++ b/tests/unit/test_core.py @@ -11,7 +11,6 @@ from ops.jujuversion import JujuVersion from ops.model import ActiveStatus, BlockedStatus, Container, WaitingStatus -from charm import IndicoOperatorCharm from tests.unit.test_base import TestBase @@ -45,9 +44,8 @@ def test_missing_relations(self): self.harness.model.unit.status, WaitingStatus("Waiting for database availability") ) - @patch.object(IndicoOperatorCharm, "_config_nginx_exporter") @patch.object(Container, "exec") - def test_indico_nginx_pebble_ready(self, mock_exec, _): + def test_indico_nginx_pebble_ready(self, mock_exec): """ arrange: charm created act: trigger container pebble ready event for nginx container From fb1120f62d74c964c54a4bfd6b3487bfa64e5bf8 Mon Sep 17 00:00:00 2001 From: Franco Forneron Date: Thu, 3 Aug 2023 16:50:53 -0300 Subject: [PATCH 6/8] remove debug terminal --- .github/workflows/integration_test.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/integration_test.yaml b/.github/workflows/integration_test.yaml index b57e0831..a220c82d 100644 --- a/.github/workflows/integration_test.yaml +++ b/.github/workflows/integration_test.yaml @@ -8,7 +8,6 @@ jobs: uses: canonical/operator-workflows/.github/workflows/integration_test.yaml@main secrets: inherit with: - tmate-debug: true chaos-app-label: app.kubernetes.io/name=indico chaos-enabled: true chaos-experiments: pod-delete From 10b07e3df4f88aab15bbd923854442beec98f6b0 Mon Sep 17 00:00:00 2001 From: Franco Forneron Date: Mon, 7 Aug 2023 17:01:17 -0300 Subject: [PATCH 7/8] addressing comments --- pyproject.toml | 2 +- requirements.txt | 2 +- src/charm.py | 10 ++++------ tests/unit/test_core.py | 1 + 4 files changed, 7 insertions(+), 8 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index fb9e1790..bd5175d6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -54,4 +54,4 @@ markers = [ ] [tool.pylint] -disable = "wrong-import-order, duplicate-code" +disable = "wrong-import-order" diff --git a/requirements.txt b/requirements.txt index 5b7992f6..b5611a20 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,2 +1,2 @@ -ops>=2.0.0 +ops<=2.3.0 ops-lib-pgsql diff --git a/src/charm.py b/src/charm.py index 105a4cfe..c6440354 100755 --- a/src/charm.py +++ b/src/charm.py @@ -262,12 +262,10 @@ def _config_pebble(self, container: Container) -> None: pebble_config = pebble_config_func(container) container.add_layer(container.name, pebble_config, combine=True) if container.name == "indico": - for item in ["statsd", "celery"]: - pebble_config_func = getattr( - self, f"_get_{item}_prometheus_exporter_pebble_config" - ) - pebble_config = pebble_config_func(container) - container.add_layer(item, pebble_config, combine=True) + celery_config = self._get_celery_prometheus_exporter_pebble_config(container) + statsd_config = self._get_statsd_prometheus_exporter_pebble_config(container) + container.add_layer("celery", celery_config, combine=True) + container.add_layer("statsd", statsd_config, combine=True) self._download_customization_changes(container) if container.name == "indico-nginx": pebble_config = self._get_nginx_prometheus_exporter_pebble_config(container) diff --git a/tests/unit/test_core.py b/tests/unit/test_core.py index 2a82c8c2..d32d1d37 100644 --- a/tests/unit/test_core.py +++ b/tests/unit/test_core.py @@ -451,6 +451,7 @@ def test_config_changed_when_saml_target_url_invalid(self, mock_exec): self.set_up_all_relations() self.harness.set_leader(True) + # pylint: disable=duplicate-code self.is_ready( [ "indico", From 256a4a85bd985539969287263271cd32f7f99ad0 Mon Sep 17 00:00:00 2001 From: Franco Forneron Date: Wed, 9 Aug 2023 11:38:50 -0300 Subject: [PATCH 8/8] create fixture --- tests/unit/test_actions.py | 33 +++---------------------- tests/unit/test_base.py | 12 +++++++++ tests/unit/test_core.py | 50 ++++---------------------------------- 3 files changed, 20 insertions(+), 75 deletions(-) diff --git a/tests/unit/test_actions.py b/tests/unit/test_actions.py index 0c679aac..59b357d7 100644 --- a/tests/unit/test_actions.py +++ b/tests/unit/test_actions.py @@ -29,16 +29,7 @@ def test_refresh_external_resources_when_customization_and_plugins_set(self, moc """ mock_exec.return_value = MagicMock(wait_output=MagicMock(return_value=("", None))) self.harness.disable_hooks() - self.set_up_all_relations() - self.harness.set_leader(True) - - self.is_ready( - [ - "indico", - "indico-celery", - "indico-nginx", - ] - ) + self.set_relations_and_leader() self.harness.update_config( { "customization_sources_url": "https://example.com/custom", @@ -69,16 +60,7 @@ def test_add_admin(self, mock_exec): """ mock_exec.return_value = MagicMock(wait_output=MagicMock(return_value=("", None))) - self.set_up_all_relations() - self.harness.set_leader(True) - - self.is_ready( - [ - "indico", - "indico-celery", - "indico-nginx", - ] - ) + self.set_relations_and_leader() self.harness.disable_hooks() container = self.harness.model.unit.get_container("indico") @@ -173,16 +155,7 @@ def mock_exec_side_effect(*args, **kwargs): # pylint: disable=unused-argument wait_output=mock_wo, ) - self.set_up_all_relations() - self.harness.set_leader(True) - - self.is_ready( - [ - "indico", - "indico-celery", - "indico-nginx", - ] - ) + self.set_relations_and_leader() self.harness.disable_hooks() container = self.harness.model.unit.get_container("indico") diff --git a/tests/unit/test_base.py b/tests/unit/test_base.py index 5fffc50d..7d6c7bda 100644 --- a/tests/unit/test_base.py +++ b/tests/unit/test_base.py @@ -59,3 +59,15 @@ def is_ready(self, apps: List[str]): """ for app_name in apps: self.harness.container_pebble_ready(app_name) + + def set_relations_and_leader(self): + """Set Indico relations, the leader and check container readiness.""" + self.set_up_all_relations() + self.harness.set_leader(True) + self.is_ready( + [ + "indico", + "indico-celery", + "indico-nginx", + ] + ) diff --git a/tests/unit/test_core.py b/tests/unit/test_core.py index d32d1d37..fe8656a2 100644 --- a/tests/unit/test_core.py +++ b/tests/unit/test_core.py @@ -235,17 +235,7 @@ def test_config_changed(self, mock_exec): # pylint: disable=R0915 """ mock_exec.return_value = MagicMock(wait_output=MagicMock(return_value=("", None))) - self.set_up_all_relations() - self.harness.set_leader(True) - - # pylint: disable=duplicate-code - self.is_ready( - [ - "indico", - "indico-celery", - "indico-nginx", - ] - ) + self.set_relations_and_leader() self.harness.update_config( { "customization_debug": True, @@ -374,17 +364,7 @@ def test_config_changed_when_config_invalid(self, mock_exec): """ mock_exec.return_value = MagicMock(wait_output=MagicMock(return_value=("", None))) - self.set_up_all_relations() - self.harness.set_leader(True) - - # pylint: disable=duplicate-code - self.is_ready( - [ - "indico", - "indico-celery", - "indico-nginx", - ] - ) + self.set_relations_and_leader() self.harness.update_config({"site_url": "example.local"}) self.assertEqual( self.harness.model.unit.status, @@ -400,24 +380,14 @@ def test_config_changed_with_external_resources(self, mock_exec): """ mock_exec.return_value = MagicMock(wait_output=MagicMock(return_value=("", None))) - self.set_up_all_relations() - self.harness.set_leader(True) - - # pylint: disable=duplicate-code - self.is_ready( - [ - "indico", - "indico-celery", - "indico-nginx", - ] - ) + self.set_relations_and_leader() self.harness.update_config( { "customization_sources_url": "https://example.com/custom", "external_plugins": "git+https://example.git/#subdirectory=themes_cern", } ) - + # pylint: disable=duplicate-code mock_exec.assert_any_call( ["git", "clone", "https://example.com/custom", "."], working_dir="/srv/indico/custom", @@ -448,17 +418,7 @@ def test_config_changed_when_saml_target_url_invalid(self, mock_exec): """ mock_exec.return_value = MagicMock(wait_output=MagicMock(return_value=("", None))) - self.set_up_all_relations() - self.harness.set_leader(True) - - # pylint: disable=duplicate-code - self.is_ready( - [ - "indico", - "indico-celery", - "indico-nginx", - ] - ) + self.set_relations_and_leader() self.harness.update_config({"saml_target_url": "sample.com/saml"}) self.assertEqual(