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

[Robert Gildein]

This MP add check of availability zones resources and it's used by CephOsd.verify_reboot and CephOsd.verify_shutdown.

Closses-bug: LP#1817600

[🤖 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)]

FAILED: Continuous integration, rev:9fbf8d7a8273fd0c5da26cbdd4ee472bc109e1fd
https://jenkins.canonical.com/bootstack/job/juju-verify-test/44/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/generic-make-test/65/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/44/

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

review: Needs Fixing (continuous-integration)

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

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

3374f1d... by Robert Gildein on 2021-03-15

fix after rebase

091b6ed... by Robert Gildein on 2021-03-15

use new action "get-availability-zone" output

Unmerged commits

091b6ed... by Robert Gildein on 2021-03-15

use new action "get-availability-zone" output

3374f1d... by Robert Gildein on 2021-03-15

fix after rebase

58e9c34... by Robert Gildein on 2021-03-10

add unit test

3629ced... by Robert Gildein on 2021-03-10

finalized check_availability_zones_resources

ba99202... by Robert Gildein on 2021-03-10

add check_az_availability

ddadaeb... by Robert Gildein on 2021-03-08

add helper functions

diff --git a/juju_verify/utils/action.py b/juju_verify/utils/action.py
index bc31512..99c5f68 100644
--- a/juju_verify/utils/action.py
+++ b/juju_verify/utils/action.py
@@ -15,15 +15,22 @@
 # You should have received a copy of the GNU General Public License along with
 # this program. If not, see https://www.gnu.org/licenses/.
 """Helper function to manage Juju action."""
+from typing import Dict
+
 from juju.action import Action
 
 
+def results_from_action(action: Action) -> Dict:
+    """Extract action's results."""
+    return action.data.get('results', {})
+
+
 def data_from_action(action: Action, key: str, default: str = '') -> str:
     """Extract value from Action.data['results'] dictionary.
 
     :param action: juju.Action instance
     :param key: key to search for in action's results
     :param default: default value to return if the 'key' is not found
-    :return: value from the action's result identified by 'key' or default
+    :return: value from the action's results identified by 'key' or default
     """
-    return action.data.get('results', {}).get(key, default)
+    return results_from_action(action).get(key, default)
diff --git a/juju_verify/utils/unit.py b/juju_verify/utils/unit.py
index 0318cdb..1d4a4c2 100644
--- a/juju_verify/utils/unit.py
+++ b/juju_verify/utils/unit.py
@@ -16,6 +16,7 @@
 # this program. If not, see https://www.gnu.org/licenses/.
 """Helper function to manage Juju unit."""
 import asyncio
+import os
 import re
 from typing import List, Dict
 
@@ -59,7 +60,7 @@ def run_action_on_units(units: List[Unit], action: str,
                                                 unit))
 
     if failed_actions_msg:
-        raise VerificationError('\n'.join(failed_actions_msg))
+        raise VerificationError(os.linesep.join(failed_actions_msg))
 
     return result_map
 
diff --git a/juju_verify/verifiers/ceph.py b/juju_verify/verifiers/ceph.py
index 28bca0a..cef7ead 100644
--- a/juju_verify/verifiers/ceph.py
+++ b/juju_verify/verifiers/ceph.py
@@ -15,19 +15,70 @@
 # 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 Set
+from dataclasses import dataclass
+from typing import Set, Dict, List, Optional
 
 from juju.unit import Unit
 
 from juju_verify.utils.action import data_from_action
-from juju_verify.utils.unit import verify_charm_unit, run_action_on_units
+from juju_verify.utils.unit import (
+    verify_charm_unit, run_action_on_units, parse_charm_name)
 from juju_verify.verifiers.base import BaseVerifier
 from juju_verify.verifiers.result import aggregate_results, Result
 
 logger = logging.getLogger()
 
 
