Post uptgrade steps accept units to check if reached expected target. (…

…#249)

- changed name from _check_upgrade to _get_reached_expected_target_step
- description shows which units are going to be checked if workload got
upgraded

cou/apps/base.py

@@ -19,7 +19,7 @@
 from collections import defaultdict
 from dataclasses import dataclass, field
 from io import StringIO
-from typing import Any, Optional
+from typing import Any, Iterable, Optional
 
 from ruamel.yaml import YAML
 
@@ -319,30 +319,6 @@ def new_origin(self, target: OpenStackRelease) -> str:
         """
         return f"cloud:{self.series}-{target.codename}"
 
-    async def _check_upgrade(self, target: OpenStackRelease) -> None:
-        """Check if an application has upgraded its workload version.
-
-        :param target: OpenStack release as target to upgrade.
-        :type target: OpenStackRelease
-        :raises ApplicationError: When the workload version of the charm doesn't upgrade.
-        """
-        status = await self.model.get_status()
-        app_status = status.applications.get(self.name)
-        units_not_upgraded = []
-        for unit in app_status.units.keys():
-            workload_version = app_status.units[unit].workload_version
-            compatible_os_versions = OpenStackCodenameLookup.find_compatible_versions(
-                self.charm, workload_version
-            )
-            if target not in compatible_os_versions:
-                units_not_upgraded.append(unit)
-
-        if units_not_upgraded:
-            units_not_upgraded_string = ", ".join(units_not_upgraded)
-            raise ApplicationError(
-                f"Cannot upgrade units '{units_not_upgraded_string}' to {target}."
-            )
-
     def pre_upgrade_steps(
         self, target: OpenStackRelease, units: Optional[list[COUUnit]]
     ) -> list[PreUpgradeStep]:
@@ -397,19 +373,23 @@ def upgrade_steps(
             self._get_change_install_repository_step(target),
         ]
 
-    def post_upgrade_steps(self, target: OpenStackRelease) -> list[PostUpgradeStep]:
+    def post_upgrade_steps(
+        self, target: OpenStackRelease, units: Optional[Iterable[COUUnit]]
+    ) -> list[PostUpgradeStep]:
         """Post Upgrade steps planning.
 
         Wait until the application reaches the idle state and then check the target workload.
 
         :param target: OpenStack release as target to upgrade.
         :type target: OpenStackRelease
+        :param units: Units to generate post upgrade plan
+        :type units: Optional[Iterable[COUUnit]]
         :return: List of post upgrade steps.
         :rtype: list[PostUpgradeStep]
         """
         return [
             self._get_wait_step(),
-            self._get_reached_expected_target_step(target),
+            self._get_reached_expected_target_step(target, units),
         ]
 
     def generate_upgrade_plan(
@@ -437,7 +417,7 @@ def generate_upgrade_plan(
         all_steps = (
             self.pre_upgrade_steps(target, units)
             + self.upgrade_steps(target, units, force)
-            + self.post_upgrade_steps(target)
+            + self.post_upgrade_steps(target, units)
         )
         for step in all_steps:
             if step:
@@ -661,19 +641,55 @@ def _get_change_install_repository_step(self, target: OpenStackRelease) -> Upgra
         )
         return UpgradeStep()
 
-    def _get_reached_expected_target_step(self, target: OpenStackRelease) -> PostUpgradeStep:
+    def _get_reached_expected_target_step(
+        self, target: OpenStackRelease, units: Optional[Iterable[COUUnit]]
+    ) -> PostUpgradeStep:
         """Get post upgrade step to check if application workload has been upgraded.
 
         :param target: OpenStack release as target to upgrade.
         :type target: OpenStackRelease
+        :param units: Units to generate post upgrade plan
+        :type units: Optional[Iterable[COUUnit]]
         :return: Post Upgrade step to check if application workload has been upgraded.
         :rtype: PostUpgradeStep
         """
+        if not units:
+            units = list(self.units.values())
         return PostUpgradeStep(
-            description=f"Check if the workload of '{self.name}' has been upgraded",
-            coro=self._check_upgrade(target),
+            description=(
+                f"Check if the workload of '{self.name}' has been upgraded on units: "
+                f"{', '.join([unit.name for unit in units])}"
+            ),
+            coro=self._verify_workload_upgrade(target, units),
         )
 
+    async def _verify_workload_upgrade(
+        self, target: OpenStackRelease, units: Iterable[COUUnit]
+    ) -> None:
+        """Check if an application has upgraded its workload version.
+
+        :param target: OpenStack release as target to upgrade.
+        :type target: OpenStackRelease
+        :param units: Units to check if got upgraded
+        :type units: Iterable[COUUnit]
+        :raises ApplicationError: When the workload version of the charm doesn't upgrade.
+        """
+        status = await self.model.get_status()
+        app_status = status.applications.get(self.name)
+        units_not_upgraded = []
+        for unit in units:
+            workload_version = app_status.units[unit.name].workload_version
+            compatible_os_versions = OpenStackCodenameLookup.find_compatible_versions(
+                self.charm, workload_version
+            )
+            if target not in compatible_os_versions:
+                units_not_upgraded.append(unit.name)
+
+        if units_not_upgraded:
+            raise ApplicationError(
+                f"Cannot upgrade units '{', '.join(units_not_upgraded)}' to {target}."
+            )
+
     def _get_wait_step(self) -> PostUpgradeStep:
         """Get wait step for entire model or application.
 

cou/apps/channel_based.py

@@ -13,6 +13,7 @@
 # limitations under the License.
 """Channel based application class."""
 import logging
+from typing import Iterable, Optional
 
 from cou.apps.base import OpenStackApplication
 from cou.apps.factory import AppFactory
@@ -59,17 +60,21 @@ def is_versionless(self) -> bool:
         """
         return not all(unit.workload_version for unit in self.units.values())
 
-    def post_upgrade_steps(self, target: OpenStackRelease) -> list[PostUpgradeStep]:
+    def post_upgrade_steps(
+        self, target: OpenStackRelease, units: Optional[Iterable[COUUnit]]
+    ) -> list[PostUpgradeStep]:
         """Post Upgrade steps planning.
 
         Wait until the application reaches the idle state and then check the target workload.
         In case the application is versionless, there are no post upgrade steps to run.
 
         :param target: OpenStack release as target to upgrade.
         :type target: OpenStackRelease
+        :param units: Units to generate post upgrade plan
+        :type units: Optional[Iterable[COUUnit]]
         :return: List of post upgrade steps.
         :rtype: list[PostUpgradeStep]
         """
         if self.is_versionless:
             return []
-        return super().post_upgrade_steps(target)
+        return super().post_upgrade_steps(target, units)

cou/apps/subordinate.py

@@ -13,7 +13,7 @@
 #  limitations under the License.
 """Subordinate application class."""
 import logging
-from typing import Optional
+from typing import Iterable, Optional
 
 from cou.apps.base import OpenStackApplication
 from cou.apps.factory import AppFactory
@@ -57,11 +57,15 @@ def upgrade_steps(
         """
         return [self._get_upgrade_charm_step(target)]
 
-    def post_upgrade_steps(self, target: OpenStackRelease) -> list[PostUpgradeStep]:
+    def post_upgrade_steps(
+        self, target: OpenStackRelease, units: Optional[Iterable[COUUnit]]
+    ) -> list[PostUpgradeStep]:
         """Post Upgrade steps planning.
 
         :param target: OpenStack release as target to upgrade.
         :type target: OpenStackRelease
+        :param units: Units to generate post upgrade plan
+        :type units: Optional[Iterable[COUUnit]]
         :return: List of post upgrade steps.
         :rtype: list[PostUpgradeStep]
         """

tests/functional/tests/smoke.py

@@ -143,7 +143,8 @@ def generate_expected_plan(self, backup: bool = True) -> str:
             "\t\t\t\tUpgrade software packages on unit designate-bind/0\n"
             "\t\t\tUpgrade 'designate-bind' to the new channel: 'victoria/stable'\n"
             "\t\t\tWait 300s for app designate-bind to reach the idle state.\n"
-            "\t\t\tCheck if the workload of 'designate-bind' has been upgraded\n"
+            "\t\t\tCheck if the workload of 'designate-bind' has been upgraded on units:"
+            " designate-bind/0\n"
             "\t\tUpgrade plan for 'mysql-innodb-cluster' to victoria\n"
             "\t\t\tUpgrade software packages of 'mysql-innodb-cluster' "
             "from the current APT repositories\n"
@@ -153,7 +154,8 @@ def generate_expected_plan(self, backup: bool = True) -> str:
             "\t\t\tChange charm config of 'mysql-innodb-cluster' 'source' to "
             "'cloud:focal-victoria'\n"
             "\t\t\tWait 1800s for app mysql-innodb-cluster to reach the idle state.\n"
-            "\t\t\tCheck if the workload of 'mysql-innodb-cluster' has been upgraded\n"
+            "\t\t\tCheck if the workload of 'mysql-innodb-cluster' has been upgraded on units: "
+            "mysql-innodb-cluster/0, mysql-innodb-cluster/1, mysql-innodb-cluster/2\n"
         )
 
     def test_help(self) -> None:

