From 3b841fc7fa5c13dbc212053aeb8deda012a3de0d Mon Sep 17 00:00:00 2001 From: Sebastian Wagner Date: Mon, 14 Sep 2020 11:18:38 +0200 Subject: [PATCH] mgr/cephadm: Scheduler: fetch hosts only once Otherwise we might be prone to races, e.g. a new hosts gets added in between calls to `get_hosts_func` Signed-off-by: Sebastian Wagner --- src/pybind/mgr/cephadm/schedule.py | 28 +++++++++++-------- .../mgr/cephadm/tests/test_scheduling.py | 20 ++++++------- .../ceph/deployment/service_spec.py | 6 ++-- 3 files changed, 29 insertions(+), 25 deletions(-) diff --git a/src/pybind/mgr/cephadm/schedule.py b/src/pybind/mgr/cephadm/schedule.py index 5fada81961c1a..da1beb158dcce 100644 --- a/src/pybind/mgr/cephadm/schedule.py +++ b/src/pybind/mgr/cephadm/schedule.py @@ -1,6 +1,6 @@ import logging import random -from typing import List, Optional, Callable, Iterable, Tuple, TypeVar, Set +from typing import List, Optional, Callable, Iterable, TypeVar, Set import orchestrator from ceph.deployment.service_spec import PlacementSpec, HostPlacementSpec, ServiceSpec @@ -63,11 +63,17 @@ class HostAssignment(object): assert spec and get_hosts_func and get_daemons_func self.spec = spec # type: ServiceSpec self.scheduler = scheduler if scheduler else SimpleScheduler(self.spec) - self.get_hosts_func = get_hosts_func + self.hosts: List[orchestrator.HostSpec] = get_hosts_func(as_hostspec=True) self.filter_new_host = filter_new_host self.service_name = spec.service_name() self.daemons = get_daemons_func(self.service_name) + def hosts_by_label(self, label: str) -> List[orchestrator.HostSpec]: + return [h for h in self.hosts if label in h.labels] + + def get_hostnames(self) -> List[str]: + return [h.hostname for h in self.hosts] + def validate(self): self.spec.validate() @@ -77,20 +83,20 @@ class HostAssignment(object): 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())) + unknown_hosts = explicit_hostnames.difference(set(self.get_hostnames())) if unknown_hosts: raise OrchestratorValidationError( f'Cannot place {self.spec.one_line_str()} on {", ".join(sorted(unknown_hosts))}: Unknown hosts') if self.spec.placement.host_pattern: - pattern_hostnames = self.spec.placement.filter_matching_hosts(self.get_hosts_func) + pattern_hostnames = self.spec.placement.filter_matching_hostspecs(self.hosts) 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(label=self.spec.placement.label) - if not label_hostnames: + label_hosts = self.hosts_by_label(self.spec.placement.label) + if not label_hosts: raise OrchestratorValidationError( f'Cannot place {self.spec.one_line_str()}: No matching ' f'hosts for label {self.spec.placement.label}') @@ -193,13 +199,13 @@ class HostAssignment(object): return self.spec.placement.hosts elif self.spec.placement.label: return [ - HostPlacementSpec(x, '', '') - for x in self.get_hosts_func(label=self.spec.placement.label) + HostPlacementSpec(x.hostname, '', '') + for x in self.hosts_by_label(self.spec.placement.label) ] elif self.spec.placement.host_pattern: return [ HostPlacementSpec(x, '', '') - for x in self.spec.placement.filter_matching_hosts(self.get_hosts_func) + for x in self.spec.placement.filter_matching_hostspecs(self.hosts) ] # If none of the above and also no if self.spec.placement.count is None: @@ -207,8 +213,8 @@ class HostAssignment(object): "placement spec is empty: no hosts, no label, no pattern, no count") # backward compatibility: consider an empty placements to be the same pattern = * return [ - HostPlacementSpec(x, '', '') - for x in self.get_hosts_func() + HostPlacementSpec(x.hostname, '', '') + for x in self.hosts ] def hosts_with_daemons(self, candidates: List[HostPlacementSpec]) -> List[HostPlacementSpec]: diff --git a/src/pybind/mgr/cephadm/tests/test_scheduling.py b/src/pybind/mgr/cephadm/tests/test_scheduling.py index ac5ad9226661d..9287f170b3a9f 100644 --- a/src/pybind/mgr/cephadm/tests/test_scheduling.py +++ b/src/pybind/mgr/cephadm/tests/test_scheduling.py @@ -93,13 +93,11 @@ def mk_spec_and_host(spec_section, hosts, explicit_key, explicit, count): hosts=explicit, count=count, )) - mk_hosts = lambda _: hosts elif spec_section == 'label': mk_spec = lambda: ServiceSpec('mon', placement=PlacementSpec( label='mylabel', count=count, )) - mk_hosts = lambda l: [e for e in explicit if e in hosts] if l == 'mylabel' else hosts elif spec_section == 'host_pattern': pattern = { 'e': 'notfound', @@ -111,16 +109,16 @@ def mk_spec_and_host(spec_section, hosts, explicit_key, explicit, count): host_pattern=pattern, count=count, )) - mk_hosts = lambda _: hosts else: assert False - def _get_hosts_wrapper(label=None, as_hostspec=False): - hosts = mk_hosts(label) - if as_hostspec: - return list(map(HostSpec, hosts)) - return hosts - return mk_spec, _get_hosts_wrapper + def mk_hosts(*_, **__): + return [ + HostSpec(h, labels=['mylabel']) if h in explicit else HostSpec(h) + for h in hosts + ] + + return mk_spec, mk_hosts def run_scheduler_test(results, mk_spec, get_hosts_func, get_daemons_func, key_elems): @@ -431,7 +429,7 @@ class NodeAssignmentTest(NamedTuple): def test_node_assignment(service_type, placement, hosts, daemons, expected): def get_hosts_func(label=None, as_hostspec=False): if as_hostspec: - return [HostSpec(h) for h in hosts] + return [HostSpec(h, labels=['foo']) for h in hosts] return hosts service_id = None @@ -522,7 +520,7 @@ def test_node_assignment2(service_type, placement, hosts, daemons, expected_len, in_set): def get_hosts_func(label=None, as_hostspec=False): if as_hostspec: - return [HostSpec(h) for h in hosts] + return [HostSpec(h, labels=['foo']) for h in hosts] return hosts hosts = HostAssignment( diff --git a/src/python-common/ceph/deployment/service_spec.py b/src/python-common/ceph/deployment/service_spec.py index 05a1389f694b1..33d6b1bc6da61 100644 --- a/src/python-common/ceph/deployment/service_spec.py +++ b/src/python-common/ceph/deployment/service_spec.py @@ -3,7 +3,7 @@ import fnmatch import re from collections import namedtuple, OrderedDict from functools import wraps -from typing import Optional, Dict, Any, List, Union, Callable, Iterator +from typing import Optional, Dict, Any, List, Union, Callable, Iterable import yaml @@ -191,7 +191,7 @@ class PlacementSpec(object): def filter_matching_hosts(self, _get_hosts_func: Callable) -> List[str]: return self.filter_matching_hostspecs(_get_hosts_func(as_hostspec=True)) - def filter_matching_hostspecs(self, hostspecs: Iterator[HostSpec]) -> List[str]: + def filter_matching_hostspecs(self, hostspecs: Iterable[HostSpec]) -> List[str]: if self.hosts: all_hosts = [hs.hostname for hs in hostspecs] return [h.hostname for h in self.hosts if h.hostname in all_hosts] @@ -205,7 +205,7 @@ class PlacementSpec(object): # get_host_selection_size return [] - def get_host_selection_size(self, hostspecs: Iterator[HostSpec]): + def get_host_selection_size(self, hostspecs: Iterable[HostSpec]): if self.count: return self.count return len(self.filter_matching_hostspecs(hostspecs)) -- 2.39.5