mgr/cephadm: make scheduling make sense

Like,

```yaml
placement:
  hosts:
  - myhost
  count: 3
```
will always only schedule one daemon on `myhost`.

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 fe5f70b2b48c872b3dee120928c3ec7213292266..a4452a616b9bfe24ecb5ae15575b82eaf9c75c7a 100644 (file)
--- a/src/pybind/mgr/cephadm/schedule.py
+++ b/src/pybind/mgr/cephadm/schedule.py
@@ -1,12 +1,13 @@
 import logging
 import random
-from typing import List, Optional, Callable
+from typing import List, Optional, Callable, Iterable, Tuple, TypeVar
 
 import orchestrator
 from ceph.deployment.service_spec import PlacementSpec, HostPlacementSpec, ServiceSpec
 from orchestrator import OrchestratorValidationError
 
 logger = logging.getLogger(__name__)
+T = TypeVar('T')
 
 class BaseScheduler(object):
     """
@@ -22,7 +23,7 @@ class BaseScheduler(object):
         self.placement_spec = placement_spec
 
     def place(self, host_pool, count=None):
-        # type: (List, Optional[int]) -> List[HostPlacementSpec]
+        # type: (List[T], Optional[int]) -> List[T]
         raise NotImplementedError
 
 
@@ -36,7 +37,7 @@ class SimpleScheduler(BaseScheduler):
         super(SimpleScheduler, self).__init__(placement_spec)
 
     def place(self, host_pool, count=None):
-        # type: (List, Optional[int]) -> List[HostPlacementSpec]
+        # type: (List[T], Optional[int]) -> List[T]
         if not host_pool:
             return []
         host_pool = [x for x in host_pool]
@@ -46,21 +47,11 @@ class SimpleScheduler(BaseScheduler):
 
 
 class HostAssignment(object):
-    """
-    A class to detect if hosts are being passed imperative or declarative
-    If the spec is populated via the `hosts/hosts` field it will not load
-    any hosts into the list.
-    If the spec isn't populated, i.e. when only num or label is present (declarative)
-    it will use the provided `get_host_func` to load it from the inventory.
-
-    Schedulers can be assigned to pick hosts from the pool.
-    """
 
     def __init__(self,
                  spec,  # type: ServiceSpec
                  get_hosts_func,  # type: Callable
                  get_daemons_func, # type: Callable[[str],List[orchestrator.DaemonDescription]]
-
                  filter_new_host=None, # type: Optional[Callable[[str],bool]]
                  scheduler=None,  # type: Optional[BaseScheduler]
                  ):
@@ -68,10 +59,9 @@ class HostAssignment(object):
         self.spec = spec  # type: ServiceSpec
         self.scheduler = scheduler if scheduler else SimpleScheduler(self.spec.placement)
         self.get_hosts_func = get_hosts_func
-        self.get_daemons_func = get_daemons_func
         self.filter_new_host = filter_new_host
         self.service_name = spec.service_name()
-
+        self.daemons = get_daemons_func(self.service_name)
 
     def validate(self):
         self.spec.validate()
@@ -99,95 +89,104 @@ class HostAssignment(object):
     def place(self):
         # type: () -> List[HostPlacementSpec]
         """