+@dataclass
+class AZ:
+    """Availability zone."""
+
+    # pylint: disable=too-many-instance-attributes
+    root: Optional[str] = None
+    region: Optional[str] = None
+    datacenter: Optional[str] = None
+    room: Optional[str] = None
+    pod: Optional[str] = None
+    pdu: Optional[str] = None
+    row: Optional[str] = None
+    rack: Optional[str] = None
+    chassis: Optional[str] = None
+    host: Optional[str] = None
+
+    @property
+    def crush_map_hierarchy(self) -> List[str]:
+        """Get Ceph Crush Map hierarchy."""
+        return [
+            "root",  # 10
+            "region",  # 9
+            "datacenter",  # 8
+            "room",  # 7
+            "pod",  # 6
+            "pdu",  # 5
+            "row",  # 4
+            "rack",  # 3
+            "chassis",  # 2
+            "host",  # 1
+            "osd",  # 0
+        ]
+
+    def __str__(self) -> str:
+        """Return string representation of AZ objects."""
+        crush_map_types = []
+        for crush_map_type in self.crush_map_hierarchy:
+            value = getattr(self, crush_map_type, None)
+            if value:
+                crush_map_types.append(f"{crush_map_type}={value}")
+
+        return ",".join(crush_map_types)
+
+    def __hash__(self) -> int:
+        """Return hash representation of AZ objects."""
+        return hash(self.__str__())
+
+
 class CephCommon(BaseVerifier):  # pylint: disable=W0223
     """Parent class for CephMon and CephOsd verifier."""
 
@@ -61,6 +112,7 @@ class CephOsd(CephCommon):
     """Implementation of verification checks for the ceph-osd charm."""
 
     NAME = 'ceph-osd'
+    _azs = None
 
     def get_ceph_mon_units(self) -> Set[Unit]:
         """Get Ceph-mon units related to verified units.
@@ -87,9 +139,91 @@ class CephOsd(CephCommon):
 
         return ceph_mon_units
 
+    @property
+    def azs(self) -> Dict[str, AZ]:
+        """Get information about availability zone for each ceph-osd units."""
+        if self._azs is None:
+            self._azs = self.get_availability_zones()
+
+        return self._azs
+
+    def get_availability_zones(self) -> Dict[str, AZ]:
+        """Get information about availability zone for each ceph-osd units.
+
+        This function return dictionary contain unit.entity_id as key and AZ info
+        as value. e.g. {"ceph-osd/0": AZ(per_host=False, rack="nova", row=None)}
+        """
+        units = [unit for unit in self.model.units.values()
+                 if parse_charm_name(unit.charm_url) == "ceph-osd"]
+        # NOTE (rgildein): This part should be tested after the review
+        #                  https://review.opendev.org/c/openstack/charm-ceph-osd/+/778159
+        #                  is merged.
+        action_map = run_action_on_units(units, "get-availability-zone")
+        availability_zones = {}
+        for entity_id, action in action_map.items():
+            action_result = data_from_action(action, "availability-zone", "{}")
+            unit_az = json.loads(action_result).get("unit")
+            availability_zones[entity_id] = AZ(**unit_az)
+
+        return availability_zones
+
+    def group_availability_zones(self) -> Dict[AZ, Dict[str, List[Unit]]]:
+        """Group availability zones related to self.units."""
+        grouped_az = {}
+        for unit in self.units:
+            unit_az = self.azs[unit.entity_id]
+
+            if unit_az not in grouped_az:
+                grouped_az[unit_az] = {
+                    "units": [self.model.units.get(unit_id) for unit_id, az
+                              in self.azs.items() if az == unit_az],
+                    "verification_units": [unit],
+                }
+            else:
+                grouped_az[unit_az]["verification_units"].append(unit)
+
+        return grouped_az
+
+    @staticmethod
+    def get_number_of_replication_units(availability_zone: AZ) -> int:
+        """Get number of replication units for availability zone."""
+        if availability_zone.rack is None and availability_zone.row is None:
+            return 1
+
+        # NOTE (rgildein): This is part of LP#1917599 and should return real number
+        #                  of replication units in a rack or row.
+        return 1
+
+    def check_availability_zones_resources(self) -> Result:
+        """Check availability zones resources.
+
+        This function checks whether the units can be shutdown/reboot without
+        interrupting operation in the availability zone.
+        """
+        result = Result(True)
+        grouped_az = self.group_availability_zones()
+
+        for availability_zone, az_units in grouped_az.items():
+            number_verification_units = len(az_units["verification_units"])
+            number_replication_units = self.get_number_of_replication_units(
+                availability_zone)
+
+            if number_replication_units - number_verification_units < 0:
+                units = ",".join([unit.entity_id for unit
+                                  in az_units["verification_units"]])
+                result += Result(
+                    False,
+                    f"It's not safe to shutdown/reboot units {units} in the "
+                    f"availability zone {availability_zone}. [number_replication_units="
+                    f"{number_replication_units}]"
+                )
+
+        return result
+
     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()))
+        return aggregate_results(self.check_cluster_health(*self.get_ceph_mon_units()),
+                                 self.check_availability_zones_resources())
 
     def verify_shutdown(self) -> Result:
         """Verify that it's safe to shutdown selected ceph-osd units."""
