From: Sebastian Wagner <sewagner@redhat.com>
Date: Wed, 1 Sep 2021 13:36:01 +0000 (+0200)
Subject: pyhton-common: DriveGroupSpec: Allow unnamed OSD specs
X-Git-Tag: v17.1.0~253^2~4
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=8b567e132d75711179febac126c5ec8a250b8952;p=ceph.git

pyhton-common: DriveGroupSpec: Allow unnamed OSD specs

Cause it never actually worked as expected.

Remove duplicated service_id check, cause it's already
verified by parent method.

Fixes: https://tracker.ceph.com/issues/46253

Signed-off-by: Sebastian Wagner <sewagner@redhat.com>
---

diff --git a/src/pybind/mgr/orchestrator/tests/test_orchestrator.py b/src/pybind/mgr/orchestrator/tests/test_orchestrator.py
index 2b5600b9c8dc..5e3f2883b8aa 100644
--- a/src/pybind/mgr/orchestrator/tests/test_orchestrator.py
+++ b/src/pybind/mgr/orchestrator/tests/test_orchestrator.py
@@ -73,11 +73,11 @@ def test_daemon_description():
 def test_apply():
     to = _TestOrchestrator('', 0, 0)
     completion = to.apply([
-        ServiceSpec(service_type='nfs'),
-        ServiceSpec(service_type='nfs'),
-        ServiceSpec(service_type='nfs'),
+        ServiceSpec(service_type='nfs', service_id='foo'),
+        ServiceSpec(service_type='nfs', service_id='foo'),
+        ServiceSpec(service_type='nfs', service_id='foo'),
     ])
-    res = '<NFSServiceSpec for service_name=nfs>'
+    res = '<NFSServiceSpec for service_name=nfs.foo>'
     assert completion.result == [res, res, res]
 
 
diff --git a/src/python-common/ceph/deployment/drive_group.py b/src/python-common/ceph/deployment/drive_group.py
index 6ba6346e255b..09ef17bc59fe 100644
--- a/src/python-common/ceph/deployment/drive_group.py
+++ b/src/python-common/ceph/deployment/drive_group.py
@@ -132,7 +132,8 @@ class DriveGroupValidationError(SpecValidationError):
     if it was raised in a different mgr module.
     """
 
-    def __init__(self, name: str, msg: str):
+    def __init__(self, name: Optional[str], msg: str):
+        name = name or "<unnamed>"
         super(DriveGroupValidationError, self).__init__(
             f'Failed to validate OSD spec "{name}": {msg}')
 
@@ -255,9 +256,7 @@ class DriveGroupSpec(ServiceSpec):
 
         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']
+        s_id = args.get('service_id', '<unnamed>')
         try:
             args['placement'] = PlacementSpec.from_json(json_drive_group.pop('placement'))
         except KeyError:
@@ -302,9 +301,6 @@ 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, str) and \
                 self.placement.host_pattern is not None:
             raise DriveGroupValidationError(self.service_id, 'host_pattern must be of type string')
diff --git a/src/python-common/ceph/deployment/service_spec.py b/src/python-common/ceph/deployment/service_spec.py
index 0cb5c78e659e..caf17f591c31 100644
--- a/src/python-common/ceph/deployment/service_spec.py
+++ b/src/python-common/ceph/deployment/service_spec.py
@@ -415,7 +415,7 @@ class ServiceSpec(object):
     KNOWN_SERVICE_TYPES = 'alertmanager crash grafana iscsi mds mgr mon nfs ' \
                           'node-exporter osd prometheus rbd-mirror rgw agent ' \
                           'container ingress cephfs-mirror'.split()
-    REQUIRES_SERVICE_ID = 'iscsi mds nfs osd rgw container ingress '.split()
+    REQUIRES_SERVICE_ID = 'iscsi mds nfs rgw container ingress '.split()
     MANAGED_CONFIG_OPTIONS = [
         'mds_join_fs',
     ]
@@ -481,7 +481,7 @@ class ServiceSpec(object):
         #: ``container``, ``ingress``
         self.service_id = None
 
-        if self.service_type in self.REQUIRES_SERVICE_ID:
+        if self.service_type in self.REQUIRES_SERVICE_ID or self.service_type == 'osd':
             self.service_id = service_id
 
         #: If set to ``true``, the orchestrator will not deploy nor remove
@@ -641,15 +641,17 @@ class ServiceSpec(object):
         if not self.service_type:
             raise SpecValidationError('Cannot add Service: type required')
 
-        if self.service_type in self.REQUIRES_SERVICE_ID:
-            if not self.service_id:
+        if self.service_type != 'osd':
+            if self.service_type in self.REQUIRES_SERVICE_ID and not self.service_id:
                 raise SpecValidationError('Cannot add Service: id required')
+            if self.service_type not in self.REQUIRES_SERVICE_ID and self.service_id:
+                raise SpecValidationError(
+                        f'Service of type \'{self.service_type}\' should not contain a service id')
+
+        if self.service_id:
             if not re.match('^[a-zA-Z0-9_.-]+$', self.service_id):
                 raise SpecValidationError('Service id contains invalid characters, '
                                           'only [a-zA-Z0-9_.-] allowed')
-        elif self.service_id:
-            raise SpecValidationError(
-                    f'Service of type \'{self.service_type}\' should not contain a service id')
 
         if self.placement is not None:
             self.placement.validate()
diff --git a/src/python-common/ceph/tests/test_drive_group.py b/src/python-common/ceph/tests/test_drive_group.py
index 84db12d934a2..5c11f2f9e5c4 100644
--- a/src/python-common/ceph/tests/test_drive_group.py
+++ b/src/python-common/ceph/tests/test_drive_group.py
@@ -39,7 +39,12 @@ def test_DriveGroup(test_input):
         ''
     ),
     (
-        "Failed to validate Drive Group: OSD spec needs a `placement` key.",
+        'Failed to validate OSD spec "": `placement` key required',
+        """data_devices:
+  all: True
+"""
+    ),
+    (
         'Failed to validate OSD spec "mydg.data_devices": device selection cannot be empty', """
 service_type: osd
 service_id: mydg