From: Sage Weil <sage@redhat.com>
Date: Sun, 1 Mar 2020 13:48:17 +0000 (-0600)
Subject: mgr/orch: fix ServiceSpec fields
X-Git-Tag: v15.1.1~191^2~3
X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=bfb9b4e8c416543a0f08174437100c9a658848cb;p=ceph-ci.git

mgr/orch: fix ServiceSpec fields

- service_type is required.  Make it the first position arg to the ctor.
- service_id is the id *only* and optional.
- service_name() is the full service name (no change)

The old 'name' was previously used as the id only, so it was poorly named,
and optional, but in this series was changed to include the type, breaking
naming for a bunch of things (e.g., daemons called mds.mds.fsname.xyz).

Signed-off-by: Sage Weil <sage@redhat.com>
---

diff --git a/src/pybind/mgr/cephadm/module.py b/src/pybind/mgr/cephadm/module.py
index 1f7f3cdc050..e87d855d368 100644
--- a/src/pybind/mgr/cephadm/module.py
+++ b/src/pybind/mgr/cephadm/module.py
@@ -2134,9 +2134,7 @@ class CephadmOrchestrator(orchestrator.Orchestrator, MgrModule):
             raise OrchestratorError('must specify host(s) to deploy on')
         if not spec.placement.count:
             spec.placement.count = len(spec.placement.hosts)
-        # TODO: rename service_name to spec.name if works
-        service_name = spec.name
-        daemons = self.cache.get_daemons_by_service(service_name)
+        daemons = self.cache.get_daemons_by_service(spec.service_name())
         return self._create_daemons(daemon_type, spec, daemons,
                                     create_func, config_func)
 
@@ -2152,7 +2150,7 @@ class CephadmOrchestrator(orchestrator.Orchestrator, MgrModule):
         args = [] # type: ignore
         for host, network, name in spec.placement.hosts:
             daemon_id = self.get_unique_name(daemon_type, host, daemons,
-                                             spec.name, name)
+                                             spec.service_id, name)
             self.log.debug('Placing %s.%s on host %s' % (
                 daemon_type, daemon_id, host))
             if daemon_type == 'mon':
@@ -2239,12 +2237,12 @@ class CephadmOrchestrator(orchestrator.Orchestrator, MgrModule):
 
     def _config_mds(self, spec):
         # ensure mds_join_fs is set for these daemons
-        assert spec.name
+        assert spec.service_id
         ret, out, err = self.mon_command({
             'prefix': 'config set',
-            'who': 'mds.' + spec.name,
+            'who': 'mds.' + spec.service_id,
             'name': 'mds_join_fs',
-            'value': spec.name,
+            'value': spec.service_id,
         })
 
     @async_map_completion
@@ -2266,7 +2264,7 @@ class CephadmOrchestrator(orchestrator.Orchestrator, MgrModule):
         # ensure rgw_realm and rgw_zone is set for these daemons
         ret, out, err = self.mon_command({
             'prefix': 'config set',
-            'who': 'client.rgw.' + spec.name,
+            'who': 'client.rgw.' + spec.service_id,
             'name': 'rgw_zone',
             'value': spec.rgw_zone,
         })
diff --git a/src/pybind/mgr/cephadm/tests/test_cephadm.py b/src/pybind/mgr/cephadm/tests/test_cephadm.py
index ec92ee72de8..c1a43e5d030 100644
--- a/src/pybind/mgr/cephadm/tests/test_cephadm.py
+++ b/src/pybind/mgr/cephadm/tests/test_cephadm.py
@@ -137,12 +137,12 @@ class TestCephadm(object):
     def test_mon_update(self, _send_command, _get_connection, _save_host, _rm_host, cephadm_module):
         with self._with_host(cephadm_module, 'test'):
             ps = PlacementSpec(hosts=['test:0.0.0.0=a'], count=1)
-            c = cephadm_module.add_mon(ServiceSpec(placement=ps, service_type='mon'))
+            c = cephadm_module.add_mon(ServiceSpec('mon', placement=ps))
             assert wait(cephadm_module, c) == ["Deployed mon.a on host 'test'"]
 
             with pytest.raises(OrchestratorError, match="is missing a network spec"):
                 ps = PlacementSpec(hosts=['test'], count=1)