diff --git a/tests/unit/utils/test_action.py b/tests/unit/utils/test_action.py
index 8ab8669..718566a 100644
--- a/tests/unit/utils/test_action.py
+++ b/tests/unit/utils/test_action.py
@@ -20,7 +20,16 @@ from unittest.mock import PropertyMock
 from juju.action import Action
 from juju.model import Model
 
-from juju_verify.utils.action import data_from_action
+from juju_verify.utils.action import data_from_action, results_from_action
+
+
+def test_results_from_action(mocker):
+    """Test helper function parse results from Action.data."""
+    data = {'results': "test_results"}
+
+    mocker.patch.object(Action, 'data', new_callable=PropertyMock(return_value=data))
+
+    assert results_from_action(Action('0', Model())) == "test_results"
 
 
 def test_data_from_action(mocker):
@@ -30,8 +39,7 @@ def test_data_from_action(mocker):
     data = {'results': {host_key: host_value}}
     default = 'default'
 
-    mocker.patch.object(Action, 'data',
-                        new_callable=PropertyMock(return_value=data))
+    mocker.patch.object(Action, 'data', new_callable=PropertyMock(return_value=data))
     action = Action('0', Model())
 
     output = data_from_action(action, host_key, default)
diff --git a/tests/unit/verifiers/test_ceph.py b/tests/unit/verifiers/test_ceph.py
index 728a4cb..2a8e23e 100644
--- a/tests/unit/verifiers/test_ceph.py
+++ b/tests/unit/verifiers/test_ceph.py
@@ -15,14 +15,16 @@
 # 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 dataclasses import asdict
 from unittest import mock
 from unittest.mock import MagicMock
 
 import pytest
 
-from juju_verify.verifiers import CephOsd, Result
-from juju_verify.verifiers.ceph import CephCommon
+from juju_verify.verifiers.ceph import AZ, CephCommon, CephOsd
+from juju_verify.verifiers.result import Result
 
 
 @mock.patch("juju_verify.verifiers.ceph.run_action_on_units")
@@ -88,25 +90,108 @@ def test_get_ceph_mon_units(model):
     assert {ceph_mon_units[0]} == units
 
 
