Merge ~xavpaice/juju-verify:ceph-mon into ~canonical-solutions-engineering/juju-verify:master

[Xav Paice]

Add verifier for ceph-mon

This change adds a CephMon verifier for the ceph-mon charm. It uses
existing actions to ensure the cluster has HEALTH_OK status.

The change relies on ceph-mon get-quorum-status action, introduced in
https://review.opendev.org/c/openstack/charm-ceph-mon/+/778837.

Also uses Juju changes in 2.8.10, and introduces a dependency on that
version.

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

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

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

Unable to determine commit message from repository - please click "Set commit message" and enter the commit message manually.

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

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

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

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

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

review: Approve (continuous-integration)

[Martin Kalcok (martin-kalcok)]

Code-wise this change is LGTM, thanks. I just have one in-line suggestion that I think is worth considering.

General question: I looked through the documentation and I see that odd number of monitors is not necessary for quorum, but it's strongly recommended [1]. Do you think it would be worth it to add warning output (via logger) that would warn user that the action would result in sub-optimal configuration of ceph-mon? (i.e. having 2 mons is not much better than 1, as it provides no fault tolerance).

[1] https://docs.ceph.com/en/latest/rados/operations/add-or-rm-mons/

f12707b... by Xav Paice on 2021-03-25

Update ceph action output field names

https://review.opendev.org/c/openstack/charm-ceph-mon/+/778837 changed
the field names and format during the review process, this reflects that
change.

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

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

review: Approve (continuous-integration)

[Xav Paice (xavpaice)]

This will want a rebase and functest update once the functest change is finalized.

[Robert Gildein (rgildein)]

Overall, it looks good, the only problem is using "\n".

[Robert Gildein (rgildein)]

review: Needs Fixing

Unmerged commits

f12707b... by Xav Paice on 2021-03-25

Update ceph action output field names

https://review.opendev.org/c/openstack/charm-ceph-mon/+/778837 changed
the field names and format during the review process, this reflects that
change.

afd975b... by Xav Paice on 2021-03-04

Add verifier for ceph-mon

This change adds a CephMon verifier for the ceph-mon charm. It uses
existing actions to ensure the cluster has HEALTH_OK status.

The change relies on ceph-mon get-quorum-status action, introduced in
https://review.opendev.org/c/openstack/charm-ceph-mon/+/778837.

Also uses Juju changes in 2.8.10, and introduces a dependency on that
version.

4def417... by Xav Paice on 2021-03-23

update unittests target to exclude functests

diff --git a/juju_verify/verifiers/__init__.py b/juju_verify/verifiers/__init__.py
index 4400e0f..83329e1 100644
--- a/juju_verify/verifiers/__init__.py
+++ b/juju_verify/verifiers/__init__.py
@@ -25,7 +25,7 @@ from juju.unit import Unit
 from juju_verify.exceptions import CharmException
 from juju_verify.utils.unit import parse_charm_name
 from juju_verify.verifiers.base import BaseVerifier
-from juju_verify.verifiers.ceph import CephOsd
+from juju_verify.verifiers.ceph import CephOsd, CephMon
 from juju_verify.verifiers.nova_compute import NovaCompute
 from juju_verify.verifiers.result import Result
 
@@ -35,6 +35,7 @@ logger = logging.getLogger(__name__)
 SUPPORTED_CHARMS = {
     'nova-compute': NovaCompute,
     'ceph-osd': CephOsd,
+    'ceph-mon': CephMon,
 }
 
 
diff --git a/juju_verify/verifiers/ceph.py b/juju_verify/verifiers/ceph.py
index 96253bd..524d31f 100644
--- a/juju_verify/verifiers/ceph.py
+++ b/juju_verify/verifiers/ceph.py
@@ -15,6 +15,10 @@
 # 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."""
+<<<<<<< juju_verify/verifiers/ceph.py
+=======
+
+>>>>>>> juju_verify/verifiers/ceph.py
 import json
 import logging
 from typing import Dict, Optional, Any, List
@@ -22,9 +26,9 @@ from typing import Dict, Optional, Any, List
 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 run_action_on_units, verify_charm_unit
 from juju_verify.verifiers.base import BaseVerifier
-from juju_verify.verifiers.result import aggregate_results, Result
+from juju_verify.verifiers.result import Result, aggregate_results
 
 logger = logging.getLogger()
 
@@ -43,6 +47,7 @@ class CephCommon(BaseVerifier):  # pylint: disable=W0223
         verify_charm_unit("ceph-mon", *units)
         result = Result(success=True)
         action_map = run_action_on_units(list(units), "get-health")
