fix: DPE-6485 optional restart on instance configuration (#607)

* optional restart for pebble managed mysqld

* refactor to ditch rolling ops event defer and reuse recovery method

* method moved to charm.py

* remove `charm`

* revert change in method

* fix test

* fix sed string

* pythonic failure injection method

tests consistently fails on CI but work on my machine(tm), so this eliminates any differences from the env

Author: paulomach

lib/charms/mysql/v0/mysql.py

@@ -133,7 +133,7 @@ def wait_until_mysql_connection(self) -> None:
 # Increment this major API version when introducing breaking changes
 LIBAPI = 0
 
-LIBPATCH = 82
+LIBPATCH = 83
 
 UNIT_TEARDOWN_LOCKNAME = "unit-teardown"
 UNIT_ADD_LOCKNAME = "unit-add"
@@ -1463,7 +1463,11 @@ def get_variable_value(self, variable: str) -> str:
         return rows[0][1]
 
     def configure_instance(self, create_cluster_admin: bool = True) -> None:
-        """Configure the instance to be used in an InnoDB cluster."""
+        """Configure the instance to be used in an InnoDB cluster.
+
+        Args:
+            create_cluster_admin: Whether to create the cluster admin user.
+        """
         options = {
             "restart": "true",
         }
@@ -1955,6 +1959,7 @@ def is_instance_in_cluster(self, unit_label: str) -> bool:
                 user=self.server_config_user,
                 password=self.server_config_password,
                 host=self.instance_def(self.server_config_user),
+                exception_as_warning=True,
             )
             return (
                 MySQLMemberState.ONLINE in output.lower()

src/charm.py

@@ -873,28 +873,50 @@ def join_unit_to_cluster(self) -> None:
         self.unit.status = ActiveStatus(self.active_status_message)
         logger.info(f"Instance {instance_label} added to cluster")
 
-    def _restart(self, event: EventBase) -> None:
-        """Restart the MySQL service."""
-        if self.peers.units != self.restart_peers.units:
-            # defer restart until all units are in the relation
-            logger.debug("Deferring restart until all units are in the relation")
-            event.defer()
+    def recover_unit_after_restart(self) -> None:
+        """Wait for unit recovery/rejoin after restart."""
+        recovery_attempts = 30
+        logger.info("Recovering unit")
+        if self.app.planned_units() == 1:
+            self._mysql.reboot_from_complete_outage()
+        else:
+            try:
+                for attempt in Retrying(
+                    stop=stop_after_attempt(recovery_attempts), wait=wait_fixed(15)
+                ):
+                    with attempt:
+                        self._mysql.hold_if_recovering()
+                        if not self._mysql.is_instance_in_cluster(self.unit_label):
+                            logger.debug(
+                                "Instance not yet back in the cluster."
+                                f" Retry {attempt.retry_state.attempt_number}/{recovery_attempts}"
+                            )
+                            raise Exception
+            except RetryError:
+                raise
+
+    def _restart(self, _: EventBase) -> None:
+        """Restart the service."""
+        if not self.unit_initialized:
+            logger.debug("Restarting standalone mysqld")
+            self._mysql.restart_mysqld()
             return
-        if self.peers.units and self._mysql.is_unit_primary(self.unit_label):
-            restart_states = {
-                self.restart_peers.data[unit].get("state", "unset") for unit in self.peers.units
-            }
-            if restart_states == {"unset"}:
-                logger.debug("Restarting leader")
-            elif restart_states != {"release"}:
-                # Wait other units restart first to minimize primary switchover
-                logger.debug("Primary is waiting for other units to restart")
-                event.defer()
-                return
 
+        if self.app.planned_units() > 1 and self._mysql.is_unit_primary(self.unit_label):
+            try:
+                new_primary = self.get_unit_address(self.peers.units.pop())
+                logger.debug(f"Switching primary to {new_primary}")
+                self._mysql.set_cluster_primary(new_primary)
+            except MySQLSetClusterPrimaryError:
+                logger.warning("Changing primary failed")
+
+        logger.debug("Restarting mysqld")
         self.unit.status = MaintenanceStatus("restarting MySQL")
         self._mysql.restart_mysqld()
+        self.unit.status = MaintenanceStatus("recovering unit after restart")
         sleep(10)
+        self.recover_unit_after_restart()
+
         self._on_update_status(None)
 
 

src/upgrade.py

@@ -29,7 +29,6 @@
 from ops import RelationDataContent
 from ops.model import BlockedStatus, MaintenanceStatus, Unit
 from pydantic import BaseModel
-from tenacity import RetryError, Retrying, stop_after_attempt, wait_fixed
 from typing_extensions import override
 
 from constants import CHARMED_MYSQL_COMMON_DIRECTORY
@@ -40,9 +39,6 @@
 logger = logging.getLogger(__name__)
 
 
-RECOVER_ATTEMPTS = 30
-
-
 class MySQLVMDependenciesModel(BaseModel):
     """MySQL dependencies model."""
 
@@ -239,10 +235,7 @@ def _on_upgrade_granted(self, event: UpgradeGrantedEvent) -> None:  # noqa: C901
         self.charm.unit.status = MaintenanceStatus("recovering unit after upgrade")
 
         try:
-            if self.charm.app.planned_units() > 1:
-                self._recover_multi_unit_cluster()
-            else:
-                self._recover_single_unit_cluster()
+            self.charm.recover_unit_after_restart()
 
             logger.debug("Upgraded unit is healthy. Set upgrade state to `completed`")
             self.set_unit_completed()
@@ -257,29 +250,6 @@ def _on_upgrade_granted(self, event: UpgradeGrantedEvent) -> None:  # noqa: C901
                 "upgrade failed. Check logs for rollback instruction"
             )
 
