From d0b91f6d39834c658510b803829a118b426e8ef5 Mon Sep 17 00:00:00 2001 From: Adam King Date: Thu, 1 Jun 2023 19:23:45 -0400 Subject: [PATCH] mgr/cephadm: add is_host_ functions to HostCache A bunch of places were doing list compression to see if a host was unreachable/draining/schedulable by hostname. This is meant to replace all those instances of list compression with a function call that does the same Fixes: https://tracker.ceph.com/issues/61548 Signed-off-by: Adam King (cherry picked from commit b4002529b6ec4fae83ae3958f9fc288b22106f90) --- src/pybind/mgr/cephadm/agent.py | 2 +- src/pybind/mgr/cephadm/inventory.py | 15 +++++++++++++-- src/pybind/mgr/cephadm/module.py | 3 +-- src/pybind/mgr/cephadm/serve.py | 7 +++---- src/pybind/mgr/cephadm/tests/test_cephadm.py | 18 +++++++----------- .../mgr/cephadm/tests/test_tuned_profiles.py | 9 +++++++++ src/pybind/mgr/cephadm/tuned_profiles.py | 4 ++-- 7 files changed, 36 insertions(+), 22 deletions(-) diff --git a/src/pybind/mgr/cephadm/agent.py b/src/pybind/mgr/cephadm/agent.py index fa75a8759bbcf..294a17f2c2e36 100644 --- a/src/pybind/mgr/cephadm/agent.py +++ b/src/pybind/mgr/cephadm/agent.py @@ -476,7 +476,7 @@ class CephadmAgentHelpers: def _agent_down(self, host: str) -> bool: # if host is draining or drained (has _no_schedule label) there should not # be an agent deployed there and therefore we should return False - if host not in [h.hostname for h in self.mgr.cache.get_non_draining_hosts()]: + if self.mgr.cache.is_host_draining(host): return False # if we haven't deployed an agent on the host yet, don't say an agent is down if not self.mgr.cache.get_daemons_by_type('agent', host=host): diff --git a/src/pybind/mgr/cephadm/inventory.py b/src/pybind/mgr/cephadm/inventory.py index ec2e4514efa81..f83e687031c87 100644 --- a/src/pybind/mgr/cephadm/inventory.py +++ b/src/pybind/mgr/cephadm/inventory.py @@ -1038,6 +1038,18 @@ class HostCache(): ) ] + def is_host_unreachable(self, hostname: str) -> bool: + # take hostname and return if it matches the hostname of an unreachable host + return hostname in [h.hostname for h in self.get_unreachable_hosts()] + + def is_host_schedulable(self, hostname: str) -> bool: + # take hostname and return if it matches the hostname of a schedulable host + return hostname in [h.hostname for h in self.get_schedulable_hosts()] + + def is_host_draining(self, hostname: str) -> bool: + # take hostname and return if it matches the hostname of a draining host + return hostname in [h.hostname for h in self.get_draining_hosts()] + def get_facts(self, host: str) -> Dict[str, Any]: return self.facts.get(host, {}) @@ -1287,8 +1299,7 @@ class HostCache(): return True def all_host_metadata_up_to_date(self) -> bool: - unreachables = [h.hostname for h in self.get_unreachable_hosts()] - if [h for h in self.get_hosts() if (not self.host_metadata_up_to_date(h) and h not in unreachables)]: + if [h for h in self.get_hosts() if (not self.host_metadata_up_to_date(h) and not self.is_host_unreachable(h))]: # this function is primarily for telling if it's safe to try and apply a service # spec. Since offline/maintenance hosts aren't considered in that process anyway # we don't want to return False if the host without up-to-date metadata is in one diff --git a/src/pybind/mgr/cephadm/module.py b/src/pybind/mgr/cephadm/module.py index 3fdbb64f9c316..b7d2215f2ca97 100644 --- a/src/pybind/mgr/cephadm/module.py +++ b/src/pybind/mgr/cephadm/module.py @@ -2685,7 +2685,6 @@ Then run the following: def _get_candidate_hosts(self, placement: PlacementSpec) -> List[str]: """Return a list of candidate hosts according to the placement specification.""" all_hosts = self.cache.get_schedulable_hosts() - draining_hosts = [dh.hostname for dh in self.cache.get_draining_hosts()] candidates = [] if placement.hosts: candidates = [h.hostname for h in placement.hosts if h.hostname in placement.hosts] @@ -2695,7 +2694,7 @@ Then run the following: candidates = [x for x in placement.filter_matching_hostspecs(all_hosts)] elif (placement.count is not None or placement.count_per_host is not None): candidates = [x.hostname for x in all_hosts] - return [h for h in candidates if h not in draining_hosts] + return [h for h in candidates if not self.cache.is_host_draining(h)] def _validate_one_shot_placement_spec(self, spec: PlacementSpec) -> None: """Validate placement specification for TunedProfileSpec and ClientKeyringSpec.""" diff --git a/src/pybind/mgr/cephadm/serve.py b/src/pybind/mgr/cephadm/serve.py index 268d21963422c..8f540c013aef2 100644 --- a/src/pybind/mgr/cephadm/serve.py +++ b/src/pybind/mgr/cephadm/serve.py @@ -209,7 +209,7 @@ class CephadmServe: if ( not self.mgr.use_agent - or host not in [h.hostname for h in self.mgr.cache.get_non_draining_hosts()] + or self.mgr.cache.is_host_draining(host) or host in agents_down ): if self.mgr.cache.host_needs_daemon_refresh(host): @@ -900,8 +900,7 @@ class CephadmServe: self.mgr.spec_store.mark_configured(spec.service_name()) if self.mgr.use_agent: # can only send ack to agents if we know for sure port they bound to - hosts_altered = set([h for h in hosts_altered if (h in self.mgr.agent_cache.agent_ports and h in [ - h2.hostname for h2 in self.mgr.cache.get_non_draining_hosts()])]) + hosts_altered = set([h for h in hosts_altered if (h in self.mgr.agent_cache.agent_ports and not self.mgr.cache.is_host_draining(h))]) self.mgr.agent_helpers._request_agent_acks(hosts_altered, increment=True) if r is None: @@ -1146,7 +1145,7 @@ class CephadmServe: client_files: Dict[str, Dict[str, Tuple[int, int, int, bytes, str]]], host: str) -> None: updated_files = False - if host in [h.hostname for h in self.mgr.cache.get_unreachable_hosts()]: + if self.mgr.cache.is_host_unreachable(host): return old_files = self.mgr.cache.get_host_client_files(host).copy() for path, m in client_files.get(host, {}).items(): diff --git a/src/pybind/mgr/cephadm/tests/test_cephadm.py b/src/pybind/mgr/cephadm/tests/test_cephadm.py index eb5247e227368..8e09eb9400116 100644 --- a/src/pybind/mgr/cephadm/tests/test_cephadm.py +++ b/src/pybind/mgr/cephadm/tests/test_cephadm.py @@ -1753,15 +1753,11 @@ class TestCephadm(object): # being in offline/maint mode should disqualify hosts from being # candidates for scheduling - candidates = [ - h.hostname for h in cephadm_module.cache.get_schedulable_hosts()] - assert 'test2' in candidates - assert 'test3' in candidates + assert cephadm_module.cache.is_host_schedulable('test2') + assert cephadm_module.cache.is_host_schedulable('test3') - unreachable = [ - h.hostname for h in cephadm_module.cache.get_unreachable_hosts()] - assert 'test2' in unreachable - assert 'test3' in unreachable + assert cephadm_module.cache.is_host_unreachable('test2') + assert cephadm_module.cache.is_host_unreachable('test3') with with_service(cephadm_module, ServiceSpec('crash', placement=PlacementSpec(host_pattern='*'))): # re-apply services. No mgr should be removed from maint/offline hosts @@ -1942,9 +1938,9 @@ class TestCephadm(object): cephadm_module.offline_hosts.add('host3') # verify host2 and host3 are correctly marked as unreachable but host1 is not - assert 'host1' not in [h.hostname for h in cephadm_module.cache.get_unreachable_hosts()] - assert 'host2' in [h.hostname for h in cephadm_module.cache.get_unreachable_hosts()] - assert 'host3' in [h.hostname for h in cephadm_module.cache.get_unreachable_hosts()] + assert not cephadm_module.cache.is_host_unreachable('host1') + assert cephadm_module.cache.is_host_unreachable('host2') + assert cephadm_module.cache.is_host_unreachable('host3') _get_client_files.side_effect = Exception('Called _get_client_files') diff --git a/src/pybind/mgr/cephadm/tests/test_tuned_profiles.py b/src/pybind/mgr/cephadm/tests/test_tuned_profiles.py index 2e0e9e5a9fe3d..66feaee319494 100644 --- a/src/pybind/mgr/cephadm/tests/test_tuned_profiles.py +++ b/src/pybind/mgr/cephadm/tests/test_tuned_profiles.py @@ -37,6 +37,15 @@ class FakeCache: def get_draining_hosts(self): return [] + def is_host_unreachable(self, hostname: str): + return hostname in [h.hostname for h in self.get_unreachable_hosts()] + + def is_host_schedulable(self, hostname: str): + return hostname in [h.hostname for h in self.get_schedulable_hosts()] + + def is_host_draining(self, hostname: str): + return hostname in [h.hostname for h in self.get_draining_hosts()] + @property def networks(self): return {h: {'a': {'b': ['c']}} for h in self.hosts} diff --git a/src/pybind/mgr/cephadm/tuned_profiles.py b/src/pybind/mgr/cephadm/tuned_profiles.py index 871b8b2e38cdd..8ec30bd536be7 100644 --- a/src/pybind/mgr/cephadm/tuned_profiles.py +++ b/src/pybind/mgr/cephadm/tuned_profiles.py @@ -67,7 +67,7 @@ class TunedProfileUtils(): SHOULD be on the host. Then if we see any file names that don't match this, but DO include "-cephadm-tuned-profile.conf" (implying they're from us), remove them. """ - if host in [h.hostname for h in self.mgr.cache.get_unreachable_hosts()]: + if self.mgr.cache.is_host_unreachable(host): return cmd = ['ls', SYSCTL_DIR] found_files = self.mgr.ssh.check_execute_command(host, cmd, log_command=self.mgr.log_refresh_metadata).split('\n') @@ -88,7 +88,7 @@ class TunedProfileUtils(): self.mgr.ssh.check_execute_command(host, ['sysctl', '--system']) def _write_tuned_profiles(self, host: str, profiles: List[Dict[str, str]]) -> None: - if host in [h.hostname for h in self.mgr.cache.get_unreachable_hosts()]: + if self.mgr.cache.is_host_unreachable(host): return updated = False for p in profiles: -- 2.39.5