- removed is_data_plane from machine

- channel_codename property started failing coverage and we don't need to make another check because channel is already checked. - finished unit tests
gabrielcocenza · Jan 24, 2024 · 5ab9ca7 · 5ab9ca7
1 parent 646d3c8
commit 5ab9ca7
Show file tree

Hide file tree

Showing 9 changed files with 137 additions and 100 deletions.
diff --git a/cou/apps/auxiliary.py b/cou/apps/auxiliary.py
@@ -185,7 +185,7 @@ def pre_upgrade_plan(self, target: OpenStackRelease) -> list[PreUpgradeStep]:
         return super().pre_upgrade_plan(target)
 
 
-@AppFactory.register_application(["mysql_innodb_cluster"])
+@AppFactory.register_application(["mysql-innodb-cluster"])
 class MysqlInnodbClusterApplication(OpenStackAuxiliaryApplication):
     """Application for mysql-innodb-cluster charm."""
 

diff --git a/cou/apps/base.py b/cou/apps/base.py
@@ -372,16 +372,8 @@ def channel_codename(self) -> OpenStackRelease:
         :return: OpenStackRelease object
         :rtype: OpenStackRelease
         """
-        try:
-            # get the OpenStack release from the channel track of the application.
-            os_track_release_channel = OpenStackRelease(self._get_track_from_channel(self.channel))
-        except ValueError:
-            logger.debug(
-                "The current channel of '%s' does not exist or is unexpectedly formatted",
-                self.name,
-            )
-            os_track_release_channel = self.current_os_release
-        return os_track_release_channel
+        # get the OpenStack release from the channel track of the application.
+        return OpenStackRelease(self._get_track_from_channel(self.channel))
 
     @property
     def can_upgrade_current_channel(self) -> bool:

diff --git a/cou/apps/machine.py b/cou/apps/machine.py
@@ -25,7 +25,6 @@ class Machine:
     machine_id: str
     hostname: str
     az: Optional[str]  # simple deployments may not have azs
-    is_data_plane: bool
 
     def __repr__(self) -> str:
         """Representation of the juju Machine.

diff --git a/cou/steps/analyze.py b/cou/steps/analyze.py
@@ -22,7 +22,7 @@
 from cou.apps.base import OpenStackApplication
 from cou.apps.factory import AppFactory
 from cou.utils import juju_utils
-from cou.utils.openstack import UPGRADE_ORDER, OpenStackRelease
+from cou.utils.openstack import DATA_PLANE_CHARMS, UPGRADE_ORDER, OpenStackRelease
 
 logger = logging.getLogger(__name__)
 
@@ -39,8 +39,6 @@ class Analysis:
     :type apps_data_plane:  list[OpenStackApplication]
     """
 
-    # pylint: disable=too-many-instance-attributes
-
     model: juju_utils.COUModel
     apps_control_plane: list[OpenStackApplication]
     apps_data_plane: list[OpenStackApplication]
@@ -68,9 +66,25 @@ def _split_apps(
         :return: Control plane and data plane application lists.
         :rtype: tuple[list[OpenStackApplication], list[OpenStackApplication]]
         """
+
+        def is_data_plane(app: OpenStackApplication) -> bool:
+            """Check if app belong to data plane.
+
+            :param app: application
+            :type app: OpenStackApplication
+            :return: boolean
+            :rtype: bool
+            """
+            return app.charm in DATA_PLANE_CHARMS
+
         control_plane, data_plane = [], []
+        data_plane_machines = {
+            unit.machine for app in apps if is_data_plane(app) for unit in app.units
+        }
         for app in apps:
-            if any(unit.machine.is_data_plane for unit in app.units):
+            if is_data_plane(app):
+                data_plane.append(app)
+            elif any(unit.machine in data_plane_machines for unit in app.units):
                 data_plane.append(app)
             else:
                 control_plane.append(app)
@@ -91,11 +105,7 @@ async def create(cls, model: juju_utils.COUModel) -> Analysis:
 
         control_plane, data_plane = cls._split_apps(apps)
 
