From 036fa91cba8d1db95bafc9077f68b24f2b721f6d Mon Sep 17 00:00:00 2001 From: Sebastian Wagner Date: Fri, 14 Feb 2020 15:37:50 +0100 Subject: [PATCH] mgr/cephadm: mon: Don't show traceback for user errors E.g. when complaining about a missing netowork. Signed-off-by: Sebastian Wagner --- src/pybind/mgr/cephadm/module.py | 10 +++---- src/pybind/mgr/cephadm/tests/test_cephadm.py | 5 ++++ src/pybind/mgr/orchestrator/__init__.py | 1 + src/pybind/mgr/orchestrator/_interface.py | 29 ++++++++++++++++---- src/pybind/mgr/tests/test_orchestrator.py | 5 ++-- 5 files changed, 37 insertions(+), 13 deletions(-) diff --git a/src/pybind/mgr/cephadm/module.py b/src/pybind/mgr/cephadm/module.py index ca00adbeeb8d1..fcf5ff10d0cb5 100644 --- a/src/pybind/mgr/cephadm/module.py +++ b/src/pybind/mgr/cephadm/module.py @@ -1869,13 +1869,12 @@ class CephadmOrchestrator(MgrModule, orchestrator.OrchestratorClientMixin): keyring=keyring, extra_config=extra_config) + @async_completion def add_mon(self, spec): # type: (orchestrator.ServiceSpec) -> orchestrator.Completion # current support requires a network to be specified - for host, network, _ in spec.placement.hosts: - if not network: - raise RuntimeError("Host '{}' is missing a network spec".format(host)) + orchestrator.servicespec_validate_hosts_have_network_spec(spec) def add_mons(daemons): for _, _, name in spec.placement.hosts: @@ -1894,6 +1893,7 @@ class CephadmOrchestrator(MgrModule, orchestrator.OrchestratorClientMixin): return self._get_daemons('mon').then(add_mons) + @async_completion def apply_mon(self, spec): # type: (orchestrator.ServiceSpec) -> orchestrator.Completion """ @@ -1924,9 +1924,7 @@ class CephadmOrchestrator(MgrModule, orchestrator.OrchestratorClientMixin): [self._require_hosts(host.hostname) for host in spec.placement.hosts] # current support requires a network to be specified - for host, network, _ in spec.placement.hosts: - if not network: - raise RuntimeError("Host '{}' is missing a network spec".format(host)) + orchestrator.servicespec_validate_hosts_have_network_spec(spec) def update_mons_with_daemons(daemons): for _, _, name in spec.placement.hosts: diff --git a/src/pybind/mgr/cephadm/tests/test_cephadm.py b/src/pybind/mgr/cephadm/tests/test_cephadm.py index cf9b1bfe3c23d..e702b5ce38d6a 100644 --- a/src/pybind/mgr/cephadm/tests/test_cephadm.py +++ b/src/pybind/mgr/cephadm/tests/test_cephadm.py @@ -113,6 +113,11 @@ class TestCephadm(object): c = cephadm_module.apply_mon(ServiceSpec(placement=ps)) assert wait(cephadm_module, c) == ["Deployed mon.a on host 'test'"] + with pytest.raises(OrchestratorError, match="is missing a network spec"): + ps = PlacementSpec(hosts=['test'], count=1) + c = cephadm_module.apply_mon(ServiceSpec(placement=ps)) + wait(cephadm_module, c) + @mock.patch("cephadm.module.CephadmOrchestrator._run_cephadm", _run_cephadm('[]')) @mock.patch("cephadm.module.CephadmOrchestrator.send_command") @mock.patch("cephadm.module.CephadmOrchestrator.mon_command", mon_command) diff --git a/src/pybind/mgr/orchestrator/__init__.py b/src/pybind/mgr/orchestrator/__init__.py index 857d4b5ffc0c3..61cf50e0ff6bf 100644 --- a/src/pybind/mgr/orchestrator/__init__.py +++ b/src/pybind/mgr/orchestrator/__init__.py @@ -9,6 +9,7 @@ from ._interface import \ Orchestrator, OrchestratorClientMixin, \ OrchestratorValidationError, OrchestratorError, NoOrchestrator, \ ServiceSpec, NFSServiceSpec, RGWSpec, HostPlacementSpec, \ + servicespec_validate_add, servicespec_validate_hosts_have_network_spec, \ ServiceDescription, InventoryFilter, PlacementSpec, HostSpec, \ DaemonDescription, \ InventoryNode, DeviceLightLoc, \ diff --git a/src/pybind/mgr/orchestrator/_interface.py b/src/pybind/mgr/orchestrator/_interface.py index e6915b9c0a349..8863c62e865af 100644 --- a/src/pybind/mgr/orchestrator/_interface.py +++ b/src/pybind/mgr/orchestrator/_interface.py @@ -1366,9 +1366,28 @@ class ServiceSpec(object): else: self.count = 1 - def validate_add(self): - if not self.name: - raise OrchestratorValidationError('Cannot add Service: Name required') + +def servicespec_validate_add(self: ServiceSpec): + # This must not be a method of ServiceSpec, otherwise you'll hunt + # sub-interpreter affinity bugs. + if not self.name: + raise OrchestratorValidationError('Cannot add Service: Name required') + + +def servicespec_validate_hosts_have_network_spec(self: ServiceSpec): + # This must not be a method of ServiceSpec, otherwise you'll hunt + # sub-interpreter affinity bugs. + if not self.placement.hosts: + raise OrchestratorValidationError('Service specification: no hosts provided') + + for host, network, _ in self.placement.hosts: + if not network: + m = "Host '{host}' is missing a network spec\nE.g. {host}:1.2.3.0/24".format( + host=host) + logger.error( + f'validate_hosts_have_network_spec: id(OrchestratorValidationError)={id(OrchestratorValidationError)}') + raise OrchestratorValidationError(m) + class NFSServiceSpec(ServiceSpec): @@ -1382,8 +1401,8 @@ class NFSServiceSpec(ServiceSpec): self.namespace = namespace def validate_add(self): - super(NFSServiceSpec, self).validate_add() - + servicespec_validate_add(self) + if not self.pool: raise OrchestratorValidationError('Cannot add NFS: No Pool specified') diff --git a/src/pybind/mgr/tests/test_orchestrator.py b/src/pybind/mgr/tests/test_orchestrator.py index acd1b567dc27f..532fdd5b8bd70 100644 --- a/src/pybind/mgr/tests/test_orchestrator.py +++ b/src/pybind/mgr/tests/test_orchestrator.py @@ -6,7 +6,8 @@ from tests import mock import pytest from ceph.deployment import inventory -from orchestrator import raise_if_exception, RGWSpec, Completion, ProgressReference +from orchestrator import raise_if_exception, RGWSpec, Completion, ProgressReference, \ + servicespec_validate_add from orchestrator import InventoryNode, ServiceDescription from orchestrator import OrchestratorValidationError from orchestrator import HostPlacementSpec @@ -110,7 +111,7 @@ def test_rgwspec(): """ example = json.loads(test_rgwspec.__doc__.strip()) spec = RGWSpec.from_json(example) - assert spec.validate_add() is None + assert servicespec_validate_add(spec) is None def test_promise(): -- 2.39.5