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

[Robert Gildein]

Add check for number of replication

These changes provide a check number of replications. It groups the
units into an application and obtains the minimum number of
replications from the relation of the ceph-osd and ceph-mon
applications. The idea is to find the minimum number from the
replication list calculated from the pools "size" - "min_size".

If no pools are available, the check returns the Result(True).

Output example: [1]

Closes-bug: LP1917599

---
[1]: https://pastebin.ubuntu.com/p/8W4YyBVj6d/
[LP1917599]: https://bugs.launchpad.net/juju-verify/+bug/1917599

[🤖 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:d2f26e7b49bb228df13b0a7998089008e27f5a3b
https://jenkins.canonical.com/bootstack/job/juju-verify-test/56/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/generic-make-test/77/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/56/

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

review: Approve (continuous-integration)

[Alvaro Uria (aluria)]

In the Ceph charms, they use the word "replicas" instead of "replications". I'm fine using any of them as both are correct. However, I've suggested for the output string to use the word "replicas".

Code-related, it looks good and I will give it a final pass tomorrow morning.

[🤖 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:0e059f90938b2186bc9e92e5f4d33563c6f54235
https://jenkins.canonical.com/bootstack/job/juju-verify-test/57/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/generic-make-test/78/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/57/

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

review: Approve (continuous-integration)

[Alvaro Uria (aluria)]

Added a comment about how the minimum value of size/min_size is retrieved.

[Martin Kalcok (martin-kalcok)]

Overall lgtm, but I do have few recommendations (see inline comments).

[Alvaro Uria (aluria)]

Amended by comment about min(size - min_size).

[Robert Gildein (rgildein)]

Thanks for comments.

[🤖 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:3645e1c0aa35ca38cdac7268d4b642ef5c5e4903
https://jenkins.canonical.com/bootstack/job/juju-verify-test/59/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/generic-make-test/80/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/59/

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

review: Approve (continuous-integration)

[🤖 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:b84dcbc47d4f502a8476b51c139ef2d5bb60811a
https://jenkins.canonical.com/bootstack/job/juju-verify-test/60/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/generic-make-test/81/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/60/

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

review: Approve (continuous-integration)

[Martin Kalcok (martin-kalcok)]

+1 I'll give final approve after CI run. Thanks.

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:f291f852910e6db8caae92f7f82488f17fdedcdb
https://jenkins.canonical.com/bootstack/job/juju-verify-test/61/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/generic-make-test/82/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/61/

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

review: Approve (continuous-integration)

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

Change successfully merged at revision 1681157330e4f7208529c10dbec1b6cbd3fabfd9

diff --git a/juju_verify/utils/unit.py b/juju_verify/utils/unit.py
index 0318cdb..05a9785 100644
--- a/juju_verify/utils/unit.py
+++ b/juju_verify/utils/unit.py
@@ -17,7 +17,7 @@
 """Helper function to manage Juju unit."""
 import asyncio
 import re
-from typing import List, Dict
+from typing import List, Dict, Any
 
 from juju.action import Action
 from juju.unit import Unit
@@ -27,8 +27,8 @@ from juju_verify.exceptions import VerificationError, CharmException
 CHARM_URL_PATTERN = re.compile(r'^(.*):(.*/)?(?P<charm>.*)(-\d+)$')
 
 
-def run_action_on_units(units: List[Unit], action: str,
-                        **params: str) -> Dict[str, Action]:
+def run_action_on_units(
+        units: List[Unit], action: str, **params: Any) -> Dict[str, Action]:
     """Run juju action on specified units.
 
     :param units: List/Tuple of Unit object
diff --git a/juju_verify/verifiers/ceph.py b/juju_verify/verifiers/ceph.py
index 32556cc..96253bd 100644
--- a/juju_verify/verifiers/ceph.py
+++ b/juju_verify/verifiers/ceph.py
@@ -15,8 +15,9 @@
 # You should have received a copy of the GNU General Public License along with
 # this program. If not, see https://www.gnu.org/licenses/.
 """ceph-osd verification."""
+import json
 import logging
-from typing import Dict, Optional
+from typing import Dict, Optional, Any, List
 
 from juju.unit import Unit
 
@@ -36,6 +37,8 @@ class CephCommon(BaseVerifier):  # pylint: disable=W0223
         """Check Ceph cluster health for specific units.
 
         This will execute `get-health` against each unit provided.
+
+        :raises CharmException: if the units do not belong to the ceph-mon charm
         """
         verify_charm_unit("ceph-mon", *units)
         result = Result(success=True)
@@ -56,12 +59,52 @@ class CephCommon(BaseVerifier):  # pylint: disable=W0223
 
         return result
 
+    @classmethod
+    def get_replication_number(cls, unit: Unit) -> Optional[int]:
+        """Get minimum replication number from ceph-mon unit.
+
+        This function runs the `list-pools` action with the parameter 'detail=true'
+        to get the replication number.
+        :raises CharmException: if the unit does not belong to the ceph-mon charm
+        :raises TypeError: if the object pools is not iterable
+        :raises KeyError: if the pool detail does not contain `size` or `min_size`
+        :raises json.decoder.JSONDecodeError: if json.loads failed
+        """
+        verify_charm_unit("ceph-mon", unit)
+        action_map = run_action_on_units([unit], "list-pools", detail=True)
+        action_output = data_from_action(action_map.get(unit.entity_id), "pools")
+        logger.debug("parse information about pools: %s", action_output)
+        pools: List[Dict[str, Any]] = json.loads(action_output)
+
+        if pools:
+            return min([pool["size"] - pool["min_size"] for pool in pools])
+
+        return None
+
 
 class CephOsd(CephCommon):
     """Implementation of verification checks for the ceph-osd charm."""
 
     NAME = 'ceph-osd'
 
+    def __init__(self, units: List[Unit]):
+        """Ceph-osd charm verifier."""
+        super().__init__(units=units)
+        self._ceph_mon_app_map: Optional[Dict[str, Unit]] = None
+
+    @property
+    def ceph_mon_app_map(self) -> Dict[str, Unit]:
+        """Get a map between ceph-osd applications and the first ceph-mon unit.
+
+        :returns: Dictionary with keys as distinct applications of verified units and
+                  values as the first ceph-mon unit obtained from the relation with the
+                  ceph-mon application (<application_name>:mon).
+        """
+        if self._ceph_mon_app_map is None:
+            self._ceph_mon_app_map = self._get_ceph_mon_app_map()
+
+        return self._ceph_mon_app_map
+
     def _get_ceph_mon_unit(self, app_name: str) -> Optional[Unit]:
         """Get first ceph-mon unit from relation."""
         try:
@@ -74,7 +117,7 @@ class CephOsd(CephCommon):
 
         return None
 
-    def get_ceph_mon_units(self) -> Dict[str, Unit]:
+    def _get_ceph_mon_app_map(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
@@ -95,12 +138,38 @@ class CephOsd(CephCommon):
 
         return ceph_mon_app_map
 
+    def check_ceph_cluster_health(self) -> Result:
+        """Check Ceph cluster health for unique ceph-mon units from ceph_mon_app_map."""
+        unique_ceph_mon_units = set(self.ceph_mon_app_map.values())
+        return self.check_cluster_health(*unique_ceph_mon_units)
+
+    def check_replication_number(self) -> Result:
+        """Check the minimum number of replications for related applications."""
+        result = Result(True)
+
+        for app_name, ceph_mon_unit in self.ceph_mon_app_map.items():
+            min_replication_number = self.get_replication_number(ceph_mon_unit)
+            units = len([unit for unit in self.units if unit.application == app_name])
+            inactive_units = len(
+                [unit for unit in self.model.applications[app_name].units
+                 if unit.workload_status != "active"]
+            )
+
+            if (min_replication_number and
+                    (units + inactive_units) > min_replication_number):
+                result += Result(
+                    False,
+                    f"The minimum number of replicas in '{app_name}' is "
+                    f"{min_replication_number:d} and it's not safe to restart/shutdown "
+                    f"{units:d} units. {inactive_units:d} units are not active."
+                )
+
+        return result
+
     def verify_reboot(self) -> Result:
         """Verify that it's safe to reboot selected ceph-osd 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))
+        return aggregate_results(self.check_ceph_cluster_health(),
+                                 self.check_replication_number())
 
     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 8a7b55a..17b2f80 100644
--- a/tests/unit/conftest.py
+++ b/tests/unit/conftest.py
@@ -56,7 +56,6 @@ 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, 'applications')
     session_mocker.patch.object(Unit, 'data')
     session_mocker.patch.object(Unit, 'machine')
     session_mocker.patch.object(Unit, 'run_action', new_callable=MagicMock)
@@ -64,15 +63,26 @@ def model(session_mocker, all_units):
     session_mocker.patch.object(Action, 'status').return_value = 'pending'
     session_mocker.patch.object(Action, 'data')
     units = session_mocker.patch('juju.model.Model.units', new_callable=PropertyMock)
+    applications = session_mocker.patch('juju.model.Model.applications',
+                                        new_callable=PropertyMock)
 
-    unit_map = {}
+    unit_map, app_map = {}, {}
     for unit_id in all_units:
         unit = Unit(unit_id, mock_model)
         charm_name = unit_id.rstrip(digits + '/')
         unit.data = {'charm-url': 'cs:focal/{}-1'.format(charm_name),
-                     "application": charm_name}
+                     "application": charm_name, "workload-status": {"current": "active"}}
         unit_map[unit_id] = unit
         unit.run_action.return_value = Action(unit_id + '-action', mock_model)
+
+        # add unit to model.applications
+        if charm_name not in app_map:
+            app_map[charm_name] = MagicMock()
+            app_map[charm_name].units = []
+
+        app_map[charm_name].units.append(unit)
+
     units.return_value = unit_map
+    applications.return_value = app_map
 
     return mock_model
diff --git a/tests/unit/verifiers/test_ceph.py b/tests/unit/verifiers/test_ceph.py
index f5d16ad..b9fb4b5 100644
--- a/tests/unit/verifiers/test_ceph.py
+++ b/tests/unit/verifiers/test_ceph.py
@@ -15,12 +15,14 @@
 # You should have received a copy of the GNU General Public License along with
 # this program. If not, see https://www.gnu.org/licenses/.
 """CephOsd verifier class test suite."""
+import json
 import os
 from unittest import mock
 from unittest.mock import MagicMock
 
 import pytest
 
+from juju_verify.exceptions import CharmException
 from juju_verify.verifiers import CephOsd, Result
 from juju_verify.verifiers.ceph import CephCommon
 
@@ -74,6 +76,37 @@ 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_check_cluster_health_error(model):
+    """Test check Ceph cluster health raise CharmException."""
+    with pytest.raises(CharmException):
+        CephCommon.check_cluster_health(model.units["ceph-osd/0"])
+
+
[e-mail]("juju_verify.verifiers.ceph.run_action_on_units")
+def test_get_replication_number(mock_run_action_on_units, model):
+    """Test get minimum replication number from ceph-mon unit."""
+    action = MagicMock()
+    action.data.get.side_effect = {"results": {"pools": json.dumps([
+        {"pool": 1, "name": "test_1", "size": 5, "min_size": 2},
+        {"pool": 2, "name": "test_2", "size": 5, "min_size": 2},
+        {"pool": 3, "name": "test_3", "size": 3, "min_size": 2},
+    ])}}.get
+    mock_run_action_on_units.return_value = {"ceph-mon/0": action}
+
+    # test find minimum replication in list of 3 pools
+    assert CephCommon.get_replication_number(model.units["ceph-mon/0"]) == 1
+
+    # test return None if list of pools is empty
+    action.data.get.side_effect = {"results": {"pools": json.dumps([])}}.get
+    assert CephCommon.get_replication_number(model.units["ceph-mon/0"]) is None
+
+
+def test_get_replication_number_error(model):
+    """Test get minimum replication number from ceph-mon unit raise CharmException."""
+    with pytest.raises(CharmException):
+        CephCommon.get_replication_number(model.units["ceph-osd/0"])
+
+
 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"],
@@ -81,56 +114,114 @@ def test_get_ceph_mon_unit(model):
     mock_relation = MagicMock()
     mock_relation.matches = {"ceph-osd:mon": True}.get
     mock_relation.provides.application.units = ceph_mon_units
-    mock_relations = MagicMock()
-    mock_relations.relations = [mock_relation]
-    model.applications = {"ceph-osd": mock_relations}
+    model.applications["ceph-osd"].relations = [mock_relation]
 
     # 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")
+    unit = CephOsd([model.units["ceph-osd/0"]])._get_ceph_mon_unit("ceph-osd-cluster")
     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
 
 
 @mock.patch("juju_verify.verifiers.ceph.CephOsd._get_ceph_mon_unit")
-def test_get_ceph_mon_units(mock_get_ceph_mon_unit, model):
+def test_get_ceph_mon_app_map(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()
+    units = CephOsd(ceph_osd_units)._get_ceph_mon_app_map()
 
     assert units == {"ceph-osd": model.units["ceph-mon/0"]}
 
 
[e-mail]("juju_verify.verifiers.ceph.CephOsd.get_ceph_mon_units")
[e-mail]("juju_verify.verifiers.ceph.CephOsd._get_ceph_mon_app_map")
[e-mail]("juju_verify.verifiers.ceph.CephCommon.check_cluster_health")
+def test_check_ceph_cluster_health(
+        mock_check_cluster_health, mock_get_ceph_mon_app_map, model):
+    """Test check the Ceph cluster health for unique ceph-mon units."""
+    mock_get_ceph_mon_app_map.return_value = {"ceph-osd": model.units["ceph-mon/0"]}
+    mock_check_cluster_health.return_value = Result(True)
+
+    ceph_osd_verifier = CephOsd([model.units["ceph-osd/0"]])
+    assert ceph_osd_verifier.check_ceph_cluster_health() == Result(True)
+    mock_check_cluster_health.assert_called_once_with(model.units["ceph-mon/0"])
+
+
[e-mail]("juju_verify.verifiers.ceph.CephOsd._get_ceph_mon_app_map")
[e-mail]("juju_verify.verifiers.ceph.CephCommon.get_replication_number")
+def test_check_replication_number(
+        mock_get_replication_number, mock_get_ceph_mon_app_map, model):
+    """Test check the minimum number of replications for related applications."""
+    mock_get_replication_number.return_value = 1
+    mock_get_ceph_mon_app_map.return_value = {"ceph-osd": model.units["ceph-mon/0"]}
+
+    # verified one ceph-osd unit
+    ceph_osd_verifier = CephOsd([model.units["ceph-osd/0"]])
+    assert ceph_osd_verifier.check_replication_number() == Result(True)
+
+    # verified two ceph-osd unit
+    ceph_osd_verifier = CephOsd([model.units["ceph-osd/0"], model.units["ceph-osd/1"]])
+    assert ceph_osd_verifier.check_replication_number() == Result(
+        False, "The minimum number of replicas in 'ceph-osd' is 1 and "
+               "it's not safe to restart/shutdown 2 units. 0 units are not active."
+    )
+
+    # verified one ceph-osd unit, if there is an unit that is not in an active state
+    model.units["ceph-osd/1"].data["workload-status"]["current"] = "blocked"
+    ceph_osd_verifier = CephOsd([model.units["ceph-osd/0"]])
+    assert ceph_osd_verifier.check_replication_number() == Result(
+        False, "The minimum number of replicas in 'ceph-osd' is 1 and "
+               "it's not safe to restart/shutdown 1 units. 1 units are not active."
+    )
+    model.units["ceph-osd/1"].data["workload-status"]["current"] = "active"
+
+
[e-mail]("juju_verify.verifiers.ceph.CephOsd._get_ceph_mon_app_map")
 @mock.patch("juju_verify.verifiers.ceph.CephCommon.check_cluster_health")
-def test_verify_reboot(mock_check_cluster_health, mock_get_ceph_mon_units, model):
[e-mail]("juju_verify.verifiers.ceph.CephCommon.get_replication_number")
+def test_verify_reboot(
+        mock_get_replication_number,
+        mock_check_cluster_health,
+        mock_get_ceph_mon_app_map,
+        model
+):
     """Test reboot verification on CephOsd."""
-    mock_get_ceph_mon_units.return_value = {"ceph-osd": model.units["ceph-mon/0"]}
+    mock_get_replication_number.return_value = 1
+    mock_get_ceph_mon_app_map.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()
+    assert result == Result(True, "Ceph cluster is healthy")
 
+    mock_get_replication_number.return_value = None  # empty list of pools
+    result = CephOsd([model.units["ceph-osd/0"]]).verify_reboot()
     assert result == Result(True, "Ceph cluster is healthy")
 
 
[e-mail]("juju_verify.verifiers.ceph.CephOsd.get_ceph_mon_units")
[e-mail]("juju_verify.verifiers.ceph.CephOsd._get_ceph_mon_app_map")
 @mock.patch("juju_verify.verifiers.ceph.CephCommon.check_cluster_health")
-def test_verify_shutdown(mock_check_cluster_health, mock_get_ceph_mon_units, model):
[e-mail]("juju_verify.verifiers.ceph.CephCommon.get_replication_number")
+def test_verify_shutdown(
+        mock_get_replication_number,
+        mock_check_cluster_health,
+        mock_get_ceph_mon_app_map,
+        model
+):
     """Test shutdown verification on CephOsd."""
-    mock_get_ceph_mon_units.return_value = {"ceph-osd": model.units["ceph-mon/0"]}
+    mock_get_replication_number.return_value = 1
+    mock_get_ceph_mon_app_map.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()
+    assert result == Result(True, "Ceph cluster is healthy")
 
+    mock_get_replication_number.return_value = None  # empty list of pools
+    result = CephOsd([model.units["ceph-osd/0"]]).verify_shutdown()
     assert result == Result(True, "Ceph cluster is healthy")