From 8ab380d232324b44d76175eaa05f7af31b2f9839 Mon Sep 17 00:00:00 2001 From: Ray Chan Date: Fri, 19 Jul 2024 18:07:29 +0800 Subject: [PATCH] Refactor how to restart service when `enable-auto-restart` is False. (#504) --- cou/apps/auxiliary.py | 157 +++++++++++++++++++----------- tests/unit/apps/test_auxiliary.py | 78 +++++++++------ tox.ini | 2 +- 3 files changed, 150 insertions(+), 87 deletions(-) diff --git a/cou/apps/auxiliary.py b/cou/apps/auxiliary.py index 385f4f04..6aca663a 100644 --- a/cou/apps/auxiliary.py +++ b/cou/apps/auxiliary.py @@ -21,7 +21,12 @@ from cou.apps.base import LONG_IDLE_TIMEOUT, OpenStackApplication from cou.apps.factory import AppFactory from cou.exceptions import ApplicationError -from cou.steps import ApplicationUpgradePlan, PostUpgradeStep, PreUpgradeStep +from cou.steps import ( + ApplicationUpgradePlan, + PostUpgradeStep, + PreUpgradeStep, + UnitUpgradeStep, +) from cou.utils.app_utils import set_require_osd_release_option from cou.utils.juju_utils import Unit from cou.utils.openstack import ( @@ -169,6 +174,98 @@ def _need_current_channel_refresh(self, target: OpenStackRelease) -> bool: o7k_release <= target for o7k_release in compatible_o7k_releases ) + def get_run_deferred_hooks_and_restart_pre_upgrade_step( + self, units: Optional[list[Unit]] + ) -> list[PreUpgradeStep]: + """Get the steps for run deferred hook and restart services for before upgrade. + + This step will run the `run-deferred-hooks` action to clear any + potential event and wait until the app is ready before performing + upgrade. If there are no pending events, this step should be a no-op, + so it's safe to run anyways. + + :param units: Units to generate upgrade plan + :type units: Optional[list[Unit]] + :return: Steps for run deferred hooks and restart service + :rtype: List of PreUpgradeStep + """ + run_hook_step = PreUpgradeStep( + description=( + f"Execute run-deferred-hooks for all '{self.name}' units " + "to clear any leftover events" + ), + parallel=False, + ) + run_hook_step.add_steps( + [ + UnitUpgradeStep( + description=f"Execute run-deferred-hooks on unit: '{unit.name}'", + coro=self.model.run_action( + unit.name, "run-deferred-hooks", raise_on_failure=True + ), + ) + for unit in units or self.units.values() + ] + ) + wait_step = PreUpgradeStep( + description=( + f"Wait for up to {self.wait_timeout}s for app '{self.name}'" + " to reach the idle state" + ), + parallel=False, + coro=self.model.wait_for_active_idle(self.wait_timeout, apps=[self.name]), + ) + return [ + run_hook_step, + wait_step, + ] + + def get_run_deferred_hooks_and_restart_post_upgrade_step( + self, units: Optional[list[Unit]] + ) -> list[PostUpgradeStep]: + """Get the step for run deferred hook and restart services for after upgrade. + + This step will wait for the app to complete the upgrade step and then + run the `run-deferred-hooks` action to restart the service. If there + are no pending events, this step should be a no-op, so it's safe to run + anyways. + + :param units: Units to generate upgrade plan + :type units: Optional[list[Unit]] + :return: Step for run deferred hooks and restart service + :rtype: PostUpgradeStep + """ + wait_step = PostUpgradeStep( + description=( + f"Wait for up to {self.wait_timeout}s for app '{self.name}'" + " to reach the idle state" + ), + parallel=False, + coro=self.model.wait_for_active_idle(self.wait_timeout, apps=[self.name]), + ) + run_hook_step = PostUpgradeStep( + description=( + f"Execute run-deferred-hooks for all '{self.name}' units " + "to restart the service after upgrade" + ), + parallel=False, + ) + run_hook_step.add_steps( + [ + UnitUpgradeStep( + description=f"Execute run-deferred-hooks on unit: '{unit.name}'", + coro=self.model.run_action( + unit.name, "run-deferred-hooks", raise_on_failure=True + ), + ) + for unit in units or self.units.values() + ] + ) + return [ + wait_step, + run_hook_step, + ] + @AppFactory.register_application(["rabbitmq-server"]) class RabbitMQServer(AuxiliaryApplication): @@ -196,35 +293,8 @@ def pre_upgrade_steps( :rtype: list[PreUpgradeStep] """ steps = super().pre_upgrade_steps(target, units) - # Since auto restart is disabled, we don't know the if the service - # has pending events or not, so we want to run `run-deferred-hooks` - # action to clear the events before performing upgrade. If there - # are no pending events, this step should be a no-op, so it's safe - # to run anyway if self.config.get("enable-auto-restarts", {}).get("value") is False: - # Run any deferred events and restart the service. See - # https://charmhub.io/rabbitmq-server/actions#run-deferred-hooks - units_to_run_action = self.units.values() if units is None else units - steps += [ - PreUpgradeStep( - description="Auto restarts is disabled, will" - f" execute run-deferred-hooks for unit: '{unit.name}'", - coro=self.model.run_action( - unit.name, "run-deferred-hooks", raise_on_failure=True - ), - ) - for unit in units_to_run_action - ] - steps += [ - PreUpgradeStep( - description=( - f"Wait for up to {self.wait_timeout}s for app '{self.name}'" - " to reach the idle state" - ), - parallel=False, - coro=self.model.wait_for_active_idle(self.wait_timeout, apps=[self.name]), - ) - ] + steps.extend(self.get_run_deferred_hooks_and_restart_pre_upgrade_step(units)) return steps def post_upgrade_steps( @@ -242,34 +312,9 @@ def post_upgrade_steps( :rtype: list[PostUpgradeStep] """ steps = [] - # Since the auto restart is disabled, we need to run the - # `run-deferred-hooks` action, and restart the service after the - # upgrade. if self.config.get("enable-auto-restarts", {}).get("value") is False: - steps += [ - PostUpgradeStep( - description=( - f"Wait for up to {self.wait_timeout}s for app '{self.name}'" - " to reach the idle state" - ), - parallel=False, - coro=self.model.wait_for_active_idle(self.wait_timeout, apps=[self.name]), - ) - ] - # Run any deferred events and restart the service. See - # https://charmhub.io/rabbitmq-server/actions#run-deferred-hooks - units_to_run_action = self.units.values() if units is None else units - steps += [ - PostUpgradeStep( - description="Auto restarts is disabled, will" - f" execute run-deferred-hooks for unit: '{unit.name}'", - coro=self.model.run_action( - unit.name, "run-deferred-hooks", raise_on_failure=True - ), - ) - for unit in units_to_run_action - ] - steps += super().post_upgrade_steps(target, units) + steps.extend(self.get_run_deferred_hooks_and_restart_post_upgrade_step(units)) + steps.extend(super().post_upgrade_steps(target, units)) return steps def _check_auto_restarts(self) -> None: diff --git a/tests/unit/apps/test_auxiliary.py b/tests/unit/apps/test_auxiliary.py index d535e664..68ea3674 100644 --- a/tests/unit/apps/test_auxiliary.py +++ b/tests/unit/apps/test_auxiliary.py @@ -389,27 +389,58 @@ def test_rabbitmq_server_upgrade_plan_ussuri_to_victoria_auto_restart_False(mode for unit in app.units.values() ) + run_deferred_hooks_and_restart_pre_upgrades = PreUpgradeStep( + description=( + f"Execute run-deferred-hooks for all '{app.name}' units " + "to clear any leftover events" + ), + parallel=False, + ) + run_deferred_hooks_and_restart_pre_upgrades.add_steps( + [ + UnitUpgradeStep( + description=f"Execute run-deferred-hooks on unit: '{unit.name}'", + coro=model.run_action(unit.name, "run-deferred-hooks", raise_on_failure=True), + ) + for unit in app.units.values() + ] + ) + run_deferred_hooks_and_restart_pre_wait_step = PreUpgradeStep( + description=(f"Wait for up to 2400s for app '{app.name}'" " to reach the idle state"), + parallel=False, + coro=model.wait_for_active_idle(2400, apps=[app.name]), + ) + + run_deferred_hooks_and_restart_post_wait_step = PostUpgradeStep( + description=(f"Wait for up to 2400s for app '{app.name}'" " to reach the idle state"), + parallel=False, + coro=model.wait_for_active_idle(2400, apps=[app.name]), + ) + run_deferred_hooks_and_restart_post_upgrades = PostUpgradeStep( + description=( + f"Execute run-deferred-hooks for all '{app.name}' units " + "to restart the service after upgrade" + ), + parallel=False, + ) + run_deferred_hooks_and_restart_post_upgrades.add_steps( + [ + UnitUpgradeStep( + description=f"Execute run-deferred-hooks on unit: '{unit.name}'", + coro=model.run_action(unit.name, "run-deferred-hooks", raise_on_failure=True), + ) + for unit in app.units.values() + ] + ) + upgrade_steps = [ upgrade_packages, PreUpgradeStep( description=f"Refresh '{app.name}' to the latest revision of '3.9/stable'", coro=model.upgrade_charm(app.name, "3.9/stable"), ), - ] - upgrade_steps += [ - PreUpgradeStep( - description="Auto restarts is disabled, will" - f" execute run-deferred-hooks for unit: '{unit.name}'", - coro=model.run_action(unit.name, "run-deferred-hooks", raise_on_failure=True), - ) - for unit in app.units.values() - ] - upgrade_steps += [ - PreUpgradeStep( - description=f"Wait for up to 2400s for app '{app.name}' to reach the idle state", - parallel=False, - coro=model.wait_for_active_idle(2400, apps=[app.name]), - ), + run_deferred_hooks_and_restart_pre_upgrades, + run_deferred_hooks_and_restart_pre_wait_step, UpgradeStep( description=f"Change charm config of '{app.name}' " f"'{app.origin_setting}' to 'cloud:focal-victoria'", @@ -418,21 +449,8 @@ def test_rabbitmq_server_upgrade_plan_ussuri_to_victoria_auto_restart_False(mode app.name, {f"{app.origin_setting}": "cloud:focal-victoria"} ), ), - PostUpgradeStep( - description=f"Wait for up to 2400s for app '{app.name}' to reach the idle state", - parallel=False, - coro=model.wait_for_active_idle(2400, apps=[app.name]), - ), - ] - upgrade_steps += [ - PostUpgradeStep( - description="Auto restarts is disabled, will" - f" execute run-deferred-hooks for unit: '{unit.name}'", - coro=model.run_action(unit.name, "run-deferred-hooks", raise_on_failure=True), - ) - for unit in app.units.values() - ] - upgrade_steps += [ + run_deferred_hooks_and_restart_post_wait_step, + run_deferred_hooks_and_restart_post_upgrades, PostUpgradeStep( description=f"Wait for up to 2400s for model '{model.name}' to reach the idle state", parallel=False, diff --git a/tox.ini b/tox.ini index 5d13ad4b..4a4ad47c 100644 --- a/tox.ini +++ b/tox.ini @@ -4,7 +4,7 @@ [tox] skipsdist=True -envlist = lint, unit, mocked-plans, func +envlist = lint, unit, mocked-plans skip_missing_interpreters = True [testenv]