From 82c12b21c14699526387e680b2c0eae234ca50b5 Mon Sep 17 00:00:00 2001
From: Sebastian Wagner <sebastian.wagner@suse.com>
Date: Tue, 17 Mar 2020 15:00:12 +0100
Subject: [PATCH] mgr/cephadm: add HostAssignment.validate()

Fixes: https://tracker.ceph.com/issues/44607
Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
---
 src/pybind/mgr/cephadm/module.py              | 34 +++++++++++++++
 .../mgr/cephadm/tests/test_scheduling.py      | 42 +++++++++++++++++++
 .../ceph/deployment/service_spec.py           |  3 ++
 3 files changed, 79 insertions(+)

diff --git a/src/pybind/mgr/cephadm/module.py b/src/pybind/mgr/cephadm/module.py
index 887543d105e..b6ac1ca4561 100644
--- a/src/pybind/mgr/cephadm/module.py
+++ b/src/pybind/mgr/cephadm/module.py
@@ -2555,6 +2555,13 @@ class CephadmOrchestrator(orchestrator.Orchestrator, MgrModule):
              spec.placement.count < 1:
             raise OrchestratorError('cannot scale %s service below 1' % (
                 spec.service_type))
+
+        HostAssignment(
+            spec=spec,
+            get_hosts_func=self._get_hosts,
+            get_daemons_func=self.cache.get_daemons_by_service,
+        ).validate()
+
         self.log.info('Saving service %s spec with placement %s' % (
             spec.service_name(), spec.placement.pretty_str()))
         self.spec_store.save(spec)
@@ -3181,11 +3188,38 @@ class HostAssignment(object):
         self.filter_new_host = filter_new_host
         self.service_name = spec.service_name()
 
+
+    def validate(self):
+        self.spec.validate()
+
+        if self.spec.placement.hosts:
+            explicit_hostnames = {h.hostname for h in self.spec.placement.hosts}
+            unknown_hosts = explicit_hostnames.difference(set(self.get_hosts_func(None)))
+            if unknown_hosts:
+                raise OrchestratorValidationError(
+                    f'Cannot place {self.spec.one_line_str()} on {unknown_hosts}: Unknown hosts')
+
+        if self.spec.placement.host_pattern:
+            pattern_hostnames = self.spec.placement.pattern_matches_hosts(self.get_hosts_func(None))
+            if not pattern_hostnames:
+                raise OrchestratorValidationError(
+                    f'Cannot place {self.spec.one_line_str()}: No matching hosts')
+
+        if self.spec.placement.label:
+            label_hostnames = self.get_hosts_func(self.spec.placement.label)
+            if not label_hostnames:
+                raise OrchestratorValidationError(
+                    f'Cannot place {self.spec.one_line_str()}: No matching '
+                    f'hosts for label {self.spec.placement.label}')
+
     def place(self):
         # type: () -> List[HostPlacementSpec]
         """
         Load hosts into the spec.placement.hosts container.
         """
+
+        self.validate()
+
         # count == 0
         if self.spec.placement.count == 0:
             return []
diff --git a/src/pybind/mgr/cephadm/tests/test_scheduling.py b/src/pybind/mgr/cephadm/tests/test_scheduling.py
index f7f7f7e9eda..5c01d6d75ae 100644
--- a/src/pybind/mgr/cephadm/tests/test_scheduling.py
+++ b/src/pybind/mgr/cephadm/tests/test_scheduling.py
@@ -253,3 +253,45 @@ def test_bad_placements(placement):
         assert False
     except ServiceSpecValidationError as e:
         pass
+
+
+class NodeAssignmentTestBadSpec(NamedTuple):
+    service_type: str
+    placement: PlacementSpec
+    hosts: List[str]
+    daemons: List[DaemonDescription]
+    expected: str
+@pytest.mark.parametrize("service_type,placement,hosts,daemons,expected",
+    [
+        # unknown host
+        NodeAssignmentTestBadSpec(
+            'mon',
+            PlacementSpec(hosts=['unknownhost']),
+            ['knownhost'],
+            [],
+            "Cannot place <ServiceSpec for service_name=mon> on {'unknownhost'}: Unknown hosts"
+        ),
+        # unknown host pattern
+        NodeAssignmentTestBadSpec(
+            'mon',
+            PlacementSpec(host_pattern='unknownhost'),
+            ['knownhost'],
+            [],
+            "Cannot place <ServiceSpec for service_name=mon>: No matching hosts"
+        ),
+        # unknown label
+        NodeAssignmentTestBadSpec(
+            'mon',
+            PlacementSpec(label='unknownlabel'),
+            [],
+            [],
+            "Cannot place <ServiceSpec for service_name=mon>: No matching hosts for label unknownlabel"
+        ),
+    ])
+def test_bad_specs(service_type, placement, hosts, daemons, expected):
+    with pytest.raises(OrchestratorValidationError) as e:
+        hosts = HostAssignment(
+            spec=ServiceSpec(service_type, placement=placement),
+            get_hosts_func=lambda _: hosts,
+            get_daemons_func=lambda _: daemons).place()
+    assert str(e.value) == expected
diff --git a/src/python-common/ceph/deployment/service_spec.py b/src/python-common/ceph/deployment/service_spec.py
index ae19d739fd2..07dd2a14f8f 100644
--- a/src/python-common/ceph/deployment/service_spec.py
+++ b/src/python-common/ceph/deployment/service_spec.py
@@ -376,6 +376,9 @@ class ServiceSpec(object):
     def __repr__(self):
         return "{}({!r})".format(self.__class__.__name__, self.__dict__)
 
+    def one_line_str(self):
+        return '<{} for service_name={}>'.format(self.__class__.__name__, self.service_name())
+
 
 def servicespec_validate_add(self: ServiceSpec):
     # This must not be a method of ServiceSpec, otherwise you'll hunt
-- 
2.39.5