From 6104c943ee195d923de5b9908a1017ba29559596 Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Wed, 19 Jun 2024 19:02:34 +0300 Subject: [PATCH 1/2] Sync configs between units --- lib/charms/tempo_k8s/v1/charm_tracing.py | 22 +++++----- src/charm.py | 13 ++---- src/relations/peers.py | 10 +++++ src/relations/pgbouncer_provider.py | 42 +++++++++---------- .../unit/relations/test_pgbouncer_provider.py | 3 ++ 5 files changed, 49 insertions(+), 41 deletions(-) diff --git a/lib/charms/tempo_k8s/v1/charm_tracing.py b/lib/charms/tempo_k8s/v1/charm_tracing.py index 000e0cb57..dc84e3f4e 100644 --- a/lib/charms/tempo_k8s/v1/charm_tracing.py +++ b/lib/charms/tempo_k8s/v1/charm_tracing.py @@ -126,15 +126,14 @@ def my_tracing_endpoint(self) -> Optional[str]: from opentelemetry.sdk.resources import Resource from opentelemetry.sdk.trace import Span, TracerProvider from opentelemetry.sdk.trace.export import BatchSpanProcessor +from opentelemetry.trace import INVALID_SPAN, Tracer +from opentelemetry.trace import get_current_span as otlp_get_current_span from opentelemetry.trace import ( - INVALID_SPAN, - Tracer, get_tracer, get_tracer_provider, set_span_in_context, set_tracer_provider, ) -from opentelemetry.trace import get_current_span as otlp_get_current_span from ops.charm import CharmBase from ops.framework import Framework @@ -147,7 +146,7 @@ def my_tracing_endpoint(self) -> Optional[str]: # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 8 +LIBPATCH = 10 PYDEPS = ["opentelemetry-exporter-otlp-proto-http==1.21.0"] @@ -295,16 +294,17 @@ def wrap_init(self: CharmBase, framework: Framework, *args, **kwargs): # self.handle = Handle(None, self.handle_kind, None) original_event_context = framework._event_context + # default service name isn't just app name because it could conflict with the workload service name + _service_name = service_name or f"{self.app.name}-charm" - _service_name = service_name or self.app.name - + unit_name = self.unit.name resource = Resource.create( attributes={ "service.name": _service_name, "compose_service": _service_name, "charm_type": type(self).__name__, # juju topology - "juju_unit": self.unit.name, + "juju_unit": unit_name, "juju_application": self.app.name, "juju_model": self.model.name, "juju_model_uuid": self.model.uuid, @@ -342,16 +342,18 @@ def wrap_init(self: CharmBase, framework: Framework, *args, **kwargs): _tracer = get_tracer(_service_name) # type: ignore _tracer_token = tracer.set(_tracer) - dispatch_path = os.getenv("JUJU_DISPATCH_PATH", "") + dispatch_path = os.getenv("JUJU_DISPATCH_PATH", "") # something like hooks/install + event_name = dispatch_path.split("/")[1] if "/" in dispatch_path else dispatch_path + root_span_name = f"{unit_name}: {event_name} event" + span = _tracer.start_span(root_span_name, attributes={"juju.dispatch_path": dispatch_path}) # all these shenanigans are to work around the fact that the opentelemetry tracing API is built # on the assumption that spans will be used as contextmanagers. # Since we don't (as we need to close the span on framework.commit), # we need to manually set the root span as current. - span = _tracer.start_span("charm exec", attributes={"juju.dispatch_path": dispatch_path}) ctx = set_span_in_context(span) - # log a trace id so we can look it up in tempo. + # log a trace id, so we can pick it up from the logs (and jhack) to look it up in tempo. root_trace_id = hex(span.get_span_context().trace_id)[2:] # strip 0x prefix logger.debug(f"Starting root trace with id={root_trace_id!r}.") diff --git a/src/charm.py b/src/charm.py index d33e735a4..f921ee523 100755 --- a/src/charm.py +++ b/src/charm.py @@ -648,7 +648,7 @@ def generate_relation_databases(self) -> Dict[str, Dict[str, Union[str, bool]]]: self.set_relation_databases(databases) return databases - def _get_relation_config(self, inject_db=None) -> [Dict[str, Dict[str, Union[str, bool]]]]: + def _get_relation_config(self) -> [Dict[str, Dict[str, Union[str, bool]]]]: """Generate pgb config for databases and admin users.""" if not self.backend.relation or not (databases := self.get_relation_databases()): return {} @@ -685,13 +685,6 @@ def _get_relation_config(self, inject_db=None) -> [Dict[str, Dict[str, Union[str "port": r_port, "auth_user": self.backend.auth_user, } - if inject_db: - pgb_dbs[inject_db] = { - "host": host, - "dbname": inject_db, - "port": port, - "auth_user": self.backend.auth_user, - } if "*" in databases: pgb_dbs["*"] = { "host": host, @@ -701,7 +694,7 @@ def _get_relation_config(self, inject_db=None) -> [Dict[str, Dict[str, Union[str } return pgb_dbs - def render_pgb_config(self, reload_pgbouncer=False, restart=False, inject_db=None): + def render_pgb_config(self, reload_pgbouncer=False, restart=False): """Derives config files for the number of required services from given config. This method takes a primary config and generates one unique config for each intended @@ -739,7 +732,7 @@ def render_pgb_config(self, reload_pgbouncer=False, restart=False, inject_db=Non reserve_pool_size = math.ceil(effective_db_connections / 4) with open("templates/pgb_config.j2", "r") as file: template = Template(file.read()) - databases = self._get_relation_config(inject_db=inject_db) + databases = self._get_relation_config() readonly_dbs = self._get_readonly_dbs(databases) enable_tls = all(self.tls.get_tls_files()) and self._is_exposed if self._is_exposed: diff --git a/src/relations/peers.py b/src/relations/peers.py index 948586210..2738ef8b3 100644 --- a/src/relations/peers.py +++ b/src/relations/peers.py @@ -26,6 +26,7 @@ """ # noqa: W505 import logging +from hashlib import shake_128 from typing import List, Optional, Set from ops.charm import CharmBase, HookEvent @@ -132,7 +133,16 @@ def _on_changed(self, event: HookEvent): if self.charm.backend.postgres: self.charm.render_prometheus_service() + # TODO if we track the secret id as well we can reload only when things change + readonly_dbs_hash = shake_128( + self.app_databag.get("readonly_dbs", "[]").encode() + ).hexdigest(16) + pgb_dbs_hash = shake_128(self.app_databag.get("pgb_dbs_config", "{}").encode()).hexdigest( + 16 + ) self.charm.render_pgb_config(reload_pgbouncer=True) + self.unit_databag["readonly_dbs"] = readonly_dbs_hash + self.unit_databag["pgb_dbs"] = pgb_dbs_hash def _on_departed(self, _): self.update_leader() diff --git a/src/relations/pgbouncer_provider.py b/src/relations/pgbouncer_provider.py index e98a925e9..d9eab5546 100644 --- a/src/relations/pgbouncer_provider.py +++ b/src/relations/pgbouncer_provider.py @@ -36,6 +36,7 @@ """ # noqa: W505 import logging +from hashlib import shake_128 from charms.data_platform_libs.v0.data_interfaces import ( DatabaseProvides, @@ -48,7 +49,7 @@ PostgreSQLDeleteUserError, PostgreSQLGetPostgreSQLVersionError, ) -from ops.charm import CharmBase, RelationBrokenEvent, RelationChangedEvent, RelationDepartedEvent +from ops.charm import CharmBase, RelationBrokenEvent, RelationDepartedEvent from ops.framework import Object from ops.model import ( Application, @@ -91,9 +92,6 @@ def __init__(self, charm: CharmBase, relation_name: str = CLIENT_RELATION_NAME) self.framework.observe( charm.on[self.relation_name].relation_broken, self._on_relation_broken ) - self.framework.observe( - charm.on[self.relation_name].relation_changed, self._on_relation_changed - ) def _depart_flag(self, relation): return f"{self.relation_name}_{relation.id}_departing" @@ -101,15 +99,6 @@ def _depart_flag(self, relation): def _unit_departing(self, relation): return self.charm.peers.unit_databag.get(self._depart_flag(relation), None) == "true" - def _on_relation_changed(self, event: RelationChangedEvent) -> None: - """Injects the database name for follower units.""" - if not self.charm.unit.is_leader(): - if "database" in event.relation.data[event.app]: - database = event.relation.data[event.app]["database"] - dbs = self.charm.generate_relation_databases() - if database not in [db["name"] for db in dbs]: - self.charm.render_pgb_config(reload_pgbouncer=True, inject_db=database) - def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: """Handle the client relation-requested event. @@ -125,7 +114,7 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: for key in self.charm.peers.relation.data.keys(): if key != self.charm.app: if self.charm.peers.relation.data[key].get("auth_file_set", "false") != "true": - logger.info("Backend credentials not yet set on all units") + logger.debug("Backend credentials not yet set on all units") event.defer() return @@ -134,6 +123,24 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: extra_user_roles = event.extra_user_roles or "" rel_id = event.relation.id + dbs = self.charm.generate_relation_databases() + dbs[str(event.relation.id)] = {"name": database, "legacy": False} + roles = extra_user_roles.lower().split(",") + if "admin" in roles or "superuser" in roles: + dbs["*"] = {"name": "*", "auth_dbname": database} + self.charm.set_relation_databases(dbs) + + pgb_dbs_hash = shake_128( + self.charm.peers.app_databag["pgb_dbs_config"].encode() + ).hexdigest(16) + for key in self.charm.peers.relation.data.keys(): + # We skip the leader so we don't have to wait on the defer + if key != self.charm.app and key != self.charm.unit: + if self.charm.peers.relation.data[key].get("pgb_dbs", "") != pgb_dbs_hash: + logger.debug("Not all units have synced configuration") + event.defer() + return + # Creates the user and the database for this specific relation. user = f"relation_id_{rel_id}" logger.debug("generating relation user") @@ -160,13 +167,6 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: ) return - dbs = self.charm.generate_relation_databases() - dbs[str(event.relation.id)] = {"name": database, "legacy": False} - roles = extra_user_roles.lower().split(",") - if "admin" in roles or "superuser" in roles: - dbs["*"] = {"name": "*", "auth_dbname": database} - self.charm.set_relation_databases(dbs) - self.charm.render_pgb_config(reload_pgbouncer=True) # Share the credentials and updated connection info with the client application. diff --git a/tests/unit/relations/test_pgbouncer_provider.py b/tests/unit/relations/test_pgbouncer_provider.py index b4e1bbbcb..a4178e13d 100644 --- a/tests/unit/relations/test_pgbouncer_provider.py +++ b/tests/unit/relations/test_pgbouncer_provider.py @@ -109,6 +109,9 @@ def test_on_database_requested( self.harness.update_relation_data( self.peers_rel_id, self.unit, {"auth_file_set": "true"} ) + self.harness.update_relation_data( + self.peers_rel_id, self.app, {"pgb_dbs_config": "{}"} + ) self.client_relation._on_database_requested(event) # Verify we've called everything we should From 9acc69eaef493af1daf3dc80fabaad4b034d4bc3 Mon Sep 17 00:00:00 2001 From: Dragomir Penev Date: Wed, 19 Jun 2024 21:05:19 +0300 Subject: [PATCH 2/2] Remove readonly db hash --- src/relations/peers.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/relations/peers.py b/src/relations/peers.py index 2738ef8b3..7c74e2f2f 100644 --- a/src/relations/peers.py +++ b/src/relations/peers.py @@ -133,15 +133,10 @@ def _on_changed(self, event: HookEvent): if self.charm.backend.postgres: self.charm.render_prometheus_service() - # TODO if we track the secret id as well we can reload only when things change - readonly_dbs_hash = shake_128( - self.app_databag.get("readonly_dbs", "[]").encode() - ).hexdigest(16) pgb_dbs_hash = shake_128(self.app_databag.get("pgb_dbs_config", "{}").encode()).hexdigest( 16 ) self.charm.render_pgb_config(reload_pgbouncer=True) - self.unit_databag["readonly_dbs"] = readonly_dbs_hash self.unit_databag["pgb_dbs"] = pgb_dbs_hash def _on_departed(self, _):