From 245cf50132e1731982d19e1114116e4150b9ea97 Mon Sep 17 00:00:00 2001 From: Sebastian Wagner Date: Fri, 11 Sep 2020 19:49:57 +0200 Subject: [PATCH] mgr/cephadm: CephadmService.create -> CephadmService.prepare_create Refactored this to simplify the control flow. It became obvious that * `CephadmService.make_daemon_spec` * `CephadmService.prepare_create` * `CephadmService.generate_config` are basically the same thing. They're all called directly before the deployment of a daemon. All of them should be unified. This PR makes this refactorization simpler By renaming `create` to `prepare_create`, we make `create` no longer being the entrypoint to call `create_daemon`. Thus all the functions above return some data structures. Signed-off-by: Sebastian Wagner --- src/pybind/mgr/cephadm/module.py | 35 ++++++++++--------- .../mgr/cephadm/services/cephadmservice.py | 26 +++++++------- src/pybind/mgr/cephadm/services/iscsi.py | 4 +-- src/pybind/mgr/cephadm/services/monitoring.py | 19 +++++----- src/pybind/mgr/cephadm/services/nfs.py | 4 +-- 5 files changed, 46 insertions(+), 42 deletions(-) diff --git a/src/pybind/mgr/cephadm/module.py b/src/pybind/mgr/cephadm/module.py index bf37872f55e3b..2ee4d85757dd2 100644 --- a/src/pybind/mgr/cephadm/module.py +++ b/src/pybind/mgr/cephadm/module.py @@ -2153,7 +2153,9 @@ To check that the host is reachable: self.log.debug('Placing %s.%s on host %s' % ( daemon_type, daemon_id, host)) - self.cephadm_services[daemon_type].create(daemon_spec) + daemon_spec = self.cephadm_services[daemon_type].prepare_create(daemon_spec) + + self._create_daemon(daemon_spec) # add to daemon list so next name(s) will also be unique sd = orchestrator.DaemonDescription( @@ -2281,7 +2283,7 @@ To check that the host is reachable: self._get_cephadm_service(daemon_type).daemon_check_post(daemon_descs) def _add_daemon(self, daemon_type, spec, - create_func: Callable[..., T], config_func=None) -> List[T]: + create_func: Callable[..., CephadmDaemonSpec], config_func=None) -> List[str]: """ Add (and place) a daemon. Require explicit host placement. Do not schedule, and do not apply the related scheduling limitations. @@ -2297,7 +2299,7 @@ To check that the host is reachable: def _create_daemons(self, daemon_type, spec, daemons, hosts, count, - create_func: Callable[..., T], config_func=None) -> List[T]: + create_func: Callable[..., CephadmDaemonSpec], config_func=None) -> List[str]: if count > len(hosts): raise OrchestratorError('too few hosts: want %d, have %s' % ( count, hosts)) @@ -2333,7 +2335,8 @@ To check that the host is reachable: @forall_hosts def create_func_map(*args): - return create_func(*args) + daemon_spec = create_func(*args) + return self._create_daemon(daemon_spec) return create_func_map(args) @@ -2344,12 +2347,12 @@ To check that the host is reachable: @trivial_completion def add_mon(self, spec): # type: (ServiceSpec) -> List[str] - return self._add_daemon('mon', spec, self.mon_service.create) + return self._add_daemon('mon', spec, self.mon_service.prepare_create) @trivial_completion def add_mgr(self, spec): # type: (ServiceSpec) -> List[str] - return self._add_daemon('mgr', spec, self.mgr_service.create) + return self._add_daemon('mgr', spec, self.mgr_service.prepare_create) def _apply(self, spec: GenericSpec) -> str: self.migration.verify_no_migration() @@ -2449,7 +2452,7 @@ To check that the host is reachable: @trivial_completion def add_mds(self, spec: ServiceSpec) -> List[str]: - return self._add_daemon('mds', spec, self.mds_service.create, self.mds_service.config) + return self._add_daemon('mds', spec, self.mds_service.prepare_create, self.mds_service.config) @trivial_completion def apply_mds(self, spec: ServiceSpec) -> str: @@ -2457,7 +2460,7 @@ To check that the host is reachable: @trivial_completion def add_rgw(self, spec) -> List[str]: - return self._add_daemon('rgw', spec, self.rgw_service.create, self.rgw_service.config) + return self._add_daemon('rgw', spec, self.rgw_service.prepare_create, self.rgw_service.config) @trivial_completion def apply_rgw(self, spec) -> str: @@ -2466,7 +2469,7 @@ To check that the host is reachable: @trivial_completion def add_iscsi(self, spec): # type: (ServiceSpec) -> List[str] - return self._add_daemon('iscsi', spec, self.iscsi_service.create, self.iscsi_service.config) + return self._add_daemon('iscsi', spec, self.iscsi_service.prepare_create, self.iscsi_service.config) @trivial_completion def apply_iscsi(self, spec) -> str: @@ -2474,7 +2477,7 @@ To check that the host is reachable: @trivial_completion def add_rbd_mirror(self, spec) -> List[str]: - return self._add_daemon('rbd-mirror', spec, self.rbd_mirror_service.create) + return self._add_daemon('rbd-mirror', spec, self.rbd_mirror_service.prepare_create) @trivial_completion def apply_rbd_mirror(self, spec) -> str: @@ -2482,7 +2485,7 @@ To check that the host is reachable: @trivial_completion def add_nfs(self, spec) -> List[str]: - return self._add_daemon('nfs', spec, self.nfs_service.create, self.nfs_service.config) + return self._add_daemon('nfs', spec, self.nfs_service.prepare_create, self.nfs_service.config) @trivial_completion def apply_nfs(self, spec) -> str: @@ -2494,7 +2497,7 @@ To check that the host is reachable: @trivial_completion def add_prometheus(self, spec) -> List[str]: - return self._add_daemon('prometheus', spec, self.prometheus_service.create) + return self._add_daemon('prometheus', spec, self.prometheus_service.prepare_create) @trivial_completion def apply_prometheus(self, spec) -> str: @@ -2504,7 +2507,7 @@ To check that the host is reachable: def add_node_exporter(self, spec): # type: (ServiceSpec) -> List[str] return self._add_daemon('node-exporter', spec, - self.node_exporter_service.create) + self.node_exporter_service.prepare_create) @trivial_completion def apply_node_exporter(self, spec) -> str: @@ -2514,7 +2517,7 @@ To check that the host is reachable: def add_crash(self, spec): # type: (ServiceSpec) -> List[str] return self._add_daemon('crash', spec, - self.crash_service.create) + self.crash_service.prepare_create) @trivial_completion def apply_crash(self, spec) -> str: @@ -2523,7 +2526,7 @@ To check that the host is reachable: @trivial_completion def add_grafana(self, spec): # type: (ServiceSpec) -> List[str] - return self._add_daemon('grafana', spec, self.grafana_service.create) + return self._add_daemon('grafana', spec, self.grafana_service.prepare_create) @trivial_completion def apply_grafana(self, spec: ServiceSpec) -> str: @@ -2532,7 +2535,7 @@ To check that the host is reachable: @trivial_completion def add_alertmanager(self, spec): # type: (ServiceSpec) -> List[str] - return self._add_daemon('alertmanager', spec, self.alertmanager_service.create) + return self._add_daemon('alertmanager', spec, self.alertmanager_service.prepare_create) @trivial_completion def apply_alertmanager(self, spec: ServiceSpec) -> str: diff --git a/src/pybind/mgr/cephadm/services/cephadmservice.py b/src/pybind/mgr/cephadm/services/cephadmservice.py index 648ca1c931e7a..94e3b9dab85ca 100644 --- a/src/pybind/mgr/cephadm/services/cephadmservice.py +++ b/src/pybind/mgr/cephadm/services/cephadmservice.py @@ -86,7 +86,7 @@ class CephadmService(metaclass=ABCMeta): network=netowrk ) - def create(self, daemon_spec: CephadmDaemonSpec): + def prepare_create(self, daemon_spec: CephadmDaemonSpec) -> CephadmDaemonSpec: raise NotImplementedError() def generate_config(self, daemon_spec: CephadmDaemonSpec) -> Tuple[Dict[str, Any], List[str]]: @@ -282,7 +282,7 @@ class CephService(CephadmService): class MonService(CephService): TYPE = 'mon' - def create(self, daemon_spec: CephadmDaemonSpec) -> str: + def prepare_create(self, daemon_spec: CephadmDaemonSpec) -> CephadmDaemonSpec: """ Create a new monitor on the given host. """ @@ -328,7 +328,7 @@ class MonService(CephService): daemon_spec.extra_config = {'config': extra_config} daemon_spec.keyring = keyring - return self.mgr._create_daemon(daemon_spec) + return daemon_spec def _check_safe_to_destroy(self, mon_id: str) -> None: ret, out, err = self.mgr.check_mon_command({ @@ -367,7 +367,7 @@ class MonService(CephService): class MgrService(CephService): TYPE = 'mgr' - def create(self, daemon_spec: CephadmDaemonSpec) -> str: + def prepare_create(self, daemon_spec: CephadmDaemonSpec) -> CephadmDaemonSpec: """ Create a new manager instance on a host. """ @@ -406,7 +406,7 @@ class MgrService(CephService): daemon_spec.keyring = keyring - return self.mgr._create_daemon(daemon_spec) + return daemon_spec def get_active_daemon(self, daemon_descrs: List[DaemonDescription]) -> DaemonDescription: for daemon in daemon_descrs: @@ -455,7 +455,7 @@ class MdsService(CephService): 'value': spec.service_id, }) - def create(self, daemon_spec: CephadmDaemonSpec) -> str: + def prepare_create(self, daemon_spec: CephadmDaemonSpec) -> CephadmDaemonSpec: assert self.TYPE == daemon_spec.daemon_type mds_id, host = daemon_spec.daemon_id, daemon_spec.host @@ -469,7 +469,7 @@ class MdsService(CephService): }) daemon_spec.keyring = keyring - return self.mgr._create_daemon(daemon_spec) + return daemon_spec def get_active_daemon(self, daemon_descrs: List[DaemonDescription]) -> DaemonDescription: active_mds_strs = list() @@ -550,7 +550,7 @@ class RgwService(CephService): spec.service_name(), spec.placement.pretty_str())) self.mgr.spec_store.save(spec) - def create(self, daemon_spec: CephadmDaemonSpec) -> str: + def prepare_create(self, daemon_spec: CephadmDaemonSpec) -> CephadmDaemonSpec: assert self.TYPE == daemon_spec.daemon_type rgw_id, host = daemon_spec.daemon_id, daemon_spec.host @@ -558,7 +558,7 @@ class RgwService(CephService): daemon_spec.keyring = keyring - return self.mgr._create_daemon(daemon_spec) + return daemon_spec def get_keyring(self, rgw_id: str): ret, keyring, err = self.mgr.check_mon_command({ @@ -690,7 +690,7 @@ class RgwService(CephService): class RbdMirrorService(CephService): TYPE = 'rbd-mirror' - def create(self, daemon_spec: CephadmDaemonSpec) -> str: + def prepare_create(self, daemon_spec: CephadmDaemonSpec) -> CephadmDaemonSpec: assert self.TYPE == daemon_spec.daemon_type daemon_id, host = daemon_spec.daemon_id, daemon_spec.host @@ -703,13 +703,13 @@ class RbdMirrorService(CephService): daemon_spec.keyring = keyring - return self.mgr._create_daemon(daemon_spec) + return daemon_spec class CrashService(CephService): TYPE = 'crash' - def create(self, daemon_spec: CephadmDaemonSpec) -> str: + def prepare_create(self, daemon_spec: CephadmDaemonSpec) -> CephadmDaemonSpec: assert self.TYPE == daemon_spec.daemon_type daemon_id, host = daemon_spec.daemon_id, daemon_spec.host @@ -722,4 +722,4 @@ class CrashService(CephService): daemon_spec.keyring = keyring - return self.mgr._create_daemon(daemon_spec) + return daemon_spec diff --git a/src/pybind/mgr/cephadm/services/iscsi.py b/src/pybind/mgr/cephadm/services/iscsi.py index 965e7492496f1..1ce858ee53584 100644 --- a/src/pybind/mgr/cephadm/services/iscsi.py +++ b/src/pybind/mgr/cephadm/services/iscsi.py @@ -23,7 +23,7 @@ class IscsiService(CephService): spec.service_name(), spec.placement.pretty_str())) self.mgr.spec_store.save(spec) - def create(self, daemon_spec: CephadmDaemonSpec[IscsiServiceSpec]) -> str: + def prepare_create(self, daemon_spec: CephadmDaemonSpec[IscsiServiceSpec]) -> CephadmDaemonSpec: assert self.TYPE == daemon_spec.daemon_type assert daemon_spec.spec @@ -70,7 +70,7 @@ class IscsiService(CephService): daemon_spec.keyring = keyring daemon_spec.extra_config = {'iscsi-gateway.cfg': igw_conf} - return self.mgr._create_daemon(daemon_spec) + return daemon_spec def config_dashboard(self, daemon_descrs: List[DaemonDescription]): def get_set_cmd_dicts(out: str) -> List[dict]: diff --git a/src/pybind/mgr/cephadm/services/monitoring.py b/src/pybind/mgr/cephadm/services/monitoring.py index e14e17fdc2300..9bf4b5e001d80 100644 --- a/src/pybind/mgr/cephadm/services/monitoring.py +++ b/src/pybind/mgr/cephadm/services/monitoring.py @@ -14,9 +14,9 @@ class GrafanaService(CephadmService): TYPE = 'grafana' DEFAULT_SERVICE_PORT = 3000 - def create(self, daemon_spec: CephadmDaemonSpec) -> str: + def prepare_create(self, daemon_spec: CephadmDaemonSpec) -> CephadmDaemonSpec: assert self.TYPE == daemon_spec.daemon_type - return self.mgr._create_daemon(daemon_spec) + return daemon_spec def generate_config(self, daemon_spec: CephadmDaemonSpec) -> Tuple[Dict[str, Any], List[str]]: assert self.TYPE == daemon_spec.daemon_type @@ -83,10 +83,10 @@ class AlertmanagerService(CephadmService): TYPE = 'alertmanager' DEFAULT_SERVICE_PORT = 9093 - def create(self, daemon_spec: CephadmDaemonSpec[AlertManagerSpec]) -> str: + def prepare_create(self, daemon_spec: CephadmDaemonSpec[AlertManagerSpec]) -> CephadmDaemonSpec: assert self.TYPE == daemon_spec.daemon_type assert daemon_spec.spec - return self.mgr._create_daemon(daemon_spec) + return daemon_spec def generate_config(self, daemon_spec: CephadmDaemonSpec[AlertManagerSpec]) -> Tuple[Dict[str, Any], List[str]]: assert self.TYPE == daemon_spec.daemon_type @@ -151,7 +151,8 @@ class AlertmanagerService(CephadmService): def config_dashboard(self, daemon_descrs: List[DaemonDescription]): dd = self.get_active_daemon(daemon_descrs) - service_url = 'http://{}:{}'.format(self._inventory_get_addr(dd.hostname), self.DEFAULT_SERVICE_PORT) + service_url = 'http://{}:{}'.format(self._inventory_get_addr(dd.hostname), + self.DEFAULT_SERVICE_PORT) self._set_service_url_on_dashboard( 'AlertManager', 'dashboard get-alertmanager-api-host', @@ -164,9 +165,9 @@ class PrometheusService(CephadmService): TYPE = 'prometheus' DEFAULT_SERVICE_PORT = 9095 - def create(self, daemon_spec: CephadmDaemonSpec) -> str: + def prepare_create(self, daemon_spec: CephadmDaemonSpec) -> CephadmDaemonSpec: assert self.TYPE == daemon_spec.daemon_type - return self.mgr._create_daemon(daemon_spec) + return daemon_spec def generate_config(self, daemon_spec: CephadmDaemonSpec) -> Tuple[Dict[str, Any], List[str]]: assert self.TYPE == daemon_spec.daemon_type @@ -257,9 +258,9 @@ class PrometheusService(CephadmService): class NodeExporterService(CephadmService): TYPE = 'node-exporter' - def create(self, daemon_spec: CephadmDaemonSpec) -> str: + def prepare_create(self, daemon_spec: CephadmDaemonSpec) -> CephadmDaemonSpec: assert self.TYPE == daemon_spec.daemon_type - return self.mgr._create_daemon(daemon_spec) + return daemon_spec def generate_config(self, daemon_spec: CephadmDaemonSpec) -> Tuple[Dict[str, Any], List[str]]: assert self.TYPE == daemon_spec.daemon_type diff --git a/src/pybind/mgr/cephadm/services/nfs.py b/src/pybind/mgr/cephadm/services/nfs.py index d3220ee4ab5c9..dce15aa9d1021 100644 --- a/src/pybind/mgr/cephadm/services/nfs.py +++ b/src/pybind/mgr/cephadm/services/nfs.py @@ -26,7 +26,7 @@ class NFSService(CephService): spec.service_name(), spec.placement.pretty_str())) self.mgr.spec_store.save(spec) - def create(self, daemon_spec: CephadmDaemonSpec[NFSServiceSpec]) -> str: + def prepare_create(self, daemon_spec: CephadmDaemonSpec[NFSServiceSpec]) -> CephadmDaemonSpec: assert self.TYPE == daemon_spec.daemon_type assert daemon_spec.spec @@ -36,7 +36,7 @@ class NFSService(CephService): logger.info('Create daemon %s on host %s with spec %s' % ( daemon_id, host, spec)) - return self.mgr._create_daemon(daemon_spec) + return daemon_spec def generate_config(self, daemon_spec: CephadmDaemonSpec[NFSServiceSpec]) -> Tuple[Dict[str, Any], List[str]]: assert self.TYPE == daemon_spec.daemon_type -- 2.39.5