From b60b51d582a97c436cb0142dd1c5b9e86b686a91 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Mon, 2 Mar 2020 10:43:42 -0600 Subject: [PATCH] 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 --- src/pybind/mgr/cephadm/module.py | 53 ++++++++----------- .../mgr/cephadm/tests/test_scheduling.py | 31 ++++++----- 2 files changed, 38 insertions(+), 46 deletions(-) diff --git a/src/pybind/mgr/cephadm/module.py b/src/pybind/mgr/cephadm/module.py index 1093677b51341..38239fc226f13 100644 --- 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 be150792a2fa5..9873233a6ed52 100644 --- 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", -- 2.39.5