+
         for unit, action in action_map.items():
             cluster_health = data_from_action(action, "message")
             logger.debug("Unit (%s): Ceph cluster health '%s'", unit, cluster_health)
@@ -85,7 +90,7 @@ class CephCommon(BaseVerifier):  # pylint: disable=W0223
 class CephOsd(CephCommon):
     """Implementation of verification checks for the ceph-osd charm."""
 
-    NAME = 'ceph-osd'
+    NAME = "ceph-osd"
 
     def __init__(self, units: List[Unit]):
         """Ceph-osd charm verifier."""
@@ -174,3 +179,69 @@ class CephOsd(CephCommon):
     def verify_shutdown(self) -> Result:
         """Verify that it's safe to shutdown selected ceph-osd units."""
         return self.verify_reboot()
+
+
+class CephMon(CephCommon):
+    """Implementation of verification checks for the ceph-mon charm."""
+
+    NAME = "ceph-mon"
+
+    def verify_reboot(self) -> Result:
+        """Verify that it's safe to reboot selected ceph-mon units."""
+        return aggregate_results(
+            self.check_quorum(), self.check_cluster_health(*self.units)
+        )
+
+    def verify_shutdown(self) -> Result:
+        """Verify that it's safe to shutdown selected units."""
+        return self.verify_reboot()
+
+    def check_quorum(self) -> Result:
+        """Check that the shutdown does not result in <50% mons alive."""
+        result = Result(success=True)
+
+        action_name = "get-quorum-status"
+        action_results = self.run_action_on_all(action_name)
+
+        mons = {}
+        affected_hosts = set()
+
+        for unit in self.units:
+            # set of affected hostnames needs to be fully populated early
+            # from online mons
+
+            if unit.machine.hostname:
+                affected_hosts.update(
+                    [unit.machine.hostname]
+                )
+            else:
+                # we don't have a hostname for this unit
+                logger.error(
+                    "The machine for unit %s did not return a hostname", unit.entity_id
+                )
+                result.success = False
+                result.reason += (
+                    "The machine for unit {} does not have a hostname attribute, please"
+                    "ensure that Juju is 2.8.10+\n".format(unit.entity_id)
+                )
+
+        # populate a list of known mons, online mons and hostnames per unit
+        for unit_id, action in action_results.items():
+            # run this per unit because we might have multiple clusters
+            mons[unit_id] = {
+                "known": set(json.loads(data_from_action(action, "known-mons"))),
+                "online": set(json.loads(data_from_action(action, "online-mons"))),
+            }
+
+        for unit in self.units:
+            mon_count = len(mons[unit.entity_id]["known"])
+            mons_after_change = len(mons[unit.entity_id]["online"] - affected_hosts)
+
+            if mons_after_change <= mon_count / 2:
+                result.success = False
+                result.reason += (
+                    "Removing unit {} will lose Ceph mon "
+                    "quorum\n".format(unit.entity_id)
+                )
+
+        return result
diff --git a/tests/functional/requirements.txt b/tests/functional/requirements.txt
index e6b19ec..fae35b2 100644
--- a/tests/functional/requirements.txt
+++ b/tests/functional/requirements.txt
@@ -1,2 +1,3 @@
 python-openstackclient
 git+https://github.com/openstack-charmers/zaza.git#egg=zaza
+git+https://github.com/juju/python-libjuju.git#egg=juju
diff --git a/tests/functional/tests/bundles/hacluster.yaml b/tests/functional/tests/bundles/hacluster.yaml
index dd166b7..6f54d8a 100644
--- a/tests/functional/tests/bundles/hacluster.yaml
+++ b/tests/functional/tests/bundles/hacluster.yaml
@@ -1,7 +1,7 @@
 series: xenial
 applications:
   dashboard-hacluster:
-    charm: cs:~xavpaice/hacluster-0
+    charm: cs:~xavpaice/hacluster
   keystone:
     charm: cs:keystone
     num_units: 1
diff --git a/tests/functional/tests/test_ceph.py b/tests/functional/tests/test_ceph.py
index 12f4ca4..3a116c8 100644
--- a/tests/functional/tests/test_ceph.py
+++ b/tests/functional/tests/test_ceph.py
@@ -1,6 +1,7 @@
 """Test deployment and functionality of juju-verify."""
 
 from juju import loop
+import zaza.model
 from tests.base import BaseTestCase
 
 from juju_verify import juju_verify
@@ -23,3 +24,31 @@ class CephTests(BaseTestCase):
         verifier = get_verifier(unit_objects)
         result = verifier.verify(check)
         self.assertTrue(result.success is True)
