From 5086374c97b1347196640728f8b57c852d68b5d6 Mon Sep 17 00:00:00 2001 From: Robert Gildein Date: Mon, 11 Mar 2024 09:17:34 +0100 Subject: [PATCH] Add post-upgrade steps for whole upgrade plan (#279) Adding ceph-mon post-upgrade step to ensure require-osd-release option matches with ceph-osd version. Add also add_steps function to BaseStep so we can easily add more steps at once. --------- Co-authored-by: TQ X --- cou/apps/auxiliary.py | 2 +- cou/steps/__init__.py | 9 ++ cou/steps/plan.py | 75 ++++++++++---- cou/utils/app_utils.py | 14 ++- tests/unit/apps/test_auxiliary.py | 4 +- tests/unit/steps/test_steps.py | 12 +++ tests/unit/steps/test_steps_plan.py | 149 +++++++++++++++++++++------- 7 files changed, 205 insertions(+), 60 deletions(-) diff --git a/cou/apps/auxiliary.py b/cou/apps/auxiliary.py index 587a9fbe..65bae050 100644 --- a/cou/apps/auxiliary.py +++ b/cou/apps/auxiliary.py @@ -195,7 +195,7 @@ def _get_change_require_osd_release_step(self) -> PreUpgradeStep: """ ceph_mon_unit, *_ = self.units.values() return PreUpgradeStep( - description="Ensure require-osd-release option matches with ceph-osd version", + description="Ensure the 'require-osd-release' option matches the 'ceph-osd' version", coro=set_require_osd_release_option(ceph_mon_unit.name, self.model), ) diff --git a/cou/steps/__init__.py b/cou/steps/__init__.py index b8bff049..cb977ee4 100644 --- a/cou/steps/__init__.py +++ b/cou/steps/__init__.py @@ -251,6 +251,15 @@ def add_step(self, step: BaseStep) -> None: self._sub_steps.append(step) + def add_steps(self, steps: Iterable[BaseStep]) -> None: + """Add multiple steps. + + :param steps: Sequence of BaseStep to be added as sub steps. + :type steps: Iterable[BaseStep] + """ + for step in steps: + self.add_step(step) + def cancel(self, safe: bool = True) -> None: """Cancel step and all its sub steps. diff --git a/cou/steps/plan.py b/cou/steps/plan.py index 28016b15..1f136ef3 100644 --- a/cou/steps/plan.py +++ b/cou/steps/plan.py @@ -43,10 +43,11 @@ NoTargetError, OutOfSupportRange, ) -from cou.steps import PreUpgradeStep, UpgradePlan +from cou.steps import PostUpgradeStep, PreUpgradeStep, UpgradePlan from cou.steps.analyze import Analysis from cou.steps.backup import backup from cou.steps.hypervisor import HypervisorUpgradePlanner +from cou.utils.app_utils import set_require_osd_release_option from cou.utils.juju_utils import DEFAULT_TIMEOUT, COUMachine from cou.utils.nova_compute import get_empty_hypervisors from cou.utils.openstack import LTS_TO_OS_RELEASE, OpenStackRelease @@ -304,7 +305,10 @@ async def generate_plan(analysis_result: Analysis, args: CLIargs) -> UpgradePlan pre_plan_sanity_checks(args, analysis_result) target = determine_upgrade_target(analysis_result) - plan = generate_common_plan(target, analysis_result, args) + plan = UpgradePlan( + f"Upgrade cloud from '{analysis_result.current_cloud_os_release}' to '{target}'" + ) + plan.add_steps(_get_pre_upgrade_steps(analysis_result, args)) # NOTE (gabrielcocenza) upgrade group as None means that the user wants to upgrade # the whole cloud. @@ -315,27 +319,22 @@ async def generate_plan(analysis_result: Analysis, args: CLIargs) -> UpgradePlan if args.upgrade_group in {DATA_PLANE, HYPERVISORS, None}: plan.sub_steps.extend(await _generate_data_plane_plan(target, analysis_result, args)) + plan.add_steps(_get_post_upgrade_steps(analysis_result, args)) + return plan -def generate_common_plan( - target: OpenStackRelease, analysis_result: Analysis, args: CLIargs -) -> UpgradePlan: - """Generate the common upgrade plan. +def _get_pre_upgrade_steps(analysis_result: Analysis, args: CLIargs) -> list[PreUpgradeStep]: + """Get the pre-upgrade steps. - :param target: Target OpenStack release - :type target: OpenStackRelease :param analysis_result: Analysis result :type analysis_result: Analysis :param args: CLI arguments :type args: CLIargs - :return: Common upgrade plan - :rtype: UpgradePlan + :return: List of pre-upgrade steps. + :rtype: list[PreUpgradeStep] """ - plan = UpgradePlan( - f"Upgrade cloud from '{analysis_result.current_cloud_os_release}' to '{target}'" - ) - plan.add_step( + steps = [ PreUpgradeStep( description="Verify that all OpenStack applications are in idle state", parallel=False, @@ -349,15 +348,57 @@ def generate_common_plan( raise_on_blocked=True, ), ) - ) + ] if args.backup: - plan.add_step( + steps.append( PreUpgradeStep( description="Backup mysql databases", coro=backup(analysis_result.model), ) ) - return plan + + return steps + + +def _get_post_upgrade_steps(analysis_result: Analysis, args: CLIargs) -> list[PostUpgradeStep]: + """Get the post-upgrade steps. + + :param analysis_result: Analysis result + :type analysis_result: Analysis + :param args: CLI arguments + :type args: CLIargs + :return: List of post-upgrade steps. + :rtype: list[PreUpgradeStep] + """ + steps = [] + if args.upgrade_group in {DATA_PLANE, None}: + steps.extend(_get_ceph_mon_post_upgrade_steps(analysis_result.apps_data_plane)) + + return steps + + +def _get_ceph_mon_post_upgrade_steps(apps: list[OpenStackApplication]) -> list[PostUpgradeStep]: + """Get the post-upgrade step for ceph-mon, where we check the require-osd-release option. + + :param apps: List of OpenStackApplication. + :type apps: list[OpenStackApplication] + :return: List of post-upgrade steps. + :rtype: list[PreUpgradeStep] + """ + ceph_mons_apps = [app for app in apps if isinstance(app, CephMon)] + + steps = [] + for app in ceph_mons_apps: + unit = list(app.units.values())[0] # getting the first unit, since we don't care which one + steps.append( + PostUpgradeStep( + f"Ensure the 'require-osd-release' option in '{app.name}' matches the 'ceph-osd' " + "version", + coro=set_require_osd_release_option(unit.name, app.model), + ) + ) + + return steps def _generate_control_plane_plan( diff --git a/cou/utils/app_utils.py b/cou/utils/app_utils.py index ca16f83d..f96f0cc2 100644 --- a/cou/utils/app_utils.py +++ b/cou/utils/app_utils.py @@ -26,6 +26,9 @@ logger = logging.getLogger(__name__) +RUN_TIMEOUT: int = 600 + + async def upgrade_packages(unit: str, model: COUModel, packages_to_hold: Optional[list]) -> None: """Run package updates and upgrades on each unit of an Application. @@ -43,7 +46,7 @@ async def upgrade_packages(unit: str, model: COUModel, packages_to_hold: Optiona packages = " ".join(packages_to_hold) command = f"apt-mark hold {packages} && {command} ; apt-mark unhold {packages}" - await model.run_on_unit(unit_name=unit, command=command, timeout=600) + await model.run_on_unit(unit_name=unit, command=command, timeout=RUN_TIMEOUT) async def set_require_osd_release_option(unit: str, model: COUModel) -> None: @@ -65,7 +68,7 @@ async def set_require_osd_release_option(unit: str, model: COUModel) -> None: if current_require_osd_release != current_running_osd_release: set_command = f"ceph osd require-osd-release {current_running_osd_release}" - await model.run_on_unit(unit_name=unit, command=set_command, timeout=600) + await model.run_on_unit(unit_name=unit, command=set_command, timeout=RUN_TIMEOUT) def validate_ovn_support(version: str) -> None: @@ -88,7 +91,6 @@ def validate_ovn_support(version: str) -> None: ) -# Private functions async def _get_required_osd_release(unit: str, model: COUModel) -> str: """Get the value of require-osd-release option on a ceph-mon unit. @@ -103,7 +105,7 @@ async def _get_required_osd_release(unit: str, model: COUModel) -> str: check_command = "ceph osd dump -f json" check_option_result = await model.run_on_unit( - unit_name=unit, command=check_command, timeout=600 + unit_name=unit, command=check_command, timeout=RUN_TIMEOUT ) current_require_osd_release = json.loads(check_option_result["Stdout"]).get( "require_osd_release", "" @@ -129,7 +131,9 @@ async def _get_current_osd_release(unit: str, model: COUModel) -> str: """ check_command = "ceph versions -f json" - check_osd_result = await model.run_on_unit(unit_name=unit, command=check_command, timeout=600) + check_osd_result = await model.run_on_unit( + unit_name=unit, command=check_command, timeout=RUN_TIMEOUT + ) osd_release_output = json.loads(check_osd_result["Stdout"]).get("osd", None) # throw exception if ceph-mon doesn't contain osd release information in `ceph` diff --git a/tests/unit/apps/test_auxiliary.py b/tests/unit/apps/test_auxiliary.py index 5685fa2f..58f4d8ad 100644 --- a/tests/unit/apps/test_auxiliary.py +++ b/tests/unit/apps/test_auxiliary.py @@ -642,7 +642,7 @@ def test_ceph_mon_upgrade_plan_xena_to_yoga(model): coro=model.upgrade_charm(app.name, "pacific/stable", switch=None), ), PreUpgradeStep( - description="Ensure require-osd-release option matches with ceph-osd version", + description="Ensure the 'require-osd-release' option matches the 'ceph-osd' version", parallel=False, coro=app_utils.set_require_osd_release_option("ceph-mon/0", model), ), @@ -731,7 +731,7 @@ def test_ceph_mon_upgrade_plan_ussuri_to_victoria(model): coro=model.upgrade_charm(app.name, "octopus/stable", switch=None), ), PreUpgradeStep( - description="Ensure require-osd-release option matches with ceph-osd version", + description="Ensure the 'require-osd-release' option matches the 'ceph-osd' version", parallel=False, coro=app_utils.set_require_osd_release_option("ceph-mon/0", model), ), diff --git a/tests/unit/steps/test_steps.py b/tests/unit/steps/test_steps.py index 70c5ad56..72c72f6e 100644 --- a/tests/unit/steps/test_steps.py +++ b/tests/unit/steps/test_steps.py @@ -251,6 +251,18 @@ def test_step_add_step_failed(): plan.add_step(MagicMock()) +def test_step_add_steps(): + """Test BaseStep adding sub steps at once.""" + exp_sub_steps = 3 + plan = BaseStep(description="plan") + plan.add_steps( + [BaseStep(description=f"sub-step-{i}", coro=mock_coro()) for i in range(exp_sub_steps)] + + [BaseStep(description="empty-step")] # we also check that empty step will not be added + ) + + assert len(plan.sub_steps) == exp_sub_steps + + def test_step_cancel_safe(): """Test step safe cancel.""" plan = BaseStep(description="plan") diff --git a/tests/unit/steps/test_steps_plan.py b/tests/unit/steps/test_steps_plan.py index 5d1ae583..0b4ae90d 100644 --- a/tests/unit/steps/test_steps_plan.py +++ b/tests/unit/steps/test_steps_plan.py @@ -15,10 +15,11 @@ import pytest +from cou.apps.auxiliary import CephMon from cou.apps.base import OpenStackApplication from cou.apps.core import Keystone from cou.apps.subordinate import SubordinateApplication -from cou.commands import CONTROL_PLANE, DATA_PLANE, HYPERVISORS +from cou.commands import CONTROL_PLANE, DATA_PLANE, HYPERVISORS, CLIargs from cou.exceptions import ( DataPlaneCannotUpgrade, DataPlaneMachineFilterError, @@ -719,16 +720,14 @@ async def test_get_upgradable_hypervisors_machines( @pytest.mark.parametrize("cli_backup", [True, False]) -def test_generate_common_plan(cli_backup, cli_args, model): +def test_get_pre_upgrade_steps(cli_backup, cli_args, model): cli_args.backup = cli_backup mock_analysis_result = MagicMock(spec=Analysis)() mock_analysis_result.current_cloud_os_release = OpenStackRelease("ussuri") mock_analysis_result.model = model - target = OpenStackRelease("victoria") - - expected_plan = UpgradePlan("Upgrade cloud from 'ussuri' to 'victoria'") - expected_plan.add_step( + expected_steps = [] + expected_steps.append( PreUpgradeStep( description="Verify that all OpenStack applications are in idle state", parallel=False, @@ -737,11 +736,8 @@ def test_generate_common_plan(cli_backup, cli_args, model): ), ) ) - - common_plan = cou_plan.generate_common_plan(target, mock_analysis_result, cli_args) - if cli_backup: - expected_plan.add_step( + expected_steps.append( PreUpgradeStep( description="Backup mysql databases", parallel=False, @@ -749,7 +745,84 @@ def test_generate_common_plan(cli_backup, cli_args, model): ) ) - assert common_plan == expected_plan + pre_upgrade_steps = cou_plan._get_pre_upgrade_steps(mock_analysis_result, cli_args) + + assert pre_upgrade_steps == expected_steps + + +@pytest.mark.parametrize("upgrade_group", [CONTROL_PLANE, HYPERVISORS]) +@patch("cou.steps.plan._get_ceph_mon_post_upgrade_steps") +def test_get_post_upgrade_steps_empty(mock_get_ceph_mon_post_upgrade_steps, upgrade_group): + """Test get post upgrade steps not run for control-plane or hypervisors group.""" + args = MagicMock(spec_set=CLIargs)() + args.upgrade_group = upgrade_group + + pre_upgrade_steps = cou_plan._get_post_upgrade_steps(MagicMock(), args) + + assert pre_upgrade_steps == [] + mock_get_ceph_mon_post_upgrade_steps.assert_not_called() + + +@pytest.mark.parametrize("upgrade_group", [DATA_PLANE, None]) +@patch("cou.steps.plan._get_ceph_mon_post_upgrade_steps") +def test_get_post_upgrade_steps_ceph_mon(mock_get_ceph_mon_post_upgrade_steps, upgrade_group): + """Test get post upgrade steps including ceph-mon.""" + args = MagicMock(spec_set=CLIargs)() + args.upgrade_group = upgrade_group + analysis_result = MagicMock(spec_set=Analysis)() + mock_get_ceph_mon_post_upgrade_steps.return_value = [MagicMock()] + + pre_upgrade_steps = cou_plan._get_post_upgrade_steps(analysis_result, args) + + assert pre_upgrade_steps == mock_get_ceph_mon_post_upgrade_steps.return_value + mock_get_ceph_mon_post_upgrade_steps.assert_called_with(analysis_result.apps_data_plane) + + +def test_get_ceph_mon_post_upgrade_steps_zero(model): + """Test get post upgrade step for ceph-mon without any ceph-mon app.""" + analysis_result = MagicMock(spec_set=Analysis)() + analysis_result.apps_control_plane = [] + + step = cou_plan._get_ceph_mon_post_upgrade_steps(analysis_result) + + assert bool(step) is False + + +def test_get_ceph_mon_post_upgrade_steps_multiple(model): + """Test get post upgrade step for ceph-mon with multiple ceph-mon.""" + machines = {"0": MagicMock(spec_set=COUMachine)} + units = { + "ceph-mon/0": COUUnit( + name="ceph-mon/0", + workload_version="17.0.1", + machine=machines["0"], + ) + } + ceph_mon = CephMon( + name="ceph-mon", + can_upgrade_to="", + charm="ceph-mon", + channel="quincy/stable", + config={"source": {"value": "distro"}}, + machines={}, + model=model, + origin="ch", + series="focal", + subordinate_to=[], + units=units, + workload_version="17.0.1", + ) + + exp_steps = 2 * [ + PostUpgradeStep( + "Ensure the 'require-osd-release' option in 'ceph-mon' matches the 'ceph-osd' version", + coro=app_utils.set_require_osd_release_option("ceph-mon/0", model), + ) + ] + + steps = cou_plan._get_ceph_mon_post_upgrade_steps([ceph_mon, ceph_mon]) + + assert steps == exp_steps @patch("cou.steps.plan.create_upgrade_group") @@ -763,17 +836,19 @@ def test_generate_control_plane_plan(mock_create_upgrade_group): @pytest.mark.asyncio -@patch("cou.steps.plan.generate_common_plan") -@patch("cou.steps.plan.determine_upgrade_target") @patch("cou.steps.plan.pre_plan_sanity_checks") -@patch("cou.steps.plan._generate_data_plane_plan") +@patch("cou.steps.plan.determine_upgrade_target") +@patch("cou.steps.plan._get_pre_upgrade_steps") @patch("cou.steps.plan._generate_control_plane_plan") +@patch("cou.steps.plan._generate_data_plane_plan") +@patch("cou.steps.plan._get_post_upgrade_steps") async def test_generate_plan_upgrade_group_None( - mock_control_plane, + mock_post_upgrade_steps, mock_data_plane, - mock_pre_plan_sanity_checks, + mock_control_plane, + mock_pre_upgrade_steps, mock_determine_upgrade_target, - mock_generate_common_plan, + mock_pre_plan_sanity_checks, cli_args, ): cli_args.upgrade_group = None @@ -783,24 +858,26 @@ async def test_generate_plan_upgrade_group_None( mock_pre_plan_sanity_checks.assert_called_once() mock_determine_upgrade_target.assert_called_once() - mock_generate_common_plan.assert_called_once() - + mock_pre_upgrade_steps.assert_called_once() mock_control_plane.assert_called_once() mock_data_plane.assert_called_once() + mock_post_upgrade_steps.assert_called_once() @pytest.mark.asyncio -@patch("cou.steps.plan.generate_common_plan") -@patch("cou.steps.plan.determine_upgrade_target") @patch("cou.steps.plan.pre_plan_sanity_checks") -@patch("cou.steps.plan._generate_data_plane_plan") +@patch("cou.steps.plan.determine_upgrade_target") +@patch("cou.steps.plan._get_pre_upgrade_steps") @patch("cou.steps.plan._generate_control_plane_plan") +@patch("cou.steps.plan._generate_data_plane_plan") +@patch("cou.steps.plan._get_post_upgrade_steps") async def test_generate_plan_upgrade_group_control_plane( - mock_control_plane, + mock_post_upgrade_steps, mock_data_plane, - mock_pre_plan_sanity_checks, + mock_control_plane, + mock_pre_upgrade_steps, mock_determine_upgrade_target, - mock_generate_common_plan, + mock_pre_plan_sanity_checks, cli_args, ): cli_args.upgrade_group = CONTROL_PLANE @@ -810,25 +887,27 @@ async def test_generate_plan_upgrade_group_control_plane( mock_pre_plan_sanity_checks.assert_called_once() mock_determine_upgrade_target.assert_called_once() - mock_generate_common_plan.assert_called_once() - + mock_pre_upgrade_steps.assert_called_once() mock_control_plane.assert_called_once() mock_data_plane.assert_not_called() + mock_post_upgrade_steps.assert_called_once() @pytest.mark.asyncio @pytest.mark.parametrize("upgrade_group", [DATA_PLANE, HYPERVISORS]) -@patch("cou.steps.plan.generate_common_plan") -@patch("cou.steps.plan.determine_upgrade_target") @patch("cou.steps.plan.pre_plan_sanity_checks") -@patch("cou.steps.plan._generate_data_plane_plan") +@patch("cou.steps.plan.determine_upgrade_target") +@patch("cou.steps.plan._get_pre_upgrade_steps") @patch("cou.steps.plan._generate_control_plane_plan") +@patch("cou.steps.plan._generate_data_plane_plan") +@patch("cou.steps.plan._get_post_upgrade_steps") async def test_generate_plan_upgrade_group_data_plane( - mock_control_plane, + mock_post_upgrade_steps, mock_data_plane, - mock_pre_plan_sanity_checks, + mock_control_plane, + mock_pre_upgrade_steps, mock_determine_upgrade_target, - mock_generate_common_plan, + mock_pre_plan_sanity_checks, upgrade_group, cli_args, ): @@ -839,7 +918,7 @@ async def test_generate_plan_upgrade_group_data_plane( mock_pre_plan_sanity_checks.assert_called_once() mock_determine_upgrade_target.assert_called_once() - mock_generate_common_plan.assert_called_once() - + mock_pre_upgrade_steps.assert_called_once() mock_control_plane.assert_not_called() mock_data_plane.assert_called_once() + mock_post_upgrade_steps.assert_called_once()