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 <sebastian.wagner@suse.com>

diff --git a/src/pybind/mgr/cephadm/schedule.py b/src/pybind/mgr/cephadm/schedule.py
index 5fada81961c1a71efa6da5a05e7723b529968922..da1beb158dcce5fcf13a2f18359caf3e3bfa2603 100644 (file)
--- 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 <count>
         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 ac5ad9226661d49dcb64b114f39d6529d75a572d..9287f170b3a9f587ddcb4d785142c865b4d1b124 100644 (file)
--- 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 05a1389f694b196cb2a2cedda164c1e50a41e177..33d6b1bc6da618c64c9e19a669ada082ce4c851e 100644 (file)
--- 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))