Merge ~rgildein/juju-verify:bug/1920131 into ~canonical-solutions-engineering/juju-verify:master

[Robert Gildein]

CephOsd.get_ceph_mon_units refactor

Change the output of the CephOsd.get_ceph_mon_units function to provide a map between the verified and ceph-mon units.

Closes-bug: LP#1920131

---
[LP#1920131]: https://bugs.launchpad.net/juju-verify/+bug/1920131

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:518e02ea3dd601e9a50a88c02644076d4a414f24
https://jenkins.canonical.com/bootstack/job/juju-verify-test/47/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/generic-make-test/68/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/47/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/juju-verify-test/47//rebuild

review: Approve (continuous-integration)

[Alvaro Uria (aluria)]

Please find some comments inline.

review: Needs Fixing

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:b0d38d1a068a2db24f22d3edb7c5f1015f362911
https://jenkins.canonical.com/bootstack/job/juju-verify-test/48/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/generic-make-test/69/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/48/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/juju-verify-test/48//rebuild

review: Approve (continuous-integration)

[Alvaro Uria (aluria)]

I have a few comments - but this solution is perfect.

review: Needs Fixing

[Alvaro Uria (aluria)]

+1 Thank you!

review: Approve

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:0439acdff2bd73afcbf2c5761b29ed95e902957d
https://jenkins.canonical.com/bootstack/job/juju-verify-test/49/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/generic-make-test/70/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/49/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/juju-verify-test/49//rebuild

review: Approve (continuous-integration)

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision d61340b5e2f6db760e387ffb1d6e256373ffab9a

diff --git a/juju_verify/verifiers/ceph.py b/juju_verify/verifiers/ceph.py
index 28bca0a..32556cc 100644
--- a/juju_verify/verifiers/ceph.py
+++ b/juju_verify/verifiers/ceph.py
@@ -16,7 +16,7 @@
 # this program. If not, see https://www.gnu.org/licenses/.
 """ceph-osd verification."""
 import logging
-from typing import Set
+from typing import Dict, Optional
 
 from juju.unit import Unit
 
@@ -62,34 +62,45 @@ class CephOsd(CephCommon):
 
     NAME = 'ceph-osd'
 
-    def get_ceph_mon_units(self) -> Set[Unit]:
-        """Get Ceph-mon units related to verified units.
-
-        1. get all distinct ceph-osd applications from provides units
-        2. get all relationships based on found apps or ceph-mon
-        3. get the first unit from the application providing the relation
+    def _get_ceph_mon_unit(self, app_name: str) -> Optional[Unit]:
+        """Get first ceph-mon unit from relation."""
+        try:
+            for relation in self.model.applications[app_name].relations:
+                if relation.matches(f"{app_name}:mon"):
+                    # selecting the first unit from the application provided by relation
+                    return relation.provides.application.units[0]
+        except (IndexError, KeyError) as error:
+            logger.debug("Error to get ceph-mon unit from relations: %s", error)
+
+        return None
+
+    def get_ceph_mon_units(self) -> Dict[str, Unit]:
+        """Get first ceph-mon units related to verified units.
+
+        This function groups by distinct application names for verified units, and then
+        finds the relation ("<application>:mon") between the application and ceph-mon.
+        The first unit of ceph-mon will be obtained from this relation.
+        :returns: Map between verified and ceph-mon units
         """
-        # get all affected ceph-osd applications
         applications = {unit.application for unit in self.units}
         logger.debug("affected applications %s", map(str, applications))
-        applications.add("ceph-osd")
 
-        # get all relation between ceph-osd and ceph-mon
-        relations = {relation for relation in self.model.relations
-                     if any(relation.matches(f"{application}:mon")
-                            for application in applications)}
-        logger.debug("found relations %s", map(str, relations))
+        ceph_mon_app_map = {}
+        for app_name in applications:
+            unit = self._get_ceph_mon_unit(app_name)
+            if unit is not None:
+                ceph_mon_app_map[app_name] = unit
 
-        # get first ceph-mon unit from relation
-        ceph_mon_units = {relation.provides.application.units[0]
-                          for relation in relations}
-        logger.debug("found units %s", map(str, ceph_mon_units))
+        logger.debug("found units %s", map(str, ceph_mon_app_map.values()))
 
-        return ceph_mon_units
+        return ceph_mon_app_map
 
     def verify_reboot(self) -> Result:
         """Verify that it's safe to reboot selected ceph-osd units."""
-        return aggregate_results(self.check_cluster_health(*self.get_ceph_mon_units()))
+        ceph_mon_app_map = self.get_ceph_mon_units()
+        # get unique ceph-mon units
+        unique_ceph_mon_units = set(ceph_mon_app_map.values())
+        return aggregate_results(self.check_cluster_health(*unique_ceph_mon_units))
 
     def verify_shutdown(self) -> Result:
         """Verify that it's safe to shutdown selected ceph-osd units."""
diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py
index 42b4426..8a7b55a 100644
--- a/tests/unit/conftest.py
+++ b/tests/unit/conftest.py
@@ -56,7 +56,7 @@ def model(session_mocker, all_units):
     mock_model = Model()
     session_mocker.patch.object(Model, 'connect_current')
     session_mocker.patch.object(Model, 'connect_model')
-    session_mocker.patch.object(Model, 'relations')
+    session_mocker.patch.object(Model, 'applications')
     session_mocker.patch.object(Unit, 'data')
     session_mocker.patch.object(Unit, 'machine')
     session_mocker.patch.object(Unit, 'run_action', new_callable=MagicMock)
diff --git a/tests/unit/verifiers/test_ceph.py b/tests/unit/verifiers/test_ceph.py
index 728a4cb..f5d16ad 100644
--- a/tests/unit/verifiers/test_ceph.py
+++ b/tests/unit/verifiers/test_ceph.py
@@ -74,25 +74,49 @@ def test_check_cluster_health_unknown_state(mock_run_action_on_units, model):
     assert result == Result(False, "Ceph cluster is in an unknown state")
 
 
-def test_get_ceph_mon_units(model):
-    """Test function to get ceph-mon units related to verified units."""
-    ceph_osd_units = [model.units["ceph-osd/0"], model.units["ceph-osd/1"]]
+def test_get_ceph_mon_unit(model):
+    """Test get ceph-mon unit related to application."""
     ceph_mon_units = [model.units["ceph-mon/0"], model.units["ceph-mon/1"],
                       model.units["ceph-mon/2"]]
     mock_relation = MagicMock()
     mock_relation.matches = {"ceph-osd:mon": True}.get
     mock_relation.provides.application.units = ceph_mon_units
-    model.relations = [mock_relation]
+    mock_relations = MagicMock()
+    mock_relations.relations = [mock_relation]
+    model.applications = {"ceph-osd": mock_relations}
+
+    # return first ceph-mon unit in "ceph-osd:mon" relations
+    unit = CephOsd([model.units["ceph-osd/0"]])._get_ceph_mon_unit("ceph-osd")
+    assert unit == ceph_mon_units[0]
+
+    # return none for non-existent application name
+    model.applications = {"ceph-osd-cluster": mock_relations}
+    unit = CephOsd([model.units["ceph-osd/0"]])._get_ceph_mon_unit("ceph-osd")
+    assert unit is None
+
+    # return none for application with no units
+    mock_relation.provides.application.units = []
+    model.applications = {"ceph-osd": mock_relations}
+    unit = CephOsd([model.units["ceph-osd/0"]])._get_ceph_mon_unit("ceph-osd")
+    assert unit is None
+
+
[e-mail]("juju_verify.verifiers.ceph.CephOsd._get_ceph_mon_unit")
+def test_get_ceph_mon_units(mock_get_ceph_mon_unit, model):
+    """Test function to get ceph-mon units related to verified units."""
+    ceph_osd_units = [model.units["ceph-osd/0"], model.units["ceph-osd/1"]]
+    mock_get_ceph_mon_unit.return_value = model.units["ceph-mon/0"]
 
     units = CephOsd(ceph_osd_units).get_ceph_mon_units()
-    assert {ceph_mon_units[0]} == units
+
+    assert units == {"ceph-osd": model.units["ceph-mon/0"]}
 
 
 @mock.patch("juju_verify.verifiers.ceph.CephOsd.get_ceph_mon_units")
 @mock.patch("juju_verify.verifiers.ceph.CephCommon.check_cluster_health")
 def test_verify_reboot(mock_check_cluster_health, mock_get_ceph_mon_units, model):
     """Test reboot verification on CephOsd."""
-    mock_get_ceph_mon_units.return_value = [model.units["ceph-mon/0"]]
+    mock_get_ceph_mon_units.return_value = {"ceph-osd": model.units["ceph-mon/0"]}
     mock_check_cluster_health.return_value = Result(True, "Ceph cluster is healthy")
 
     result = CephOsd([model.units["ceph-osd/0"]]).verify_reboot()
@@ -104,7 +128,7 @@ def test_verify_reboot(mock_check_cluster_health, mock_get_ceph_mon_units, model
 @mock.patch("juju_verify.verifiers.ceph.CephCommon.check_cluster_health")
 def test_verify_shutdown(mock_check_cluster_health, mock_get_ceph_mon_units, model):
     """Test shutdown verification on CephOsd."""
-    mock_get_ceph_mon_units.return_value = [model.units["ceph-mon/0"]]
+    mock_get_ceph_mon_units.return_value = {"ceph-osd": model.units["ceph-mon/0"]}
     mock_check_cluster_health.return_value = Result(True, "Ceph cluster is healthy")
 
     result = CephOsd([model.units["ceph-osd/0"]]).verify_shutdown()