mgr/cephadm: make NodeAssignment return a simple host list

We shouldn't modify the PlacementSpec in place (or at all).  Instead,
simply return a list of the hosts where we want to locate daemons.

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 1093677b513412ce7940926838102e784af25346..38239fc226f13e86f5701997883ccbb27960dfa6 100644 (file)
--- a/src/pybind/mgr/cephadm/module.py
+++ b/src/pybind/mgr/cephadm/module.py
@@ -2104,27 +2104,28 @@ class CephadmOrchestrator(orchestrator.Orchestrator, MgrModule):
         service_name = spec.service_name()
         self.log.debug('Applying service %s spec' % service_name)
         daemons = self.cache.get_daemons_by_service(service_name)
-        spec = HostAssignment(
+        hosts = HostAssignment(
             spec=spec,
             get_hosts_func=self._get_hosts,
-            get_daemons_func=self.cache.get_daemons_by_service).load()
-        if len(daemons) > spec.placement.count:
+            get_daemons_func=self.cache.get_daemons_by_service).place()
+        count = len(hosts)
+        if len(daemons) > count:
             # remove some
-            to_remove = len(daemons) - spec.placement.count
+            to_remove = len(daemons) - count
             args = []
             for d in daemons[0:to_remove]:
                 args.append(
                     ('%s.%s' % (d.daemon_type, d.daemon_id), d.hostname)
                 )
             return self._remove_daemon(args)
-        elif len(daemons) < spec.placement.count:
+        elif len(daemons) < count:
             # add some
-            spec.placement.count -= len(daemons)
+            count -= len(daemons)
             hosts_with_daemons = {d.hostname for d in daemons}
             hosts_without_daemons = {p for p in spec.placement.hosts
                                      if p.hostname not in hosts_with_daemons}
-            spec.placement.hosts = hosts_without_daemons
             return self._create_daemons(daemon_type, spec, daemons,
+                                        hosts_without_daemons, count,
                                         create_func, config_func)
         return trivial_result([])
 
@@ -2147,23 +2148,24 @@ class CephadmOrchestrator(orchestrator.Orchestrator, MgrModule):
         self.log.debug('_add_daemon %s spec %s' % (daemon_type, spec.placement))
         if not spec.placement.hosts:
             raise OrchestratorError('must specify host(s) to deploy on')
-        if not spec.placement.count:
-            spec.placement.count = len(spec.placement.hosts)
+        count = spec.placement.count or len(spec.placement.hosts)
         daemons = self.cache.get_daemons_by_service(spec.service_name())
         return self._create_daemons(daemon_type, spec, daemons,
+                                    spec.placement.hosts, count,
                                     create_func, config_func)
 
     def _create_daemons(self, daemon_type, spec, daemons,
+                        hosts, count,
                         create_func, config_func=None):
-        if spec.placement.count > len(spec.placement.hosts):
+        if count > len(hosts):
             raise OrchestratorError('too few hosts: want %d, have %s' % (
-                spec.placement.count, spec.placement.hosts))
+                count, hosts))
 
         if config_func:
             config_func(spec)
 
         args = [] # type: ignore
-        for host, network, name in spec.placement.hosts:
+        for host, network, name in hosts:
             daemon_id = self.get_unique_name(daemon_type, host, daemons,
                                              spec.service_id, name)
             self.log.debug('Placing %s.%s on host %s' % (
@@ -2895,17 +2897,15 @@ class HostAssignment(object):
         self.get_daemons_func = get_daemons_func
         self.service_name = spec.service_name()
 
-    def load(self):
-        # type: () -> orchestrator.ServiceSpec
+    def place(self):
+        # type: () -> List[orchestrator.HostPlacementSpec]
         """
         Load hosts into the spec.placement.hosts container.
         """
         # respect any explicit host list
         if self.spec.placement.hosts and not self.spec.placement.count:
             logger.info('Provided hosts: %s' % self.spec.placement.hosts)
-            if not self.spec.placement.count:
-                self.spec.placement.count = len(self.spec.placement.hosts)
-            return self.spec
+            return self.spec.placement.hosts
 
         # respect all_hosts=true
         if self.spec.placement.all_hosts:
@@ -2914,9 +2914,7 @@ class HostAssignment(object):
                 for x in self.get_hosts_func(None)
             ]
             logger.info('All hosts: {}'.format(candidates))
-            self.spec.placement.set_hosts(candidates)
-            self.spec.placement.count = len(self.spec.placement.hosts)
-            return self.spec
+            return candidates
 
         if self.spec.placement.hosts and \
            self.spec.placement.count and \
@@ -2929,10 +2927,7 @@ class HostAssignment(object):
 
         if not self.spec.placement.count:
             logger.info('Labeled hosts: {}'.format(hosts))
-            self.spec.placement.set_hosts(hosts)
-            if not self.spec.placement.count:
-                self.spec.placement.count = len(self.spec.placement.hosts)
-            return self.spec
+            return hosts
 
         # we need to select a subset of the candidates
 
@@ -2952,21 +2947,15 @@ class HostAssignment(object):
                 existing,
                 self.spec.placement.count - len(chosen))
             logger.info('Hosts with existing daemons: {}'.format(chosen))
