mgr/cephadm: remove magic labels

Remove the magic label behavior.  It makes the code confusing, it
makes the overall behavior hard to explain, and it makes the PlacementSpec
meaning different than what Rook is doing.

Instead, if you want mons on hosts with label 'mon', then say 'label:mon'.

Signed-off-by: Sage Weil <sage@redhat.com>

diff --git a/src/pybind/mgr/cephadm/module.py b/src/pybind/mgr/cephadm/module.py
index 3ccf0caec833f6839eb70ad42e97bc775dc80c0f..dab91bed559f1ff98056c0e0206e36ff7e99f29b 100644 (file)
--- a/src/pybind/mgr/cephadm/module.py
+++ b/src/pybind/mgr/cephadm/module.py
@@ -2994,23 +2994,44 @@ class HostAssignment(object):
             logger.debug('All hosts: {}'.format(candidates))
             return candidates
 
+        count = 0
         if self.spec.placement.hosts and \
            self.spec.placement.count and \
            len(self.spec.placement.hosts) >= self.spec.placement.count:
-            # use the provided hosts and our candidates
             hosts = self.spec.placement.hosts
+            logger.debug('place %d over provided host list: %s' % (
+                count, hosts))
+            count = self.spec.placement.count
+        elif self.spec.placement.label:
+            hosts = [
+                HostPlacementSpec(x, '', '')
+                for x in self.get_hosts_func(self.spec.placement.label)
+            ]
+            if not self.spec.placement.count:
+                logger.debug('Labeled hosts: {}'.format(hosts))
+                return hosts
+            count = self.spec.placement.count
+            logger.debug('place %d over label %s: %s' % (
+                count, self.spec.placement.label, hosts))
         else:
-            # select candidate hosts based on labels, types, etc.
-            hosts = self.pick_candidates()
-
-        if not self.spec.placement.count:
-            logger.debug('Labeled hosts: {}'.format(hosts))
-            return hosts
+            hosts = [
+                HostPlacementSpec(x, '', '')
+                for x in self.get_hosts_func(None)
+            ]
+            if self.spec.placement.count:
+                count = self.spec.placement.count
+            else:
+                # this should be a totally empty spec given all of the
+                # alternative paths above.
+                raise OrchestratorValidationError(
+                    "Cannot place service %s without any placement information" % (
+                        self.service_name))
+            logger.debug('place %d over all hosts: %s' % (count, hosts))
 
         # we need to select a subset of the candidates
 
         # if a partial host list is provided, always start with that
-        if len(self.spec.placement.hosts) < self.spec.placement.count:
+        if len(self.spec.placement.hosts) < count:
             chosen = self.spec.placement.hosts
         else:
             chosen = []
@@ -3023,14 +3044,14 @@ class HostAssignment(object):
         existing = [hs for hs in hosts
                     if hs.hostname in hosts_with_daemons and \
                     hs.hostname not in chosen_hosts]
-        if len(chosen + existing) >= self.spec.placement.count:
+        if len(chosen + existing) >= count:
             chosen = chosen + self.scheduler.place(
                 existing,
-                self.spec.placement.count - len(chosen))
+                count - len(chosen))
             logger.debug('Hosts with existing daemons: {}'.format(chosen))
             return chosen
 
-        need = self.spec.placement.count - len(existing + chosen)
+        need = count - len(existing + chosen)
         others = [hs for hs in hosts
                   if hs.hostname not in hosts_with_daemons]
         chosen = chosen + self.scheduler.place(others, need)
@@ -3038,59 +3059,3 @@ class HostAssignment(object):
             existing, chosen))
         return existing + chosen
 
-    def pick_candidates(self):
-        # type: () -> List[orchestrator.HostPlacementSpec]
-        """
-        Use the assigned scheduler to load hosts into the spec.placement.hosts
-        container
-        """
-        # is there an explicit label?
-        if self.spec.placement.label:
-            candidates = [
-                HostPlacementSpec(x, '', '')
-                for x in self.get_hosts_func(self.spec.placement.label)
-            ]
-            logger.debug('Candidate hosts with label %s: %s' % (
-                self.spec.placement.label, candidates))
-            return candidates
-
-        # otherwise, start with hosts labeled for this service or type
-        candidates = [
-            HostPlacementSpec(x, '', '')
-            for x in self.get_hosts_func(self.service_name)
-        ]
-        if candidates:
-            logger.debug('Candidate hosts with service label %s: %s' % (
-                self.service_name, candidates))
-            return candidates
-
-        # type label (e.g., mds)
-        candidates = [
-            HostPlacementSpec(x, '', '')
-            for x in self.get_hosts_func(self.spec.service_type)
-        ]
-        if candidates:
-            if self.spec.placement.count and \
-               len(candidates) < self.spec.placement.count:
-                # NOTE: Hmm, is this really the behavior we want?
-                logger.warning("Did not find enough labeled hosts to "
-                               "scale to <{}> services. Falling back to "
-                               "unlabeled hosts.".format(
-                                   self.spec.placement.count))
-            else:
-                logger.debug('Candidate hosts with service type label: %s' % (
-                    candidates))
-                return candidates
-
-        # fall back to *all* hosts?
-        if not self.spec.placement.count:
-            raise OrchestratorValidationError(
-                "Cannot place service %s without any placement information" % (
-                    self.service_name))
-
-        candidates = [
-            HostPlacementSpec(x, '', '')
-            for x in self.get_hosts_func(None)
-        ]
-        logger.debug('Candidate hosts (all): {}'.format(candidates))
-        return candidates

diff --git a/src/pybind/mgr/cephadm/tests/test_scheduling.py b/src/pybind/mgr/cephadm/tests/test_scheduling.py
index 940bd43edd191807cb19abce384fed75814c13c6..7c6132c098d3335ccae1a2bc70881f091cb2cc7d 100644 (file)
--- a/src/pybind/mgr/cephadm/tests/test_scheduling.py
+++ b/src/pybind/mgr/cephadm/tests/test_scheduling.py
@@ -100,6 +100,16 @@ class NodeAssignmentTest2(NamedTuple):
 
 @pytest.mark.parametrize("service_type,placement,hosts,daemons,expected_len,in_set",
     [
+        # empty
+        NodeAssignmentTest2(
+            'mon',
+            PlacementSpec(),
+            'host1 host2 host3'.split(),
+            [],
+            1,
+            ['host1', 'host2', 'host3'],
+        ),
+
         # just count
         NodeAssignmentTest2(
             'mon',