From 9b965e66011f642dcead6ab96cdaa5ba9487bb29 Mon Sep 17 00:00:00 2001 From: Robert Gildein Date: Fri, 5 Apr 2024 13:45:24 +0200 Subject: [PATCH 1/4] Run sanity checks in HypervisorUpgradePlanner Run sanity checks for each in HypervisorUpgradePlanner. Improve unit tests for whole HypervisorUpgradePlanner. fixes: #343 --- cou/steps/hypervisor.py | 29 +++++- tests/unit/steps/test_hypervisor.py | 144 ++++++++++++++++++++++++---- 2 files changed, 152 insertions(+), 21 deletions(-) diff --git a/cou/steps/hypervisor.py b/cou/steps/hypervisor.py index 9b5cc1b2..72103ec1 100644 --- a/cou/steps/hypervisor.py +++ b/cou/steps/hypervisor.py @@ -160,6 +160,31 @@ def get_azs(self, target: OpenStackRelease) -> AZs: return azs + def _upgrade_plan_sanity_checks( + self, target: OpenStackRelease, group: HypervisorGroup + ) -> None: + """Run sanity checks before generating upgrade plan for hypervisor group. + + :param target: OpenStack codename to upgrade. + :type target: OpenStackRelease + :param group: HypervisorGroup object + :type group: HypervisorGroup + """ + for app in self.apps: + if app.name not in group.app_units: + logger.debug( + "skipping application %s because it is not part of group %s", + app.name, + group.name, + ) + continue + + units = group.app_units[app.name] + logger.info("running sanoty checks for %s units of %s app", app.name, units) + # Note(rgildein): We don't catch the error here because we shouldn't generate any + # update plan if sanity checks for any application in the group fails. + app.upgrade_plan_sanity_checks(target, units) + def _generate_pre_upgrade_steps( self, target: OpenStackRelease, group: HypervisorGroup ) -> list[PreUpgradeStep]: @@ -216,7 +241,6 @@ def _generate_upgrade_steps( units = group.app_units[app.name] logger.info("generating upgrade steps for %s units of %s app", app.name, units) - steps.extend(app.upgrade_steps(target, units, force)) return steps @@ -269,6 +293,9 @@ def generate_upgrade_plan(self, target: OpenStackRelease, force: bool) -> Upgrad hypervisor_plan = HypervisorUpgradePlan( f"Upgrade plan for '{group.name}' to '{target}'" ) + # snity checks + logger.debug("running sanity checks for %s AZ", az) + self._upgrade_plan_sanity_checks(target, group) # pre upgrade steps logger.debug("generating pre-upgrade steps for %s AZ", az) diff --git a/tests/unit/steps/test_hypervisor.py b/tests/unit/steps/test_hypervisor.py index 01618296..c6f4010a 100644 --- a/tests/unit/steps/test_hypervisor.py +++ b/tests/unit/steps/test_hypervisor.py @@ -14,56 +14,160 @@ """Test hypervisor package.""" -from unittest.mock import MagicMock +from unittest.mock import AsyncMock, MagicMock, patch from cou.apps.base import OpenStackApplication from cou.apps.core import NovaCompute -from cou.steps import PostUpgradeStep, PreUpgradeStep, UpgradeStep +from cou.steps import ( + HypervisorUpgradePlan, + PostUpgradeStep, + PreUpgradeStep, + UpgradeStep, +) from cou.steps.hypervisor import AZs, HypervisorGroup, HypervisorUpgradePlanner from cou.utils.juju_utils import Application, Machine, Unit from cou.utils.openstack import OpenStackRelease from tests.unit.utils import dedent_plan, generate_cou_machine -def _generate_app() -> MagicMock: +def _generate_app(name: str) -> MagicMock: app = MagicMock(spec_set=OpenStackApplication)() - app.pre_upgrade_steps.return_value = [MagicMock(spec_set=PreUpgradeStep)()] - app.upgrade_steps.return_value = [MagicMock(spec_set=UpgradeStep)()] - app.post_upgrade_steps.return_value = [MagicMock(spec_set=PostUpgradeStep)()] + app.name = name + app.upgrade_plan_sanity_checks = MagicMock() + app.pre_upgrade_steps.return_value = [PreUpgradeStep(f"{name}-pre-upgrade", coro=AsyncMock())] + app.upgrade_steps.return_value = [UpgradeStep(f"{name}-upgrade", coro=AsyncMock())] + app.post_upgrade_steps.return_value = [ + PostUpgradeStep(f"{name}-post-upgrade", coro=AsyncMock()) + ] return app +def test_upgrade_plan_sanity_checks(): + """Test run app sanity checks.""" + target = OpenStackRelease("victoria") + machines = [Machine(f"{i}", (), f"az{i}") for i in range(3)] + app_units = { + "app1": [Unit(f"app1/{i}", machines[i], "1") for i in range(3)], + "app2": [Unit(f"app2/{i}", machines[i], "1") for i in range(3)], + } + apps = [_generate_app("app1"), _generate_app("app2"), _generate_app("app3")] + # Note(rgildein): it contains only two apps, so app3 should be skipped + group = HypervisorGroup("test", app_units) + planner = HypervisorUpgradePlanner(apps, machines) + + planner._upgrade_plan_sanity_checks(target, group) + + apps[0].upgrade_plan_sanity_checks.assert_called_once_with(target, app_units["app1"]) + apps[1].upgrade_plan_sanity_checks.assert_called_once_with(target, app_units["app2"]) + apps[2].upgrade_plan_sanity_checks.assert_not_called() + + def test_generate_pre_upgrade_steps(): """Test generating of pre-upgrade steps.""" target = OpenStackRelease("victoria") - units = ["1", "2", "3"] machines = [Machine(f"{i}", (), f"az{i}") for i in range(3)] - apps = [_generate_app() for _ in range(3)] + app_units = { + "app1": [Unit(f"app1/{i}", machines[i], "1") for i in range(3)], + "app2": [Unit(f"app2/{i}", machines[i], "1") for i in range(3)], + } + apps = [_generate_app("app1"), _generate_app("app2"), _generate_app("app3")] + exp_steps = apps[0].pre_upgrade_steps.return_value + apps[1].pre_upgrade_steps.return_value + # Note(rgildein): it contains only two apps, so app3 should be skipped + group = HypervisorGroup("test", app_units) planner = HypervisorUpgradePlanner(apps, machines) - group = HypervisorGroup("test", {app.name.return_value: units for app in apps}) - planner = HypervisorUpgradePlanner(apps, machines) steps = planner._generate_pre_upgrade_steps(target, group) - for step, app in zip(steps, apps): - app.pre_upgrade_steps.assert_called_once_with(target, units=units) - assert step == app.pre_upgrade_steps.return_value[0] # mocked app contain single step + apps[0].pre_upgrade_steps.assert_called_once_with(target, app_units["app1"]) + apps[1].pre_upgrade_steps.assert_called_once_with(target, app_units["app2"]) + apps[2].pre_upgrade_steps.assert_not_called() + + assert steps == exp_steps + + +def test_generate_upgrade_steps(): + """Test generating of upgrade steps.""" + target = OpenStackRelease("victoria") + machines = [Machine(f"{i}", (), f"az{i}") for i in range(3)] + app_units = { + "app1": [Unit(f"app1/{i}", machines[i], "1") for i in range(3)], + "app2": [Unit(f"app2/{i}", machines[i], "1") for i in range(3)], + } + apps = [_generate_app("app1"), _generate_app("app2"), _generate_app("app3")] + exp_steps = apps[0].upgrade_steps.return_value + apps[1].upgrade_steps.return_value + # Note(rgildein): it contains only two apps, so app3 should be skipped + group = HypervisorGroup("test", app_units) + planner = HypervisorUpgradePlanner(apps, machines) + + steps = planner._generate_upgrade_steps(target, False, group) + + apps[0].upgrade_steps.assert_called_once_with(target, app_units["app1"], False) + apps[1].upgrade_steps.assert_called_once_with(target, app_units["app2"], False) + apps[2].upgrade_steps.assert_not_called() + + assert steps == exp_steps def test_generate_post_upgrade_steps(): """Test generating of post-upgrade steps.""" target = OpenStackRelease("victoria") - units = ["1", "2", "3"] machines = [Machine(f"{i}", (), f"az{i}") for i in range(3)] - apps = [_generate_app() for _ in range(3)] - group = HypervisorGroup("test", {app.name.return_value: units for app in apps}) - + app_units = { + "app1": [Unit(f"app1/{i}", machines[i], "1") for i in range(3)], + "app2": [Unit(f"app2/{i}", machines[i], "1") for i in range(3)], + } + apps = [_generate_app("app1"), _generate_app("app2"), _generate_app("app3")] + exp_steps = apps[0].post_upgrade_steps.return_value + apps[1].post_upgrade_steps.return_value + # Note(rgildein): it contains only two apps, so app3 should be skipped + group = HypervisorGroup("test", app_units) planner = HypervisorUpgradePlanner(apps, machines) + steps = planner._generate_post_upgrade_steps(target, group) - for step, app in zip(steps, apps[::-1]): # using reversed order of applications - app.post_upgrade_steps.assert_called_once_with(target, units=units) - assert step == app.post_upgrade_steps.return_value[0] # mocked app contain single step + apps[0].post_upgrade_steps.assert_called_once_with(target, units=app_units["app1"]) + apps[1].post_upgrade_steps.assert_called_once_with(target, units=app_units["app2"]) + apps[2].post_upgrade_steps.assert_not_called() + + assert steps == exp_steps + + +@patch("cou.steps.hypervisor.HypervisorUpgradePlanner.get_azs") +@patch("cou.steps.hypervisor.HypervisorUpgradePlanner._upgrade_plan_sanity_checks") +@patch("cou.steps.hypervisor.HypervisorUpgradePlanner._generate_pre_upgrade_steps") +@patch("cou.steps.hypervisor.HypervisorUpgradePlanner._generate_upgrade_steps") +@patch("cou.steps.hypervisor.HypervisorUpgradePlanner._generate_post_upgrade_steps") +def test_generate_upgrade_plan( + post_upgrade_steps, upgrade_steps, pre_upgrade_steps, sanity_checks, get_azs +): + """Test generating upgrade plan with hypervisors upgrade planer.""" + target = OpenStackRelease("victoria") + group = MagicMock(spec_set=HypervisorGroup)() + get_azs.return_value = {"az0": group} + # Note(rgildein): We need to define return value, because plan will not add empty steps. + pre_upgrade_steps.return_value = [PreUpgradeStep("pre-upgrade", coro=AsyncMock())] + upgrade_steps.return_value = [UpgradeStep("upgrade", coro=AsyncMock())] + post_upgrade_steps.return_value = [PostUpgradeStep("post-upgrade", coro=AsyncMock())] + + # Note(rgildein): We do not need to provide apps or machines, since everything is mocked. + planner = HypervisorUpgradePlanner([], []) + + plan = planner.generate_upgrade_plan(target, False) + + sanity_checks.assert_called_once_with(target, group) + pre_upgrade_steps.assert_called_once_with(target, group) + upgrade_steps.assert_called_once_with(target, False, group) + post_upgrade_steps.assert_called_once_with(target, group) + + assert plan.description == "Upgrading all applications deployed on machines with hypervisor." + assert len(plan.sub_steps) == 1 + assert isinstance(plan.sub_steps[0], HypervisorUpgradePlan) + assert plan.sub_steps[0].description == f"Upgrade plan for '{group.name}' to '{target}'" + assert ( + plan.sub_steps[0].sub_steps + == pre_upgrade_steps.return_value + + upgrade_steps.return_value + + post_upgrade_steps.return_value + ) def test_hypervisor_group(): From fdbfd7d7cf48727cbf15b800add488d5dc4dbe7e Mon Sep 17 00:00:00 2001 From: Robert Gildein Date: Mon, 8 Apr 2024 13:31:33 +0200 Subject: [PATCH 2/4] move sanity checks before get_azs --- cou/steps/hypervisor.py | 32 ++++++++++------------------- tests/unit/steps/test_hypervisor.py | 22 ++++++++------------ 2 files changed, 20 insertions(+), 34 deletions(-) diff --git a/cou/steps/hypervisor.py b/cou/steps/hypervisor.py index 72103ec1..e136f13a 100644 --- a/cou/steps/hypervisor.py +++ b/cou/steps/hypervisor.py @@ -160,30 +160,19 @@ def get_azs(self, target: OpenStackRelease) -> AZs: return azs - def _upgrade_plan_sanity_checks( - self, target: OpenStackRelease, group: HypervisorGroup - ) -> None: - """Run sanity checks before generating upgrade plan for hypervisor group. + def _upgrade_plan_sanity_checks(self, target: OpenStackRelease) -> None: + """Run sanity checks for al applications. :param target: OpenStack codename to upgrade. :type target: OpenStackRelease - :param group: HypervisorGroup object - :type group: HypervisorGroup """ for app in self.apps: - if app.name not in group.app_units: - logger.debug( - "skipping application %s because it is not part of group %s", - app.name, - group.name, - ) - continue - - units = group.app_units[app.name] - logger.info("running sanoty checks for %s units of %s app", app.name, units) + logger.info("running sanity checks for '%s'", app.name) # Note(rgildein): We don't catch the error here because we shouldn't generate any - # update plan if sanity checks for any application in the group fails. - app.upgrade_plan_sanity_checks(target, units) + # update plan if sanity checks for any application fails. + # Note(rgildein): Running sanity checks for all units of applications. We cannot pass + # None here, since _check_mismatched_versions could be raised. + app.upgrade_plan_sanity_checks(target, list(app.units.values())) def _generate_pre_upgrade_steps( self, target: OpenStackRelease, group: HypervisorGroup @@ -288,14 +277,15 @@ def generate_upgrade_plan(self, target: OpenStackRelease, force: bool) -> Upgrad :return: Full upgrade plan :rtype: UpgradePlan """ + # snity checks + logger.debug("running sanity checks") + self._upgrade_plan_sanity_checks(target) + plan = UpgradePlan("Upgrading all applications deployed on machines with hypervisor.") for az, group in self.get_azs(target).items(): hypervisor_plan = HypervisorUpgradePlan( f"Upgrade plan for '{group.name}' to '{target}'" ) - # snity checks - logger.debug("running sanity checks for %s AZ", az) - self._upgrade_plan_sanity_checks(target, group) # pre upgrade steps logger.debug("generating pre-upgrade steps for %s AZ", az) diff --git a/tests/unit/steps/test_hypervisor.py b/tests/unit/steps/test_hypervisor.py index c6f4010a..89861239 100644 --- a/tests/unit/steps/test_hypervisor.py +++ b/tests/unit/steps/test_hypervisor.py @@ -46,20 +46,16 @@ def test_upgrade_plan_sanity_checks(): """Test run app sanity checks.""" target = OpenStackRelease("victoria") machines = [Machine(f"{i}", (), f"az{i}") for i in range(3)] - app_units = { - "app1": [Unit(f"app1/{i}", machines[i], "1") for i in range(3)], - "app2": [Unit(f"app2/{i}", machines[i], "1") for i in range(3)], - } - apps = [_generate_app("app1"), _generate_app("app2"), _generate_app("app3")] - # Note(rgildein): it contains only two apps, so app3 should be skipped - group = HypervisorGroup("test", app_units) - planner = HypervisorUpgradePlanner(apps, machines) + app1 = _generate_app("app1") + app1.units = {f"app1/{i}": Unit(f"app1/{i}", machines[i], "1") for i in range(3)} + app2 = _generate_app("app2") + app1.units = {f"app2/{i}": Unit(f"app2/{i}", machines[i], "1") for i in range(3)} + planner = HypervisorUpgradePlanner([app1, app2], machines) - planner._upgrade_plan_sanity_checks(target, group) + planner._upgrade_plan_sanity_checks(target) - apps[0].upgrade_plan_sanity_checks.assert_called_once_with(target, app_units["app1"]) - apps[1].upgrade_plan_sanity_checks.assert_called_once_with(target, app_units["app2"]) - apps[2].upgrade_plan_sanity_checks.assert_not_called() + app1.upgrade_plan_sanity_checks.assert_called_once_with(target, list(app1.units.values())) + app2.upgrade_plan_sanity_checks.assert_called_once_with(target, list(app2.units.values())) def test_generate_pre_upgrade_steps(): @@ -153,7 +149,7 @@ def test_generate_upgrade_plan( plan = planner.generate_upgrade_plan(target, False) - sanity_checks.assert_called_once_with(target, group) + sanity_checks.assert_called_once_with(target) pre_upgrade_steps.assert_called_once_with(target, group) upgrade_steps.assert_called_once_with(target, False, group) post_upgrade_steps.assert_called_once_with(target, group) From 9b894a553dfc42433a02c1499759d2edb916f0b1 Mon Sep 17 00:00:00 2001 From: Robert Gildein Date: Tue, 9 Apr 2024 09:28:42 +0200 Subject: [PATCH 3/4] Update cou/steps/hypervisor.py Co-authored-by: TQ X --- cou/steps/hypervisor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cou/steps/hypervisor.py b/cou/steps/hypervisor.py index e136f13a..2a033e72 100644 --- a/cou/steps/hypervisor.py +++ b/cou/steps/hypervisor.py @@ -277,7 +277,7 @@ def generate_upgrade_plan(self, target: OpenStackRelease, force: bool) -> Upgrad :return: Full upgrade plan :rtype: UpgradePlan """ - # snity checks + # sanity checks logger.debug("running sanity checks") self._upgrade_plan_sanity_checks(target) From 37d62077d539e8aca7d45a539e23b6c1968a978e Mon Sep 17 00:00:00 2001 From: Robert Gildein Date: Wed, 10 Apr 2024 22:52:50 +0200 Subject: [PATCH 4/4] revert running sanity check per each group --- cou/steps/hypervisor.py | 31 +++++++++++++++-------- tests/unit/steps/test_hypervisor.py | 39 ++++++++++++++++------------- 2 files changed, 43 insertions(+), 27 deletions(-) diff --git a/cou/steps/hypervisor.py b/cou/steps/hypervisor.py index 2a033e72..f8f40bce 100644 --- a/cou/steps/hypervisor.py +++ b/cou/steps/hypervisor.py @@ -160,19 +160,30 @@ def get_azs(self, target: OpenStackRelease) -> AZs: return azs - def _upgrade_plan_sanity_checks(self, target: OpenStackRelease) -> None: - """Run sanity checks for al applications. + def _upgrade_plan_sanity_checks( + self, target: OpenStackRelease, group: HypervisorGroup + ) -> None: + """Run sanity checks before generating upgrade plan for hypervisor group. :param target: OpenStack codename to upgrade. :type target: OpenStackRelease + :param group: HypervisorGroup object + :type group: HypervisorGroup """ for app in self.apps: - logger.info("running sanity checks for '%s'", app.name) + if app.name not in group.app_units: + logger.debug( + "skipping application %s because it is not part of group %s", + app.name, + group.name, + ) + continue + + units = group.app_units[app.name] + logger.info("running sanity checks for %s units of %s app", app.name, units) # Note(rgildein): We don't catch the error here because we shouldn't generate any # update plan if sanity checks for any application fails. - # Note(rgildein): Running sanity checks for all units of applications. We cannot pass - # None here, since _check_mismatched_versions could be raised. - app.upgrade_plan_sanity_checks(target, list(app.units.values())) + app.upgrade_plan_sanity_checks(target, units) def _generate_pre_upgrade_steps( self, target: OpenStackRelease, group: HypervisorGroup @@ -277,16 +288,16 @@ def generate_upgrade_plan(self, target: OpenStackRelease, force: bool) -> Upgrad :return: Full upgrade plan :rtype: UpgradePlan """ - # sanity checks - logger.debug("running sanity checks") - self._upgrade_plan_sanity_checks(target) - plan = UpgradePlan("Upgrading all applications deployed on machines with hypervisor.") for az, group in self.get_azs(target).items(): hypervisor_plan = HypervisorUpgradePlan( f"Upgrade plan for '{group.name}' to '{target}'" ) + # sanity checks + logger.debug("running sanity checks for %s AZ", az) + self._upgrade_plan_sanity_checks(target, group) + # pre upgrade steps logger.debug("generating pre-upgrade steps for %s AZ", az) hypervisor_plan.add_steps(self._generate_pre_upgrade_steps(target, group)) diff --git a/tests/unit/steps/test_hypervisor.py b/tests/unit/steps/test_hypervisor.py index 89861239..418eb230 100644 --- a/tests/unit/steps/test_hypervisor.py +++ b/tests/unit/steps/test_hypervisor.py @@ -14,7 +14,7 @@ """Test hypervisor package.""" -from unittest.mock import AsyncMock, MagicMock, patch +from unittest.mock import AsyncMock, MagicMock, call, patch from cou.apps.base import OpenStackApplication from cou.apps.core import NovaCompute @@ -46,16 +46,20 @@ def test_upgrade_plan_sanity_checks(): """Test run app sanity checks.""" target = OpenStackRelease("victoria") machines = [Machine(f"{i}", (), f"az{i}") for i in range(3)] - app1 = _generate_app("app1") - app1.units = {f"app1/{i}": Unit(f"app1/{i}", machines[i], "1") for i in range(3)} - app2 = _generate_app("app2") - app1.units = {f"app2/{i}": Unit(f"app2/{i}", machines[i], "1") for i in range(3)} - planner = HypervisorUpgradePlanner([app1, app2], machines) + app_units = { + "app1": [Unit(f"app1/{i}", machines[i], "1") for i in range(3)], + "app2": [Unit(f"app2/{i}", machines[i], "1") for i in range(3)], + } + apps = [_generate_app("app1"), _generate_app("app2"), _generate_app("app3")] + # Note(rgildein): it contains only two apps, so app3 should be skipped + group = HypervisorGroup("test", app_units) + planner = HypervisorUpgradePlanner(apps, machines) - planner._upgrade_plan_sanity_checks(target) + planner._upgrade_plan_sanity_checks(target, group) - app1.upgrade_plan_sanity_checks.assert_called_once_with(target, list(app1.units.values())) - app2.upgrade_plan_sanity_checks.assert_called_once_with(target, list(app2.units.values())) + apps[0].upgrade_plan_sanity_checks.assert_called_once_with(target, app_units["app1"]) + apps[1].upgrade_plan_sanity_checks.assert_called_once_with(target, app_units["app2"]) + apps[2].upgrade_plan_sanity_checks.assert_not_called() def test_generate_pre_upgrade_steps(): @@ -137,8 +141,9 @@ def test_generate_upgrade_plan( ): """Test generating upgrade plan with hypervisors upgrade planer.""" target = OpenStackRelease("victoria") - group = MagicMock(spec_set=HypervisorGroup)() - get_azs.return_value = {"az0": group} + group1 = MagicMock(spec_set=HypervisorGroup)() + group2 = MagicMock(spec_set=HypervisorGroup)() + get_azs.return_value = {"az0": group1, "az1": group2} # Note(rgildein): We need to define return value, because plan will not add empty steps. pre_upgrade_steps.return_value = [PreUpgradeStep("pre-upgrade", coro=AsyncMock())] upgrade_steps.return_value = [UpgradeStep("upgrade", coro=AsyncMock())] @@ -149,15 +154,15 @@ def test_generate_upgrade_plan( plan = planner.generate_upgrade_plan(target, False) - sanity_checks.assert_called_once_with(target) - pre_upgrade_steps.assert_called_once_with(target, group) - upgrade_steps.assert_called_once_with(target, False, group) - post_upgrade_steps.assert_called_once_with(target, group) + sanity_checks.assert_has_calls([call(target, group1), call(target, group2)]) + pre_upgrade_steps.assert_has_calls([call(target, group1), call(target, group2)]) + upgrade_steps.assert_has_calls([call(target, False, group1), call(target, False, group2)]) + post_upgrade_steps.assert_has_calls([call(target, group1), call(target, group2)]) assert plan.description == "Upgrading all applications deployed on machines with hypervisor." - assert len(plan.sub_steps) == 1 + assert len(plan.sub_steps) == 2 assert isinstance(plan.sub_steps[0], HypervisorUpgradePlan) - assert plan.sub_steps[0].description == f"Upgrade plan for '{group.name}' to '{target}'" + assert plan.sub_steps[0].description == f"Upgrade plan for '{group1.name}' to '{target}'" assert ( plan.sub_steps[0].sub_steps == pre_upgrade_steps.return_value