tests/unit/apps/test_auxiliary.py

@@ -175,9 +175,12 @@ def test_auxiliary_upgrade_plan_ussuri_to_victoria_change_channel(model):
             coro=model.wait_for_active_idle(1800, apps=None),
         ),
         PostUpgradeStep(
-            description=f"Check if the workload of '{app.name}' has been upgraded",
+            description=(
+                f"Check if the workload of '{app.name}' has been upgraded on units: "
+                f"{', '.join([unit for unit in app.units.keys()])}"
+            ),
             parallel=False,
-            coro=app._check_upgrade(target),
+            coro=app._verify_workload_upgrade(target, app.units.values()),
         ),
     ]
     add_steps(expected_plan, upgrade_steps)
@@ -250,9 +253,12 @@ def test_auxiliary_upgrade_plan_ussuri_to_victoria(model):
             coro=model.wait_for_active_idle(1800, apps=None),
         ),
         PostUpgradeStep(
-            description=f"Check if the workload of '{app.name}' has been upgraded",
+            description=(
+                f"Check if the workload of '{app.name}' has been upgraded on units: "
+                f"{', '.join([unit for unit in app.units.keys()])}"
+            ),
             parallel=False,
-            coro=app._check_upgrade(target),
+            coro=app._verify_workload_upgrade(target, app.units.values()),
         ),
     ]
     add_steps(expected_plan, upgrade_steps)
