From 1b0d2b3922f09a62f9acb7d9d1f7678312369227 Mon Sep 17 00:00:00 2001 From: Ryan Barry Date: Mon, 28 Mar 2022 18:50:34 -0400 Subject: [PATCH] Make sure that ingress works flexibly with `GrafanaSourceProvider` (#253) * Update the grafana source libraries * Pass _external_url to GrafanaSourceProvider and add unit tests * Remove unnecessary test setup * Update to the latest draft of grafana_source * Update Traefik ingress-unit library Co-authored-by: Michele Mancioppi --- lib/charms/grafana_k8s/v0/grafana_source.py | 222 +++++++++++++----- lib/charms/traefik_k8s/v0/ingress_per_unit.py | 13 +- src/charm.py | 14 +- tests/unit/test_charm.py | 17 +- 4 files changed, 203 insertions(+), 63 deletions(-) diff --git a/lib/charms/grafana_k8s/v0/grafana_source.py b/lib/charms/grafana_k8s/v0/grafana_source.py index 482d523a..09ee7389 100644 --- a/lib/charms/grafana_k8s/v0/grafana_source.py +++ b/lib/charms/grafana_k8s/v0/grafana_source.py @@ -25,12 +25,16 @@ The default arguments are: `charm`: `self` from the charm instantiating this library + `source_type`: None + `source_port`: None + `source_url`: None `relation_name`: grafana-source `refresh_event`: A `PebbleReady` event from `charm`, used to refresh the IP address sent to Grafana on a charm lifecycle event or pod restart - `source_type`: prometheus - `source_port`: 9090 + +The value of `source_url` should be a fully-resolvable URL for a valid Grafana +source, e.g., `http://example.com/api` or similar. If your configuration requires any changes from these defaults, they may be set from the class constructor. It may be instantiated as @@ -42,7 +46,9 @@ class FooCharm: def __init__(self, *args): super().__init__(*args, **kwargs) ... - self.grafana_source_provider = GrafanaSourceProvider(self) + self.grafana_source_provider = GrafanaSourceProvider( + self, source_type="prometheus", source_port="9090" + ) ... The first argument (`self`) should be a reference to the parent (datasource) @@ -66,7 +72,8 @@ def __init__(self, *args): "type": source_type, }, "unit/0": { - "uri": {ip_address}:{port} # `ip_address` is derived at runtime, `port` from the constructor + "uri": {ip_address}:{port}{path} # `ip_address` is derived at runtime, `port` from the constructor, + # and `path` from the constructor, if specified }, ``` @@ -120,11 +127,13 @@ def __init__(self, *args): import json import logging -from typing import Dict, List, Optional, Union +import re +from typing import Any, Dict, List, Optional, Union from ops.charm import ( CharmBase, CharmEvents, + RelationChangedEvent, RelationDepartedEvent, RelationEvent, RelationJoinedEvent, @@ -150,11 +159,12 @@ 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 = 8 +LIBPATCH = 9 logger = logging.getLogger(__name__) DEFAULT_RELATION_NAME = "grafana-source" +DEFAULT_PEER_NAME = "grafana" RELATION_INTERFACE_NAME = "grafana_datasource" @@ -305,15 +315,14 @@ class GrafanaSourceEvents(ObjectEvents): class GrafanaSourceProvider(Object): """A provider object for Grafana datasources.""" - _stored = StoredState() - def __init__( self, charm: CharmBase, + source_type: str, + source_port: Optional[str] = "", + source_url: Optional[str] = "", refresh_event: Optional[BoundEvent] = None, relation_name: str = DEFAULT_RELATION_NAME, - source_type: Optional[str] = "prometheus", - source_port: Optional[str] = "9090", ) -> None: """Construct a Grafana charm client. @@ -335,6 +344,15 @@ def __init__( charm: a :class:`CharmBase` object which manages this :class:`GrafanaSourceProvider` object. Generally this is `self` in the instantiating class. + source_type: an optional (default `prometheus`) source type + required for Grafana configuration. The value must match + the DataSource type from the Grafana perspective. + source_port: an optional (default `9090`) source port + required for Grafana configuration. + source_url: an optional source URL which can be used, for example, if + ingress for a source is enabled, or a URL path to the API consumed + by the datasource must be specified for another reason. If set, + 'source_port' will not be used. relation_name: string name of the relation that is provides the Grafana source service. It is strongly advised not to change the default, so that people deploying your charm will have a @@ -343,11 +361,6 @@ def __init__( refresh_event: a :class:`CharmEvents` event on which the IP address should be refreshed in case of pod or machine/VM restart. - source_type: an optional (default `prometheus`) source type - required for Grafana configuration. The value must match - the DataSource type from the Grafana perspective. - source_port: an optional (default `9090`) source port - required for Grafana configuration. """ _validate_relation_by_interface_and_direction( charm, relation_name, RELATION_INTERFACE_NAME, RelationRole.provides @@ -358,23 +371,58 @@ def __init__( self._relation_name = relation_name events = self._charm.on[relation_name] - refresh_event = refresh_event or self._charm.on.pebble_ready - self._source_type = source_type + + if not refresh_event: + if len(self._charm.meta.containers) == 1: + container = list(self._charm.meta.containers.values())[0] + refresh_event = self._charm.on[container.name.replace("-", "_")].pebble_ready + + if source_port and source_url: + logger.warning( + "Both `source_port` and `source_url` were specified! Using " + "`source_url` as the address." + ) + + if source_url and not re.match(r"^\w+://", source_url): + logger.warning( + "'source_url' should start with a scheme, such as " + "'http://'. Assuming 'http://' since none is present." + ) + source_url = "http://{}".format(source_url) + self._source_port = source_port + self._source_url = source_url + + self.framework.observe(events.relation_joined, self._set_sources_from_event) + if refresh_event: + self.framework.observe(refresh_event, self._set_unit_details) + + def update_source(self, source_url: Optional[str] = ""): + """Trigger the update of relation data.""" + if source_url: + self._source_url = source_url + + rel = self._charm.model.get_relation(self._relation_name) + + if not rel: + return + + self._set_sources(rel) - self.framework.observe(events.relation_joined, self._set_sources) - self.framework.observe(refresh_event, self._set_unit_ip) + def _set_sources_from_event(self, event: RelationJoinedEvent) -> None: + """Get a `Relation` object from the event to pass on.""" + self._set_sources(event.relation) - def _set_sources(self, event: RelationJoinedEvent): + def _set_sources(self, rel: Relation): """Inform the consumer about the source configuration.""" - self._set_unit_ip(event) + self._set_unit_details(rel) if not self._charm.unit.is_leader(): return logger.debug("Setting Grafana data sources: %s", self._scrape_data) - event.relation.data[self._charm.app]["grafana_source_data"] = json.dumps(self._scrape_data) + rel.data[self._charm.app]["grafana_source_data"] = json.dumps(self._scrape_data) @property def _scrape_data(self) -> Dict: @@ -391,8 +439,8 @@ def _scrape_data(self) -> Dict: } return data - def _set_unit_ip(self, _: Union[BoundEvent, RelationEvent]): - """Set unit host address. + def _set_unit_details(self, _: Union[BoundEvent, RelationEvent, Relation]): + """Set unit host details. Each time a provider charm container is restarted it updates its own host address in the unit relation data for the Prometheus consumer. @@ -402,12 +450,18 @@ def _set_unit_ip(self, _: Union[BoundEvent, RelationEvent]): # that it's valid before passing it. Otherwise, we'll catch is on pebble_ready. # The provider side already skips adding it if `grafana_source_host` is not set, # so no additional guards needed - address = self._charm.model.get_binding(relation).network.bind_address - if address: - relation.data[self._charm.unit]["grafana_source_host"] = "{}:{}".format( - str(address), - self._source_port, - ) + url = None + if self._source_url: + url = self._source_url + else: + address = self._charm.model.get_binding(relation).network.bind_address + if address: + url = "{}:{}".format(str(address), self._source_port) + + # If _source_url was not set in the constructor and there are no units in the + # relation or pebble or address was not bound, this may not be set + if url: + relation.data[self._charm.unit]["grafana_source_host"] = url class GrafanaSourceConsumer(Object): @@ -416,7 +470,11 @@ class GrafanaSourceConsumer(Object): on = GrafanaSourceEvents() _stored = StoredState() - def __init__(self, charm: CharmBase, relation_name: str = DEFAULT_RELATION_NAME) -> None: + def __init__( + self, + charm: CharmBase, + relation_name: str = DEFAULT_RELATION_NAME, + ) -> None: """A Grafana based Monitoring service consumer, i.e., the charm that uses a datasource. Args: @@ -437,6 +495,8 @@ def __init__(self, charm: CharmBase, relation_name: str = DEFAULT_RELATION_NAME) self._charm = charm events = self._charm.on[relation_name] + # We're stuck with this forever now so upgrades work, or until such point as we can + # break compatibility self._stored.set_default( sources=dict(), sources_to_delete=set(), @@ -444,6 +504,10 @@ def __init__(self, charm: CharmBase, relation_name: str = DEFAULT_RELATION_NAME) self.framework.observe(events.relation_changed, self._on_grafana_source_relation_changed) self.framework.observe(events.relation_departed, self._on_grafana_source_relation_departed) + self.framework.observe( + self._charm.on[DEFAULT_PEER_NAME].relation_changed, + self._on_grafana_peer_changed, + ) def _on_grafana_source_relation_changed(self, event: CharmEvents) -> None: """Handle relation changes in related providers. @@ -458,19 +522,24 @@ def _on_grafana_source_relation_changed(self, event: CharmEvents) -> None: The Grafana charm can then respond to the event to update its configuration. """ - if not self._charm.unit.is_leader(): - return + if self._charm.unit.is_leader(): + sources = {} - sources = {} + for rel in self._charm.model.relations[self._relation_name]: + source = self._get_source_config(rel) + if source: + sources[rel.id] = source - for rel in self._charm.model.relations[self._relation_name]: - source = self._get_source_config(rel) - if source: - sources[rel.id] = source + self.set_peer_data("sources", sources) - self._stored.sources = sources + self.on.sources_changed.emit() + def _on_grafana_peer_changed(self, _: RelationChangedEvent) -> None: + """Emit source events on peer events so secondary charm data updates.""" + if self._charm.unit.is_leader(): + return self.on.sources_changed.emit() + self.on.sources_to_delete_changed.emit() def _get_source_config(self, rel: Relation): """Generate configuration from data stored in relation data by providers.""" @@ -480,6 +549,7 @@ def _get_source_config(self, rel: Relation): data = [] + sources_to_delete = self.get_peer_data("sources_to_delete") for unit_name, host_addr in self._relation_hosts(rel).items(): unique_source_name = "juju_{}_{}_{}_{}".format( source_data["model"], @@ -488,17 +558,22 @@ def _get_source_config(self, rel: Relation): unit_name.split("/")[1], ) + host = ( + "http://{}".format(host_addr) if not re.match(r"^\w+://", host_addr) else host_addr + ) + host_data = { "unit": unit_name, "source_name": unique_source_name, "source_type": source_data["type"], - "url": "http://{}".format(host_addr), + "url": host, } - if host_data["source_name"] in self._stored.sources_to_delete: - self._stored.sources_to_delete.remove(host_data["source_name"]) + if host_data["source_name"] in sources_to_delete: + sources_to_delete.remove(host_data["source_name"]) data.append(host_data) + self.set_peer_data("sources_to_delete", list(sources_to_delete)) return data def _relation_hosts(self, rel: Relation) -> Dict: @@ -528,20 +603,26 @@ def _on_grafana_source_relation_departed(self, event: RelationDepartedEvent) -> added to a list of sources to remove, and other providers are informed through a :class:`GrafanaSourcesChanged` event. """ - if not self._charm.unit.is_leader(): - return + removed_source = False + if self._charm.unit.is_leader(): + removed_source = self._remove_source_from_datastore(event) - self._remove_source_from_datastore(event) + if removed_source: + self.on.sources_to_delete_changed.emit() - def _remove_source_from_datastore(self, event: RelationDepartedEvent) -> None: + def _remove_source_from_datastore(self, event: RelationDepartedEvent) -> bool: """Remove the grafana-source from the datastore. Add the name to the list of sources to remove when a relation is broken. + + Returns a boolean indicating whether an event should be emitted. """ rel_id = event.relation.id logger.debug("Removing all data for relation: {}".format(rel_id)) - removed_source = self._stored.sources.pop(rel_id, None) + stored_sources = self.get_peer_data("sources") + + removed_source = stored_sources.pop(str(rel_id), None) if removed_source: if event.unit: # Remove one unit only @@ -549,23 +630,29 @@ def _remove_source_from_datastore(self, event: RelationDepartedEvent) -> None: self._remove_source(dead_unit["source_name"]) # Re-update the list of stored sources - self._stored.sources[rel_id] = [ + stored_sources[rel_id] = [ dict(s) for s in removed_source if s["unit"] != event.unit.name ] else: for host in removed_source: self._remove_source(host["source_name"]) - self.on.sources_to_delete_changed.emit() + self.set_peer_data("sources", stored_sources) + return True + return False def _remove_source(self, source_name: str) -> None: """Remove a datasource by name.""" - self._stored.sources_to_delete.add(source_name) + sources_to_delete = self.get_peer_data("sources_to_delete") + if source_name not in sources_to_delete: + sources_to_delete.append(source_name) + self.set_peer_data("sources_to_delete", sources_to_delete) def upgrade_keys(self) -> None: """On upgrade, ensure stored data maintains compatibility.""" # self._stored.sources may have hyphens instead of underscores in key names. # Make sure they reconcile. + self._set_default_data() sources = _type_convert_stored(self._stored.sources) for rel_id in sources.keys(): for i in range(len(sources[rel_id])): @@ -573,13 +660,26 @@ def upgrade_keys(self) -> None: {k.replace("-", "_"): v for k, v in sources[rel_id][i].items()} ) - self._stored.sources = sources + # If there's stored data, merge it and purge it + if self._stored.sources: + self._stored.sources = {} + peer_sources = self.get_peer_data("sources") + sources.update(peer_sources) + self.set_peer_data("sources", sources) + + if self._stored.sources_to_delete: + old_sources_to_delete = _type_convert_stored(self._stored.sources_to_delete) + self._stored.sources_to_delete = set() + peer_sources_to_delete = set(self.get_peer_data("sources_to_delete")) + sources_to_delete = set.union(old_sources_to_delete, peer_sources_to_delete) + self.set_peer_data("sources_to_delete", sources_to_delete) @property def sources(self) -> List[dict]: """Returns an array of sources the source_consumer knows about.""" sources = [] - for source in self._stored.sources.values(): + stored_sources = self.get_peer_data("sources") + for source in stored_sources.values(): sources.extend([host for host in _type_convert_stored(source)]) return sources @@ -587,4 +687,20 @@ def sources(self) -> List[dict]: @property def sources_to_delete(self) -> List[str]: """Returns an array of source names which have been removed.""" - return [_type_convert_stored(source) for source in self._stored.sources_to_delete] + return self.get_peer_data("sources_to_delete") + + def _set_default_data(self) -> None: + """Set defaults if they are not in peer relation data.""" + data = {"sources": {}, "sources_to_delete": []} # type: ignore + for k, v in data.items(): + if not self.get_peer_data(k): + self.set_peer_data(k, v) + + def set_peer_data(self, key: str, data: Any) -> None: + """Put information into the peer data bucket instead of `StoredState`.""" + self._charm.peers.data[self._charm.app][key] = json.dumps(data) # type: ignore + + def get_peer_data(self, key: str) -> Any: + """Retrieve information from the peer data bucket instead of `StoredState`.""" + data = self._charm.peers.data[self._charm.app].get(key, "") # type: ignore + return json.loads(data) if data else {} diff --git a/lib/charms/traefik_k8s/v0/ingress_per_unit.py b/lib/charms/traefik_k8s/v0/ingress_per_unit.py index 661000e5..21c58a98 100644 --- a/lib/charms/traefik_k8s/v0/ingress_per_unit.py +++ b/lib/charms/traefik_k8s/v0/ingress_per_unit.py @@ -52,7 +52,7 @@ def _handle_ingress_per_unit(self, event): import logging from typing import Optional -from ops.charm import CharmBase, RelationEvent, RelationRole +from ops.charm import CharmBase, RelationBrokenEvent, RelationEvent, RelationRole from ops.framework import EventSource from ops.model import Relation, Unit @@ -85,7 +85,7 @@ def _handle_ingress_per_unit(self, event): # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 4 +LIBPATCH = 6 log = logging.getLogger(__name__) @@ -352,6 +352,10 @@ def __init__( if port: self.auto_data = self._complete_request(host or "", port) + # Workaround for SDI not marking the EndpointWrapper as not + # ready upon a relation broken event + self.is_relation_broken = False + self.framework.observe( self.charm.on[self.endpoint].relation_changed, self._emit_ingress_change_event ) @@ -360,6 +364,9 @@ def __init__( ) def _emit_ingress_change_event(self, event): + if isinstance(event, RelationBrokenEvent): + self.is_relation_broken = True + # TODO Avoid spurious events, emit only when URL changes self.on.ingress_changed.emit(self.relation) @@ -398,7 +405,7 @@ def urls(self): May return an empty dict if the URLs aren't available yet. """ - if not self.is_ready(): + if self.is_relation_broken or not self.is_ready(): return {} data = self.unwrap(self.relation) ingress = data[self.relation.app].get("ingress", {}) diff --git a/src/charm.py b/src/charm.py index d10e2c79..c9fe18ca 100755 --- a/src/charm.py +++ b/src/charm.py @@ -51,12 +51,6 @@ def __init__(self, *args): # Relation handler objects - # Allows Grafana to aggregate metrics - self.grafana_source_consumer = GrafanaSourceProvider( - charm=self, - refresh_event=self.on.prometheus_pebble_ready, - ) - # Gathers scrape job information from metrics endpoints self.metrics_consumer = MetricsEndpointConsumer(self) @@ -75,6 +69,13 @@ def __init__(self, *args): endpoint_path=f"{external_url.path}/api/v1/write", ) + # Allows Grafana to aggregate metrics + self.grafana_source_consumer = GrafanaSourceProvider( + charm=self, + source_type="prometheus", + source_url=self._external_url, + ) + # Maintains list of Alertmanagers to which alerts are forwarded self.alertmanager_consumer = AlertmanagerConsumer(self, relation_name="alertmanager") @@ -155,6 +156,7 @@ def _configure(self, _): # Make sure that if the remote_write endpoint changes, it is reflected in relation data. self.remote_write_provider.update_endpoint() + self.grafana_source_consumer.update_source(self._external_url) self.unit.status = ActiveStatus() diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index c1d76fe3..ada743cf 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -34,10 +34,25 @@ def setUp(self, *unused): def test_grafana_is_provided_port_and_source(self, *unused): rel_id = self.harness.add_relation("grafana-source", "grafana") self.harness.add_relation_unit(rel_id, "grafana/0") + fqdn = socket.getfqdn() grafana_host = self.harness.get_relation_data(rel_id, self.harness.model.unit.name)[ "grafana_source_host" ] - self.assertEqual(grafana_host, "{}:{}".format("1.1.1.1", "9090")) + self.assertEqual(grafana_host, "http://{}:{}".format(fqdn, "9090")) + + @patch_network_get(private_address="1.1.1.1") + def test_web_external_url_is_passed_to_grafana(self, *unused): + self.harness.set_leader(True) + self.harness.update_config({"web_external_url": "http://test:80/foo/bar"}) + + grafana_rel_id = self.harness.add_relation("grafana-source", "grafana") + self.harness.add_relation_unit(grafana_rel_id, "grafana/0") + + grafana_host = self.harness.get_relation_data( + grafana_rel_id, self.harness.model.unit.name + )["grafana_source_host"] + + self.assertEqual(grafana_host, "http://test:80/foo/bar") def test_default_cli_log_level_is_info(self): plan = self.harness.get_container_pebble_plan("prometheus")