-                c = cephadm_module.add_mon(ServiceSpec(placement=ps, service_type='mon'))
+                c = cephadm_module.add_mon(ServiceSpec('mon', placement=ps))
                 wait(cephadm_module, c)
 
     @mock.patch("cephadm.module.CephadmOrchestrator._run_cephadm", _run_cephadm('[]'))
@@ -154,7 +154,7 @@ class TestCephadm(object):
     def test_mgr_update(self, _send_command, _get_connection, _save_host, _rm_host, cephadm_module):
         with self._with_host(cephadm_module, 'test'):
             ps = PlacementSpec(hosts=['test:0.0.0.0=a'], count=1)
-            c = cephadm_module._apply_service(ServiceSpec(placement=ps, service_type='mgr'))
+            c = cephadm_module._apply_service(ServiceSpec('mgr', placement=ps))
             [out] = wait(cephadm_module, c)
             match_glob(out, "Deployed mgr.* on host 'test'")
 
@@ -202,7 +202,7 @@ class TestCephadm(object):
     def test_mds(self, _send_command, _get_connection, _save_host, _rm_host, cephadm_module):
         with self._with_host(cephadm_module, 'test'):
             ps = PlacementSpec(hosts=['test'], count=1)
-            c = cephadm_module.add_mds(ServiceSpec(name='name', placement=ps, service_type='mds'))
+            c = cephadm_module.add_mds(ServiceSpec('mds', 'name', placement=ps))
             [out] = wait(cephadm_module, c)
             match_glob(out, "Deployed mds.name.* on host 'test'")
 
@@ -216,7 +216,7 @@ class TestCephadm(object):
 
         with self._with_host(cephadm_module, 'test'):
             ps = PlacementSpec(hosts=['test'], count=1)
-            c = cephadm_module.add_rgw(RGWSpec('realm', 'zone', placement=ps, service_type='rgw'))
+            c = cephadm_module.add_rgw(RGWSpec('realm', 'zone', placement=ps))
             [out] = wait(cephadm_module, c)
             match_glob(out, "Deployed rgw.realm.zone.* on host 'test'")
 
@@ -232,12 +232,12 @@ class TestCephadm(object):
         with self._with_host(cephadm_module, 'host1'):
             with self._with_host(cephadm_module, 'host2'):
                 ps = PlacementSpec(hosts=['host1'], count=1)
-                c = cephadm_module.add_rgw(RGWSpec('realm', 'zone1', placement=ps, service_type='rgw'))
+                c = cephadm_module.add_rgw(RGWSpec('realm', 'zone1', placement=ps))
                 [out] = wait(cephadm_module, c)
                 match_glob(out, "Deployed rgw.realm.zone1.host1.* on host 'host1'")
 
                 ps = PlacementSpec(hosts=['host1', 'host2'], count=2)
-                c = cephadm_module._apply_service(RGWSpec('realm', 'zone1', placement=ps, service_type='rgw'))
+                c = cephadm_module._apply_service(RGWSpec('realm', 'zone1', placement=ps))
                 [out] = wait(cephadm_module, c)
                 match_glob(out, "Deployed rgw.realm.zone1.host2.* on host 'host2'")
 
@@ -252,12 +252,12 @@ class TestCephadm(object):
         with self._with_host(cephadm_module, 'host1'):
             with self._with_host(cephadm_module, 'host2'):
                 ps = PlacementSpec(hosts=['host1'], count=1)
-                c = cephadm_module.add_rgw(RGWSpec('realm', 'zone1', placement=ps, service_type='rgw'))
+                c = cephadm_module.add_rgw(RGWSpec('realm', 'zone1', placement=ps))
                 [out] = wait(cephadm_module, c)
                 match_glob(out, "Deployed rgw.realm.zone1.host1.* on host 'host1'")
 
                 ps = PlacementSpec(hosts=['host2'], count=1)
-                c = cephadm_module.add_rgw(RGWSpec('realm', 'zone2', placement=ps, service_type='rgw'))
+                c = cephadm_module.add_rgw(RGWSpec('realm', 'zone2', placement=ps))
                 [out] = wait(cephadm_module, c)
                 match_glob(out, "Deployed rgw.realm.zone2.host2.* on host 'host2'")
 
@@ -267,7 +267,7 @@ class TestCephadm(object):
 
                 with pytest.raises(OrchestratorError):
                     ps = PlacementSpec(hosts=['host1', 'host2'], count=3)
