mgr/cephadm: moving Prometheus spec check to service_spec module

Fixes: https://tracker.ceph.com/issues/57894
Signed-off-by: Redouane Kachach <rkachach@redhat.com>

diff --git a/src/pybind/mgr/cephadm/module.py b/src/pybind/mgr/cephadm/module.py
index 251eb55e537ff681b9ed1175f0127ed1e31006ee..94eecc9fef77484fbaa3bb9ee482dae264dd7c34 100644 (file)
--- 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 fd36ed6c561ac80a48134fc9cf18afed88d8e22f..e6fb4e5c2ca644416877b1706dd0b868f1b4a7d6 100644 (file)
--- 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 36fcd86fc2d2b1e3667cb47858223becc6ece1a2..dc186330c38c7d25a1a0d738cf759cd48d1bdc51 100644 (file)
--- 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 e04a513c60feb2309423a6d92eff778660c92bda..6d3ae905ef3f9534aa74291ddc359efcd94630da 100644 (file)
--- 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",