From 387813ab5f3154a91e82a0fcad082932aaa2a7f8 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Sun, 8 Mar 2020 12:00:45 -0500 Subject: [PATCH] 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 --- src/pybind/mgr/cephadm/module.py | 99 ++++++------------- .../mgr/cephadm/tests/test_scheduling.py | 10 ++ 2 files changed, 42 insertions(+), 67 deletions(-) diff --git a/src/pybind/mgr/cephadm/module.py b/src/pybind/mgr/cephadm/module.py index 3ccf0caec833f..dab91bed559f1 100644 --- 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 940bd43edd191..7c6132c098d33 100644 --- 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', -- 2.39.5