-        Load hosts into the spec.placement.hosts container.
+        Generate a list of HostPlacementSpec taking into account:
+
+        * all known hosts
+        * hosts with existing daemons
+        * placement spec
+        * self.filter_new_host
         """
 
         self.validate()
 
-        # count == 0
-        if self.spec.placement.count == 0:
-            return []
+        count = self.spec.placement.count
+        assert count != 0
 
-        # respect any explicit host list
-        if self.spec.placement.hosts and not self.spec.placement.count:
+        chosen = self.get_candidates()
+        if count is None:
             logger.debug('Provided hosts: %s' % self.spec.placement.hosts)
-            return self.spec.placement.hosts
+            return chosen
 
-        # respect host_pattern
-        if self.spec.placement.host_pattern:
-            candidates = [
-                HostPlacementSpec(x, '', '')
-                for x in self.spec.placement.filter_matching_hosts(self.get_hosts_func)
-            ]
-            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:
-            hosts = self.spec.placement.hosts
-            logger.debug('place %d over provided host list: %s' % (
-                count, hosts))
-            count = self.spec.placement.count
+        # prefer hosts that already have services
+        existing = self.hosts_with_daemons(chosen)
+
+        need = count - len(existing)
+        others = difference_hostspecs(chosen, existing)
+
+        if need < 0:
+            return self.scheduler.place(existing, count)
+        else:
+            if self.filter_new_host:
+                old = others
+                others = [h for h in others if self.filter_new_host(h.hostname)]
+                logger.debug('filtered %s down to %s' % (old, chosen))
+
+            others = self.scheduler.place(others, need)
+            logger.debug('Combine hosts with existing daemons %s + new hosts %s' % (
+                existing, others))
+            return list(merge_hostspecs(existing, others))
+
+    def get_candidates(self) -> List[HostPlacementSpec]:
+        if self.spec.placement.hosts:
+            return self.spec.placement.hosts
         elif self.spec.placement.label:
-            hosts = [
+            return [
                 HostPlacementSpec(x, '', '')
                 for x in self.get_hosts_func(label=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:
-            hosts = [
+        elif self.spec.placement.host_pattern:
+            return [
                 HostPlacementSpec(x, '', '')
-                for x in self.get_hosts_func()
+                for x in self.spec.placement.filter_matching_hosts(self.get_hosts_func)
             ]
-            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.
-                assert self.spec.placement.count is None
-                assert not self.spec.placement.hosts
-                assert not self.spec.placement.label
-                count = 1
-            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) < count:
-            chosen = self.spec.placement.hosts
-        else:
-            chosen = []
+        if self.spec.placement.count is None:
+            raise OrchestratorValidationError("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()
+        ]
+
+    def hosts_with_daemons(self, chosen: List[HostPlacementSpec]) -> List[HostPlacementSpec]:
+        """
+        Prefer hosts with daemons. Otherwise we'll constantly schedule daemons
+        on different hosts all the time. This is about keeping daemons where
+        they are. This isn't about co-locating.
+        """
+        hosts = {d.hostname for d in self.daemons}
 
-        # prefer hosts that already have services
-        daemons = self.get_daemons_func(self.service_name)
-        hosts_with_daemons = {d.hostname for d in daemons}
         # calc existing daemons (that aren't already in chosen)
-        chosen_hosts = [hs.hostname for hs in chosen]
-        existing = [hs for hs in hosts
-                    if hs.hostname in hosts_with_daemons and \
-                    hs.hostname not in chosen_hosts]
-        if len(chosen + existing) >= count:
-            chosen = chosen + self.scheduler.place(
-                existing,
-                count - len(chosen))
-            logger.debug('Hosts with existing daemons: {}'.format(chosen))
-            return chosen
+        existing = [hs for hs in chosen if hs.hostname in hosts]
+
+        logger.debug('Hosts with existing daemons: {}'.format(existing))
+        return existing
+
+
+def merge_hostspecs(l: List[HostPlacementSpec], r: List[HostPlacementSpec]) -> Iterable[HostPlacementSpec]:
+    """
+    Merge two lists of HostPlacementSpec by hostname. always returns `l` first.
+
+    >>> list(merge_hostspecs([HostPlacementSpec(hostname='h', name='x', network='')],
+    ...                      [HostPlacementSpec(hostname='h', name='y', network='')]))
+    [HostPlacementSpec(hostname='h', network='', name='x')]
+
+    """
+    l_names = {h.hostname for h in l}
+    yield from l
+    yield from (h for h in r if h.hostname not in l_names)
+
+
+def difference_hostspecs(l: List[HostPlacementSpec], r: List[HostPlacementSpec]) -> List[HostPlacementSpec]:
+    """
+    returns l "minus" r by hostname.
+
+    >>> list(difference_hostspecs([HostPlacementSpec(hostname='h1', name='x', network=''),
+    ...                           HostPlacementSpec(hostname='h2', name='y', network='')],
+    ...                           [HostPlacementSpec(hostname='h2', name='', network='')]))
+    [HostPlacementSpec(hostname='h1', network='', name='x')]
+
+    """
+    r_names = {h.hostname for h in r}
+    return [h for h in l if h.hostname not in r_names]
+
 
-        need = count - len(existing + chosen)
-        others = [hs for hs in hosts
-                  if hs.hostname not in hosts_with_daemons]
-        if self.filter_new_host:
-            old = others
-            others = [h for h in others if self.filter_new_host(h.hostname)]
-            logger.debug('filtered %s down to %s' % (old, hosts))
-        chosen = chosen + self.scheduler.place(others, need)
-        logger.debug('Combine hosts with existing daemons %s + new hosts %s' % (
-            existing, chosen))
-        return existing + chosen

diff --git a/src/pybind/mgr/cephadm/tests/test_scheduling.py b/src/pybind/mgr/cephadm/tests/test_scheduling.py
index 6a913316966b3c26c0edd0071f3989599212cc33..8612960b529bf217c24dd8dab04af216fdbc3e64 100644 (file)
--- a/src/pybind/mgr/cephadm/tests/test_scheduling.py
+++ b/src/pybind/mgr/cephadm/tests/test_scheduling.py
@@ -82,7 +82,7 @@ def get_result(key, results):
 
 
 # * first match from the top wins
-# * where N=None, e=[], *=any
+# * where e=[], *=any
 #
 #       + list of known hosts available for scheduling
 #       |   + hosts used for explict placement
@@ -90,43 +90,20 @@ def get_result(key, results):
 #       |   |   |
 test_explicit_scheduler_results = [
     (k("*   *   0"), error(ServiceSpecValidationError, 'num/count must be > 1')),
-    (k("e   N   *"), none()),  # bug: should be exception
-    (k("e   e   *"), none()),
+    (k("*   e   N"), error(OrchestratorValidationError, 'placement spec is empty: no hosts, no label, no pattern, no count')),
+    (k("*   e   *"), none),
     (k("e   1   *"), error(OrchestratorValidationError, "Cannot place <ServiceSpec for service_name=mon> on 1: Unknown hosts")),
     (k("e   12  *"), error(OrchestratorValidationError, "Cannot place <ServiceSpec for service_name=mon> on 1, 2: Unknown hosts")),
     (k("e   123 *"), error(OrchestratorValidationError, "Cannot place <ServiceSpec for service_name=mon> on 1, 2, 3: Unknown hosts")),
-    (k("1   N   *"), exactly('1')),  # bug: should be exception (empty placement should be invalid)
-    (k("1   e   *"), exactly('1')),  # bug: should be exception (empty placement should be invalid)
-    (k("1   1   2"), exactly('1', '1')),  # bug: should be exactly('1')
-    (k("1   1   3"), exactly('1', '1')),  # bug: should be exactly('1')
-    (k("1   1   *"), one_of('1')),
+    (k("1   1   *"), exactly('1')),
     (k("1   12  *"), error(OrchestratorValidationError, "Cannot place <ServiceSpec for service_name=mon> on 2: Unknown hosts")),
     (k("1   123 *"), error(OrchestratorValidationError, "Cannot place <ServiceSpec for service_name=mon> on 2, 3: Unknown hosts")),
-    (k("12  N   2"), exactly('1', '2')),  # bug: should be exception (empty placement should be invalid)
-    (k("12  N   3"), exactly('1', '2')),  # bug: should be exception (empty placement should be invalid)
-    (k("12  N   *"), one_of('1', '2')),  # bug: should be exception (empty placement should be invalid)
-    (k("12  e   2"), exactly('1', '2')),  # bug: should be exception
-    (k("12  e   3"), exactly('1', '2')),  # bug: should be exception
-    (k("12  e   *"), one_of('1', '2')),  # bug: should be exception
-    (k("12  1   2"), _or(exactly('1', '1'), exactly('1', '2'))),   # bug: should be exactly('1')
-    (k("12  1   3"), exactly('1', '1', '2')),   # bug: should be exactly('1')
     (k("12  1   *"), exactly('1')),
     (k("12  12  1"), one_of('1', '2')),
-    (k("12  12  3"), _or(exactly('1', '2', '2'), exactly('1', '1', '2'))), # bug: one_of('1', '2')
     (k("12  12  *"), exactly('1', '2')),
     (k("12  123 *"), error(OrchestratorValidationError, "Cannot place <ServiceSpec for service_name=mon> on 3: Unknown hosts")),
-    (k("123 N   2"), two_of('1', '2', '3')),  # bug: should be exception (empty placement should be invalid)
-    (k("123 N   3"), exactly('1', '2', '3')),  # bug: should be exception (empty placement should be invalid)
-    (k("123 N   *"), one_of('1', '2', '3')),  # bug: should be exception (empty placement should be invalid)
-    (k("123 e   2"), two_of('1', '2', '3')),  # bug: should be exception (empty placement should be invalid)
-    (k("123 e   3"), exactly('1', '2', '3')),  # bug: should be exception (empty placement should be invalid)
-    (k("123 e   *"), one_of('1', '2', '3')),  # bug: should be exception (empty placement should be invalid)
-    (k("123 e   2"), _or(exactly('1', '3'), exactly('1', '2'), exactly('1', '1'))), # bug. should be exactly('1')
-    (k("123 1   2"), _always_true), # bug. should be exactly('1')
-    (k("123 1   3"), _always_true), # bug. should be exactly('1')
     (k("123 1   *"), exactly('1')),
     (k("123 12  1"), one_of('1', '2')),
-    (k("123 12  3"), _always_true),  # bug: should be exactly('1', '2')
     (k("123 12  *"), exactly('1', '2')),
     (k("123 123 1"), one_of('1', '2', '3')),
     (k("123 123 2"), two_of('1', '2', '3')),
@@ -143,7 +120,6 @@ test_explicit_scheduler_results = [
     ])
 @pytest.mark.parametrize("explicit_key, explicit",
     [
-        ('N', None),
         ('e', []),
         ('1', ['1']),
         ('12', ['1', '2']),
@@ -227,7 +203,7 @@ class NodeAssignmentTest(NamedTuple):
                 DaemonDescription('mon', 'a', 'host1'),
                 DaemonDescription('mon', 'b', 'host2'),
             ],
-            ['host1', 'host2', 'host3']
+            ['host3']
         ),
         # count 1 + partial host list
         NodeAssignmentTest(
@@ -248,7 +224,7 @@ class NodeAssignmentTest(NamedTuple):
             [
                 DaemonDescription('mon', 'a', 'host1'),
             ],
-            ['host1', 'host3']
+            ['host3']
         ),
         # count + partial host list + existing (deterministic)
         NodeAssignmentTest(
@@ -258,7 +234,7 @@ class NodeAssignmentTest(NamedTuple):
             [
                 DaemonDescription('mon', 'a', 'host1'),
             ],
-            ['host1', 'host2']
+            ['host1']
         ),
         # count + partial host list + existing (deterministic)
         NodeAssignmentTest(
@@ -268,7 +244,7 @@ class NodeAssignmentTest(NamedTuple):
             [
                 DaemonDescription('mon', 'a', 'host2'),
             ],
-            ['host1', 'host2']
+            ['host1']
         ),
         # label only
         NodeAssignmentTest(
@@ -305,16 +281,6 @@ 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',
@@ -361,7 +327,7 @@ class NodeAssignmentTest2(NamedTuple):
             PlacementSpec(count=2, hosts=['host3']),
             'host1 host2 host3'.split(),
             [],
-            2,
+            1,
             ['host1', 'host2', 'host3']
         ),
         # label + count
@@ -392,7 +358,7 @@ def test_node_assignment2(service_type, placement, hosts,
             PlacementSpec(count=3, hosts='host3'.split()),
             'host1 host2 host3'.split(),
             [],
-            3,
+            1,
             ['host3']
         ),
         # count + partial host list
@@ -401,7 +367,7 @@ def test_node_assignment2(service_type, placement, hosts,
             PlacementSpec(count=2, hosts=['host3']),
             'host1 host2 host3'.split(),
             [],
-            2,
+            1,
             ['host3']
         ),
     ])

diff --git a/src/pybind/mgr/orchestrator/_interface.py b/src/pybind/mgr/orchestrator/_interface.py
index db520c0f25da20a082987226726985163d69d2e9..7fdfbfb1eaa32df6b24e59952beac947258f5a65 100644 (file)
--- a/src/pybind/mgr/orchestrator/_interface.py
+++ b/src/pybind/mgr/orchestrator/_interface.py
@@ -1277,7 +1277,7 @@ class DaemonDescription(object):
                  osdspec_affinity=None,
                  last_deployed=None):
         # Host is at the same granularity as InventoryHost
-        self.hostname = hostname
+        self.hostname: str = hostname
 
         # Not everyone runs in containers, but enough people do to
         # justify having the container_id (runtime id) and container_image