diff --git a/src/python-common/ceph/deployment/drive_group.py b/src/python-common/ceph/deployment/drive_group.py index ef69d69c0c1..6ba6346e255 100644 --- a/src/python-common/ceph/deployment/drive_group.py +++ b/src/python-common/ceph/deployment/drive_group.py @@ -65,32 +65,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) @@ -134,8 +132,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): @@ -254,81 +253,107 @@ class DriveGroupSpec(ServiceSpec): json_drive_group['placement'] = {'host_pattern': json_drive_group['host_pattern']} del json_drive_group['host_pattern'] + 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() - 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', '') - # 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 or ') + raise DriveGroupValidationError( + self.service_id, + 'filter_logic must be either or ') if self.method not in [None, 'lvm', 'raw']: - raise DriveGroupValidationError('method must be one of None, lvm, raw') + raise DriveGroupValidationError( + self.service_id, + 'method must be one of None, lvm, raw') if self.method == 'raw' and self.objectstore == 'filestore': - raise DriveGroupValidationError('method raw only supports bluestore') + raise DriveGroupValidationError( + self.service_id, + 'method raw only supports bluestore') 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 282f35a7cf9..84db12d934a 100644 --- 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 or ', """ + 'Failed to validate OSD spec "mydg": filter_logic must be either 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():