From: Michael Fritch <mfritch@suse.com>
Date: Tue, 30 Jun 2020 22:06:20 +0000 (-0600)
Subject: python-common: clean-up ServiceSpec.service_id handling
X-Git-Tag: wip-pdonnell-testing-20200918.022351~575^2~1
X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=eecc8fcbcc2ee88e4c873579c69f2012dac8c652;p=ceph-ci.git

python-common: clean-up ServiceSpec.service_id handling

service_id is required for iscsi, mds, nfs, osd, rgw.

any other service_type (mon, mgr, etc.) should not contain a service_id

Fixes: https://tracker.ceph.com/issues/46175
Signed-off-by: Michael Fritch <mfritch@suse.com>
---

diff --git a/src/pybind/mgr/orchestrator/_interface.py b/src/pybind/mgr/orchestrator/_interface.py
index 29ac9c8431d..586662a508f 100644
--- a/src/pybind/mgr/orchestrator/_interface.py
+++ b/src/pybind/mgr/orchestrator/_interface.py
@@ -1333,13 +1333,13 @@ class DaemonDescription(object):
             # daemon_id == "service_id"
             return self.daemon_id
 
-        if self.daemon_type in ['mds', 'nfs', 'iscsi', 'rgw']:
+        if self.daemon_type in ServiceSpec.REQUIRES_SERVICE_ID:
             return _match()
 
         return self.daemon_id
 
     def service_name(self):
-        if self.daemon_type in ['rgw', 'mds', 'nfs', 'iscsi']:
+        if self.daemon_type in ServiceSpec.REQUIRES_SERVICE_ID:
             return f'{self.daemon_type}.{self.service_id()}'
         return self.daemon_type
 
diff --git a/src/python-common/ceph/deployment/service_spec.py b/src/python-common/ceph/deployment/service_spec.py
index 2e887c67a68..a677ff419a0 100644
--- a/src/python-common/ceph/deployment/service_spec.py
+++ b/src/python-common/ceph/deployment/service_spec.py
@@ -372,6 +372,7 @@ class ServiceSpec(object):
     """
     KNOWN_SERVICE_TYPES = 'alertmanager crash grafana iscsi mds mgr mon nfs ' \
                           'node-exporter osd prometheus rbd-mirror rgw'.split()
+    REQUIRES_SERVICE_ID = 'iscsi mds nfs osd rgw'.split()
 
     @classmethod
     def _cls(cls, service_type):
@@ -414,7 +415,9 @@ class ServiceSpec(object):
 
         assert service_type in ServiceSpec.KNOWN_SERVICE_TYPES, service_type
         self.service_type = service_type
-        self.service_id = service_id
+        self.service_id = None
+        if self.service_type in self.REQUIRES_SERVICE_ID:
+            self.service_id = service_id
         self.unmanaged = unmanaged
         self.preview_only = preview_only
 
@@ -527,8 +530,12 @@ class ServiceSpec(object):
         if not self.service_type:
             raise ServiceSpecValidationError('Cannot add Service: type required')
 
-        if self.service_type in ['mds', 'rgw', 'nfs', 'iscsi'] and not self.service_id:
-            raise ServiceSpecValidationError('Cannot add Service: id required')
+        if self.service_type in self.REQUIRES_SERVICE_ID:
+            if not self.service_id:
+                raise ServiceSpecValidationError('Cannot add Service: id required')
+        elif self.service_id:
+            raise ServiceSpecValidationError(
+                    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_service_spec.py b/src/python-common/ceph/tests/test_service_spec.py
index f91e55af618..2d923c6c7d2 100644
--- a/src/python-common/ceph/tests/test_service_spec.py
+++ b/src/python-common/ceph/tests/test_service_spec.py
@@ -75,18 +75,7 @@ def test_parse_host_placement_specs_raises_wrong_format(test_input):
         HostPlacementSpec.parse(test_input)
 
 
-@pytest.mark.parametrize(
-    "s_type,o_spec,s_id",
-    [
-        ("mgr", ServiceSpec, 'test'),
-        ("mon", ServiceSpec, 'test'),
-        ("mds", ServiceSpec, 'test'),
-        ("rgw", RGWSpec, 'realm.zone'),
-        ("nfs", NFSServiceSpec, 'test'),
-        ("iscsi", IscsiServiceSpec, 'test'),
-        ("osd", DriveGroupSpec, 'test'),
-    ])
-def test_servicespec_map_test(s_type, o_spec, s_id):
+def _get_dict_spec(s_type, s_id):
     dict_spec = {
         "service_id": s_id,
         "service_type": s_type,
@@ -105,7 +94,26 @@ def test_servicespec_map_test(s_type, o_spec, s_id):
                 'all': True
             }
         }
-    spec = ServiceSpec.from_json(dict_spec)
+    elif s_type == 'rgw':
+        dict_spec['rgw_realm'] = 'realm'
+        dict_spec['rgw_zone'] = 'zone'
+
+    return dict_spec
+
+
+@pytest.mark.parametrize(
+    "s_type,o_spec,s_id",
+    [
+        ("mgr", ServiceSpec, 'test'),
+        ("mon", ServiceSpec, 'test'),
+        ("mds", ServiceSpec, 'test'),
+        ("rgw", RGWSpec, 'realm.zone'),
+        ("nfs", NFSServiceSpec, 'test'),
+        ("iscsi", IscsiServiceSpec, 'test'),
+        ("osd", DriveGroupSpec, 'test'),
+    ])
+def test_servicespec_map_test(s_type, o_spec, s_id):
+    spec = ServiceSpec.from_json(_get_dict_spec(s_type, s_id))
     assert isinstance(spec, o_spec)
     assert isinstance(spec.placement, PlacementSpec)
     assert isinstance(spec.placement.hosts[0], HostPlacementSpec)
@@ -183,7 +191,7 @@ spec:
                                          service_type='mon',
                                          service_id='foo'
                                      ),
-                                     False
+                                     True
                              ),
                              # Add service_type='mgr'
                              (
@@ -225,3 +233,19 @@ def test_spec_hash_eq(spec1: ServiceSpec,
                       eq: bool):
 
     assert (spec1 == spec2) is eq
+
+@pytest.mark.parametrize(
+    "s_type,s_id,s_name",
+    [
+        ('mgr', 's_id', 'mgr'),
+        ('mon', 's_id', 'mon'),
+        ('mds', 's_id', 'mds.s_id'),
+        ('rgw', 's_id', 'rgw.s_id'),
+        ('nfs', 's_id', 'nfs.s_id'),
+        ('iscsi', 's_id', 'iscsi.s_id'),
+        ('osd', 's_id', 'osd.s_id'),
+    ])
+def test_service_name(s_type, s_id, s_name):
+    spec = ServiceSpec.from_json(_get_dict_spec(s_type, s_id))
+    spec.validate()
+    assert spec.service_name() == s_name