From 6486725eadaf5a36c2f001617918b52b9021859b Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 4 Dec 2024 12:10:05 +0100 Subject: [PATCH 01/12] precommit --- .pre-commit-config.yaml | 59 +++++++++++++++++++ tests/interface/conftest.py | 18 ++++++ .../test_grafana_datasource_exchange.py | 13 ++++ 3 files changed, 90 insertions(+) create mode 100644 .pre-commit-config.yaml create mode 100644 tests/interface/test_grafana_datasource_exchange.py diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml new file mode 100644 index 0000000..d32099b --- /dev/null +++ b/.pre-commit-config.yaml @@ -0,0 +1,59 @@ +repos: + - repo: https://github.com/pre-commit/pre-commit-hooks + rev: v4.4.0 + hooks: + - id: check-ast + - id: check-builtin-literals + - id: check-docstring-first + - id: check-merge-conflict + - id: check-yaml + - id: check-toml + - id: debug-statements + - id: trailing-whitespace + - repo: local + hooks: + - id: fmt + name: fmt + entry: tox -e fmt -- + language: system + - repo: local + hooks: + - id: static-charm + name: static-charm + entry: tox -e static-charm -- + language: system + - repo: local + hooks: + - id: static-lib + name: static-lib + entry: tox -e static-lib -- + language: system + - repo: https://github.com/asottile/blacken-docs + rev: 1.13.0 + hooks: + - id: blacken-docs + additional_dependencies: [black==23.3] + - repo: https://github.com/pre-commit/mirrors-prettier + rev: "v2.7.1" + hooks: + - id: prettier + additional_dependencies: + - prettier@2.7.1 + - "@prettier/plugin-xml@2.2" + args: ["--print-width=120", "--prose-wrap=always"] + - repo: https://github.com/igorshubovych/markdownlint-cli + rev: v0.33.0 + hooks: + - id: markdownlint + - repo: local + hooks: + - id: changelogs-rst + name: Changelog filenames + language: fail + entry: "changelog files must be named ####.(feature|bugfix|doc|removal|misc).rst" + # exclude: ^docs/changelog/(\d+\.(feature|bugfix|doc|removal|misc).rst|template.jinja2) + files: ^docs/changelog/ + - repo: meta + hooks: + - id: check-hooks-apply + - id: check-useless-excludes diff --git a/tests/interface/conftest.py b/tests/interface/conftest.py index 16a1217..4981640 100644 --- a/tests/interface/conftest.py +++ b/tests/interface/conftest.py @@ -1,5 +1,6 @@ # Copyright 2022 Canonical Ltd. # See LICENSE file for licensing details. +import json from contextlib import ExitStack from unittest.mock import MagicMock, patch @@ -63,6 +64,10 @@ }, ) +grafana_source_relation = Relation( + "grafana-source", remote_app_data={"datasources": json.dumps({})} +) + peers = PeerRelation("peers", peers_data={1: {}}) k8s_resource_patch_ready = MagicMock(return_value=True) @@ -142,3 +147,16 @@ def grafana_datasource_tester(interface_tester: InterfaceTester): ), ) yield interface_tester + + +@pytest.fixture +def grafana_datasource_exchange_tester(interface_tester: InterfaceTester): + interface_tester.configure( + charm_type=TempoCoordinatorCharm, + state_template=State( + leader=True, + containers=[nginx_container, nginx_prometheus_exporter_container], + relations=[peers, cluster_relation, grafana_source_relation], + ), + ) + yield interface_tester diff --git a/tests/interface/test_grafana_datasource_exchange.py b/tests/interface/test_grafana_datasource_exchange.py new file mode 100644 index 0000000..b2301de --- /dev/null +++ b/tests/interface/test_grafana_datasource_exchange.py @@ -0,0 +1,13 @@ +# Copyright 2024 Canonical Ltd. +# See LICENSE file for licensing details. +from interface_tester import InterfaceTester + + +def test_grafana_datasource_exchange_v0_interface( + grafana_datasource_exchange_tester: InterfaceTester, +): + grafana_datasource_exchange_tester.configure( + interface_name="grafana_datasource_exchange", + interface_version=0, + ) + grafana_datasource_exchange_tester.run() From 8954a3ddc1f6a86353543219794bf47cebd03c79 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 4 Dec 2024 12:49:57 +0100 Subject: [PATCH 02/12] ds exchange implementation --- charmcraft.yaml | 44 ++++++++++----- requirements.txt | 2 +- src/charm.py | 53 ++++++++++++++++++- tests/interface/conftest.py | 4 +- .../test_grafana_datasource_exchange.py | 1 + 5 files changed, 87 insertions(+), 17 deletions(-) diff --git a/charmcraft.yaml b/charmcraft.yaml index 44b535d..3eaefdd 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -60,18 +60,22 @@ provides: interface: tracing description: | Integration to offer other charms the possibility to send traces to Tempo. - + provide-datasource-exchange: + interface: grafana_datasource_exchange + description: | + Integration to share with other charms this charm's datasources. This interface is symmetric, + so it doesn't matter whether you integrate the provider or the requirer endpoint. requires: self-charm-tracing: interface: tracing description: | - Integration to enable Tempo to send its own charm traces to another Tempo instance. + Integration to enable Tempo to send its own charm traces to another Tempo instance. limit: 1 self-workload-tracing: interface: tracing description: | - Integration to enable Tempo to send its own workload traces to another Tempo instance. + Integration to enable Tempo to send its own workload traces to another Tempo instance. limit: 1 s3: interface: s3 @@ -92,12 +96,17 @@ requires: interface: tls-certificates limit: 1 description: | - Certificate and key files for securing Tempo internal and external + Certificate and key files for securing Tempo internal and external communications with TLS. send-remote-write: interface: prometheus_remote_write description: | Prometheus-like remote write endpoints to push traces' metrics generated by the `metrics-generator` component. + require-datasource-exchange: + interface: grafana_datasource_exchange + description: | + Integration to share with other charms this charm's datasources. This interface is symmetric, + so it doesn't matter whether you integrate the provider or the requirer endpoint. storage: data: @@ -115,7 +124,6 @@ peers: description: | peer relation for internal coordination - bases: - build-on: - name: "ubuntu" @@ -126,7 +134,7 @@ bases: parts: charm: -# uncomment this if you add git+ dependencies in requirements.txt + # uncomment this if you add git+ dependencies in requirements.txt # build-packages: # - "git" charm-binary-python-packages: @@ -139,28 +147,38 @@ config: options: retention-period: description: | - Maximum trace retention period, in hours. This will be used to configure the compactor to clean up trace data after this time. + Maximum trace retention period, in hours. This will be used to configure the compactor to clean up trace data after this time. Defaults to 720 hours, which is equivalent to 30 days. Per-stream retention limits are currently not supported. type: int default: 720 always_enable_zipkin: - description: Force-enable the receiver for the 'zipkin' protocol in Tempo, even if there is no integration currently requesting it. + description: + Force-enable the receiver for the 'zipkin' protocol in Tempo, even if there is no integration currently + requesting it. type: boolean default: false always_enable_otlp_grpc: - description: Force-enable the receiver for the 'otlp_grpc' protocol in Tempo, even if there is no integration currently requesting it. + description: + Force-enable the receiver for the 'otlp_grpc' protocol in Tempo, even if there is no integration currently + requesting it. type: boolean default: false always_enable_otlp_http: - description: Force-enable the receiver for the 'otlp_http' protocol in Tempo, even if there is no integration currently requesting it. + description: + Force-enable the receiver for the 'otlp_http' protocol in Tempo, even if there is no integration currently + requesting it. type: boolean default: false always_enable_jaeger_thrift_http: - description: Force-enable the receiver for the 'jaeger_thrift_http' protocol in Tempo, even if there is no integration currently requesting it. + description: + Force-enable the receiver for the 'jaeger_thrift_http' protocol in Tempo, even if there is no integration + currently requesting it. type: boolean default: false always_enable_jaeger_grpc: - description: Force-enable the receiver for the 'jaeger_grpc' protocol in Tempo, even if there is no integration currently requesting it. + description: + Force-enable the receiver for the 'jaeger_grpc' protocol in Tempo, even if there is no integration currently + requesting it. type: boolean default: false cpu_limit: @@ -174,4 +192,4 @@ config: K8s memory resource limit, e.g. "1Gi". Default is unset (no limit). This value is used for the "limits" portion of the resource requirements. See https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ - type: string \ No newline at end of file + type: string diff --git a/requirements.txt b/requirements.txt index 0662053..f84188d 100644 --- a/requirements.txt +++ b/requirements.txt @@ -19,4 +19,4 @@ cryptography # lib/charms/tempo_coordinator_k8s/v1/tracing.py pydantic>=2 # lib/charms/prometheus_k8s/v0/prometheus_scrape.py -cosl>=0.0.43 +cosl @ git+https://github.com/canonical/cos-lib@datasource_exchange diff --git a/src/charm.py b/src/charm.py index c30ac8f..3e10fbd 100755 --- a/src/charm.py +++ b/src/charm.py @@ -24,8 +24,9 @@ ) from charms.traefik_k8s.v0.traefik_route import TraefikRouteRequirer from cosl.coordinated_workers.coordinator import ClusterRolesConfig, Coordinator -from cosl.coordinated_workers.interface import DatabagModel from cosl.coordinated_workers.nginx import CA_CERT_PATH, CERT_PATH, KEY_PATH +from cosl.interfaces.datasource_exchange import DatasourceDict +from cosl.interfaces.utils import DatabagModel from ops import CollectStatusEvent from ops.charm import CharmBase @@ -108,6 +109,8 @@ def __init__(self, *args): "s3": "s3", "charm-tracing": "self-charm-tracing", "workload-tracing": "self-workload-tracing", + "provide-datasource-exchange": "provide-datasource-exchange", + "require-datasource-exchange": "require-datasource-exchange", }, nginx_config=NginxConfig(server_name=self.hostname).config, workers_config=self.tempo.config, @@ -443,6 +446,53 @@ def remote_write_endpoints(self): """Return remote-write endpoints.""" return self._remote_write.endpoints + def _update_source_exchange(self) -> None: + """Update the grafana-datasource relations.""" + + # This degree of multiplexing requires some in-depth explanation. + # each grafana we're sending our data to gives us back a mapping from unit names to datasource UIDs. + # so if we have two tempo units, and we're related to two grafanas, we'll get back: + # { + # "grafana/0": { + # "tempo/0": "0000-0000-0000-0000", + # "tempo/1": "0000-0000-0000-0001" + # }, + # "grafana/1": { + # "tempo/0": "0000-0000-0000-0000", + # "tempo/1": "0000-0000-0000-0001" + # }, + # } + # This is an implementation detail, but the UID is 'unique-per-grafana' and ATM it turns out to be + # deterministic given tempo's jujutopology, so if we are related to multiple grafanas, + # the UIDs they assign to our units will be the same. + # Either way, we assume for simplicity (also, why not) that we're sending the same data to + # different grafanas, and not somehow different subsets to different places. Therefore, it should not + # matter which grafana you talk to, to cross-reference the data this unit is presenting to the world. + # However, if at some point grafana's algorithm to generate source UIDs changes, we'd be in trouble since + # the unit we are sharing our data to might be talking to 'another grafana' which might be using a + # different UID convention! + + # To simplify our lives, we're going to assume that you're only relating each tempo to a single grafana!!! + grafanas_to_units_to_uids = self.grafana_source_provider.get_source_uids() + + if len(grafanas_to_units_to_uids) > 1: + logger.warning( + "Multiple grafanas are using this application as datasource. " + "Datasource correlation features might misbehave. " + "File a bug if you experience weirdness." + ) + + raw_datasources: List[DatasourceDict] = [] + + for _grafana_name, ds_uids in grafanas_to_units_to_uids.items(): + # we don't use the grafana name + for _unit_name, ds_uid in ds_uids.items(): + # we also don't care about which unit's server we're looking at, since hopefully the data is the same. + raw_datasources.append({"type": "tempo", "uid": ds_uid}) + + # submit() already sorts the data for us, to prevent databag flapping and ensuing event storms + self.coordinator.datasource_exchange.submit(raw_datasources=raw_datasources) + def _reconcile(self): # This method contains unconditional update logic, i.e. logic that should be executed # regardless of the event we are processing. @@ -450,6 +500,7 @@ def _reconcile(self): # we need to 'remember' to run this logic as soon as we become ready, which is hard and error-prone self._update_ingress_relation() self._update_tracing_relations() + self._update_source_exchange() if __name__ == "__main__": # pragma: nocover diff --git a/tests/interface/conftest.py b/tests/interface/conftest.py index 4981640..71ed652 100644 --- a/tests/interface/conftest.py +++ b/tests/interface/conftest.py @@ -65,7 +65,8 @@ ) grafana_source_relation = Relation( - "grafana-source", remote_app_data={"datasources": json.dumps({})} + "grafana-source", + remote_app_data={"datasources": json.dumps({"tempo/0": {"type": "tempo", "uid": "01234"}})}, ) peers = PeerRelation("peers", peers_data={1: {}}) @@ -87,7 +88,6 @@ def patch_all(): ) ) stack.enter_context(charm_tracing_disabled()) - yield diff --git a/tests/interface/test_grafana_datasource_exchange.py b/tests/interface/test_grafana_datasource_exchange.py index b2301de..ea52e79 100644 --- a/tests/interface/test_grafana_datasource_exchange.py +++ b/tests/interface/test_grafana_datasource_exchange.py @@ -8,6 +8,7 @@ def test_grafana_datasource_exchange_v0_interface( ): grafana_datasource_exchange_tester.configure( interface_name="grafana_datasource_exchange", + branch="grafana_datasource_exchange", interface_version=0, ) grafana_datasource_exchange_tester.run() From cc08a1808688276ef2409c5ea7719c9a54ad2387 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 4 Dec 2024 13:03:18 +0100 Subject: [PATCH 03/12] interface tests passing --- lib/charms/grafana_k8s/v0/grafana_source.py | 4 ++-- tests/interface/conftest.py | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/charms/grafana_k8s/v0/grafana_source.py b/lib/charms/grafana_k8s/v0/grafana_source.py index f3492be..e545dfb 100644 --- a/lib/charms/grafana_k8s/v0/grafana_source.py +++ b/lib/charms/grafana_k8s/v0/grafana_source.py @@ -162,7 +162,7 @@ def __init__(self, *args): # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 22 +LIBPATCH = 23 logger = logging.getLogger(__name__) @@ -438,7 +438,7 @@ def get_source_uids(self) -> Dict[str, Dict[str, str]]: for rel in self._charm.model.relations.get(self._relation_name, []): if not rel: continue - uids[rel.app.name] = json.loads(rel.data[rel.app]["datasource_uids"]) + uids[rel.app.name] = json.loads(rel.data[rel.app].get("datasource_uids", "{}")) return uids def _set_sources_from_event(self, event: RelationJoinedEvent) -> None: diff --git a/tests/interface/conftest.py b/tests/interface/conftest.py index 71ed652..484588c 100644 --- a/tests/interface/conftest.py +++ b/tests/interface/conftest.py @@ -1,7 +1,9 @@ # Copyright 2022 Canonical Ltd. # See LICENSE file for licensing details. import json +import pathlib from contextlib import ExitStack +from shutil import rmtree from unittest.mock import MagicMock, patch import pytest @@ -89,6 +91,7 @@ def patch_all(): ) stack.enter_context(charm_tracing_disabled()) yield + rmtree(pathlib.Path(__file__).parent / "src") # Interface tests are centrally hosted at https://github.com/canonical/charm-relation-interfaces. @@ -143,7 +146,7 @@ def grafana_datasource_tester(interface_tester: InterfaceTester): state_template=State( leader=True, containers=[nginx_container, nginx_prometheus_exporter_container], - relations=[peers, cluster_relation], + relations=[peers, s3_relation, cluster_relation], ), ) yield interface_tester @@ -156,7 +159,7 @@ def grafana_datasource_exchange_tester(interface_tester: InterfaceTester): state_template=State( leader=True, containers=[nginx_container, nginx_prometheus_exporter_container], - relations=[peers, cluster_relation, grafana_source_relation], + relations=[peers, s3_relation, cluster_relation, grafana_source_relation], ), ) yield interface_tester From e4839fbc101cef507e9bdebe165578478ae229a5 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 4 Dec 2024 13:11:05 +0100 Subject: [PATCH 04/12] explicit CRI dep --- requirements.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/requirements.txt b/requirements.txt index f84188d..b4f25b4 100644 --- a/requirements.txt +++ b/requirements.txt @@ -20,3 +20,4 @@ cryptography pydantic>=2 # lib/charms/prometheus_k8s/v0/prometheus_scrape.py cosl @ git+https://github.com/canonical/cos-lib@datasource_exchange +charm-relation-interfaces @ git+https://github.com/canonical/charm-relation-interfaces@grafana_datasource_exchange \ No newline at end of file From c38e8f045a79ab865879788bf34b4b22e79224c3 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 4 Dec 2024 13:59:55 +0100 Subject: [PATCH 05/12] unpinned CRI branch --- requirements.txt | 2 +- src/charm.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/requirements.txt b/requirements.txt index b4f25b4..db592a0 100644 --- a/requirements.txt +++ b/requirements.txt @@ -20,4 +20,4 @@ cryptography pydantic>=2 # lib/charms/prometheus_k8s/v0/prometheus_scrape.py cosl @ git+https://github.com/canonical/cos-lib@datasource_exchange -charm-relation-interfaces @ git+https://github.com/canonical/charm-relation-interfaces@grafana_datasource_exchange \ No newline at end of file +charm-relation-interfaces @ git+https://github.com/canonical/charm-relation-interfaces \ No newline at end of file diff --git a/src/charm.py b/src/charm.py index 3e10fbd..6836e8b 100755 --- a/src/charm.py +++ b/src/charm.py @@ -491,7 +491,7 @@ def _update_source_exchange(self) -> None: raw_datasources.append({"type": "tempo", "uid": ds_uid}) # submit() already sorts the data for us, to prevent databag flapping and ensuing event storms - self.coordinator.datasource_exchange.submit(raw_datasources=raw_datasources) + self.coordinator.datasource_exchange.publish(datasources=raw_datasources) def _reconcile(self): # This method contains unconditional update logic, i.e. logic that should be executed From d7d5f0fb4a6fb80996548ded6440934bd4386a38 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 4 Dec 2024 14:42:18 +0100 Subject: [PATCH 06/12] git build dep --- charmcraft.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/charmcraft.yaml b/charmcraft.yaml index 3eaefdd..e604343 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -135,8 +135,8 @@ bases: parts: charm: # uncomment this if you add git+ dependencies in requirements.txt - # build-packages: - # - "git" + build-packages: + - "git" charm-binary-python-packages: - "pydantic>=2" - "cryptography" From 969b953184585997232020eb35b5b75cef6988bf Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Mon, 9 Dec 2024 12:55:20 +0100 Subject: [PATCH 07/12] added grafana uid --- charmcraft.yaml | 10 +-- lib/charms/grafana_k8s/v0/grafana_source.py | 24 ++++- src/charm.py | 14 +-- tests/interface/conftest.py | 6 +- .../test_grafana_datasource_exchange.py | 2 +- tests/scenario/test_datasources.py | 90 +++++++++++++++++++ 6 files changed, 128 insertions(+), 18 deletions(-) create mode 100644 tests/scenario/test_datasources.py diff --git a/charmcraft.yaml b/charmcraft.yaml index e604343..0c2d540 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -60,11 +60,6 @@ provides: interface: tracing description: | Integration to offer other charms the possibility to send traces to Tempo. - provide-datasource-exchange: - interface: grafana_datasource_exchange - description: | - Integration to share with other charms this charm's datasources. This interface is symmetric, - so it doesn't matter whether you integrate the provider or the requirer endpoint. requires: self-charm-tracing: @@ -102,11 +97,10 @@ requires: interface: prometheus_remote_write description: | Prometheus-like remote write endpoints to push traces' metrics generated by the `metrics-generator` component. - require-datasource-exchange: + receive-datasource: interface: grafana_datasource_exchange description: | - Integration to share with other charms this charm's datasources. This interface is symmetric, - so it doesn't matter whether you integrate the provider or the requirer endpoint. + Integration to share with other COS components this charm's datasources, and receive theirs. storage: data: diff --git a/lib/charms/grafana_k8s/v0/grafana_source.py b/lib/charms/grafana_k8s/v0/grafana_source.py index e545dfb..f1a10f2 100644 --- a/lib/charms/grafana_k8s/v0/grafana_source.py +++ b/lib/charms/grafana_k8s/v0/grafana_source.py @@ -154,6 +154,8 @@ def __init__(self, *args): ) from ops.model import Relation +from charms.observability_libs.v0.juju_topology import JujuTopology + # The unique Charmhub library identifier, never change it LIBID = "974705adb86f40228298156e34b460dc" @@ -432,13 +434,22 @@ def update_source(self, source_url: Optional[str] = ""): def get_source_uids(self) -> Dict[str, Dict[str, str]]: """Get the datasource UID(s) assigned by the remote end(s) to this datasource. - Returns a mapping from remote application names to unit names to datasource uids. + Returns a mapping from remote application UIDs to unit names to datasource uids. """ uids = {} for rel in self._charm.model.relations.get(self._relation_name, []): if not rel: continue - uids[rel.app.name] = json.loads(rel.data[rel.app].get("datasource_uids", "{}")) + app_databag = rel.data[rel.app] + grafana_uid = app_databag.get("grafana_uid") + if not grafana_uid: + logger.warning( + "remote end is using an old grafana_datasource interface: " + "`grafana_uid` field not found." + ) + continue + + uids[grafana_uid] = json.loads(app_databag.get("datasource_uids", "{}")) return uids def _set_sources_from_event(self, event: RelationJoinedEvent) -> None: @@ -568,6 +579,15 @@ def _publish_source_uids(self, rel: Relation, uids: Dict[str, str]): Assumes only leader unit will call this method """ + juju_topology = JujuTopology.from_charm(self._charm) + unique_grafana_name = "juju_{}_{}_{}_{}".format( + juju_topology.model, + juju_topology.model_uuid, + juju_topology.application, + juju_topology.unit.split("/")[1], + ) + + rel.data[self._charm.app]["grafana_uid"] = unique_grafana_name rel.data[self._charm.app]["datasource_uids"] = json.dumps(uids) def _get_source_config(self, rel: Relation): diff --git a/src/charm.py b/src/charm.py index 6836e8b..61d0de0 100755 --- a/src/charm.py +++ b/src/charm.py @@ -109,8 +109,8 @@ def __init__(self, *args): "s3": "s3", "charm-tracing": "self-charm-tracing", "workload-tracing": "self-workload-tracing", - "provide-datasource-exchange": "provide-datasource-exchange", - "require-datasource-exchange": "require-datasource-exchange", + "send-datasource": None, + "receive-datasource": "receive-datasource", }, nginx_config=NginxConfig(server_name=self.hostname).config, workers_config=self.tempo.config, @@ -473,9 +473,9 @@ def _update_source_exchange(self) -> None: # different UID convention! # To simplify our lives, we're going to assume that you're only relating each tempo to a single grafana!!! - grafanas_to_units_to_uids = self.grafana_source_provider.get_source_uids() + grafana_uids_to_units_to_uids = self.grafana_source_provider.get_source_uids() - if len(grafanas_to_units_to_uids) > 1: + if len(grafana_uids_to_units_to_uids) > 1: logger.warning( "Multiple grafanas are using this application as datasource. " "Datasource correlation features might misbehave. " @@ -484,11 +484,13 @@ def _update_source_exchange(self) -> None: raw_datasources: List[DatasourceDict] = [] - for _grafana_name, ds_uids in grafanas_to_units_to_uids.items(): + for grafana_uid, ds_uids in grafana_uids_to_units_to_uids.items(): # we don't use the grafana name for _unit_name, ds_uid in ds_uids.items(): # we also don't care about which unit's server we're looking at, since hopefully the data is the same. - raw_datasources.append({"type": "tempo", "uid": ds_uid}) + raw_datasources.append( + {"type": "tempo", "uid": ds_uid, "grafana_uid": grafana_uid} + ) # submit() already sorts the data for us, to prevent databag flapping and ensuing event storms self.coordinator.datasource_exchange.publish(datasources=raw_datasources) diff --git a/tests/interface/conftest.py b/tests/interface/conftest.py index 484588c..14870cf 100644 --- a/tests/interface/conftest.py +++ b/tests/interface/conftest.py @@ -91,7 +91,11 @@ def patch_all(): ) stack.enter_context(charm_tracing_disabled()) yield - rmtree(pathlib.Path(__file__).parent / "src") + + # cleanup: some tests create a spurious src folder for alert rules in ./ + src_root = pathlib.Path(__file__).parent / "src" + if src_root.exists(): + rmtree(src_root) # Interface tests are centrally hosted at https://github.com/canonical/charm-relation-interfaces. diff --git a/tests/interface/test_grafana_datasource_exchange.py b/tests/interface/test_grafana_datasource_exchange.py index ea52e79..cc4bd7e 100644 --- a/tests/interface/test_grafana_datasource_exchange.py +++ b/tests/interface/test_grafana_datasource_exchange.py @@ -8,7 +8,7 @@ def test_grafana_datasource_exchange_v0_interface( ): grafana_datasource_exchange_tester.configure( interface_name="grafana_datasource_exchange", - branch="grafana_datasource_exchange", + branch="add-grafana-uid-to-DS", interface_version=0, ) grafana_datasource_exchange_tester.run() diff --git a/tests/scenario/test_datasources.py b/tests/scenario/test_datasources.py new file mode 100644 index 0000000..e4b8ad2 --- /dev/null +++ b/tests/scenario/test_datasources.py @@ -0,0 +1,90 @@ +import json +from unittest.mock import patch + +import pytest +import scenario +from cosl.interfaces.datasource_exchange import DatasourceExchange, DSExchangeAppData +from interfaces.grafana_datasource_exchange.v0.schema import GrafanaDatasource +from scenario import Context, PeerRelation, Relation, State + + +@pytest.fixture(scope="function") +def context(tempo_charm): + return Context(charm_type=tempo_charm) + + +@patch("charm.TempoCoordinatorCharm.is_workload_ready", return_value=True) +def test_datasource_receive( + workload_ready_mock, + context, + s3, + all_worker, + nginx_container, + nginx_prometheus_exporter_container, +): + # GIVEN a regular HA deployment and two ds_exchange integrations with a mimir and a loki + ds_loki = [ + {"type": "loki", "uid": "3", "grafana_uid": "4"}, + ] + + ds_mimir = [ + {"type": "prometheus", "uid": "8", "grafana_uid": "9"}, + ] + + mimir_dsx = Relation( + "receive-datasource", + remote_app_data=DSExchangeAppData( + datasources=json.dumps(sorted(ds_mimir, key=lambda raw_ds: raw_ds["uid"])) + ).dump(), + ) + loki_dsx = Relation( + "receive-datasource", + remote_app_data=DSExchangeAppData( + datasources=json.dumps(sorted(ds_loki, key=lambda raw_ds: raw_ds["uid"])) + ).dump(), + ) + + ds = Relation( + "grafana-source", + remote_app_data={ + "grafana_uid": "foo-something-bars", + "datasource_uids": json.dumps({"tempo/0": "1234"}), + }, + ) + + state_in = State( + relations=[ + PeerRelation("peers", peers_data={1: {}, 2: {}}), + s3, + all_worker, + ds, + mimir_dsx, + loki_dsx, + ], + containers=[nginx_container, nginx_prometheus_exporter_container], + unit_status=scenario.ActiveStatus(), + leader=True, + ) + + # WHEN we receive any event + with context(context.on.update_status(), state_in) as mgr: + charm = mgr.charm + # THEN we can find all received datasource uids in the coordinator + dsx: DatasourceExchange = charm.coordinator.datasource_exchange + received = dsx.received_datasources + assert received == ( + GrafanaDatasource(type="loki", uid="3", grafana_uid="4"), + GrafanaDatasource(type="prometheus", uid="8", grafana_uid="9"), + ) + state_out = mgr.run() + + # AND THEN we forward our own datasource information to mimir and loki + assert state_out.unit_status.name == "active" + published_dsx_mimir = state_out.get_relation(mimir_dsx.id).local_app_data + published_dsx_loki = state_out.get_relation(loki_dsx.id).local_app_data + assert published_dsx_loki == published_dsx_mimir + assert json.loads(published_dsx_loki["datasources"])[0] == { + "type": "tempo", + "uid": "1234", + "grafana_uid": "foo-something-bars", + } From d68fa89b892a25235a1d0107890116ef7243fc2f Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Mon, 9 Dec 2024 12:55:54 +0100 Subject: [PATCH 08/12] removed ctx --- tests/scenario/test_datasources.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/tests/scenario/test_datasources.py b/tests/scenario/test_datasources.py index e4b8ad2..244e8a6 100644 --- a/tests/scenario/test_datasources.py +++ b/tests/scenario/test_datasources.py @@ -1,16 +1,10 @@ import json from unittest.mock import patch -import pytest import scenario from cosl.interfaces.datasource_exchange import DatasourceExchange, DSExchangeAppData from interfaces.grafana_datasource_exchange.v0.schema import GrafanaDatasource -from scenario import Context, PeerRelation, Relation, State - - -@pytest.fixture(scope="function") -def context(tempo_charm): - return Context(charm_type=tempo_charm) +from scenario import PeerRelation, Relation, State @patch("charm.TempoCoordinatorCharm.is_workload_ready", return_value=True) From deece3a7038743a6bceba5c4c4d71b6259daeb6a Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Tue, 10 Dec 2024 13:21:08 +0100 Subject: [PATCH 09/12] fixed ds test --- requirements.txt | 13 ++++++------- tests/scenario/test_datasources.py | 7 +++++-- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/requirements.txt b/requirements.txt index db592a0..c03c4cb 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,11 +1,12 @@ -# pin importlib-metadata version else charmcraft pack will fail to resolve the dependencies for the pydeps-installed otlp library -importlib-metadata~=6.0.0 ops>=2.17 crossplane jsonschema==4.17.0 lightkube>=0.15.4 lightkube-models>=1.24.1.4 tenacity==8.2.3 +pydantic>=2 +cosl>=0.0.46 + # crossplane is a package from nginxinc to interact with the Nginx config crossplane @@ -16,8 +17,6 @@ opentelemetry-exporter-otlp-proto-http==1.21.0 # lib/charms/tls_certificates_interface/v2/tls_certificates.py jsonschema cryptography -# lib/charms/tempo_coordinator_k8s/v1/tracing.py -pydantic>=2 -# lib/charms/prometheus_k8s/v0/prometheus_scrape.py -cosl @ git+https://github.com/canonical/cos-lib@datasource_exchange -charm-relation-interfaces @ git+https://github.com/canonical/charm-relation-interfaces \ No newline at end of file + +# pin importlib-metadata version else charmcraft pack will fail to resolve the dependencies for the pydeps-installed otlp library +importlib-metadata~=6.0.0 diff --git a/tests/scenario/test_datasources.py b/tests/scenario/test_datasources.py index 244e8a6..e1f555c 100644 --- a/tests/scenario/test_datasources.py +++ b/tests/scenario/test_datasources.py @@ -2,8 +2,11 @@ from unittest.mock import patch import scenario -from cosl.interfaces.datasource_exchange import DatasourceExchange, DSExchangeAppData -from interfaces.grafana_datasource_exchange.v0.schema import GrafanaDatasource +from cosl.interfaces.datasource_exchange import ( + DatasourceExchange, + DSExchangeAppData, + GrafanaDatasource, +) from scenario import PeerRelation, Relation, State From 84389588016a6fc2634dec29b85d5a40b2e223f0 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 11 Dec 2024 12:40:26 +0100 Subject: [PATCH 10/12] pr comments --- lib/charms/grafana_k8s/v0/grafana_source.py | 13 +++++-------- src/charm.py | 19 +++++++------------ tests/scenario/conftest.py | 10 ++++++++++ 3 files changed, 22 insertions(+), 20 deletions(-) diff --git a/lib/charms/grafana_k8s/v0/grafana_source.py b/lib/charms/grafana_k8s/v0/grafana_source.py index f1a10f2..6e09c50 100644 --- a/lib/charms/grafana_k8s/v0/grafana_source.py +++ b/lib/charms/grafana_k8s/v0/grafana_source.py @@ -154,8 +154,6 @@ def __init__(self, *args): ) from ops.model import Relation -from charms.observability_libs.v0.juju_topology import JujuTopology - # The unique Charmhub library identifier, never change it LIBID = "974705adb86f40228298156e34b460dc" @@ -164,7 +162,7 @@ def __init__(self, *args): # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 23 +LIBPATCH = 24 logger = logging.getLogger(__name__) @@ -579,12 +577,11 @@ def _publish_source_uids(self, rel: Relation, uids: Dict[str, str]): Assumes only leader unit will call this method """ - juju_topology = JujuTopology.from_charm(self._charm) unique_grafana_name = "juju_{}_{}_{}_{}".format( - juju_topology.model, - juju_topology.model_uuid, - juju_topology.application, - juju_topology.unit.split("/")[1], + self._charm.model.name, + self._charm.model.uuid, + self._charm.model.app.name, + self._charm.model.unit.name.split("/")[1], # type: ignore ) rel.data[self._charm.app]["grafana_uid"] = unique_grafana_name diff --git a/src/charm.py b/src/charm.py index 61d0de0..ec41fd1 100755 --- a/src/charm.py +++ b/src/charm.py @@ -447,10 +447,13 @@ def remote_write_endpoints(self): return self._remote_write.endpoints def _update_source_exchange(self) -> None: - """Update the grafana-datasource relations.""" + """Update the grafana-datasource-exchange relations with what we receive from grafana-source.""" - # This degree of multiplexing requires some in-depth explanation. - # each grafana we're sending our data to gives us back a mapping from unit names to datasource UIDs. + # leader operation only + if not self.unit.is_leader(): + return + + # Each grafana we're sending our data to gives us back a mapping from unit names to datasource UIDs. # so if we have two tempo units, and we're related to two grafanas, we'll get back: # { # "grafana/0": { @@ -474,14 +477,6 @@ def _update_source_exchange(self) -> None: # To simplify our lives, we're going to assume that you're only relating each tempo to a single grafana!!! grafana_uids_to_units_to_uids = self.grafana_source_provider.get_source_uids() - - if len(grafana_uids_to_units_to_uids) > 1: - logger.warning( - "Multiple grafanas are using this application as datasource. " - "Datasource correlation features might misbehave. " - "File a bug if you experience weirdness." - ) - raw_datasources: List[DatasourceDict] = [] for grafana_uid, ds_uids in grafana_uids_to_units_to_uids.items(): @@ -492,7 +487,7 @@ def _update_source_exchange(self) -> None: {"type": "tempo", "uid": ds_uid, "grafana_uid": grafana_uid} ) - # submit() already sorts the data for us, to prevent databag flapping and ensuing event storms + # publish() already sorts the data for us, to prevent databag flapping and ensuing event storms self.coordinator.datasource_exchange.publish(datasources=raw_datasources) def _reconcile(self): diff --git a/tests/scenario/conftest.py b/tests/scenario/conftest.py index 06c2550..d7523b9 100644 --- a/tests/scenario/conftest.py +++ b/tests/scenario/conftest.py @@ -1,4 +1,6 @@ import json +from pathlib import Path +from shutil import rmtree from unittest.mock import MagicMock, patch import pytest @@ -18,6 +20,14 @@ def patch_buffer_file_for_charm_tracing(tmp_path): yield +@pytest.fixture(autouse=True, scope="session") +def cleanup_prometheus_alert_rules(tmp_path): + # some tests trigger the charm to generate prometheus alert rules file in ./src; clean it up + yield + src_path = Path(__file__).parent / "src" + rmtree(src_path) + + @pytest.fixture(autouse=True, scope="session") def disable_charm_tracing(): with charm_tracing_disabled(): From 22bcae1e834377904d80dca2c7a40b945b02820b Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 11 Dec 2024 12:53:01 +0100 Subject: [PATCH 11/12] pr comments --- charmcraft.yaml | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/charmcraft.yaml b/charmcraft.yaml index 0c2d540..9a760d8 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -128,9 +128,9 @@ bases: parts: charm: - # uncomment this if you add git+ dependencies in requirements.txt - build-packages: - - "git" + # uncomment this if you add git+ dependencies in requirements.txt: + #build-packages: + # - "git" charm-binary-python-packages: - "pydantic>=2" - "cryptography" @@ -146,31 +146,31 @@ config: type: int default: 720 always_enable_zipkin: - description: + description: | Force-enable the receiver for the 'zipkin' protocol in Tempo, even if there is no integration currently requesting it. type: boolean default: false always_enable_otlp_grpc: - description: + description: | Force-enable the receiver for the 'otlp_grpc' protocol in Tempo, even if there is no integration currently requesting it. type: boolean default: false always_enable_otlp_http: - description: + description: | Force-enable the receiver for the 'otlp_http' protocol in Tempo, even if there is no integration currently requesting it. type: boolean default: false always_enable_jaeger_thrift_http: - description: + description: | Force-enable the receiver for the 'jaeger_thrift_http' protocol in Tempo, even if there is no integration currently requesting it. type: boolean default: false always_enable_jaeger_grpc: - description: + description: | Force-enable the receiver for the 'jaeger_grpc' protocol in Tempo, even if there is no integration currently requesting it. type: boolean From 91b841999f848a822cf7d72f9236ee375a994610 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Thu, 12 Dec 2024 10:44:06 +0100 Subject: [PATCH 12/12] adjusted comment --- src/charm.py | 15 +++++++++------ .../interface/test_grafana_datasource_exchange.py | 1 - 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/charm.py b/src/charm.py index ec41fd1..ba67c70 100755 --- a/src/charm.py +++ b/src/charm.py @@ -454,8 +454,10 @@ def _update_source_exchange(self) -> None: return # Each grafana we're sending our data to gives us back a mapping from unit names to datasource UIDs. - # so if we have two tempo units, and we're related to two grafanas, we'll get back: - # { + # so if we have two tempo units, and we're related to two grafanas, + # the grafana-source databag will look something like this: + # {"grafana_uid": "1234-1234-1234-1234", + # "datasource_uids": { # "grafana/0": { # "tempo/0": "0000-0000-0000-0000", # "tempo/1": "0000-0000-0000-0001" @@ -464,7 +466,7 @@ def _update_source_exchange(self) -> None: # "tempo/0": "0000-0000-0000-0000", # "tempo/1": "0000-0000-0000-0001" # }, - # } + # }} # This is an implementation detail, but the UID is 'unique-per-grafana' and ATM it turns out to be # deterministic given tempo's jujutopology, so if we are related to multiple grafanas, # the UIDs they assign to our units will be the same. @@ -475,13 +477,14 @@ def _update_source_exchange(self) -> None: # the unit we are sharing our data to might be talking to 'another grafana' which might be using a # different UID convention! - # To simplify our lives, we're going to assume that you're only relating each tempo to a single grafana!!! + # we might have multiple grafana-source relations, this method collects them all and returns a mapping from + # the `grafana_uid` to the contents of the `datasource_uids` field grafana_uids_to_units_to_uids = self.grafana_source_provider.get_source_uids() raw_datasources: List[DatasourceDict] = [] for grafana_uid, ds_uids in grafana_uids_to_units_to_uids.items(): - # we don't use the grafana name - for _unit_name, ds_uid in ds_uids.items(): + # we don't need the grafana unit name + for _, ds_uid in ds_uids.items(): # we also don't care about which unit's server we're looking at, since hopefully the data is the same. raw_datasources.append( {"type": "tempo", "uid": ds_uid, "grafana_uid": grafana_uid} diff --git a/tests/interface/test_grafana_datasource_exchange.py b/tests/interface/test_grafana_datasource_exchange.py index cc4bd7e..b2301de 100644 --- a/tests/interface/test_grafana_datasource_exchange.py +++ b/tests/interface/test_grafana_datasource_exchange.py @@ -8,7 +8,6 @@ def test_grafana_datasource_exchange_v0_interface( ): grafana_datasource_exchange_tester.configure( interface_name="grafana_datasource_exchange", - branch="add-grafana-uid-to-DS", interface_version=0, ) grafana_datasource_exchange_tester.run()