+
+    def test_single_mon_unit(self):
+        """Test that shutdown of a single mon unit returns OK."""
+        # juju-verify shutdown --units ceph-mon/0
+
+        units = ['ceph-mon/0']
+        log_level = 'info'
+        check = 'shutdown'
+        juju_verify.config_logger(log_level)
+        model = loop.run(juju_verify.connect_model(zaza.model.CURRENT_MODEL))
+        unit_objects = loop.run(juju_verify.find_units(model, units))
+        verifier = get_verifier(unit_objects)
+        result = verifier.verify(check)
+        self.assertTrue(result.success is True)
+
+    def test_two_mon_unit(self):
+        """Test that shutdown of multiple mon units fails."""
+        # juju-verify shutdown --units ceph-mon/0 ceph-mon/1
+
+        units = ['ceph-mon/0', 'ceph-mon/1']
+        log_level = 'info'
+        check = 'shutdown'
+        juju_verify.config_logger(log_level)
+        model = loop.run(juju_verify.connect_model(zaza.model.CURRENT_MODEL))
+        unit_objects = loop.run(juju_verify.find_units(model, units))
+        verifier = get_verifier(unit_objects)
+        result = verifier.verify(check)
+        self.assertTrue(result.success is False)
diff --git a/tests/unit/verifiers/test_ceph.py b/tests/unit/verifiers/test_ceph.py
index b9fb4b5..e443a02 100644
--- a/tests/unit/verifiers/test_ceph.py
+++ b/tests/unit/verifiers/test_ceph.py
@@ -21,20 +21,31 @@ from unittest import mock
 from unittest.mock import MagicMock
 
 import pytest
+from juju.model import Model
+from juju.unit import Unit
 
+<<<<<<< tests/unit/verifiers/test_ceph.py
 from juju_verify.exceptions import CharmException
 from juju_verify.verifiers import CephOsd, Result
+=======
+from juju_verify.verifiers import CephMon, CephOsd, Result
+>>>>>>> tests/unit/verifiers/test_ceph.py
 from juju_verify.verifiers.ceph import CephCommon
 
 
 @mock.patch("juju_verify.verifiers.ceph.run_action_on_units")
[e-mail]("message, exp_result", [
-    ("HEALTH_OK ...", Result(True, "ceph-mon/0: Ceph cluster is healthy")),
-    ("HEALTH_WARN ...", Result(False, "ceph-mon/0: Ceph cluster is unhealthy")),
-    ("HEALTH_ERR ...", Result(False, "ceph-mon/0: Ceph cluster is unhealthy")),
-    ("not valid message",
-     Result(False, "ceph-mon/0: Ceph cluster is in an unknown state")),
-])
[e-mail](
+    "message, exp_result",
+    [
+        ("HEALTH_OK ...", Result(True, "ceph-mon/0: Ceph cluster is healthy")),
+        ("HEALTH_WARN ...", Result(False, "ceph-mon/0: Ceph cluster is unhealthy")),
+        ("HEALTH_ERR ...", Result(False, "ceph-mon/0: Ceph cluster is unhealthy")),
+        (
+            "not valid message",
+            Result(False, "ceph-mon/0: Ceph cluster is in an unknown state"),
+        ),
+    ],
+)
 def test_check_cluster_health(mock_run_action_on_units, message, exp_result, model):
     """Test check Ceph cluster health."""
     action = MagicMock()
