From d0af04794698df4ae2a5166f30607ebb1d7cd975 Mon Sep 17 00:00:00 2001 From: shayancanonical <99665202+shayancanonical@users.noreply.github.com> Date: Mon, 9 Dec 2024 07:33:57 -0500 Subject: [PATCH] Standardize abstract_charm.py file with the K8s charm (#182) ## Issue The `abstract_charm.py` has diverged with the K8s charm following in support of HACluster charm integration + `expose-external` config in the K8s charm. ## Solution Standardize the files Counterpart PR in K8s charm: https://github.com/canonical/mysql-router-k8s-operator/pull/328 --- .github/workflows/ci.yaml | 2 +- .github/workflows/release.yaml | 4 +- .github/workflows/sync_docs.yaml | 2 +- .../data_platform_libs/v0/data_interfaces.py | 21 +++++- .../tempo_coordinator_k8s/v0/charm_tracing.py | 5 +- .../tempo_coordinator_k8s/v0/tracing.py | 9 ++- src/abstract_charm.py | 53 ++++++++------ src/machine_charm.py | 26 +++++-- src/relations/database_providers_wrapper.py | 32 ++++----- src/relations/database_provides.py | 72 ++++++++++--------- 10 files changed, 141 insertions(+), 85 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index e3fd72fd..813683ea 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -66,7 +66,7 @@ jobs: build: name: Build charm - uses: canonical/data-platform-workflows/.github/workflows/build_charm.yaml@v23.1.0 + uses: canonical/data-platform-workflows/.github/workflows/build_charm.yaml@v23.1.1 with: # Use of cache blocked by https://github.com/canonical/charmcraft/issues/1456 # Details: https://github.com/canonical/charmcraftcache/issues/3 diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index 9233dd6a..3eca85dd 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -15,14 +15,14 @@ jobs: build: name: Build charm - uses: canonical/data-platform-workflows/.github/workflows/build_charm.yaml@v23.1.0 + uses: canonical/data-platform-workflows/.github/workflows/build_charm.yaml@v23.1.1 release: name: Release charm needs: - ci-tests - build - uses: canonical/data-platform-workflows/.github/workflows/release_charm.yaml@v23.1.0 + uses: canonical/data-platform-workflows/.github/workflows/release_charm.yaml@v23.1.1 with: channel: dpe/edge artifact-prefix: ${{ needs.build.outputs.artifact-prefix }} diff --git a/.github/workflows/sync_docs.yaml b/.github/workflows/sync_docs.yaml index a094605c..14aa1c26 100644 --- a/.github/workflows/sync_docs.yaml +++ b/.github/workflows/sync_docs.yaml @@ -10,7 +10,7 @@ on: jobs: sync-docs: name: Sync docs from Discourse - uses: canonical/data-platform-workflows/.github/workflows/sync_docs.yaml@v23.1.0 + uses: canonical/data-platform-workflows/.github/workflows/sync_docs.yaml@v23.1.1 with: reviewers: a-velasco permissions: diff --git a/lib/charms/data_platform_libs/v0/data_interfaces.py b/lib/charms/data_platform_libs/v0/data_interfaces.py index aaed2e52..3bc2dd85 100644 --- a/lib/charms/data_platform_libs/v0/data_interfaces.py +++ b/lib/charms/data_platform_libs/v0/data_interfaces.py @@ -331,7 +331,7 @@ def _on_topic_requested(self, event: TopicRequestedEvent): # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 39 +LIBPATCH = 40 PYDEPS = ["ops>=2.0.0"] @@ -391,6 +391,10 @@ class IllegalOperationError(DataInterfacesError): """To be used when an operation is not allowed to be performed.""" +class PrematureDataAccessError(DataInterfacesError): + """To be raised when the Relation Data may be accessed (written) before protocol init complete.""" + + ############################################################################## # Global helpers / utilities ############################################################################## @@ -1453,6 +1457,8 @@ def _on_relation_changed_event(self, event: RelationChangedEvent) -> None: class ProviderData(Data): """Base provides-side of the data products relation.""" + RESOURCE_FIELD = "database" + def __init__( self, model: Model, @@ -1618,6 +1624,15 @@ def _fetch_my_specific_relation_data( def _update_relation_data(self, relation: Relation, data: Dict[str, str]) -> None: """Set values for fields not caring whether it's a secret or not.""" req_secret_fields = [] + + keys = set(data.keys()) + if self.fetch_relation_field(relation.id, self.RESOURCE_FIELD) is None and ( + keys - {"endpoints", "read-only-endpoints", "replset"} + ): + raise PrematureDataAccessError( + "Premature access to relation data, update is forbidden before the connection is initialized." + ) + if relation.app: req_secret_fields = get_encoded_list(relation, relation.app, REQ_SECRET_FIELDS) @@ -3290,6 +3305,8 @@ class KafkaRequiresEvents(CharmEvents): class KafkaProviderData(ProviderData): """Provider-side of the Kafka relation.""" + RESOURCE_FIELD = "topic" + def __init__(self, model: Model, relation_name: str) -> None: super().__init__(model, relation_name) @@ -3539,6 +3556,8 @@ class OpenSearchRequiresEvents(CharmEvents): class OpenSearchProvidesData(ProviderData): """Provider-side of the OpenSearch relation.""" + RESOURCE_FIELD = "index" + def __init__(self, model: Model, relation_name: str) -> None: super().__init__(model, relation_name) diff --git a/lib/charms/tempo_coordinator_k8s/v0/charm_tracing.py b/lib/charms/tempo_coordinator_k8s/v0/charm_tracing.py index 1e7ff840..3aea50f0 100644 --- a/lib/charms/tempo_coordinator_k8s/v0/charm_tracing.py +++ b/lib/charms/tempo_coordinator_k8s/v0/charm_tracing.py @@ -69,6 +69,9 @@ def my_tracing_endpoint(self) -> Optional[str]: - every event as a span (including custom events) - every charm method call (except dunders) as a span +We recommend that you scale up your tracing provider and relate it to an ingress so that your tracing requests +go through the ingress and get load balanced across all units. Otherwise, if the provider's leader goes down, your tracing goes down. + ## TLS support If your charm integrates with a TLS provider which is also trusted by the tracing provider (the Tempo charm), @@ -269,7 +272,7 @@ def _remove_stale_otel_sdk_packages(): # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 2 +LIBPATCH = 3 PYDEPS = ["opentelemetry-exporter-otlp-proto-http==1.21.0"] diff --git a/lib/charms/tempo_coordinator_k8s/v0/tracing.py b/lib/charms/tempo_coordinator_k8s/v0/tracing.py index 1f92867f..2035dffd 100644 --- a/lib/charms/tempo_coordinator_k8s/v0/tracing.py +++ b/lib/charms/tempo_coordinator_k8s/v0/tracing.py @@ -34,7 +34,7 @@ def __init__(self, *args): `TracingEndpointRequirer.request_protocols(*protocol:str, relation:Optional[Relation])` method. Using this method also allows you to use per-relation protocols. -Units of provider charms obtain the tempo endpoint to which they will push their traces by calling +Units of requirer charms obtain the tempo endpoint to which they will push their traces by calling `TracingEndpointRequirer.get_endpoint(protocol: str)`, where `protocol` is, for example: - `otlp_grpc` - `otlp_http` @@ -44,7 +44,10 @@ def __init__(self, *args): If the `protocol` is not in the list of protocols that the charm requested at endpoint set-up time, the library will raise an error. -## Requirer Library Usage +We recommend that you scale up your tracing provider and relate it to an ingress so that your tracing requests +go through the ingress and get load balanced across all units. Otherwise, if the provider's leader goes down, your tracing goes down. + +## Provider Library Usage The `TracingEndpointProvider` object may be used by charms to manage relations with their trace sources. For this purposes a Tempo-like charm needs to do two things @@ -107,7 +110,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 = 2 +LIBPATCH = 3 PYDEPS = ["pydantic"] diff --git a/src/abstract_charm.py b/src/abstract_charm.py index 4d1ec976..bb01307f 100644 --- a/src/abstract_charm.py +++ b/src/abstract_charm.py @@ -100,23 +100,29 @@ def _logrotate(self) -> logrotate.LogRotate: @property @abc.abstractmethod - def _read_write_endpoint(self) -> str: + def _read_write_endpoints(self) -> str: """MySQL Router read-write endpoint""" @property @abc.abstractmethod - def _read_only_endpoint(self) -> str: + def _read_only_endpoints(self) -> str: """MySQL Router read-only endpoint""" @property @abc.abstractmethod - def _exposed_read_write_endpoint(self) -> str: - """The exposed read-write endpoint""" + def _exposed_read_write_endpoints(self) -> typing.Optional[str]: + """The exposed read-write endpoint. + + Only defined in vm charm. + """ @property @abc.abstractmethod - def _exposed_read_only_endpoint(self) -> str: - """The exposed read-only endpoint""" + def _exposed_read_only_endpoints(self) -> typing.Optional[str]: + """The exposed read-only endpoint. + + Only defined in vm charm. + """ @abc.abstractmethod def is_externally_accessible(self, *, event) -> typing.Optional[bool]: @@ -125,6 +131,11 @@ def is_externally_accessible(self, *, event) -> typing.Optional[bool]: Only defined in vm charm to return True/False. In k8s charm, returns None. """ + @property + @abc.abstractmethod + def _status(self) -> ops.StatusBase: + """Status of the charm.""" + @property def _tls_certificate_saved(self) -> bool: """Whether a TLS certificate is available to use""" @@ -202,6 +213,8 @@ def _determine_app_status(self, *, event) -> ops.StatusBase: # (Relations should not be modified during upgrade.) return upgrade_status statuses = [] + if self._status: + statuses.append(self._status) for endpoint in (self._database_requires, self._database_provides): if status := endpoint.get_status(event): statuses.append(status) @@ -236,8 +249,8 @@ def wait_until_mysql_router_ready(self, *, event) -> None: """ @abc.abstractmethod - def _reconcile_node_port(self, *, event) -> None: - """Reconcile node port. + def _reconcile_service(self) -> None: + """Reconcile service. Only applies to Kubernetes charm """ @@ -249,6 +262,10 @@ def _reconcile_ports(self, *, event) -> None: Only applies to Machine charm """ + @abc.abstractmethod + def _update_endpoints(self) -> None: + """Update the endpoints in the provider relation if necessary.""" + # ======================= # Handlers # ======================= @@ -332,23 +349,17 @@ def reconcile(self, event=None) -> None: # noqa: C901 and isinstance(workload_, workload.AuthenticatedWorkload) and workload_.container_ready ): - self._reconcile_node_port(event=event) + self._reconcile_service() self._database_provides.reconcile_users( event=event, - router_read_write_endpoint=self._read_write_endpoint, - router_read_only_endpoint=self._read_only_endpoint, - exposed_read_write_endpoint=self._exposed_read_write_endpoint, - exposed_read_only_endpoint=self._exposed_read_only_endpoint, + router_read_write_endpoints=self._read_write_endpoints, + router_read_only_endpoints=self._read_only_endpoints, + exposed_read_write_endpoints=self._exposed_read_write_endpoints, + exposed_read_only_endpoints=self._exposed_read_only_endpoints, shell=workload_.shell, ) - # _ha_cluster only assigned a value in machine charms - if self._ha_cluster: - self._database_provides.update_endpoints( - router_read_write_endpoint=self._read_write_endpoint, - router_read_only_endpoint=self._read_only_endpoint, - exposed_read_write_endpoint=self._exposed_read_write_endpoint, - exposed_read_only_endpoint=self._exposed_read_only_endpoint, - ) + self._update_endpoints() + if workload_.container_ready: workload_.reconcile( event=event, diff --git a/src/machine_charm.py b/src/machine_charm.py index 45c727e0..9d6e5f6a 100755 --- a/src/machine_charm.py +++ b/src/machine_charm.py @@ -79,6 +79,10 @@ def _upgrade(self) -> typing.Optional[machine_upgrade.Upgrade]: except upgrade.PeerRelationNotReady: pass + @property + def _status(self) -> ops.StatusBase: + pass + @property def _logrotate(self) -> machine_logrotate.LogRotate: return machine_logrotate.LogRotate(container_=self._container) @@ -95,25 +99,25 @@ def host_address(self) -> str: return str(self.model.get_binding("juju-info").network.bind_address) @property - def _read_write_endpoint(self) -> str: + def _read_write_endpoints(self) -> str: return f'file://{self._container.path("/run/mysqlrouter/mysql.sock")}' @property - def _read_only_endpoint(self) -> str: + def _read_only_endpoints(self) -> str: return f'file://{self._container.path("/run/mysqlrouter/mysqlro.sock")}' @property - def _exposed_read_write_endpoint(self) -> str: + def _exposed_read_write_endpoints(self) -> typing.Optional[str]: return f"{self.host_address}:{self._READ_WRITE_PORT}" @property - def _exposed_read_only_endpoint(self) -> str: + def _exposed_read_only_endpoints(self) -> typing.Optional[str]: return f"{self.host_address}:{self._READ_ONLY_PORT}" def is_externally_accessible(self, *, event) -> typing.Optional[bool]: return self._database_provides.external_connectivity(event) - def _reconcile_node_port(self, *, event) -> None: + def _reconcile_service(self) -> None: """Only applies to Kubernetes charm, so no-op.""" pass @@ -124,6 +128,18 @@ def _reconcile_ports(self, *, event) -> None: ports = [] self.unit.set_ports(*ports) + def _update_endpoints(self) -> None: + self._database_provides.update_endpoints( + router_read_write_endpoints=self._read_write_endpoints, + router_read_only_endpoints=self._read_only_endpoints, + exposed_read_write_endpoints=self._exposed_read_write_endpoints, + exposed_read_only_endpoints=self._exposed_read_only_endpoints, + ) + + def _wait_until_service_reconciled(self) -> None: + """Only applies to Kubernetes charm, so no-op.""" + pass + def wait_until_mysql_router_ready(self, *, event) -> None: logger.debug("Waiting until MySQL Router is ready") self.unit.status = ops.MaintenanceStatus("MySQL Router starting") diff --git a/src/relations/database_providers_wrapper.py b/src/relations/database_providers_wrapper.py index 66f42a5e..394e738a 100644 --- a/src/relations/database_providers_wrapper.py +++ b/src/relations/database_providers_wrapper.py @@ -45,27 +45,27 @@ def external_connectivity(self, event) -> bool: def update_endpoints( self, *, - router_read_write_endpoint: str, - router_read_only_endpoint: str, - exposed_read_write_endpoint: str, - exposed_read_only_endpoint: str, + router_read_write_endpoints: str, + router_read_only_endpoints: str, + exposed_read_write_endpoints: str, + exposed_read_only_endpoints: str, ) -> None: """Update the endpoints in the provides relationship databags.""" self._database_provides.update_endpoints( - router_read_write_endpoint=router_read_write_endpoint, - router_read_only_endpoint=router_read_only_endpoint, - exposed_read_write_endpoint=exposed_read_write_endpoint, - exposed_read_only_endpoint=exposed_read_only_endpoint, + router_read_write_endpoints=router_read_write_endpoints, + router_read_only_endpoints=router_read_only_endpoints, + exposed_read_write_endpoints=exposed_read_write_endpoints, + exposed_read_only_endpoints=exposed_read_only_endpoints, ) def reconcile_users( self, *, event, - router_read_write_endpoint: str, - router_read_only_endpoint: str, - exposed_read_write_endpoint: str, - exposed_read_only_endpoint: str, + router_read_write_endpoints: str, + router_read_only_endpoints: str, + exposed_read_write_endpoints: str, + exposed_read_only_endpoints: str, shell: mysql_shell.Shell, ) -> None: """Create requested users and delete inactive users. @@ -76,10 +76,10 @@ def reconcile_users( """ self._database_provides.reconcile_users( event=event, - router_read_write_endpoint=router_read_write_endpoint, - router_read_only_endpoint=router_read_only_endpoint, - exposed_read_write_endpoint=exposed_read_write_endpoint, - exposed_read_only_endpoint=exposed_read_only_endpoint, + router_read_write_endpoints=router_read_write_endpoints, + router_read_only_endpoints=router_read_only_endpoints, + exposed_read_write_endpoints=exposed_read_write_endpoints, + exposed_read_only_endpoints=exposed_read_only_endpoints, shell=shell, ) self._deprecated_shared_db.reconcile_users(event=event, shell=shell) diff --git a/src/relations/database_provides.py b/src/relations/database_provides.py index 1d5666a4..b1876c94 100644 --- a/src/relations/database_provides.py +++ b/src/relations/database_provides.py @@ -107,10 +107,10 @@ def _set_databag( def create_database_and_user( self, *, - router_read_write_endpoint: str, - router_read_only_endpoint: str, - exposed_read_write_endpoint: str, - exposed_read_only_endpoint: str, + router_read_write_endpoints: str, + router_read_only_endpoints: str, + exposed_read_write_endpoints: str, + exposed_read_only_endpoints: str, shell: mysql_shell.Shell, ) -> None: """Create database & user and update databag.""" @@ -128,12 +128,14 @@ def create_database_and_user( ) rw_endpoint = ( - exposed_read_write_endpoint + exposed_read_write_endpoints if self.external_connectivity - else router_read_write_endpoint + else router_read_write_endpoints ) ro_endpoint = ( - exposed_read_only_endpoint if self.external_connectivity else router_read_only_endpoint + exposed_read_only_endpoints + if self.external_connectivity + else router_read_only_endpoints ) self._set_databag( @@ -164,28 +166,30 @@ def __init__( def update_endpoints( self, *, - router_read_write_endpoint: str, - router_read_only_endpoint: str, - exposed_read_write_endpoint: str, - exposed_read_only_endpoint: str, + router_read_write_endpoints: str, + router_read_only_endpoints: str, + exposed_read_write_endpoints: str, + exposed_read_only_endpoints: str, ) -> None: """Update the endpoints in the databag.""" logger.debug( - f"Updating endpoints {self._id} {router_read_write_endpoint=}, {router_read_only_endpoint=} {exposed_read_write_endpoint=} {exposed_read_only_endpoint=}" + f"Updating endpoints {self._id} {router_read_write_endpoints=}, {router_read_only_endpoints=} {exposed_read_write_endpoints=} {exposed_read_only_endpoints=}" ) rw_endpoint = ( - exposed_read_write_endpoint + exposed_read_write_endpoints if self.external_connectivity - else router_read_write_endpoint + else router_read_write_endpoints ) ro_endpoint = ( - exposed_read_only_endpoint if self.external_connectivity else router_read_only_endpoint + exposed_read_only_endpoints + if self.external_connectivity + else router_read_only_endpoints ) self._interface.set_endpoints(self._id, rw_endpoint) self._interface.set_read_only_endpoints(self._id, ro_endpoint) logger.debug( - f"Updated endpoints {self._id} {router_read_write_endpoint=}, {router_read_only_endpoint=} {exposed_read_write_endpoint=} {exposed_read_only_endpoint=}" + f"Updated endpoints {self._id} {router_read_write_endpoints=}, {router_read_only_endpoints=} {exposed_read_write_endpoints=} {exposed_read_only_endpoints=}" ) def delete_databag(self) -> None: @@ -249,28 +253,28 @@ def external_connectivity(self, event) -> bool: def update_endpoints( self, *, - router_read_write_endpoint: str, - router_read_only_endpoint: str, - exposed_read_write_endpoint: str, - exposed_read_only_endpoint: str, + router_read_write_endpoints: str, + router_read_only_endpoints: str, + exposed_read_write_endpoints: str, + exposed_read_only_endpoints: str, ) -> None: """Update endpoints in the databags.""" for relation in self._shared_users: relation.update_endpoints( - router_read_write_endpoint=router_read_write_endpoint, - router_read_only_endpoint=router_read_only_endpoint, - exposed_read_write_endpoint=exposed_read_write_endpoint, - exposed_read_only_endpoint=exposed_read_only_endpoint, + router_read_write_endpoints=router_read_write_endpoints, + router_read_only_endpoints=router_read_only_endpoints, + exposed_read_write_endpoints=exposed_read_write_endpoints, + exposed_read_only_endpoints=exposed_read_only_endpoints, ) def reconcile_users( self, *, event, - router_read_write_endpoint: str, - router_read_only_endpoint: str, - exposed_read_write_endpoint: str, - exposed_read_only_endpoint: str, + router_read_write_endpoints: str, + router_read_only_endpoints: str, + exposed_read_write_endpoints: str, + exposed_read_only_endpoints: str, shell: mysql_shell.Shell, ) -> None: """Create requested users and delete inactive users. @@ -280,7 +284,7 @@ def reconcile_users( relation is broken. """ logger.debug( - f"Reconciling users {event=}, {router_read_write_endpoint=}, {router_read_only_endpoint=}" + f"Reconciling users {event=}, {router_read_write_endpoints=}, {router_read_only_endpoints=}" ) requested_users = [] for relation in self._interface.relations: @@ -300,17 +304,17 @@ def reconcile_users( for relation in requested_users: if relation not in self._shared_users: relation.create_database_and_user( - router_read_write_endpoint=router_read_write_endpoint, - router_read_only_endpoint=router_read_only_endpoint, - exposed_read_write_endpoint=exposed_read_write_endpoint, - exposed_read_only_endpoint=exposed_read_only_endpoint, + router_read_write_endpoints=router_read_write_endpoints, + router_read_only_endpoints=router_read_only_endpoints, + exposed_read_write_endpoints=exposed_read_write_endpoints, + exposed_read_only_endpoints=exposed_read_only_endpoints, shell=shell, ) for relation in self._shared_users: if relation not in requested_users: relation.delete_user(shell=shell) logger.debug( - f"Reconciled users {event=}, {router_read_write_endpoint=}, {router_read_only_endpoint=}" + f"Reconciled users {event=}, {router_read_write_endpoints=}, {router_read_only_endpoints=}" ) def delete_all_databags(self) -> None: