python-common: OSD specs: Improve quality of error messages

Fixes: https://tracker.ceph.com/issues/47401
Signed-off-by: Sebastian Wagner <sewagner@redhat.com>
(cherry picked from commit 4142c52d7406bb67042d9ad7b26d8e84f5a734ba)

Conflicts:
	src/python-common/ceph/deployment/drive_group.py

diff --git a/src/python-common/ceph/deployment/drive_group.py b/src/python-common/ceph/deployment/drive_group.py
index 0785ffff32d7560d65499b37174cf2256402d0af..0844768c67427f1d7cc2e56e22d4158c69493314 100644 (file)
--- a/src/python-common/ceph/deployment/drive_group.py
+++ b/src/python-common/ceph/deployment/drive_group.py
@@ -59,32 +59,30 @@ class DeviceSelection(object):
         #: Matches all devices. Can only be used for data devices
         self.all = all
 
-        self.validate()
-
-    def validate(self):
-        # type: () -> None
+    def validate(self, name: str) -> None:
         props = [self.model, self.vendor, self.size, self.rotational]  # type: List[Any]
         if self.paths and any(p is not None for p in props):
             raise DriveGroupValidationError(
-                'DeviceSelection: `paths` and other parameters are mutually exclusive')
+                name,
+                'device selection: `paths` and other parameters are mutually exclusive')
         is_empty = not any(p is not None and p != [] for p in [self.paths] + props)
         if not self.all and is_empty:
-            raise DriveGroupValidationError('DeviceSelection cannot be empty')
+            raise DriveGroupValidationError(name, 'device selection cannot be empty')
 
         if self.all and not is_empty:
             raise DriveGroupValidationError(
-                'DeviceSelection `all` and other parameters are mutually exclusive. {}'.format(
+                name,
+                'device selection: `all` and other parameters are mutually exclusive. {}'.format(
                     repr(self)))
 
     @classmethod
     def from_json(cls, device_spec):
         # type: (dict) -> Optional[DeviceSelection]
         if not device_spec:
-            return  # type: ignore
+            return None
         for applied_filter in list(device_spec.keys()):
             if applied_filter not in cls._supported_filters:
-                raise DriveGroupValidationError(
-                    "Filtering for <{}> is not supported".format(applied_filter))
+                raise KeyError(applied_filter)
 
         return cls(**device_spec)
 
@@ -128,8 +126,9 @@ class DriveGroupValidationError(SpecValidationError):
     if it was raised in a different mgr module.
     """
 
-    def __init__(self, msg: str):
-        super(DriveGroupValidationError, self).__init__('Failed to validate Drive Group: ' + msg)
+    def __init__(self, name: str, msg: str):
+        super(DriveGroupValidationError, self).__init__(
+            f'Failed to validate OSD spec "{name}": {msg}')
 
 
 class DriveGroupSpec(ServiceSpec):
@@ -240,76 +239,98 @@ class DriveGroupSpec(ServiceSpec):
             json_drive_group['placement'] = {'host_pattern': json_drive_group['host_pattern']}
             del json_drive_group['host_pattern']
 
-        try:
-            args['placement'] = PlacementSpec.from_json(json_drive_group.pop('placement'))
-        except KeyError:
-            args['placement'] = PlacementSpec()
-
         args['service_type'] = json_drive_group.pop('service_type', 'osd')
 
         # service_id was not required in early octopus.
         args['service_id'] = json_drive_group.pop('service_id', '')
+        s_id = args['service_id']
+        try:
+            args['placement'] = PlacementSpec.from_json(json_drive_group.pop('placement'))
+        except KeyError:
+            args['placement'] = PlacementSpec()
 
         # spec: was not mandatory in octopus
         if 'spec' in json_drive_group:
-            args.update(cls._drive_group_spec_from_json(json_drive_group.pop('spec')))
+            args.update(cls._drive_group_spec_from_json(s_id, json_drive_group.pop('spec')))
         else:
-            args.update(cls._drive_group_spec_from_json(json_drive_group))
+            args.update(cls._drive_group_spec_from_json(s_id, json_drive_group))
 
         args['unmanaged'] = json_drive_group.pop('unmanaged', False)
 
         return cls(**args)
 
     @classmethod
-    def _drive_group_spec_from_json(cls, json_drive_group: dict) -> dict:
+    def _drive_group_spec_from_json(cls, name: str, json_drive_group: dict) -> dict:
         for applied_filter in list(json_drive_group.keys()):
             if applied_filter not in cls._supported_features:
                 raise DriveGroupValidationError(
-                    "Feature <{}> is not supported".format(applied_filter))
+                    name,
+                    "Feature `{}` is not supported".format(applied_filter))
 
         try:
-            args = {k: (DeviceSelection.from_json(v) if k.endswith('_devices') else v) for k, v in
+            def to_selection(key: str, vals: dict) -> Optional[DeviceSelection]:
+                try:
+                    return DeviceSelection.from_json(vals)
+                except KeyError as e:
+                    raise DriveGroupValidationError(
+                        f'{name}.{key}',
+                        f"Filtering for `{e.args[0]}` is not supported")
+
+            args = {k: (to_selection(k, v) if k.endswith('_devices') else v) for k, v in
                     json_drive_group.items()}
             if not args:
-                raise DriveGroupValidationError("Didn't find Drivegroup specs")
+                raise DriveGroupValidationError(name, "Didn't find drive selections")
             return args
         except (KeyError, TypeError) as e:
-            raise DriveGroupValidationError(str(e))
+            raise DriveGroupValidationError(name, str(e))
 
     def validate(self):
         # type: () -> None
         super(DriveGroupSpec, self).validate()
 
         if not self.service_id:
-            raise DriveGroupValidationError('service_id is required')
+            raise DriveGroupValidationError('', 'service_id is required')
 
         if not isinstance(self.placement.host_pattern, str) and \
                 self.placement.host_pattern is not None:
-            raise DriveGroupValidationError('host_pattern must be of type string')
+            raise DriveGroupValidationError(self.service_id, 'host_pattern must be of type string')
 
         if self.data_devices is None:
-            raise DriveGroupValidationError("`data_devices` element is required.")
+            raise DriveGroupValidationError(self.service_id, "`data_devices` element is required.")
 
-        specs = [self.data_devices, self.db_devices, self.wal_devices, self.journal_devices]
-        for s in filter(None, specs):
-            s.validate()
+        specs_names = "data_devices db_devices wal_devices journal_devices".split()
+        specs = dict(zip(specs_names, [getattr(self, k) for k in specs_names]))
+        for k, s in [ks for ks in specs.items() if ks[1] is not None]:
+            assert s is not None
+            s.validate(f'{self.service_id}.{k}')
         for s in filter(None, [self.db_devices, self.wal_devices, self.journal_devices]):
             if s.all:
-                raise DriveGroupValidationError("`all` is only allowed for data_devices")
+                raise DriveGroupValidationError(
+                    self.service_id,
+                    "`all` is only allowed for data_devices")
 
         if self.objectstore not in ('bluestore'):
-            raise DriveGroupValidationError(f"{self.objectstore} is not supported. Must be "
+            raise DriveGroupValidationError(self.service_id,
+                                            f"{self.objectstore} is not supported. Must be "
                                             f"one of ('bluestore')")
 
         if self.block_wal_size is not None and type(self.block_wal_size) not in [int, str]:
-            raise DriveGroupValidationError('block_wal_size must be of type int or string')
+            raise DriveGroupValidationError(
+                self.service_id,
+                'block_wal_size must be of type int or string')
         if self.block_db_size is not None and type(self.block_db_size) not in [int, str]:
-            raise DriveGroupValidationError('block_db_size must be of type int or string')
+            raise DriveGroupValidationError(
+                self.service_id,
+                'block_db_size must be of type int or string')
         if self.journal_size is not None and type(self.journal_size) not in [int, str]:
-            raise DriveGroupValidationError('journal_size must be of type int or string')
+            raise DriveGroupValidationError(
+                self.service_id,
+                'journal_size must be of type int or string')
 
         if self.filter_logic not in ['AND', 'OR']:
-            raise DriveGroupValidationError('filter_logic must be either <AND> or <OR>')
+            raise DriveGroupValidationError(
+                self.service_id,
+                'filter_logic must be either <AND> or <OR>')
 
 
 yaml.add_representer(DriveGroupSpec, DriveGroupSpec.yaml_representer)

diff --git a/src/python-common/ceph/tests/test_drive_group.py b/src/python-common/ceph/tests/test_drive_group.py
index f3896a6e7fc1c386bfa56f70678bd536c0f9ffd8..7e1bbcd0658af629d04a9434029c1072e8b4c389 100644 (file)
--- a/src/python-common/ceph/tests/test_drive_group.py
+++ b/src/python-common/ceph/tests/test_drive_group.py
@@ -39,7 +39,8 @@ def test_DriveGroup(test_input):
         ''
     ),
     (
-        'Failed to validate Drive Group: DeviceSelection cannot be empty', """
+        "Failed to validate Drive Group: OSD spec needs a `placement` key.",
+        'Failed to validate OSD spec "mydg.data_devices": device selection cannot be empty', """
 service_type: osd
 service_id: mydg
 placement:
@@ -49,7 +50,7 @@ data_devices:
 """
     ),
     (
-        'Failed to validate Drive Group: filter_logic must be either <AND> or <OR>', """
+        'Failed to validate OSD spec "mydg": filter_logic must be either <AND> or <OR>', """
 service_type: osd
 service_id: mydg
 placement:
@@ -60,7 +61,7 @@ filter_logic: XOR
 """
     ),
     (
-        'Failed to validate Drive Group: `data_devices` element is required.', """
+        'Failed to validate OSD spec "mydg": `data_devices` element is required.', """
 service_type: osd
 service_id: mydg
 placement:
@@ -68,6 +69,29 @@ placement:
 spec:
   db_devices:
     model: model
+"""
+    ),
+    (
+        'Failed to validate OSD spec "mydg.db_devices": Filtering for `unknown_key` is not supported', """
+service_type: osd
+service_id: mydg
+placement:
+  host_pattern: '*'
+spec:
+  db_devices:
+    unknown_key: 1
+"""
+    ),
+    (
+        'Failed to validate OSD spec "mydg": Feature `unknown_key` is not supported', """
+service_type: osd
+service_id: mydg
+placement:
+  host_pattern: '*'
+spec:
+  db_devices:
+    all: true
+  unknown_key: 1
 """
     ),
 ])
@@ -95,7 +119,8 @@ def test_drive_selection():
     assert spec.data_devices.paths[0].path == '/dev/sda'
 
     with pytest.raises(DriveGroupValidationError, match='exclusive'):
-        DeviceSelection(paths=['/dev/sda'], rotational=False)
+        ds = DeviceSelection(paths=['/dev/sda'], rotational=False)
+        ds.validate('')
 
 
 def test_ceph_volume_command_0():