-        return Analysis(
-            model=model,
-            apps_data_plane=data_plane,
-            apps_control_plane=control_plane,
-        )
+        return Analysis(model=model, apps_data_plane=data_plane, apps_control_plane=control_plane)
 
     @classmethod
     async def _populate(cls, model: juju_utils.COUModel) -> list[OpenStackApplication]:
@@ -111,15 +121,14 @@ async def _populate(cls, model: juju_utils.COUModel) -> list[OpenStackApplicatio
         :rtype: List[OpenStackApplication]
         """
         juju_status = await model.get_status()
-        juju_applications = await model.get_applications()
         juju_machines = await model.get_model_machines()
         apps = {
             AppFactory.create(
                 name=app,
                 status=app_status,
-                config=juju_applications[app].get_config(),
+                config=await model.get_application_config(app),
                 model=model,
-                charm=juju_applications[app].charm_name,
+                charm=await model.get_charm_name(app),
                 machines={
                     unit_status.machine: juju_machines[unit_status.machine]
                     for unit_status in app_status.units.values()

diff --git a/cou/utils/juju_utils.py b/cou/utils/juju_utils.py
@@ -25,6 +25,7 @@
 from juju.client.connector import NoConnectionException
 from juju.client.jujudata import FileJujuData
 from juju.errors import JujuAppError, JujuError, JujuUnitError
+from juju.machine import Machine as JujuMachine
 from juju.model import Model
 from juju.unit import Unit
 from macaroonbakery.httpbakery import BakeryException
@@ -40,7 +41,7 @@
     UnitNotFound,
     WaitForApplicationsTimeout,
 )
-from cou.utils.openstack import DATA_PLANE_CHARMS, is_charm_supported
+from cou.utils.openstack import is_charm_supported
 
 JUJU_MAX_FRAME_SIZE: int = 2**30
 DEFAULT_TIMEOUT: int = int(os.environ.get("COU_TIMEOUT", 10))
@@ -191,14 +192,14 @@ async def _get_action_result(self, action: Action, raise_on_failure: bool) -> Ac
 
         return result
 
-    async def get_applications(self) -> dict[str, Application]:
-        """Get all applications from the model.
+    async def _get_machines(self) -> dict[str, JujuMachine]:
+        """Get all machines from the model.
 
-        :return: Applications from the model connected.
+        :return: Machines from the model connected.
         :rtype: dict[str, Application]
         """
         model = await self._get_model()
-        return model.applications
+        return model.machines
 
     async def _get_application(self, name: str) -> Application:
         """Get juju.application.Application from model.
@@ -516,20 +517,12 @@ async def get_model_machines(self) -> dict[str, Machine]:
         :return: Dictionary of the machines found in the model. E.g: {'0': Machine0}
         :rtype: dict[str, Machine]
         """
-        juju_applications = await self.get_applications()
-        machines: dict[str, Machine] = {}
-        for app in juju_applications.values():
-            charm_name = app.charm_name
-            for unit in app.units:
-                unit_machine = unit.machine.data
-                machine = machines.get(
-                    unit_machine["id"],
-                    Machine(
-                        machine_id=unit_machine["id"],
-                        hostname=unit_machine["hostname"],
-                        az=unit_machine["hardware-characteristic"].get("availability-zone"),
-                        is_data_plane=charm_name in DATA_PLANE_CHARMS,
-                    ),
-                )
-                machines[unit_machine["id"]] = machine
-        return machines
+        juju_machines = await self._get_machines()
+        return {
+            machine.id: Machine(
+                machine_id=machine.data["id"],
+                hostname=machine.data["hostname"],
+                az=machine.data["hardware-characteristics"].get("availability-zone"),
+            )
+            for machine in juju_machines.values()
+        }
diff --git a/tests/unit/apps/test_machines.py b/tests/unit/apps/test_machines.py
@@ -17,40 +17,29 @@
 
 
 @pytest.mark.parametrize(
-    "machine_id, hostname, az, is_data_plane",
+    "machine_id, hostname, az",
     [
         # one field different is considered another machine
-        ("0", "juju-c307f8-my_model-0", "zone-1", True),
-        ("1", "juju-c307f8-my_model-1", "zone-2", False),
+        ("0", "juju-c307f8-my_model-0", "zone-3"),
+        ("1", "juju-c307f8-my_model-1", "zone-2"),
     ],
 )
-def test_machine_not_eq(machine_id, hostname, az, is_data_plane):
-    machine_0 = Machine(
-        machine_id="0", hostname="juju-c307f8-my_model-0", az="zone-1", is_data_plane=False
-    )
-
-    machine_1 = Machine(
-        machine_id=machine_id, hostname=hostname, az=az, is_data_plane=is_data_plane
-    )
+def test_machine_not_eq(machine_id, hostname, az):
+    machine_0 = Machine(machine_id="0", hostname="juju-c307f8-my_model-0", az="zone-1")
+    machine_1 = Machine(machine_id=machine_id, hostname=hostname, az=az)
 
     assert machine_0 != machine_1
 
 
 def test_machine_eq():
-    machine_0 = Machine(
-        machine_id="0", hostname="juju-c307f8-my_model-0", az="zone-1", is_data_plane=False
-    )
+    machine_0 = Machine(machine_id="0", hostname="juju-c307f8-my_model-0", az="zone-1")
 
-    machine_1 = Machine(
-        machine_id="0", hostname="juju-c307f8-my_model-0", az="zone-1", is_data_plane=False
-    )
+    machine_1 = Machine(machine_id="0", hostname="juju-c307f8-my_model-0", az="zone-1")
 
     assert machine_0 == machine_1
 
 
 def test_machine_repr():
-    machine_0 = Machine(
-        machine_id="0", hostname="juju-c307f8-my_model-0", az="zone-1", is_data_plane=False
-    )
+    machine_0 = Machine(machine_id="0", hostname="juju-c307f8-my_model-0", az="zone-1")
     expected_repr = "Machine[0]"
     assert repr(machine_0) == expected_repr
diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py
@@ -105,34 +105,30 @@ def _generate_units(units_machines_workloads):
 @pytest.fixture
 def apps_machines():
     return {
-        **_generate_apps_machines("keystone", KEYSTONE_MACHINES, STANDARD_AZS, False),
-        **_generate_apps_machines("cinder", CINDER_MACHINES, STANDARD_AZS, False),
-        **_generate_apps_machines("nova-compute", NOVA_MACHINES, STANDARD_AZS, True),
-        **_generate_apps_machines("rmq", RMQ_MACHINES, STANDARD_AZS, False),
-        **_generate_apps_machines("ceph-mon", CEPH_MON_MACHINES, STANDARD_AZS, False),
-        **_generate_apps_machines("ovn-central", OVN_MACHINES, STANDARD_AZS, False),
-        **_generate_apps_machines("mysql-innodb-cluster", MYSQL_MACHINES, STANDARD_AZS, False),
+        **_generate_apps_machines("keystone", KEYSTONE_MACHINES, STANDARD_AZS),
+        **_generate_apps_machines("cinder", CINDER_MACHINES, STANDARD_AZS),
+        **_generate_apps_machines("nova-compute", NOVA_MACHINES, STANDARD_AZS),
+        **_generate_apps_machines("rmq", RMQ_MACHINES, STANDARD_AZS),
+        **_generate_apps_machines("ceph-mon", CEPH_MON_MACHINES, STANDARD_AZS),
+        **_generate_apps_machines("ovn-central", OVN_MACHINES, STANDARD_AZS),
+        **_generate_apps_machines("mysql-innodb-cluster", MYSQL_MACHINES, STANDARD_AZS),
         **_generate_apps_machines(
-            "glance-simplestreams-sync", GLANCE_SIMPLE_MACHINES, STANDARD_AZS, False
+            "glance-simplestreams-sync", GLANCE_SIMPLE_MACHINES, STANDARD_AZS
         ),
-        **_generate_apps_machines("gnocchi", GNOCCHI_MACHINES, STANDARD_AZS, False),
-        **_generate_apps_machines("designate-bind", DESIGNATE_MACHINES, STANDARD_AZS, False),
-        **_generate_apps_machines("ceph-osd", CEPH_OSD_MACHINES, STANDARD_AZS, True),
-        **_generate_apps_machines("my-app", MY_APP_MACHINES, STANDARD_AZS, False),
+        **_generate_apps_machines("gnocchi", GNOCCHI_MACHINES, STANDARD_AZS),
+        **_generate_apps_machines("designate-bind", DESIGNATE_MACHINES, STANDARD_AZS),
+        **_generate_apps_machines("ceph-osd", CEPH_OSD_MACHINES, STANDARD_AZS),
+        **_generate_apps_machines("my-app", MY_APP_MACHINES, STANDARD_AZS),
     }
 
 
-def _generate_apps_machines(charm, machines, azs, is_data_plane):
+def _generate_apps_machines(charm, machines, azs):
     hostnames = [f"{HOSTNAME_PREFIX}-{machine}" for machine in machines]
-    machines_hostnames_azs_is_data_plane = zip_longest(
-        machines, hostnames, azs, [is_data_plane], fillvalue=is_data_plane
-    )
+    machines_hostnames_azs = zip(machines, hostnames, azs)
     return {
         charm: {
-            machine_id: Machine(
-                machine_id=machine_id, hostname=hostname, az=az, is_data_plane=is_data_plane
-            )
-            for machine_id, hostname, az, is_data_plane in machines_hostnames_azs_is_data_plane
+            machine_id: Machine(machine_id=machine_id, hostname=hostname, az=az)
+            for machine_id, hostname, az in machines_hostnames_azs
         }
     }
 

diff --git a/tests/unit/steps/test_steps_analyze.py b/tests/unit/steps/test_steps_analyze.py
@@ -193,35 +193,32 @@ def _app(name, units):
     return app
 
 
-def _unit(machine_id, is_data_plane):
+def _unit(machine_id):
     unit = MagicMock(spec_set=ApplicationUnit).return_value
-    mock_machine = MagicMock(spec_set=Machine("1", "juju-efc45", "zone-1", False))
-    mock_machine.machine_id = machine_id
-    mock_machine.is_data_plane = is_data_plane
-    unit.machine = mock_machine
+    unit.machine = Machine(machine_id, "juju-efc45", "zone-1")
     return unit
 
 
 @pytest.mark.parametrize(
     "exp_control_plane, exp_data_plane",
     [
         (
-            [_app("keystone", [_unit("0", False), _unit("1", False), _unit("2", False)])],
-            [_app("ceph-osd", [_unit("3", True), _unit("4", True), _unit("5", True)])],
+            [_app("keystone", [_unit("0"), _unit("1"), _unit("2")])],
+            [_app("ceph-osd", [_unit("3"), _unit("4"), _unit("5")])],
         ),
         (
             [],
             [
-                _app("nova-compute", [_unit("0", True), _unit("1", True), _unit("2", True)]),
-                _app("keystone", [_unit("0", False), _unit("1", False), _unit("2", False)]),
-                _app("ceph-osd", [_unit("3", True), _unit("4", True), _unit("5", True)]),
+                _app("nova-compute", [_unit("0"), _unit("1"), _unit("2")]),
+                _app("keystone", [_unit("0"), _unit("1"), _unit("2")]),
+                _app("ceph-osd", [_unit("3"), _unit("4"), _unit("5")]),
             ],
         ),
         (
-            [_app("keystone", [_unit("6", False), _unit("7", False), _unit("8", False)])],
+            [_app("keystone", [_unit("6"), _unit("7"), _unit("8")])],
             [
-                _app("nova-compute", [_unit("0", True), _unit("1", True), _unit("2", True)]),
-                _app("ceph-osd", [_unit("3", True), _unit("4", True), _unit("5", True)]),
+                _app("nova-compute", [_unit("0"), _unit("1"), _unit("2")]),
+                _app("ceph-osd", [_unit("3"), _unit("4"), _unit("5")]),
             ],
         ),
     ],