-                    c = cephadm_module._apply_service(RGWSpec('realm', 'zone1', placement=ps, service_type='rgw'))
+                    c = cephadm_module._apply_service(RGWSpec('realm', 'zone1', placement=ps))
                     [out] = wait(cephadm_module, c)
 
 
@@ -326,7 +326,7 @@ class TestCephadm(object):
         # type: (mock.Mock, mock.Mock, mock.Mock, mock.Mock, CephadmOrchestrator) -> None
         with self._with_host(cephadm_module, 'test'):
             ps = PlacementSpec(hosts=['test'], count=1)
-            c = cephadm_module.add_rbd_mirror(ServiceSpec(name='name', placement=ps, service_type='rbd-mirror'))
+            c = cephadm_module.add_rbd_mirror(ServiceSpec('rbd-mirror', placement=ps))
             [out] = wait(cephadm_module, c)
             match_glob(out, "Deployed rbd-mirror.* on host 'test'")
 
@@ -341,7 +341,7 @@ class TestCephadm(object):
         with self._with_host(cephadm_module, 'test'):
             ps = PlacementSpec(hosts=['test'], count=1)
 
-            c = cephadm_module.add_prometheus(ServiceSpec(placement=ps, service_type='prometheus'))
+            c = cephadm_module.add_prometheus(ServiceSpec('prometheus', placement=ps))
             [out] = wait(cephadm_module, c)
             match_glob(out, "Deployed prometheus.* on host 'test'")
 
@@ -356,7 +356,7 @@ class TestCephadm(object):
         with self._with_host(cephadm_module, 'test'):
             ps = PlacementSpec(hosts=['test'], count=1)
 
-            c = cephadm_module.add_node_exporter(ServiceSpec(placement=ps, service_type='node-exporter'))
+            c = cephadm_module.add_node_exporter(ServiceSpec('node-exporter', placement=ps))
             [out] = wait(cephadm_module, c)
             match_glob(out, "Deployed node-exporter.* on host 'test'")
 
@@ -371,7 +371,7 @@ class TestCephadm(object):
         with self._with_host(cephadm_module, 'test'):
             ps = PlacementSpec(hosts=['test'], count=1)
 
-            c = cephadm_module.add_grafana(ServiceSpec(placement=ps, service_type='grafana'))
+            c = cephadm_module.add_grafana(ServiceSpec('grafana', placement=ps))
             [out] = wait(cephadm_module, c)
             match_glob(out, "Deployed grafana.* on host 'test'")
 
@@ -386,7 +386,7 @@ class TestCephadm(object):
         with self._with_host(cephadm_module, 'test'):
             ps = PlacementSpec(hosts=['test'], count=1)
 
-            c = cephadm_module.add_alertmanager(ServiceSpec(placement=ps, service_type='alertmanager'))
+            c = cephadm_module.add_alertmanager(ServiceSpec('alertmanager', placement=ps))
             [out] = wait(cephadm_module, c)
             match_glob(out, "Deployed alertmanager.* on host 'test'")
 
@@ -411,7 +411,7 @@ class TestCephadm(object):
     def test_apply_mgr_save(self, _send_command, _get_connection, _save_spec, _save_host, _rm_host, cephadm_module):
         with self._with_host(cephadm_module, 'test'):
             ps = PlacementSpec(hosts=['test'], count=1)
-            spec = ServiceSpec(placement=ps, service_type='mgr')
+            spec = ServiceSpec('mgr', placement=ps)
             c = cephadm_module.apply_mgr(spec)
             _save_spec.assert_called_with(spec)
             assert wait(cephadm_module, c) == 'Scheduled mgr update...'
@@ -426,7 +426,7 @@ class TestCephadm(object):
     def test_apply_mds_save(self, _send_command, _get_connection, _save_spec, _save_host, _rm_host, cephadm_module):
         with self._with_host(cephadm_module, 'test'):
             ps = PlacementSpec(hosts=['test'], count=1)
-            spec = ServiceSpec(placement=ps, service_type='mds')
+            spec = ServiceSpec('mds', 'fsname', placement=ps)
             c = cephadm_module.apply_mds(spec)
             _save_spec.assert_called_with(spec)
             assert wait(cephadm_module, c) == 'Scheduled mds update...'
