From 38f5d5834d610e011b3608fe8cc34b927775204a Mon Sep 17 00:00:00 2001 From: Patricia Reinoso Date: Wed, 28 Jun 2023 16:10:23 +0000 Subject: [PATCH] Integrate NBI and Prometheus Remove relation between NBI and Prometheus Integration is done with a config parameter prometheus-url This parameter does not have default value. If this parameter is not set the charm is in blocked status. Change-Id: I312304b7c75835d0b1de5a59b21edc7f478666e9 Signed-off-by: Patricia Reinoso --- installers/charm/osm-nbi/config.yaml | 3 + installers/charm/osm-nbi/metadata.yaml | 3 - installers/charm/osm-nbi/src/charm.py | 25 ++++--- .../charm/osm-nbi/src/legacy_interfaces.py | 62 ----------------- .../osm-nbi/tests/integration/test_charm.py | 67 +++++++++---------- .../charm/osm-nbi/tests/unit/test_charm.py | 54 ++++++++++++--- 6 files changed, 97 insertions(+), 117 deletions(-) diff --git a/installers/charm/osm-nbi/config.yaml b/installers/charm/osm-nbi/config.yaml index 85e637a7..961cfff2 100644 --- a/installers/charm/osm-nbi/config.yaml +++ b/installers/charm/osm-nbi/config.yaml @@ -66,6 +66,9 @@ options: tls-secret-name: description: TLS secret name to use for ingress. type: string + prometheus-url: + description: Prometheus URL + type: string # Debug-mode options debug-mode: diff --git a/installers/charm/osm-nbi/metadata.yaml b/installers/charm/osm-nbi/metadata.yaml index 8a336c8e..b7902e0c 100644 --- a/installers/charm/osm-nbi/metadata.yaml +++ b/installers/charm/osm-nbi/metadata.yaml @@ -67,9 +67,6 @@ requires: keystone: interface: keystone limit: 1 - prometheus: - interface: prometheus - limit: 1 ingress: interface: ingress limit: 1 diff --git a/installers/charm/osm-nbi/src/charm.py b/installers/charm/osm-nbi/src/charm.py index 8855de27..00ef4259 100755 --- a/installers/charm/osm-nbi/src/charm.py +++ b/installers/charm/osm-nbi/src/charm.py @@ -29,6 +29,7 @@ See more: https://charmhub.io/osm import logging from typing import Any, Dict +from urllib.parse import urlparse from charms.data_platform_libs.v0.data_interfaces import DatabaseRequires from charms.kafka_k8s.v0.kafka import KafkaEvents, KafkaRequires @@ -48,7 +49,7 @@ from ops.framework import StoredState from ops.main import main from ops.model import ActiveStatus, Container -from legacy_interfaces import KeystoneClient, PrometheusClient +from legacy_interfaces import KeystoneClient HOSTPATHS = [ HostPath( @@ -86,7 +87,6 @@ class OsmNbiCharm(CharmBase): self.mongodb_client = DatabaseRequires( self, "mongodb", database_name="osm", extra_user_roles="admin" ) - self.prometheus_client = PrometheusClient(self, "prometheus") self.keystone_client = KeystoneClient(self, "keystone") self._observe_charm_events() self.container: Container = self.unit.get_container("nbi") @@ -129,6 +129,7 @@ class OsmNbiCharm(CharmBase): def _on_update_status(self, _=None) -> None: """Handler for the update-status event.""" try: + self._validate_config() self._check_relations() if self.debug_mode.started: return @@ -184,13 +185,12 @@ class OsmNbiCharm(CharmBase): self.on["kafka"].relation_broken: self._on_required_relation_broken, self.mongodb_client.on.database_created: self._on_config_changed, self.on["mongodb"].relation_broken: self._on_required_relation_broken, + self.on["keystone"].relation_changed: self._on_config_changed, + self.on["keystone"].relation_broken: self._on_required_relation_broken, # Action events self.on.get_debug_mode_information_action: self._on_get_debug_mode_information_action, self.on.nbi_relation_joined: self._update_nbi_relation, } - for relation in [self.on[rel_name] for rel_name in ["prometheus", "keystone"]]: - event_handler_mapping[relation.relation_changed] = self._on_config_changed - event_handler_mapping[relation.relation_broken] = self._on_required_relation_broken for event, handler in event_handler_mapping.items(): self.framework.observe(event, handler) @@ -208,6 +208,14 @@ class OsmNbiCharm(CharmBase): CharmError: if charm configuration is invalid. """ logger.debug("validating charm config") + url = self.config.get("prometheus-url") + if not url: + raise CharmError("need prometheus-url config") + if not self._is_valid_url(url): + raise CharmError(f"Invalid value for prometheus-url config: '{url}'") + + def _is_valid_url(self, url) -> bool: + return urlparse(url).hostname is not None def _check_relations(self) -> None: """Validate charm relations. @@ -222,8 +230,6 @@ class OsmNbiCharm(CharmBase): missing_relations.append("kafka") if not self._is_database_available(): missing_relations.append("mongodb") - if self.prometheus_client.is_missing_data_in_app(): - missing_relations.append("prometheus") if self.keystone_client.is_missing_data_in_app(): missing_relations.append("keystone") @@ -253,6 +259,7 @@ class OsmNbiCharm(CharmBase): def _get_layer(self) -> Dict[str, Any]: """Get layer for Pebble.""" + prometheus = urlparse(self.config["prometheus-url"]) return { "summary": "nbi layer", "description": "pebble config layer for nbi", @@ -282,8 +289,8 @@ class OsmNbiCharm(CharmBase): "OSMNBI_STORAGE_COLLECTION": "files", "OSMNBI_STORAGE_URI": self._get_mongodb_uri(), # Prometheus configuration - "OSMNBI_PROMETHEUS_HOST": self.prometheus_client.hostname, - "OSMNBI_PROMETHEUS_PORT": self.prometheus_client.port, + "OSMNBI_PROMETHEUS_HOST": prometheus.hostname, + "OSMNBI_PROMETHEUS_PORT": prometheus.port if prometheus.port else "", # Log configuration "OSMNBI_LOG_LEVEL": self.config["log-level"], # Authentication environments diff --git a/installers/charm/osm-nbi/src/legacy_interfaces.py b/installers/charm/osm-nbi/src/legacy_interfaces.py index 5deb3f5f..4750cffc 100644 --- a/installers/charm/osm-nbi/src/legacy_interfaces.py +++ b/installers/charm/osm-nbi/src/legacy_interfaces.py @@ -141,65 +141,3 @@ class KeystoneClient(BaseRelationClient): @property def admin_project_name(self): return self.get_data_from_app("admin_project_name") - - -class MongoClient(BaseRelationClient): - """Requires side of a Mongo Endpoint""" - - mandatory_fields_mapping = { - "reactive": ["connection_string"], - "ops": ["replica_set_uri", "replica_set_name"], - } - - def __init__(self, charm: ops.charm.CharmBase, relation_name: str): - super().__init__(charm, relation_name, mandatory_fields=[]) - - @property - def connection_string(self): - if self.is_opts(): - replica_set_uri = self.get_data_from_unit("replica_set_uri") - replica_set_name = self.get_data_from_unit("replica_set_name") - return f"{replica_set_uri}?replicaSet={replica_set_name}" - else: - return self.get_data_from_unit("connection_string") - - def is_opts(self): - return not self.is_missing_data_in_unit_ops() - - def is_missing_data_in_unit(self): - return self.is_missing_data_in_unit_ops() and self.is_missing_data_in_unit_reactive() - - def is_missing_data_in_unit_ops(self): - return not all( - [self.get_data_from_unit(field) for field in self.mandatory_fields_mapping["ops"]] - ) - - def is_missing_data_in_unit_reactive(self): - return not all( - [self.get_data_from_unit(field) for field in self.mandatory_fields_mapping["reactive"]] - ) - - -class PrometheusClient(BaseRelationClient): - """Requires side of a Prometheus Endpoint""" - - mandatory_fields = ["hostname", "port"] - - def __init__(self, charm: ops.charm.CharmBase, relation_name: str): - super().__init__(charm, relation_name, self.mandatory_fields) - - @property - def hostname(self): - return self.get_data_from_app("hostname") - - @property - def port(self): - return self.get_data_from_app("port") - - @property - def user(self): - return self.get_data_from_app("user") - - @property - def password(self): - return self.get_data_from_app("password") diff --git a/installers/charm/osm-nbi/tests/integration/test_charm.py b/installers/charm/osm-nbi/tests/integration/test_charm.py index 85551758..1528869a 100644 --- a/installers/charm/osm-nbi/tests/integration/test_charm.py +++ b/installers/charm/osm-nbi/tests/integration/test_charm.py @@ -42,17 +42,15 @@ MONGO_DB_CHARM = "mongodb-k8s" MONGO_DB_APP = "mongodb" KEYSTONE_CHARM = "osm-keystone" KEYSTONE_APP = "keystone" -PROMETHEUS_CHARM = "osm-prometheus" -PROMETHEUS_APP = "prometheus" ZOOKEEPER_CHARM = "zookeeper-k8s" ZOOKEEPER_APP = "zookeeper" INGRESS_CHARM = "nginx-ingress-integrator" INGRESS_APP = "ingress" -APPS = [KAFKA_APP, MONGO_DB_APP, MARIADB_APP, ZOOKEEPER_APP, KEYSTONE_APP, PROMETHEUS_APP, NBI_APP] +APPS = [KAFKA_APP, MONGO_DB_APP, MARIADB_APP, ZOOKEEPER_APP, KEYSTONE_APP, NBI_APP] @pytest.mark.abort_on_fail -async def test_nbi_is_deployed(ops_test: OpsTest): +async def test_nbi_and_other_charms_are_idle(ops_test: OpsTest): charm = await ops_test.build_charm(".") resources = {"nbi-image": METADATA["resources"]["nbi-image"]["upstream-source"]} @@ -64,7 +62,6 @@ async def test_nbi_is_deployed(ops_test: OpsTest): ops_test.model.deploy(MONGO_DB_CHARM, application_name=MONGO_DB_APP, channel="5/edge"), ops_test.model.deploy(MARIADB_CHARM, application_name=MARIADB_APP, channel="stable"), ops_test.model.deploy(ZOOKEEPER_CHARM, application_name=ZOOKEEPER_APP, channel="stable"), - ops_test.model.deploy(PROMETHEUS_CHARM, application_name=PROMETHEUS_APP, channel="stable"), ) # Keystone charm has to be deployed differently since # bug https://github.com/juju/python-libjuju/issues/766 @@ -74,12 +71,31 @@ async def test_nbi_is_deployed(ops_test: OpsTest): await ops_test.run(*shlex.split(cmd), check=True) async with ops_test.fast_forward(): - await ops_test.model.wait_for_idle( - apps=APPS, - ) + await ops_test.model.wait_for_idle(apps=APPS) + + +@pytest.mark.abort_on_fail +async def test_nbi_is_blocked_when_prometheus_config_is_invalid(ops_test: OpsTest): assert ops_test.model.applications[NBI_APP].status == "blocked" unit = ops_test.model.applications[NBI_APP].units[0] - assert unit.workload_status_message == "need kafka, mongodb, prometheus, keystone relations" + assert unit.workload_status_message == "need prometheus-url config" + + await ops_test.model.applications[NBI_APP].set_config({"prometheus-url": "some_url"}) + async with ops_test.fast_forward(): + await ops_test.model.wait_for_idle(apps=[NBI_APP], status="blocked") + assert unit.workload_status_message == "Invalid value for prometheus-url config: 'some_url'" + + await ops_test.model.applications[NBI_APP].set_config( + {"prometheus-url": "http://prometheus:0231"} + ) + + +@pytest.mark.abort_on_fail +async def test_nbi_is_blocked_when_relations_are_missing(ops_test: OpsTest): + async with ops_test.fast_forward(): + await ops_test.model.wait_for_idle(apps=[NBI_APP], status="blocked") + unit = ops_test.model.applications[NBI_APP].units[0] + assert unit.workload_status_message == "need kafka, mongodb, keystone relations" logger.info("Adding relations for other components") await ops_test.model.add_relation(KAFKA_APP, ZOOKEEPER_APP) @@ -90,14 +106,10 @@ async def test_nbi_is_deployed(ops_test: OpsTest): "{}:mongodb".format(NBI_APP), "{}:database".format(MONGO_DB_APP) ) await ops_test.model.add_relation(NBI_APP, KAFKA_APP) - await ops_test.model.add_relation(NBI_APP, PROMETHEUS_APP) await ops_test.model.add_relation(NBI_APP, KEYSTONE_APP) async with ops_test.fast_forward(): - await ops_test.model.wait_for_idle( - apps=APPS, - status="active", - ) + await ops_test.model.wait_for_idle(apps=APPS, status="active") @pytest.mark.abort_on_fail @@ -113,18 +125,14 @@ async def test_nbi_scales_up(ops_test: OpsTest): @pytest.mark.abort_on_fail -@pytest.mark.parametrize( - "relation_to_remove", [KAFKA_APP, MONGO_DB_APP, PROMETHEUS_APP, KEYSTONE_APP] -) +@pytest.mark.parametrize("relation_to_remove", [KAFKA_APP, MONGO_DB_APP, KEYSTONE_APP]) async def test_nbi_blocks_without_relation(ops_test: OpsTest, relation_to_remove): logger.info("Removing relation: %s", relation_to_remove) # mongoDB relation is named "database" local_relation = relation_to_remove if local_relation == MONGO_DB_APP: local_relation = "database" - await asyncio.gather( - ops_test.model.applications[relation_to_remove].remove_relation(local_relation, NBI_APP) - ) + await ops_test.model.applications[relation_to_remove].remove_relation(local_relation, NBI_APP) async with ops_test.fast_forward(): await ops_test.model.wait_for_idle(apps=[NBI_APP]) assert ops_test.model.applications[NBI_APP].status == "blocked" @@ -132,10 +140,7 @@ async def test_nbi_blocks_without_relation(ops_test: OpsTest, relation_to_remove assert unit.workload_status_message == f"need {relation_to_remove} relation" await ops_test.model.add_relation(NBI_APP, relation_to_remove) async with ops_test.fast_forward(): - await ops_test.model.wait_for_idle( - apps=APPS, - status="active", - ) + await ops_test.model.wait_for_idle(apps=APPS, status="active") @pytest.mark.abort_on_fail @@ -161,10 +166,7 @@ async def test_nbi_action_debug_mode_disabled(ops_test: OpsTest): async def test_nbi_action_debug_mode_enabled(ops_test: OpsTest): await ops_test.model.applications[NBI_APP].set_config({"debug-mode": "true"}) async with ops_test.fast_forward(): - await ops_test.model.wait_for_idle( - apps=APPS, - status="active", - ) + await ops_test.model.wait_for_idle(apps=APPS, status="active") logger.info("Running action 'get-debug-mode-information'") # list of units is not ordered unit_id = list( @@ -191,13 +193,8 @@ async def test_nbi_integration_ingress(ops_test: OpsTest): await ops_test.run(*shlex.split(cmd), check=True) async with ops_test.fast_forward(): - await ops_test.model.wait_for_idle( - apps=APPS + [INGRESS_APP], - ) + await ops_test.model.wait_for_idle(apps=APPS + [INGRESS_APP]) await ops_test.model.add_relation(NBI_APP, INGRESS_APP) async with ops_test.fast_forward(): - await ops_test.model.wait_for_idle( - apps=APPS + [INGRESS_APP], - status="active", - ) + await ops_test.model.wait_for_idle(apps=APPS + [INGRESS_APP], status="active") diff --git a/installers/charm/osm-nbi/tests/unit/test_charm.py b/installers/charm/osm-nbi/tests/unit/test_charm.py index b1604192..af248bfc 100644 --- a/installers/charm/osm-nbi/tests/unit/test_charm.py +++ b/installers/charm/osm-nbi/tests/unit/test_charm.py @@ -42,21 +42,65 @@ def harness(mocker: MockerFixture): harness.cleanup() +def _set_prometheus_config(harness: Harness): + harness.update_config({"prometheus-url": "http://prometheus:1234"}) + + +def test_default_config_prometheus_url_is_invalid_charm_is_blocked(harness: Harness): + harness.charm.on.config_changed.emit() + assert type(harness.charm.unit.status) == BlockedStatus + assert "need prometheus-url config" == harness.charm.unit.status.message + + def test_missing_relations(harness: Harness): + _set_prometheus_config(harness) harness.charm.on.config_changed.emit() assert type(harness.charm.unit.status) == BlockedStatus assert all( relation in harness.charm.unit.status.message - for relation in ["mongodb", "kafka", "prometheus", "keystone"] + for relation in ["mongodb", "kafka", "keystone"] + ) + + +def test_prometheus_url_without_schema_blocked_status(harness: Harness): + _add_relations(harness) + harness.update_config({"prometheus-url": "foo.com"}) + assert type(harness.charm.unit.status) == BlockedStatus + assert ( + "Invalid value for prometheus-url config: 'foo.com'" in harness.charm.unit.status.message ) +def test_prometheus_url_with_port_without_schema_blocked_status(harness: Harness): + _add_relations(harness) + harness.update_config({"prometheus-url": "foo.com:9090"}) + assert type(harness.charm.unit.status) == BlockedStatus + assert ( + "Invalid value for prometheus-url config: 'foo.com:9090'" + in harness.charm.unit.status.message + ) + + +def test_prometheus_url_without_port_is_valid(harness: Harness): + _add_relations(harness) + harness.update_config({"prometheus-url": "http://foo"}) + assert harness.charm.unit.status == ActiveStatus() + + +def test_prometheus_url_with_port_is_valid(harness: Harness): + _add_relations(harness) + harness.update_config({"prometheus-url": "http://foo:90"}) + assert harness.charm.unit.status == ActiveStatus() + + def test_ready(harness: Harness): + _set_prometheus_config(harness) _add_relations(harness) assert harness.charm.unit.status == ActiveStatus() def test_container_stops_after_relation_broken(harness: Harness): + _set_prometheus_config(harness) harness.charm.on[container_name].pebble_ready.emit(container_name) container = harness.charm.unit.get_container(container_name) relation_ids = _add_relations(harness) @@ -67,6 +111,7 @@ def test_container_stops_after_relation_broken(harness: Harness): def test_nbi_relation_joined(harness: Harness): + _set_prometheus_config(harness) harness.set_leader(True) _add_relations(harness) relation_id = harness.add_relation("nbi", "ng-ui") @@ -92,13 +137,6 @@ def _add_relations(harness: Harness): harness.add_relation_unit(relation_id, "kafka/0") harness.update_relation_data(relation_id, "kafka", {"host": "kafka", "port": "9092"}) relation_ids.append(relation_id) - # Add prometheus relation - relation_id = harness.add_relation("prometheus", "prometheus") - harness.add_relation_unit(relation_id, "prometheus/0") - harness.update_relation_data( - relation_id, "prometheus", {"hostname": "prometheus", "port": "9090"} - ) - relation_ids.append(relation_id) # Add keystone relation relation_id = harness.add_relation("keystone", "keystone") harness.add_relation_unit(relation_id, "keystone/0") -- 2.17.1