@@ -331,9 +337,12 @@ def test_auxiliary_upgrade_plan_ussuri_to_victoria_ch_migration(model):
             coro=model.wait_for_active_idle(1800, apps=None),
         ),
         PostUpgradeStep(
-            description=f"Check if the workload of '{app.name}' has been upgraded",
+            description=(
+                f"Check if the workload of '{app.name}' has been upgraded on units: "
+                f"{', '.join([unit for unit in app.units.keys()])}"
+            ),
             parallel=False,
-            coro=app._check_upgrade(target),
+            coro=app._verify_workload_upgrade(target, app.units.values()),
         ),
     ]
     add_steps(expected_plan, upgrade_steps)
@@ -657,9 +666,12 @@ def test_ceph_mon_upgrade_plan_xena_to_yoga(model):
             coro=model.wait_for_active_idle(1800, apps=None),
         ),
         PostUpgradeStep(
-            description=f"Check if the workload of '{app.name}' has been upgraded",
+            description=(
+                f"Check if the workload of '{app.name}' has been upgraded on units: "
+                f"{', '.join([unit for unit in app.units.keys()])}"
+            ),
             parallel=False,
-            coro=app._check_upgrade(target),
+            coro=app._verify_workload_upgrade(target, app.units.values()),
         ),
     ]
     add_steps(expected_plan, upgrade_steps)
@@ -738,9 +750,12 @@ def test_ceph_mon_upgrade_plan_ussuri_to_victoria(model):
             coro=model.wait_for_active_idle(1800, apps=None),
         ),
         PostUpgradeStep(
-            description=f"Check if the workload of '{app.name}' has been upgraded",
+            description=(
+                f"Check if the workload of '{app.name}' has been upgraded on units: "
+                f"{', '.join([unit for unit in app.units.keys()])}"
+            ),
             parallel=False,
-            coro=app._check_upgrade(target),
+            coro=app._verify_workload_upgrade(target, app.units.values()),
         ),
     ]
     add_steps(expected_plan, upgrade_steps)