[e-mail]("az_info, ex_az", [
+    ({"root": "default", "host": "test"}, "root=default,host=test"),
+    ({"root": "default", "rack": "nova", "row": "row1", "host": "test"},
+     "root=default,row=row1,rack=nova,host=test"),
+])
+def test_az(az_info, ex_az):
+    """Test availability zone object."""
+    availability_zone = AZ(**az_info)
+    assert str(availability_zone) == ex_az
+    assert hash(availability_zone) == hash(ex_az)
+
+
[e-mail]("juju_verify.verifiers.ceph.run_action_on_units")
+def test_get_get_availability_zones(mock_run_action_on_units, model):
+    """Test getting information about availability zones."""
+    exp_az = AZ(root="default", rack="nova", host="test")
+    mock_action = MagicMock()
+    mock_action.data.get.side_effect = {
+        "results": {"availability-zone": json.dumps({"unit": asdict(exp_az)})}}.get
+    mock_run_action_on_units.return_value = {
+        "ceph-osd/0": mock_action, "ceph-osd/1": mock_action
+    }
+
+    azs = CephOsd([model.units.get("ceph-osd/0")]).azs
+    mock_run_action_on_units.assert_called_once_with(
+        [model.units.get("ceph-osd/0"), model.units.get("ceph-osd/1")],
+        "get-availability-zone"
+    )
+    assert azs == {"ceph-osd/0": exp_az, "ceph-osd/1": exp_az}
+
+
+def test_group_availability_zones(model):
+    """Test grouping availability zones related to verifier.units."""
+    exp_az = AZ(root="default", rack="nova", host="test")
+    verifier = CephOsd([model.units.get("ceph-osd/0")])
+    verifier._azs = {"ceph-osd/0": exp_az, "ceph-osd/1": exp_az}
+
+    related_az_group = verifier.group_availability_zones()
+    assert related_az_group[exp_az]["units"] == [
+        model.units.get("ceph-osd/0"), model.units.get("ceph-osd/1")]
+    assert related_az_group[exp_az]["verification_units"] == [
+        model.units.get("ceph-osd/0")]
+
+
[e-mail]("availability_zone, exp_number", [
+    (AZ(root="default", host="test"), 1),
+    (AZ(root="default", rack="nova", host="test"), 1),
+    (AZ(root="default", rack="nova", row="row1", host="test"), 1),
+])
+def test_get_number_of_replication_units(availability_zone, exp_number, model):
+    """Test getting number of replication units for availability zone."""
+    verifier = CephOsd([model.units.get("ceph-osd/0")])
+    assert verifier.get_number_of_replication_units(availability_zone) == exp_number
+
+
[e-mail]("juju_verify.verifiers.ceph.CephOsd.get_number_of_replication_units")
+def test_check_availability_zones_resources(mock_get_number_of_replication_units, model):
+    """Test check number of available resources in availability zone."""
+    mock_get_number_of_replication_units.return_value = 1
+    exp_az = AZ(root="default", host="test")
+
+    # verify one cep-osd units
+    verifier = CephOsd([model.units.get("ceph-osd/0")])
+    verifier._azs = {"ceph-osd/0": exp_az, "ceph-osd/1": exp_az}
+
+    result = verifier.check_availability_zones_resources()
+    assert result == Result(True)
+    mock_get_number_of_replication_units.assert_called_once_with(exp_az)
+
+    # verify two cep-osd units
+    verifier = CephOsd([model.units.get("ceph-osd/0"), model.units.get("ceph-osd/1")])
+    verifier._azs = {"ceph-osd/0": exp_az, "ceph-osd/1": exp_az, "ceph-osd/2": exp_az}
+
+    result = verifier.check_availability_zones_resources()
+    assert result == Result(False, "It's not safe to shutdown/reboot units ceph-osd/0,"
+                                   "ceph-osd/1 in the availability zone root=default,"
+                                   "host=test. [number_replication_units=1]")
+
+
 @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):
[e-mail]("juju_verify.verifiers.ceph.CephOsd.check_availability_zones_resources")
+def test_verify_reboot(mock_check_availability_zones_resources,
+                       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_check_cluster_health.return_value = Result(True, "Ceph cluster is healthy")
+    mock_check_availability_zones_resources.return_value = Result(True)
 
-    result = CephOsd([model.units["ceph-osd/0"]]).verify_reboot()
-
+    result = CephOsd([model.units.get("ceph-osd/0")]).verify_reboot()
     assert result == Result(True, "Ceph cluster is healthy")
 
 
 @mock.patch("juju_verify.verifiers.ceph.CephOsd.get_ceph_mon_units")
 @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.CephOsd.check_availability_zones_resources")
+def test_verify_shutdown(mock_check_availability_zones_resources,
+                         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_check_cluster_health.return_value = Result(True, "Ceph cluster is healthy")
+    mock_check_availability_zones_resources.return_value = Result(True)
 
-    result = CephOsd([model.units["ceph-osd/0"]]).verify_shutdown()
-
+    result = CephOsd([model.units.get("ceph-osd/0")]).verify_shutdown()
     assert result == Result(True, "Ceph cluster is healthy")