python-common: Make YAML or OSD Specs readable

* Changes: An empty OSD Spec is now invalid.
* OSDSpec.validate() now fails, if service-id is empty

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>

diff --git a/src/python-common/ceph/deployment/drive_group.py b/src/python-common/ceph/deployment/drive_group.py
index 43ae10546ea41a3a940cb1cbf66623c473a2415c..96f93fb8db62a602f3de4443ff84dd74922823e0 100644 (file)
--- a/src/python-common/ceph/deployment/drive_group.py
+++ b/src/python-common/ceph/deployment/drive_group.py
@@ -1,3 +1,5 @@
+import yaml
+
 from ceph.deployment.inventory import Device
 from ceph.deployment.service_spec import ServiceSpecValidationError, ServiceSpec, PlacementSpec
 
@@ -45,7 +47,7 @@ class DeviceSelection(object):
         #: Size specification of format LOW:HIGH.
         #: Can also take the the form :HIGH, LOW:
         #: or an exact value (as ceph-volume inventory reports)
-        self.size = size
+        self.size:  Optional[str] = size
 
         #: is the drive rotating or not
         self.rotational = rotational
@@ -76,7 +78,7 @@ class DeviceSelection(object):
 
     @classmethod
     def from_json(cls, device_spec):
-        # type: (dict) -> DeviceSelection
+        # type: (dict) -> Optional[DeviceSelection]
         if not device_spec:
             return  # type: ignore
         for applied_filter in list(device_spec.keys()):
@@ -88,10 +90,23 @@ class DeviceSelection(object):
 
     def to_json(self):
         # type: () -> Dict[str, Any]
-        c = self.__dict__.copy()
+        ret: Dict[str, Any] = {}
         if self.paths:
-            c['paths'] = [p.path for p in self.paths]
-        return c
+            ret['paths'] = [p.path for p in self.paths]
+        if self.model:
+            ret['model'] = self.model
+        if self.vendor:
+            ret['vendor'] = self.vendor
+        if self.size:
+            ret['size'] = self.size
+        if self.rotational:
+            ret['rotational'] = self.rotational
+        if self.limit:
+            ret['limit'] = self.limit
+        if self.all:
+            ret['all'] = self.all
+
+        return ret
 
     def __repr__(self):
         keys = [
@@ -209,11 +224,32 @@ class DriveGroupSpec(ServiceSpec):
         :param json_drive_group: A valid json string with a Drive Group
                specification
         """
+        args = {}
         # legacy json (pre Octopus)
         if 'host_pattern' in json_drive_group and 'placement' not in json_drive_group:
             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:
+            raise DriveGroupValidationError('OSD spec needs a `placement` key.')
+
+        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')))
+        else:
+            args.update(cls._drive_group_spec_from_json(json_drive_group))
+
+        return cls(**args)
+
+    @classmethod
+    def _drive_group_spec_from_json(cls, json_drive_group: dict) -> dict:
         for applied_filter in list(json_drive_group.keys()):
             if applied_filter not in cls._supported_features:
                 raise DriveGroupValidationError(
@@ -225,15 +261,12 @@ class DriveGroupSpec(ServiceSpec):
                     from ceph.deployment.drive_selection import SizeMatcher
                     json_drive_group[key] = SizeMatcher.str_to_byte(json_drive_group[key])
 
-        if 'placement' in json_drive_group:
-            json_drive_group['placement'] = PlacementSpec.from_json(json_drive_group['placement'])
-
         try:
             args = {k: (DeviceSelection.from_json(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")
-            return DriveGroupSpec(**args)
+            return args
         except (KeyError, TypeError) as e:
             raise DriveGroupValidationError(str(e))
 
@@ -241,6 +274,9 @@ class DriveGroupSpec(ServiceSpec):
         # type: () -> None
         super(DriveGroupSpec, self).validate()
 
+        if not self.service_id:
+            raise DriveGroupValidationError('service_id is required')
+
         if not isinstance(self.placement.host_pattern, six.string_types) and \
                 self.placement.host_pattern is not None:
             raise DriveGroupValidationError('host_pattern must be of type string')
@@ -273,19 +309,8 @@ class DriveGroupSpec(ServiceSpec):
             ', '.join('{}={}'.format(key, repr(getattr(self, key))) for key in keys)
         )
 
-    def to_json(self):
-        # type: () -> Dict[str, Any]
-        c = self.__dict__.copy()
-        if self.placement:
-            c['placement'] = self.placement.to_json()
-        if self.data_devices:
-            c['data_devices'] = self.data_devices.to_json()
-        if self.db_devices:
-            c['db_devices'] = self.db_devices.to_json()
-        if self.wal_devices:
-            c['wal_devices'] = self.wal_devices.to_json()
-        c['service_name'] = self.service_name()
-        return c
-
     def __eq__(self, other):
         return repr(self) == repr(other)
+
+
+yaml.add_representer(DriveGroupSpec, DriveGroupSpec.yaml_representer)

diff --git a/src/python-common/ceph/tests/test_disk_selector.py b/src/python-common/ceph/tests/test_disk_selector.py
index b60e0e30f03f3b2ccec08756d6d59b994340228a..6931544d45313071b72a018cbd6e305bb20ae541 100644 (file)
--- a/src/python-common/ceph/tests/test_disk_selector.py
+++ b/src/python-common/ceph/tests/test_disk_selector.py
@@ -325,7 +325,10 @@ class TestDriveGroup(object):
             if empty:
                 raw_sample = {
                     'service_type': 'osd',
-                    'placement': {'host_pattern': 'data*'}
+                    'placement': {'host_pattern': 'data*'},
+                    'data_devices': {
+                        'all': True
+                    },
                 }
 
             dgo = DriveGroupSpec.from_json(raw_sample)
@@ -384,7 +387,7 @@ class TestDriveGroup(object):
 
     def test_data_devices_prop_empty(self, test_fix):
         test_fix = test_fix(empty=True)
-        assert test_fix.data_devices is None
+        assert test_fix.db_devices is None
 
     def test_db_devices_prop(self, test_fix):
         test_fix = test_fix()

diff --git a/src/python-common/ceph/tests/test_service_spec.py b/src/python-common/ceph/tests/test_service_spec.py
index 6aa26adc4239b8a668d45d175b830d5683a26b17..7d12ff5d358203bef021074e28e78ff459dc5560 100644 (file)
--- a/src/python-common/ceph/tests/test_service_spec.py
+++ b/src/python-common/ceph/tests/test_service_spec.py
@@ -99,6 +99,12 @@ def test_servicespec_map_test(s_type, o_spec, s_id):
         dict_spec['pool'] = 'pool'
         dict_spec['api_user'] = 'api_user'
         dict_spec['api_password'] = 'api_password'
+    elif s_type == 'osd':
+        dict_spec['spec'] = {
+            'data_devices': {
+                'all': True
+            }
+        }
     spec = ServiceSpec.from_json(dict_spec)
     assert isinstance(spec, o_spec)
     assert isinstance(spec.placement, PlacementSpec)