From 633e6c3ec76800a22c0986873b704d2f1bd2ee99 Mon Sep 17 00:00:00 2001 From: Redouane Kachach Date: Wed, 19 Oct 2022 12:28:31 +0200 Subject: [PATCH] mgr/cephadm: moving Prometheus spec check to service_spec module Fixes: https://tracker.ceph.com/issues/57894 Signed-off-by: Redouane Kachach --- src/pybind/mgr/cephadm/module.py | 15 +---- src/pybind/mgr/cephadm/tests/test_cephadm.py | 58 ------------------- .../ceph/deployment/service_spec.py | 16 +++++ .../ceph/tests/test_service_spec.py | 56 +++++++++++++++++- 4 files changed, 72 insertions(+), 73 deletions(-) diff --git a/src/pybind/mgr/cephadm/module.py b/src/pybind/mgr/cephadm/module.py index 251eb55e537ff..94eecc9fef774 100644 --- a/src/pybind/mgr/cephadm/module.py +++ b/src/pybind/mgr/cephadm/module.py @@ -28,7 +28,7 @@ from ceph.deployment.drive_group import DriveGroupSpec from ceph.deployment.service_spec import \ ServiceSpec, PlacementSpec, \ HostPlacementSpec, IngressSpec, \ - TunedProfileSpec, PrometheusSpec, IscsiServiceSpec + TunedProfileSpec, IscsiServiceSpec from ceph.utils import str_to_datetime, datetime_to_str, datetime_now from cephadm.serve import CephadmServe from cephadm.services.cephadmservice import CephadmDaemonDeploySpec @@ -2608,19 +2608,6 @@ Then run the following: # should only refresh if a change has been detected self._trigger_preview_refresh(specs=[cast(DriveGroupSpec, spec)]) - if spec.service_type == 'prometheus': - spec = cast(PrometheusSpec, spec) - if spec.retention_time: - valid_units = ['y', 'w', 'd', 'h', 'm', 's'] - m = re.search(rf"^(\d+)({'|'.join(valid_units)})$", spec.retention_time) - if not m: - raise OrchestratorError(f"Invalid retention time. Valid units are: {', '.join(valid_units)}") - if spec.retention_size: - valid_units = ['B', 'KB', 'MB', 'GB', 'TB', 'PB', 'EB'] - m = re.search(rf"^(\d+)({'|'.join(valid_units)})$", spec.retention_size) - if not m: - raise OrchestratorError(f"Invalid retention size. Valid units are: {', '.join(valid_units)}") - return self._apply_service_spec(cast(ServiceSpec, spec)) def _get_candidate_hosts(self, placement: PlacementSpec) -> List[str]: diff --git a/src/pybind/mgr/cephadm/tests/test_cephadm.py b/src/pybind/mgr/cephadm/tests/test_cephadm.py index fd36ed6c561ac..e6fb4e5c2ca64 100644 --- a/src/pybind/mgr/cephadm/tests/test_cephadm.py +++ b/src/pybind/mgr/cephadm/tests/test_cephadm.py @@ -1511,64 +1511,6 @@ class TestCephadm(object): with with_service(cephadm_module, spec, meth, 'test'): pass - @pytest.mark.parametrize( - "spec, raise_exception, msg", - [ - # Valid retention_time values (valid units: 'y', 'w', 'd', 'h', 'm', 's') - (PrometheusSpec(retention_time='1y'), False, ''), - (PrometheusSpec(retention_time=' 10w '), False, ''), - (PrometheusSpec(retention_time=' 1348d'), False, ''), - (PrometheusSpec(retention_time='2000h '), False, ''), - (PrometheusSpec(retention_time='173847m'), False, ''), - (PrometheusSpec(retention_time='200s'), False, ''), - (PrometheusSpec(retention_time=' '), False, ''), # default value will be used - - # Invalid retention_time values - (PrometheusSpec(retention_time='100k'), True, '^Invalid retention time'), # invalid unit - (PrometheusSpec(retention_time='10'), True, '^Invalid retention time'), # no unit - (PrometheusSpec(retention_time='100.00y'), True, '^Invalid retention time'), # invalid value and valid unit - (PrometheusSpec(retention_time='100.00k'), True, '^Invalid retention time'), # invalid value and invalid unit - (PrometheusSpec(retention_time='---'), True, '^Invalid retention time'), # invalid value - - # Valid retention_size values (valid units: 'B', 'KB', 'MB', 'GB', 'TB', 'PB', 'EB') - (PrometheusSpec(retention_size='123456789B'), False, ''), - (PrometheusSpec(retention_size=' 200KB'), False, ''), - (PrometheusSpec(retention_size='99999MB '), False, ''), - (PrometheusSpec(retention_size=' 10GB '), False, ''), - (PrometheusSpec(retention_size='100TB'), False, ''), - (PrometheusSpec(retention_size='500PB'), False, ''), - (PrometheusSpec(retention_size='200EB'), False, ''), - (PrometheusSpec(retention_size=' '), False, ''), # default value will be used - - # Invalid retention_size values - (PrometheusSpec(retention_size='100b'), True, '^Invalid retention size'), # invalid unit (case sensitive) - (PrometheusSpec(retention_size='333kb'), True, '^Invalid retention size'), # invalid unit (case sensitive) - (PrometheusSpec(retention_size='2000'), True, '^Invalid retention size'), # no unit - (PrometheusSpec(retention_size='200.00PB'), True, '^Invalid retention size'), # invalid value and valid unit - (PrometheusSpec(retention_size='400.B'), True, '^Invalid retention size'), # invalid value and invalid unit - (PrometheusSpec(retention_size='10.000s'), True, '^Invalid retention size'), # invalid value and invalid unit - (PrometheusSpec(retention_size='...'), True, '^Invalid retention size'), # invalid value - - # valid retention_size and valid retention_time - (PrometheusSpec(retention_time='1y', retention_size='100GB'), False, ''), - # invalid retention_time and valid retention_size - (PrometheusSpec(retention_time='1j', retention_size='100GB'), True, '^Invalid retention time'), - # valid retention_time and invalid retention_size - (PrometheusSpec(retention_time='1y', retention_size='100gb'), True, '^Invalid retention size'), - # valid retention_time and invalid retention_size - (PrometheusSpec(retention_time='1y', retention_size='100gb'), True, '^Invalid retention size'), - # invalid retention_time and invalid retention_size - (PrometheusSpec(retention_time='1i', retention_size='100gb'), True, '^Invalid retention time'), - ]) - @mock.patch("cephadm.serve.CephadmServe._run_cephadm", _run_cephadm('{}')) - def test_apply_prometheus(self, spec: PrometheusSpec, raise_exception: bool, msg: str, cephadm_module: CephadmOrchestrator): - with with_host(cephadm_module, 'test'): - if not raise_exception: - cephadm_module._apply(spec) - else: - with pytest.raises(OrchestratorError, match=msg): - cephadm_module._apply(spec) - @mock.patch("cephadm.serve.CephadmServe._run_cephadm", _run_cephadm('{}')) def test_mds_config_purge(self, cephadm_module: CephadmOrchestrator): spec = MDSSpec('mds', service_id='fsname', config={'test': 'foo'}) diff --git a/src/python-common/ceph/deployment/service_spec.py b/src/python-common/ceph/deployment/service_spec.py index 36fcd86fc2d2b..dc186330c38c7 100644 --- a/src/python-common/ceph/deployment/service_spec.py +++ b/src/python-common/ceph/deployment/service_spec.py @@ -1313,6 +1313,22 @@ class PrometheusSpec(MonitoringSpec): self.retention_time = retention_time.strip() if retention_time else None self.retention_size = retention_size.strip() if retention_size else None + def validate(self) -> None: + super(PrometheusSpec, self).validate() + + if self.retention_time: + valid_units = ['y', 'w', 'd', 'h', 'm', 's'] + m = re.search(rf"^(\d+)({'|'.join(valid_units)})$", self.retention_time) + if not m: + units = ', '.join(valid_units) + raise SpecValidationError(f"Invalid retention time. Valid units are: {units}") + if self.retention_size: + valid_units = ['B', 'KB', 'MB', 'GB', 'TB', 'PB', 'EB'] + m = re.search(rf"^(\d+)({'|'.join(valid_units)})$", self.retention_size) + if not m: + units = ', '.join(valid_units) + raise SpecValidationError(f"Invalid retention size. Valid units are: {units}") + yaml.add_representer(PrometheusSpec, ServiceSpec.yaml_representer) diff --git a/src/python-common/ceph/tests/test_service_spec.py b/src/python-common/ceph/tests/test_service_spec.py index e04a513c60feb..6d3ae905ef3f9 100644 --- a/src/python-common/ceph/tests/test_service_spec.py +++ b/src/python-common/ceph/tests/test_service_spec.py @@ -8,7 +8,7 @@ import pytest from ceph.deployment.service_spec import HostPlacementSpec, PlacementSpec, \ ServiceSpec, RGWSpec, NFSServiceSpec, IscsiServiceSpec, AlertManagerSpec, \ - CustomContainerSpec, GrafanaSpec + CustomContainerSpec, GrafanaSpec, PrometheusSpec from ceph.deployment.drive_group import DriveGroupSpec from ceph.deployment.hostspec import SpecValidationError @@ -61,6 +61,60 @@ def test_apply_grafana(spec: GrafanaSpec, raise_exception: bool, msg: str): else: spec.validate() +@pytest.mark.parametrize( + "spec, raise_exception, msg", + [ + # Valid retention_time values (valid units: 'y', 'w', 'd', 'h', 'm', 's') + (PrometheusSpec(retention_time='1y'), False, ''), + (PrometheusSpec(retention_time=' 10w '), False, ''), + (PrometheusSpec(retention_time=' 1348d'), False, ''), + (PrometheusSpec(retention_time='2000h '), False, ''), + (PrometheusSpec(retention_time='173847m'), False, ''), + (PrometheusSpec(retention_time='200s'), False, ''), + (PrometheusSpec(retention_time=' '), False, ''), # default value will be used + # Invalid retention_time values + (PrometheusSpec(retention_time='100k'), True, '^Invalid retention time'), # invalid unit + (PrometheusSpec(retention_time='10'), True, '^Invalid retention time'), # no unit + (PrometheusSpec(retention_time='100.00y'), True, '^Invalid retention time'), # invalid value and valid unit + (PrometheusSpec(retention_time='100.00k'), True, '^Invalid retention time'), # invalid value and invalid unit + (PrometheusSpec(retention_time='---'), True, '^Invalid retention time'), # invalid value + + # Valid retention_size values (valid units: 'B', 'KB', 'MB', 'GB', 'TB', 'PB', 'EB') + (PrometheusSpec(retention_size='123456789B'), False, ''), + (PrometheusSpec(retention_size=' 200KB'), False, ''), + (PrometheusSpec(retention_size='99999MB '), False, ''), + (PrometheusSpec(retention_size=' 10GB '), False, ''), + (PrometheusSpec(retention_size='100TB'), False, ''), + (PrometheusSpec(retention_size='500PB'), False, ''), + (PrometheusSpec(retention_size='200EB'), False, ''), + (PrometheusSpec(retention_size=' '), False, ''), # default value will be used + + # Invalid retention_size values + (PrometheusSpec(retention_size='100b'), True, '^Invalid retention size'), # invalid unit (case sensitive) + (PrometheusSpec(retention_size='333kb'), True, '^Invalid retention size'), # invalid unit (case sensitive) + (PrometheusSpec(retention_size='2000'), True, '^Invalid retention size'), # no unit + (PrometheusSpec(retention_size='200.00PB'), True, '^Invalid retention size'), # invalid value and valid unit + (PrometheusSpec(retention_size='400.B'), True, '^Invalid retention size'), # invalid value and invalid unit + (PrometheusSpec(retention_size='10.000s'), True, '^Invalid retention size'), # invalid value and invalid unit + (PrometheusSpec(retention_size='...'), True, '^Invalid retention size'), # invalid value + + # valid retention_size and valid retention_time + (PrometheusSpec(retention_time='1y', retention_size='100GB'), False, ''), + # invalid retention_time and valid retention_size + (PrometheusSpec(retention_time='1j', retention_size='100GB'), True, '^Invalid retention time'), + # valid retention_time and invalid retention_size + (PrometheusSpec(retention_time='1y', retention_size='100gb'), True, '^Invalid retention size'), + # valid retention_time and invalid retention_size + (PrometheusSpec(retention_time='1y', retention_size='100gb'), True, '^Invalid retention size'), + # invalid retention_time and invalid retention_size + (PrometheusSpec(retention_time='1i', retention_size='100gb'), True, '^Invalid retention time'), + ]) +def test_apply_prometheus(spec: PrometheusSpec, raise_exception: bool, msg: str): + if raise_exception: + with pytest.raises(SpecValidationError, match=msg): + spec.validate() + else: + spec.validate() @pytest.mark.parametrize( "test_input,expected", -- 2.39.5