@@ -49,18 +60,27 @@ def test_check_cluster_health(mock_run_action_on_units, message, exp_result, mod
 @mock.patch("juju_verify.verifiers.ceph.run_action_on_units")
 def test_check_cluster_health_combination(mock_run_action_on_units, model):
     """Test check Ceph cluster health combination of two diff state."""
-    exp_result = Result(False,
-                        os.linesep.join(["ceph-mon/0: Ceph cluster is healthy",
-                                         "ceph-mon/1: Ceph cluster is unhealthy"]))
+    exp_result = Result(
+        False,
+        os.linesep.join(
+            [
+                "ceph-mon/0: Ceph cluster is healthy",
+                "ceph-mon/1: Ceph cluster is unhealthy",
+            ]
+        ),
+    )
     action_healthy = MagicMock()
     action_healthy.data.get.side_effect = {"results": {"message": "HEALTH_OK"}}.get
     action_unhealthy = MagicMock()
     action_unhealthy.data.get.side_effect = {"results": {"message": "HEALTH_ERR"}}.get
-    mock_run_action_on_units.return_value = {"ceph-mon/0": action_healthy,
-                                             "ceph-mon/1": action_unhealthy}
+    mock_run_action_on_units.return_value = {
+        "ceph-mon/0": action_healthy,
+        "ceph-mon/1": action_unhealthy,
+    }
 
-    result = CephCommon.check_cluster_health(model.units["ceph-mon/0"],
-                                             model.units["ceph-mon/1"])
+    result = CephCommon.check_cluster_health(
+        model.units["ceph-mon/0"], model.units["ceph-mon/1"]
+    )
 
     assert result == exp_result
 
@@ -70,8 +90,9 @@ def test_check_cluster_health_unknown_state(mock_run_action_on_units, model):
     """Test check Ceph cluster health in unknown state."""
     mock_run_action_on_units.return_value = {}
 
-    result = CephCommon.check_cluster_health(model.units["ceph-mon/0"],
-                                             model.units["ceph-mon/1"])
+    result = CephCommon.check_cluster_health(
+        model.units["ceph-mon/0"], model.units["ceph-mon/1"]
+    )
 
     assert result == Result(False, "Ceph cluster is in an unknown state")
 
@@ -207,12 +228,17 @@ def test_verify_reboot(
 
 @mock.patch("juju_verify.verifiers.ceph.CephOsd._get_ceph_mon_app_map")
 @mock.patch("juju_verify.verifiers.ceph.CephCommon.check_cluster_health")
+<<<<<<< tests/unit/verifiers/test_ceph.py
 @mock.patch("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
+=======
+def test_verify_ceph_osd_shutdown(
+    mock_check_cluster_health, mock_get_ceph_mon_units, model
+>>>>>>> tests/unit/verifiers/test_ceph.py
 ):
     """Test shutdown verification on CephOsd."""
     mock_get_replication_number.return_value = 1
@@ -225,3 +251,48 @@ def test_verify_shutdown(
     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")
+
+
[e-mail](
+    "action_return_value, expected_result, hostname",
+    [('["host0", "host1", "host2"]', Result(True), "host1"),
+     ('["host0", "host1"]', Result(False), "host1"),
+     ('["host0", "host1", "host2"]', Result(False), None),
+     ('["host0", "host1", "host2"]', Result(True), "host5"),
+     ],
+)
+def test_check_ceph_mon_quorum(mocker, action_return_value, expected_result, hostname):
+    """Test Ceph quorum verification on CephMon."""
+    unit = Unit("ceph-mon/0", Model())
+    verifier = CephMon([unit])
+    mocker.patch.object(verifier, "run_action_on_all").return_value = {
+        "ceph-mon/0": action_return_value,
+    }
+    unit.machine = mock.Mock(hostname=hostname)
+    mocker.patch(
+        "juju_verify.verifiers.ceph.data_from_action"
+    ).return_value = action_return_value
+    result = verifier.check_quorum()
+    assert result.success == expected_result.success
+
+
+def test_verify_ceph_mon_shutdown(mocker):
+    """Test that verify_shutdown links to verify_reboot."""
+    mocker.patch.object(CephMon, "verify_reboot")
+    unit = Unit("ceph-mon/0", Model())
+    verifier = CephMon([unit])
+    verifier.verify_shutdown()
+    verifier.verify_reboot.assert_called_once()
+
+
[e-mail]("juju_verify.verifiers.ceph.CephCommon.check_cluster_health")
[e-mail]("juju_verify.verifiers.ceph.CephMon.check_quorum")
+def test_verify_ceph_mon_reboot(mock_quorum, mock_health):
+    """Test reboot verification on CephMon."""
+    unit = Unit("ceph-mon/0", Model())
+    mock_health.return_value = Result(True, "Ceph cluster is healthy")
+    mock_quorum.return_value = Result(True)
+    verifier = CephMon([unit])
+    result = verifier.verify_reboot()
+
+    assert result.success
diff --git a/tox.ini b/tox.ini
index 0cce781..321a23d 100644
--- a/tox.ini
+++ b/tox.ini
@@ -78,8 +78,8 @@ commands =
     pip install --editable {toxinidir}/
     functest-run-suite {posargs}
 deps =
-  .[dev]
   -r{toxinidir}/tests/functional/requirements.txt
+  .[dev]
 
 
 [testenv:func-debug]
@@ -88,5 +88,5 @@ commands =
     pip install --editable {toxinidir}/
     functest-run-suite {posargs} --keep-model --log DEBUG
 deps =
-  .[dev]
   -r{toxinidir}/tests/functional/requirements.txt
+  .[dev]