DPE-4850 Fix intermitent issue on aks deployments (#458)

* fix for aks setup issue

* avoid using juju for cluster initialization state
* allow catch all on non-critical/retried handler

* porting changes from vm

* for storage re usage, cluster metadata exists

* improved comments and messaging

Author: paulomach

lib/charms/mysql/v0/async_replication.py

@@ -237,7 +237,6 @@ def on_async_relation_broken(self, event: RelationBrokenEvent):  # noqa: C901
             self._charm.unit.status = BlockedStatus("Standalone read-only unit.")
             # reset flag to allow instances rejoining the cluster
             self._charm.unit_peer_data["member-state"] = "waiting"
-            del self._charm.unit_peer_data["unit-initialized"]
             if not self._charm.unit.is_leader():
                 # delay non leader to avoid `update_status` running before
                 # leader updates app peer data
@@ -914,7 +913,6 @@ def _on_consumer_non_leader_changed(self, _):
             logger.debug("Reset secondary unit to allow cluster rejoin")
             # reset unit flag to allow cluster rejoin after primary recovery
             # the unit will rejoin on the next peer relation changed or update status
-            del self._charm.unit_peer_data["unit-initialized"]
             self._charm.unit_peer_data["member-state"] = "waiting"
             self._charm.unit.status = WaitingStatus("waiting to join the cluster")
 

lib/charms/mysql/v0/mysql.py

@@ -92,7 +92,7 @@ def _on_set_tls_private_key(self, event: ActionEvent) -> None:
 
     def _on_tls_relation_joined(self, event) -> None:
         """Request certificate when TLS relation joined."""
-        if self.charm.unit_peer_data.get("unit-initialized") != "True":
+        if not self.charm.unit_initialized:
             event.defer()
             return
         self._request_certificate(None)

lib/charms/mysql/v0/tls.py

@@ -41,7 +41,7 @@
 from charms.rolling_ops.v0.rollingops import RollingOpsManager
 from charms.tempo_k8s.v1.charm_tracing import trace_charm
 from charms.tempo_k8s.v2.tracing import TracingEndpointRequirer
-from ops import EventBase, RelationBrokenEvent, RelationCreatedEvent
+from ops import EventBase, RelationBrokenEvent, RelationCreatedEvent, Unit
 from ops.charm import RelationChangedEvent, UpdateStatusEvent
 from ops.main import main
 from ops.model import ActiveStatus, BlockedStatus, Container, MaintenanceStatus, WaitingStatus
@@ -254,6 +254,10 @@ def unit_address(self) -> str:
         """Return the address of this unit."""
         return self._get_unit_fqdn()
 
+    def get_unit_address(self, unit: Unit) -> str:
+        """Return the address of a unit."""
+        return self._get_unit_fqdn(unit.name)
+
     def get_unit_hostname(self, unit_name: Optional[str] = None) -> str:
         """Get the hostname.localdomain for a unit.
 
@@ -298,14 +302,21 @@ def _get_primary_from_online_peer(self) -> Optional[str]:
 
     def _is_unit_waiting_to_join_cluster(self) -> bool:
         """Return if the unit is waiting to join the cluster."""
-        # alternatively, we could check if the instance is configured
-        # and have an empty performance_schema.replication_group_members table
+        # check base conditions for join a unit to the cluster
+        # - workload accessible
+        # - unit waiting flag set
+        # - unit configured (users created/unit set to be a cluster node)
+        # - unit not node of this cluster or cluster does not report this unit as member
+        # - cluster is initialized on any unit
         return (
             self.unit.get_container(CONTAINER_NAME).can_connect()
             and self.unit_peer_data.get("member-state") == "waiting"
-            and self._mysql.is_data_dir_initialised()
-            and not self.unit_peer_data.get("unit-initialized")
-            and int(self.app_peer_data.get("units-added-to-cluster", 0)) > 0
+            and self.unit_configured
+            and (
+                not self.unit_initialized
+                or not self._mysql.is_instance_in_cluster(self.unit_label)
+            )
+            and self.cluster_initialized
         )
 
     def join_unit_to_cluster(self) -> None:
@@ -375,7 +386,6 @@ def join_unit_to_cluster(self) -> None:
                 return
 
         # Update 'units-added-to-cluster' counter in the peer relation databag
-        self.unit_peer_data["unit-initialized"] = "True"
         self.unit_peer_data["member-state"] = "online"
         self.unit.status = ActiveStatus(self.active_status_message)
         logger.debug(f"Instance {instance_label} is cluster member")
@@ -386,7 +396,7 @@ def _reconcile_pebble_layer(self, container: Container) -> None:
         new_layer = self._pebble_layer
 
         if new_layer.services != current_layer.services:
-            logger.info("Adding pebble layer")
+            logger.info("Reconciling the pebble layer")
 
             container.add_layer(MYSQLD_SAFE_SERVICE, new_layer, combine=True)
             container.replan()
@@ -438,16 +448,18 @@ def _reconcile_mysqld_exporter(
         self, event: RelationCreatedEvent | RelationBrokenEvent
     ) -> None:
         """Handle a COS relation created or broken event."""
-        if not self.unit_peer_data.get("unit-initialized"):
-            # wait unit initialization to avoid messing
-            # with the pebble layer before the unit is initialized
-            logger.debug("Defer reconcile mysqld exporter")
-            event.defer()
+        container = self.unit.get_container(CONTAINER_NAME)
+        if not container.can_connect():
+            # reconciliation is done on pebble ready
+            logger.debug("Skip reconcile mysqld exporter: container not ready")
             return
 
-        self.current_event = event
+        if not container.pebble.get_plan():
+            # reconciliation is done on pebble ready
+            logger.debug("Skip reconcile mysqld exporter: empty pebble layer")
+            return
 
-        container = self.unit.get_container(CONTAINER_NAME)
+        self.current_event = event
         self._reconcile_pebble_layer(container)
 
     def _on_peer_relation_joined(self, _) -> None:
@@ -645,9 +657,9 @@ def _on_mysql_pebble_ready(self, event) -> None:
         logger.info("Setting up the logrotate configurations")
         self._mysql.setup_logrotate_config()
 
-        self.unit_peer_data["unit-status"] = "alive"
         if self._mysql.is_data_dir_initialised():
             # Data directory is already initialised, skip configuration
+            self.unit.status = MaintenanceStatus("Starting mysqld")
             logger.debug("Data directory is already initialised, skipping configuration")
             self._reconcile_pebble_layer(container)
             return
@@ -811,7 +823,7 @@ def _on_peer_relation_changed(self, event: RelationChangedEvent) -> None:
     def _on_database_storage_detaching(self, _) -> None:
         """Handle the database storage detaching event."""
         # Only executes if the unit was initialised
-        if not self.unit_peer_data.get("unit-initialized"):
+        if not self.unit_initialized:
             return
 
         # No need to remove the instance from the cluster if it is not a member of the cluster

src/charm.py

@@ -34,7 +34,7 @@ def start_log_rotate_manager(self):
         if (
             not isinstance(self.charm.unit.status, ActiveStatus)
             or self.charm.peers is None
-            or self.charm.unit_peer_data.get("unit-initialized") != "True"
+            or not self.charm.unit_initialized
         ):
             return
 

src/log_rotate_manager.py

@@ -136,7 +136,7 @@ def _update_status(self, _) -> None:
         if not (relation_data := self.charm.app_peer_data.get(MYSQL_RELATION_DATA_KEY)):
             return
 
-        if not self.charm.unit_peer_data.get("unit-initialized"):
+        if not self.charm.unit_initialized:
             # Skip update status for uninitialized unit
             return
 
@@ -168,9 +168,7 @@ def _on_peer_relation_changed(self, event: RelationChangedEvent) -> None:
         if not self.model.get_relation(LEGACY_MYSQL):
             return
 
-        if not self.charm._is_peer_data_set or not self.charm.unit_peer_data.get(
-            "unit-initialized"
-        ):
+        if not (self.charm._is_peer_data_set and self.charm.unit_initialized):
             # Avoid running too early
             logger.info("Unit not ready to set `mysql` relation data. Deferring")
             event.defer()
@@ -213,7 +211,7 @@ def _on_mysql_relation_created(self, event: RelationCreatedEvent) -> None:  # no
         # and for the member to be initialized and online
         if (
             not self.charm._is_peer_data_set
-            or not self.charm.unit_peer_data.get("unit-initialized")
+            or not self.charm.unit_initialized
             or self.charm.unit_peer_data.get("member-state") != "online"
         ):
             logger.info("Unit not ready to execute `mysql` relation created. Deferring")

src/relations/mysql.py

@@ -197,7 +197,13 @@ def _on_mysql_pebble_ready(self, _: PebbleReadyEvent) -> None:
         container_restarts = int(self.charm.unit_peer_data.get(CONTAINER_RESTARTS, "0"))
         self.charm.unit_peer_data[CONTAINER_RESTARTS] = str(container_restarts + 1)
 
-        self._configure_endpoints(None)
+        try:
+            self._configure_endpoints(None)
+        except Exception:
+            # catch all exception to avoid uncommitted databag keys
+            # from other pebble-ready handlers succeed
+            # see: https://github.com/canonical/mysql-k8s-operator/issues/457
+            logger.exception("Failed to update endpoints on database provider pebble ready event")
 
     def _configure_endpoints(self, _) -> None:
         """Update the endpoints + read_only_endpoints."""

src/relations/mysql_provider.py

@@ -154,9 +154,7 @@ def _on_mysql_root_relation_created(self, event: RelationCreatedEvent) -> None:
 
         # Wait until on-config-changed event is executed
         # (wait for root password to have been set) or wait until the unit is initialized
-        if not self.charm._is_peer_data_set or not self.charm.unit_peer_data.get(
-            "unit-initialized"
-        ):
+        if not (self.charm._is_peer_data_set and self.charm.unit_initialized):
             event.defer()
             return
 

src/relations/mysql_root.py

@@ -45,9 +45,9 @@ def _rotate_mysql_logs(self, _) -> None:
         """Rotate the mysql logs."""
         if (
             self.charm.peers is None
-            or self.charm.unit_peer_data.get("unit-initialized") != "True"
-            or not self.charm.upgrade.idle
             or not self.charm._mysql.is_mysqld_running()
+            or not self.charm.unit_initialized
+            or not self.charm.upgrade.idle
         ):
             # skip when not initialized, during an upgrade, or when mysqld is not running
             return

src/rotate_mysql_logs.py

@@ -112,6 +112,7 @@ def test_on_leader_elected_secrets(self):
                 secret_data[password].isalnum() and len(secret_data[password]) == PASSWORD_LENGTH
             )
 
+    @patch("mysql_k8s_helpers.MySQL.cluster_metadata_exists", return_value=False)
     @patch("mysql_k8s_helpers.MySQL.rescan_cluster")
     @patch("charms.mysql.v0.mysql.MySQLCharmBase.active_status_message", return_value="")
     @patch("upgrade.MySQLK8sUpgrade.idle", return_value=True)
@@ -155,6 +156,7 @@ def test_mysql_pebble_ready(
         _active_status_message,
         _upgrade_idle,
         _rescan_cluster,
+        _cluster_metadata_exists,
     ):
         # Check if initial plan is empty
         self.harness.set_can_connect("mysql", True)
@@ -200,14 +202,12 @@ def test_pebble_ready_set_data(
         # test on non leader
         self.harness.set_leader(is_leader=False)
         self.harness.container_pebble_ready("mysql")
-        self.assertEqual(self.charm.unit_peer_data.get("unit-initialized"), None)
         self.assertEqual(self.charm.unit_peer_data["member-role"], "secondary")
         self.assertEqual(self.charm.unit_peer_data["member-state"], "waiting")
 
         # test on leader
         self.harness.set_leader(is_leader=True)
         self.harness.container_pebble_ready("mysql")
-        self.assertEqual(self.charm.unit_peer_data["unit-initialized"], "True")
         self.assertEqual(self.charm.unit_peer_data["member-state"], "online")
         self.assertEqual(self.charm.unit_peer_data["member-role"], "primary")
 
@@ -308,6 +308,7 @@ def test_set_secret_databag(self, _):
             == "test-password"
         )
 
+    @patch("charm.MySQLOperatorCharm.unit_initialized", return_value=True)
     @patch("charms.mysql.v0.mysql.MySQLBase.is_cluster_replica", return_value=False)
     @patch("mysql_k8s_helpers.MySQL.remove_instance")
     @patch("mysql_k8s_helpers.MySQL.get_primary_label")
@@ -320,10 +321,8 @@ def test_database_storage_detaching(
         mock_get_primary_label,
         mock_remove_instance,
         mock_is_cluster_replica,
+        mock_unit_initialized,
     ):
-        self.harness.update_relation_data(
-            self.peer_relation_id, self.charm.unit.name, {"unit-initialized": "True"}
-        )
         self.harness.update_relation_data(
             self.peer_relation_id,
             self.charm.app.name,