@@ -441,7 +441,7 @@ class TestCephadm(object):
     def test_apply_rgw_save(self, _send_command, _get_connection, _save_spec, _save_host, _rm_host, cephadm_module):
         with self._with_host(cephadm_module, 'test'):
             ps = PlacementSpec(hosts=['test'], count=1)
-            spec = ServiceSpec(placement=ps, service_type='rgw')
+            spec = ServiceSpec('rgw', 'r.z', placement=ps)
             c = cephadm_module.apply_rgw(spec)
             _save_spec.assert_called_with(spec)
             assert wait(cephadm_module, c) == 'Scheduled rgw update...'
@@ -456,7 +456,7 @@ class TestCephadm(object):
     def test_apply_rbd_mirror_save(self, _send_command, _get_connection, _save_spec, _save_host, _rm_host, cephadm_module):
         with self._with_host(cephadm_module, 'test'):
             ps = PlacementSpec(hosts=['test'], count=1)
-            spec = ServiceSpec(placement=ps, service_type='rbd-mirror')
+            spec = ServiceSpec('rbd-mirror', placement=ps)
             c = cephadm_module.apply_rbd_mirror(spec)
             _save_spec.assert_called_with(spec)
             assert wait(cephadm_module, c) == 'Scheduled rbd-mirror update...'
@@ -471,7 +471,7 @@ class TestCephadm(object):
     def test_apply_prometheus_save(self, _send_command, _get_connection, _save_spec, _save_host, _rm_host, cephadm_module):
         with self._with_host(cephadm_module, 'test'):
             ps = PlacementSpec(hosts=['test'], count=1)
-            spec = ServiceSpec(placement=ps, service_type='prometheus')
+            spec = ServiceSpec('prometheus', placement=ps)
             c = cephadm_module.apply_prometheus(spec)
             _save_spec.assert_called_with(spec)
             assert wait(cephadm_module, c) == 'Scheduled prometheus update...'
@@ -486,7 +486,7 @@ class TestCephadm(object):
     def test_apply_node_exporter_save(self, _send_command, _get_connection, _save_spec, _save_host, _rm_host, cephadm_module):
         with self._with_host(cephadm_module, 'test'):
             ps = PlacementSpec(hosts=['test'], count=1)
-            spec = ServiceSpec(placement=ps, service_type='node_exporter')
+            spec = ServiceSpec('node_exporter', placement=ps)
             c = cephadm_module.apply_node_exporter(spec)
             _save_spec.assert_called_with(spec)
             assert wait(cephadm_module, c) == 'Scheduled node_exporter update...'