-    def _recover_multi_unit_cluster(self) -> None:
-        logger.debug("Recovering unit")
-        try:
-            for attempt in Retrying(
-                stop=stop_after_attempt(RECOVER_ATTEMPTS), wait=wait_fixed(10)
-            ):
-                with attempt:
-                    self.charm._mysql.hold_if_recovering()
-                    if not self.charm._mysql.is_instance_in_cluster(self.charm.unit_label):
-                        logger.debug(
-                            "Instance not yet back in the cluster."
-                            f" Retry {attempt.retry_state.attempt_number}/{RECOVER_ATTEMPTS}"
-                        )
-                        self.charm._mysql.start_group_replication()
-                        raise Exception
-        except RetryError:
-            raise
-
-    def _recover_single_unit_cluster(self) -> None:
-        """Recover single unit cluster."""
-        logger.debug("Recovering single unit cluster")
-        self.charm._mysql.reboot_from_complete_outage()
-
     def _on_upgrade_changed(self, _) -> None:
         """Handle the upgrade changed event.
 

tests/integration/high_availability/test_upgrade_rollback_incompat.py

@@ -7,7 +7,6 @@
 import os
 import pathlib
 import shutil
-import subprocess
 from time import sleep
 from zipfile import ZipFile
 
@@ -53,7 +52,7 @@ async def test_build_and_deploy(ops_test: OpsTest) -> None:
     with snap_revisions.open("w") as file:
         json.dump(old_revisions, file)
 
-    async with ops_test.fast_forward("10s"):
+    async with ops_test.fast_forward("30s"):
         await ops_test.model.deploy(
             charm,
             application_name=MYSQL_APP_NAME,
@@ -68,6 +67,7 @@ async def test_build_and_deploy(ops_test: OpsTest) -> None:
             channel="latest/edge",
             num_units=1,
             base="[email protected]",
+            config={"auto_start_writes": False, "sleep_interval": "500"},
         )
 
         await relate_mysql_and_application(ops_test, MYSQL_APP_NAME, TEST_APP)
@@ -106,15 +106,14 @@ async def test_upgrade_to_failling(
     await ensure_all_units_continuous_writes_incrementing(ops_test)
 
     application = ops_test.model.applications[MYSQL_APP_NAME]
-    logger.info("Build charm locally")
 
-    sub_regex_failing_rejoin = (
-        's/logger.debug("Recovering unit")'
-        "/self.charm._mysql.set_instance_offline_mode(True); raise RetryError/"
-    )
-    src_patch(sub_regex=sub_regex_failing_rejoin, file_name="src/upgrade.py")
-    new_charm = await charm_local_build(ops_test, refresh=True)
-    src_patch(revert=True)
+    with InjectFailure(
+        path="src/upgrade.py",
+        original_str="self.charm.recover_unit_after_restart()",
+        replace_str="raise Exception",
+    ):
+        logger.info("Build charm with failure injected")
+        new_charm = await charm_local_build(ops_test, refresh=True)
 
     logger.info("Refresh the charm")
     await application.refresh(path=new_charm)
@@ -187,15 +186,26 @@ async def test_rollback(ops_test, continuous_writes) -> None:
     await ensure_all_units_continuous_writes_incrementing(ops_test)
 
 
-def src_patch(sub_regex: str = "", file_name: str = "", revert: bool = False) -> None:
-    """Apply a patch to the source code."""
-    if revert:
-        cmd = "git checkout src/"  # revert changes on src/ dir
-        logger.info("Reverting patch on source")
-    else:
-        cmd = f"sed -i -e '{sub_regex}' {file_name}"
-        logger.info("Applying patch to source")
-    subprocess.run([cmd], shell=True, check=True)
+class InjectFailure(object):
+    def __init__(self, path: str, original_str: str, replace_str: str):
+        self.path = path
+        self.original_str = original_str
+        self.replace_str = replace_str
+        with open(path, "r") as file:
+            self.original_content = file.read()
+
+    def __enter__(self):
+        logger.info("Injecting failure")
+        assert self.original_str in self.original_content, "replace content not found"
+        new_content = self.original_content.replace(self.original_str, self.replace_str)
+        assert self.original_str not in new_content, "original string not replaced"
+        with open(self.path, "w") as file:
+            file.write(new_content)
+
+    def __exit__(self, exc_type, exc_value, traceback):
+        logger.info("Reverting failure")
+        with open(self.path, "w") as file:
+            file.write(self.original_content)
 
 
 async def charm_local_build(ops_test: OpsTest, refresh: bool = False):

tests/unit/test_mysql.py

@@ -842,6 +842,7 @@ def test_is_instance_in_cluster(self, _run_mysqlsh_script, _cluster_metadata_exi
             user="serverconfig",
             password="serverconfigpassword",
             host="127.0.0.1:33062",
+            exception_as_warning=True,
         )
 
         _run_mysqlsh_script.return_value = "NOT_A_MEMBER"

tests/unit/test_upgrade.py

@@ -13,6 +13,7 @@
     MySQLStopMySQLDError,
 )
 from ops.testing import Harness
+from tenacity import RetryError
 
 from charm import MySQLOperatorCharm
 
@@ -131,13 +132,13 @@ def test_pre_upgrade_prepare(
         mock_get_primary_label.assert_called_once()
         assert mock_set_dynamic_variable.call_count == 2
 
+    @patch("charm.MySQLOperatorCharm.recover_unit_after_restart")
     @patch("mysql_vm_helpers.MySQL.install_plugins")
     @patch("upgrade.set_cron_daemon")
     @patch("upgrade.MySQLVMUpgrade._check_server_unsupported_downgrade")
     @patch("upgrade.MySQLVMUpgrade._reset_on_unsupported_downgrade")
     @patch("mysql_vm_helpers.MySQL.hold_if_recovering")
     @patch("pathlib.Path.exists", return_value=True)
-    @patch("upgrade.RECOVER_ATTEMPTS", 1)
     @patch("mysql_vm_helpers.MySQL.get_mysql_version", return_value="8.0.33")
     @patch("charm.MySQLOperatorCharm.install_workload", return_value=True)
     @patch("charm.MySQLOperatorCharm.unit_fqdn", return_value="10.0.1.1")
@@ -162,6 +163,7 @@ def test_upgrade_granted(
         mock_check_server_unsupported_downgrade,
         mock_set_cron_daemon,
         mock_install_plugins,
+        mock_recover_unit_after_restart,
     ):
         """Test upgrade-granted hook."""
         self.charm.on.config_changed.emit()
@@ -173,7 +175,6 @@ def test_upgrade_granted(
             self.harness.get_relation_data(self.upgrade_relation_id, "mysql/1")["state"],
             "idle",  # change to `completed` - behavior not yet set in the lib
         )
-        mock_is_instance_in_cluster.assert_called_once()
         mock_check_server_upgradeability.assert_called_once()
         mock_start_mysqld.assert_called_once()
         mock_stop_mysqld.assert_called_once()
@@ -185,7 +186,7 @@ def test_upgrade_granted(
             self.upgrade_relation_id, "mysql/0", {"state": "upgrading"}
         )
         # setup for exception
-        mock_is_instance_in_cluster.return_value = False
+        mock_recover_unit_after_restart.side_effect = RetryError
 
         self.charm.upgrade._on_upgrade_granted(None)
         self.assertEqual(