@@ -918,9 +933,12 @@ def test_ovn_principal_upgrade_plan(model):
             coro=model.wait_for_active_idle(300, apps=[app.name]),
         ),
         PostUpgradeStep(
-            description=f"Check if the workload of '{app.name}' has been upgraded",
+            description=(
+                f"Check if the workload of '{app.name}' has been upgraded on units: "
+                f"{', '.join([unit for unit in app.units.keys()])}"
+            ),
             parallel=False,
-            coro=app._check_upgrade(target),
+            coro=app._verify_workload_upgrade(target, app.units.values()),
         ),
     ]
     add_steps(expected_plan, upgrade_steps)
@@ -994,9 +1012,12 @@ def test_mysql_innodb_cluster_upgrade(model):
             coro=model.wait_for_active_idle(1800, apps=[app.name]),
         ),
         PostUpgradeStep(
-            description=f"Check if the workload of '{app.name}' has been upgraded",
+            description=(
+                f"Check if the workload of '{app.name}' has been upgraded on units: "
+                f"{', '.join([unit for unit in app.units.keys()])}"
+            ),
             parallel=False,
-            coro=app._check_upgrade(target),
+            coro=app._verify_workload_upgrade(target, app.units.values()),
         ),
     ]
     add_steps(expected_plan, upgrade_steps)

tests/unit/apps/test_base.py

@@ -20,6 +20,7 @@
 from cou.exceptions import ApplicationError
 from cou.steps import UnitUpgradeStep, UpgradeStep
 from cou.utils.juju_utils import COUMachine, COUUnit
+from cou.utils.openstack import OpenStackRelease
 from tests.unit.utils import assert_steps
 
 
@@ -251,3 +252,31 @@ def test_get_upgrade_current_release_packages_step(mock_upgrade_packages, units,
 
     app._get_upgrade_current_release_packages_step(units)
     mock_upgrade_packages.assert_has_calls(expected_calls)
+
+
+@pytest.mark.parametrize(
+    "units",
+    [
+        [],
+        [COUUnit(f"my_app/{unit}", MagicMock(), MagicMock()) for unit in range(1)],
+        [COUUnit(f"my_app/{unit}", MagicMock(), MagicMock()) for unit in range(2)],
+        [COUUnit(f"my_app/{unit}", MagicMock(), MagicMock()) for unit in range(3)],
+    ],
+)
+@patch("cou.apps.base.OpenStackApplication._verify_workload_upgrade")
+def test_get_reached_expected_target_step(mock_workload_upgrade, units, model):
+    target = OpenStackRelease("victoria")
+    mock = MagicMock()
+    charm = "app"
+    app_name = "my_app"
+    channel = "ussuri/stable"
+    app_units = {f"my_app/{unit}": COUUnit(f"my_app/{unit}", mock, mock) for unit in range(3)}
+
+    app = OpenStackApplication(
+        app_name, "", charm, channel, {}, {}, model, "ch", "focal", [], app_units, "21.0.1"
+    )
+
+    expected_calls = [call(target, units)] if units else [call(target, list(app.units.values()))]
+
+    app._get_reached_expected_target_step(target, units)
+    mock_workload_upgrade.assert_has_calls(expected_calls)

tests/unit/apps/test_channel_based.py

@@ -307,9 +307,12 @@ def test_application_gnocchi_upgrade_plan_ussuri_to_victoria(model):
             coro=model.wait_for_active_idle(300, apps=[app.name]),
         ),
         PostUpgradeStep(
-            description=f"Check if the workload of '{app.name}' has been upgraded",
+            description=(
+                f"Check if the workload of '{app.name}' has been upgraded on units: "
+                f"{', '.join([unit for unit in app.units.keys()])}"
+            ),
             parallel=False,
-            coro=app._check_upgrade(target),
+            coro=app._verify_workload_upgrade(target, app.units.values()),
         ),
     ]
 
@@ -393,9 +396,12 @@ def test_application_designate_bind_upgrade_plan_ussuri_to_victoria(model):
             coro=model.wait_for_active_idle(300, apps=[app.name]),
         ),
         PostUpgradeStep(
-            description=f"Check if the workload of '{app.name}' has been upgraded",
+            description=(
+                f"Check if the workload of '{app.name}' has been upgraded on units: "
+                f"{', '.join([unit for unit in app.units.keys()])}"
+            ),
             parallel=False,
-            coro=app._check_upgrade(target),
+            coro=app._verify_workload_upgrade(target, app.units.values()),
         ),
     ]