diff --git a/src/pybind/mgr/orchestrator/_interface.py b/src/pybind/mgr/orchestrator/_interface.py
index cba4748bc1e..807cd8957ff 100644
--- a/src/pybind/mgr/orchestrator/_interface.py
+++ b/src/pybind/mgr/orchestrator/_interface.py
@@ -1535,17 +1535,16 @@ class ServiceSpec(object):
 
     """
 
-    def __init__(self, name=None, placement=None, service_type=None, count=None):
-        # type: (Optional[str], Optional[PlacementSpec], Optional[str], Optional[int]) -> None
+    def __init__(self,
+                 service_type : str,
+                 service_id : Optional[str] = None,
+                 placement : Optional[PlacementSpec] = None,
+                 count : Optional[int] = None):
         self.placement = PlacementSpec() if placement is None else placement  # type: PlacementSpec
 
         assert service_type
         self.service_type = service_type
-
-        #: Give this set of stateless services a name: typically it would
-        #: be the name of a CephFS filesystem, RGW zone, etc.  Must be unique
-        #: within one ceph cluster. Note: Not all clusters have a name
-        self.name = name or service_type
+        self.service_id = service_id
 
         if self.placement is not None and self.placement.count is not None:
             #: Count of service instances. Deprecated.
@@ -1579,8 +1578,8 @@ class ServiceSpec(object):
 
     def service_name(self):
         n = self.service_type
-        if self.name:
-            n += '.' + self.name
+        if self.service_id:
+            n += '.' + self.service_id
         return n
 
     def to_json(self):
@@ -1594,8 +1593,10 @@ class ServiceSpec(object):
 def servicespec_validate_add(self: ServiceSpec):
     # This must not be a method of ServiceSpec, otherwise you'll hunt
     # sub-interpreter affinity bugs.
-    if not self.name:
-        raise OrchestratorValidationError('Cannot add Service: Name required')
+    if not self.service_type:
+        raise OrchestratorValidationError('Cannot add Service: type required')
+    if self.service_type in ['mds', 'rgw', 'nfs'] and not self.service_id:
+        raise OrchestratorValidationError('Cannot add Service: id required')
 
 
 def servicespec_validate_hosts_have_network_spec(self: ServiceSpec):
@@ -1614,8 +1615,10 @@ def servicespec_validate_hosts_have_network_spec(self: ServiceSpec):
 
 
 class NFSServiceSpec(ServiceSpec):
-    def __init__(self, name, pool=None, namespace=None, placement=None, service_type=None):
-        super(NFSServiceSpec, self).__init__(name=name, placement=placement, service_type=service_type)
+    def __init__(self, service_id, pool=None, namespace=None, placement=None,
+                 service_type='nfs'):
+        assert service_type == 'nfs'
+        super(NFSServiceSpec, self).__init__('nfs', service_id=service_id, placement=placement)
 
         #: RADOS pool where NFS client recovery data is stored.
         self.pool = pool
@@ -1638,7 +1641,7 @@ class RGWSpec(ServiceSpec):
                  rgw_realm,  # type: str
                  rgw_zone,  # type: str
                  placement=None,
-                 service_type=None,  # type: Optional[str]
+                 service_type='rgw',
                  name=None,  # type: Optional[str]
                  hosts=None,  # type: Optional[List[str]]
                  rgw_multisite=None,  # type: Optional[bool]
@@ -1655,8 +1658,8 @@ class RGWSpec(ServiceSpec):
         # Regarding default values. Ansible has a `set_rgwspec_defaults` that sets
         # default values that makes sense for Ansible. Rook has default values implemented
         # in Rook itself. Thus we don't set any defaults here in this class.
-
-        super(RGWSpec, self).__init__(name=rgw_realm+'.'+rgw_zone, placement=placement, service_type=service_type)
+        assert service_type == 'rgw'
+        super(RGWSpec, self).__init__('rgw', service_id=rgw_realm+'.'+rgw_zone, placement=placement)
 
         #: List of hosts where RGWs should run. Not for Rook.
         if hosts:
diff --git a/src/pybind/mgr/orchestrator/module.py b/src/pybind/mgr/orchestrator/module.py
index 2302e14e8c9..d8c85c11dc8 100644
--- a/src/pybind/mgr/orchestrator/module.py
+++ b/src/pybind/mgr/orchestrator/module.py
@@ -509,7 +509,7 @@ Usage:
         placement = PlacementSpec(label=label, count=num, hosts=hosts)
         placement.validate()
 
-        spec = ServiceSpec(placement=placement, service_type='mon')
+        spec = ServiceSpec('mon', placement=placement)
 
         completion = self.add_mon(spec)
         self._orchestrator_wait([completion])
@@ -523,6 +523,7 @@ Usage:
         'Start rbd-mirror daemon(s)')
     def _daemon_add_mgr(self, num=None, hosts=None):
         spec = ServiceSpec(
+            'mgr',
             placement=PlacementSpec(hosts=hosts, count=num))
         completion = self.add_mgr(spec)
         self._orchestrator_wait([completion])
@@ -545,7 +546,7 @@ Usage:
         'Start rbd-mirror daemon(s)')
     def _rbd_mirror_add(self, num=None, hosts=None):
         spec = ServiceSpec(
-            None,
+            'rbd-mirror',
             placement=PlacementSpec(hosts=hosts, count=num))
         completion = self.add_rbd_mirror(spec)
         self._orchestrator_wait([completion])
@@ -560,7 +561,7 @@ Usage:
         'Start MDS daemon(s)')
     def _mds_add(self, fs_name, num=None, hosts=None):
         spec = ServiceSpec(
-            fs_name,
+            'mds', fs_name,
             placement=PlacementSpec(hosts=hosts, count=num))
         completion = self.add_mds(spec)
         self._orchestrator_wait([completion])
@@ -627,6 +628,7 @@ Usage:
     def _daemon_add_prometheus(self, num=None, label=None, hosts=[]):
         # type: (Optional[int], Optional[str], List[str]) -> HandleCommandResult
         spec = ServiceSpec(
+            'prometheus',
             placement=PlacementSpec(label=label, hosts=hosts, count=num),
         )
         completion = self.add_prometheus(spec)
@@ -642,6 +644,7 @@ Usage:
     def _daemon_add_node_exporter(self, num=None, label=None, hosts=[]):
         # type: (Optional[int], Optional[str], List[str]) -> HandleCommandResult
         spec = ServiceSpec(
+            'node-exporter',
             placement=PlacementSpec(label=label, hosts=hosts, count=num),
         )
         completion = self.add_node_exporter(spec)
@@ -657,6 +660,7 @@ Usage:
     def _daemon_add_grafana(self, num=None, label=None, hosts=[]):
         # type: (Optional[int], Optional[str], List[str]) -> HandleCommandResult
         spec = ServiceSpec(
+            'grafana',
             placement=PlacementSpec(label=label, hosts=hosts, count=num),
         )
         completion = self.add_grafana(spec)
@@ -672,6 +676,7 @@ Usage:
     def _daemon_add_alertmanager(self, num=None, label=None, hosts=[]):
         # type: (Optional[int], Optional[str], List[str]) -> HandleCommandResult
         spec = ServiceSpec(
+            'alertmanager',
             placement=PlacementSpec(label=label, hosts=hosts, count=num),
         )
         completion = self.add_alertmanager(spec)
@@ -750,7 +755,7 @@ Usage:
             label=label, count=num, hosts=hosts)
         placement.validate()
 
-        spec = ServiceSpec(placement=placement, service_type='mgr')
+        spec = ServiceSpec('mgr', placement=placement)
 
         completion = self.apply_mgr(spec)
         self._orchestrator_wait([completion])
@@ -770,7 +775,7 @@ Usage:
         placement = PlacementSpec(label=label, count=num, hosts=hosts)
         placement.validate()
 
-        spec = ServiceSpec(placement=placement)
+        spec = ServiceSpec('mon', placement=placement)
 
         completion = self.apply_mon(spec)
         self._orchestrator_wait([completion])
@@ -788,8 +793,7 @@ Usage:
         placement = PlacementSpec(label=label, count=num, hosts=hosts)
         placement.validate()
         spec = ServiceSpec(
-            service_type='mds',
-            name=fs_name,
+            'mds', fs_name,
             placement=placement)
         completion = self.apply_mds(spec)
         self._orchestrator_wait([completion])
@@ -804,8 +808,8 @@ Usage:
         'Update the number of rbd-mirror instances')
     def _apply_rbd_mirror(self, num, label=None, hosts=[]):
         spec = ServiceSpec(
-            placement=PlacementSpec(hosts=hosts, count=num, label=label),
-            service_type='rbd-mirror')
+            'rbd-mirror',
+            placement=PlacementSpec(hosts=hosts, count=num, label=label))
         completion = self.apply_rbd_mirror(spec)
         self._orchestrator_wait([completion])
         raise_if_exception(completion)
@@ -821,7 +825,6 @@ Usage:
         'Update the number of RGW instances for the given zone')
     def _apply_rgw(self, zone_name, realm_name, num=None, label=None, hosts=[]):
         spec = RGWSpec(
-            service_type='rgw',
             rgw_realm=realm_name,
             rgw_zone=zone_name,
             placement=PlacementSpec(hosts=hosts, label=label, count=num))
@@ -856,8 +859,8 @@ Usage:
     def _apply_prometheus(self, num=None, label=None, hosts=[]):
         # type: (Optional[int], Optional[str], List[str]) -> HandleCommandResult
         spec = ServiceSpec(
+            'prometheus',
             placement=PlacementSpec(label=label, hosts=hosts, count=num),
-            service_type='prometheus'
         )
         completion = self.apply_prometheus(spec)
         self._orchestrator_wait([completion])
@@ -872,8 +875,8 @@ Usage:
     def _apply_node_exporter(self, num=None, label=None, hosts=[]):
         # type: (Optional[int], Optional[str], List[str]) -> HandleCommandResult
         spec = ServiceSpec(
+            'node-exporter',
             placement=PlacementSpec(label=label, hosts=hosts, count=num),
-            service_type='node-exporter'
         )
         completion = self.apply_node_exporter(spec)
         self._orchestrator_wait([completion])
diff --git a/src/pybind/mgr/rook/module.py b/src/pybind/mgr/rook/module.py
index 9b603d7b62d..f2216d5ad4c 100644
--- a/src/pybind/mgr/rook/module.py
+++ b/src/pybind/mgr/rook/module.py
@@ -292,7 +292,7 @@ class RookOrchestrator(MgrModule, orchestrator.Orchestrator):
     def _service_add_decorate(self, typename, spec, func):
         return write_completion(
             on_complete=lambda : func(spec),
-            message="Creating {} services for {}".format(typename, spec.name),
+            message="Creating {} services for {}".format(typename, spec.service_id),
             mgr=self
         )
 
@@ -348,8 +348,8 @@ class RookOrchestrator(MgrModule, orchestrator.Orchestrator):
         # type: (orchestrator.ServiceSpec) -> RookCompletion
         num = spec.count
         return write_completion(
-            lambda: self.rook_cluster.update_mds_count(spec.name, num),
-            "Updating MDS server count in {0} to {1}".format(spec.name, num),
+            lambda: self.rook_cluster.update_mds_count(spec.service_id, num),
+            "Updating MDS server count in {0} to {1}".format(spec.service_id, num),
             mgr=self
         )
 
@@ -357,8 +357,8 @@ class RookOrchestrator(MgrModule, orchestrator.Orchestrator):
         # type: (orchestrator.NFSServiceSpec) -> RookCompletion
         num = spec.count
         return write_completion(
-            lambda: self.rook_cluster.update_nfs_count(spec.name, num),
-            "Updating NFS server count in {0} to {1}".format(spec.name, num),
+            lambda: self.rook_cluster.update_nfs_count(spec.service_id, num),
+            "Updating NFS server count in {0} to {1}".format(spec.service_id, num),
             mgr=self
         )
 
diff --git a/src/pybind/mgr/rook/rook_cluster.py b/src/pybind/mgr/rook/rook_cluster.py
index 59f66b06c65..2fc0d4836dc 100644
--- a/src/pybind/mgr/rook/rook_cluster.py
+++ b/src/pybind/mgr/rook/rook_cluster.py
@@ -351,7 +351,7 @@ class RookCluster(object):
         rook_fs = cfs.CephFilesystem(
             apiVersion=self.rook_env.api_name,
             metadata=dict(
-                name=spec.name,
+                name=spec.service_id,
                 namespace=self.rook_env.namespace,
             ),
             spec=cfs.Spec(
@@ -362,7 +362,7 @@ class RookCluster(object):
             )
         )
 
-        with self.ignore_409("CephFilesystem '{0}' already exists".format(spec.name)):
+        with self.ignore_409("CephFilesystem '{0}' already exists".format(spec.service_id)):
             self.rook_api_post("cephfilesystems/", body=rook_fs.to_json())
 
     def add_nfsgw(self, spec):
@@ -373,7 +373,7 @@ class RookCluster(object):
         rook_nfsgw = cnfs.CephNFS(
             apiVersion=self.rook_env.api_name,
             metadata=dict(
-                name=spec.name,
+                name=spec.service_id,
                 namespace=self.rook_env.namespace,
             ),
             spec=cnfs.Spec(
@@ -389,7 +389,7 @@ class RookCluster(object):
         if spec.namespace:
             rook_nfsgw.spec.rados.namespace = spec.namespace
 
-        with self.ignore_409("NFS cluster '{0}' already exists".format(spec.name)):
+        with self.ignore_409("NFS cluster '{0}' already exists".format(spec.service_id)):
             self.rook_api_post("cephnfses/", body=rook_nfsgw.to_json())
 
     def add_objectstore(self, spec):
@@ -397,7 +397,7 @@ class RookCluster(object):
         rook_os = cos.CephObjectStore(
             apiVersion=self.rook_env.api_name,
             metadata=dict(
-                name=spec.name,
+                name=spec.service_id,
                 namespace=self.rook_env.namespace
             ),
             spec=cos.Spec(
@@ -421,7 +421,7 @@ class RookCluster(object):
             )
         )
         
-        with self.ignore_409("CephObjectStore '{0}' already exists".format(spec.name)):
+        with self.ignore_409("CephObjectStore '{0}' already exists".format(spec.service_id)):
             self.rook_api_post("cephobjectstores/", body=rook_os.to_json())
 
     def rm_service(self, rooktype, service_id):