-            self.spec.placement.set_hosts(chosen)
-            if not self.spec.placement.count:
-                self.spec.placement.count = len(self.spec.placement.hosts)
-            return self.spec
+            return chosen
 
         need = self.spec.placement.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)
-        self.spec.placement.set_hosts(existing + chosen)
         logger.info('Combine hosts with existing daemons %s + new hosts %s' % (
             existing, chosen))
-        if not self.spec.placement.count:
-            self.spec.placement.count = len(self.spec.placement.hosts)
-        return self.spec
+        return existing + chosen
 
     def pick_candidates(self):
         # type: () -> List[orchestrator.HostPlacementSpec]

diff --git a/src/pybind/mgr/cephadm/tests/test_scheduling.py b/src/pybind/mgr/cephadm/tests/test_scheduling.py
index be150792a2fa57b579a94b5ce2d5b4b17b4d0ef5..9873233a6ed5206e1c553b1d41ca4fca54f479b3 100644 (file)
--- a/src/pybind/mgr/cephadm/tests/test_scheduling.py
+++ b/src/pybind/mgr/cephadm/tests/test_scheduling.py
@@ -64,10 +64,11 @@ class NodeAssignmentTest(NamedTuple):
         ),
     ])
 def test_node_assignment(service_type, placement, hosts, daemons, expected):
-    s = HostAssignment(spec=ServiceSpec(service_type, placement=placement),
-                       get_hosts_func=lambda _: hosts,
-                       get_daemons_func=lambda _: daemons).load()
-    assert sorted([h.hostname for h in s.placement.hosts]) == sorted(expected)
+    hosts = HostAssignment(
+        spec=ServiceSpec(service_type, placement=placement),
+        get_hosts_func=lambda _: hosts,
+        get_daemons_func=lambda _: daemons).place()
+    assert sorted([h.hostname for h in hosts]) == sorted(expected)
 
 class NodeAssignmentTest2(NamedTuple):
     service_type: str
@@ -140,11 +141,12 @@ class NodeAssignmentTest2(NamedTuple):
     ])
 def test_node_assignment2(service_type, placement, hosts,
                           daemons, expected_len, in_set):
-    s = HostAssignment(spec=ServiceSpec(service_type, placement=placement),
-                       get_hosts_func=lambda _: hosts,
-                       get_daemons_func=lambda _: daemons).load()
-    assert len(s.placement.hosts) == expected_len
-    for h in [h.hostname for h in s.placement.hosts]:
+    hosts = HostAssignment(
+        spec=ServiceSpec(service_type, placement=placement),
+        get_hosts_func=lambda _: hosts,
+        get_daemons_func=lambda _: daemons).place()
+    assert len(hosts) == expected_len
+    for h in [h.hostname for h in hosts]:
         assert h in in_set
 
 @pytest.mark.parametrize("service_type,placement,hosts,daemons,expected_len,must_have",
@@ -170,12 +172,13 @@ def test_node_assignment2(service_type, placement, hosts,
     ])
 def test_node_assignment3(service_type, placement, hosts,
                           daemons, expected_len, must_have):
-    s = HostAssignment(spec=ServiceSpec(service_type, placement=placement),
-                       get_hosts_func=lambda _: hosts,
-                       get_daemons_func=lambda _: daemons).load()
-    assert len(s.placement.hosts) == expected_len
+    hosts = HostAssignment(
+        spec=ServiceSpec(service_type, placement=placement),
+        get_hosts_func=lambda _: hosts,
+        get_daemons_func=lambda _: daemons).place()
+    assert len(hosts) == expected_len
     for h in must_have:
-        assert h in [h.hostname for h in s.placement.hosts]
+        assert h in [h.hostname for h in hosts]
 
 
 @pytest.mark.parametrize("placement",