Switch to using systemd units for radosgw

Switch to using systemd configurations to manage radosgw instances;
the radosgw init script is obsolete and will be removed at some
point in time, and the newer style of managing radosgw daemons is
inline with current best-practice.

This changeset also changes the way cephx keys are issues; before
all rgw instances shared a key, now a key is issued per host.

The key is named 'rgw.`hostname`' to identify the application
and host using the key.

Existing deployments using the radosgw init script will be switched
to use the new systemd named units; this occurs once the new key
for the unit has been presented by the ceph-mon cluster over the
mon relation. A small period of outage will occur as the radosgw
init based daemon is stopped and disabled prior to the start of
the new systemd based radosgw unit.

This commit also includes a resync for charmhelpers to pickup
support for '@' in NRPE service check names.

Change-Id: Ic0d634e619185931633712cb3e3685051a28749d
Depends-On: I289b75a2935184817b424c5eceead16235c3f53b
Closes-Bug: 1808140

hooks/ceph.py

@@ -21,23 +21,51 @@ from utils import get_pkg_version
 from charmhelpers.core.hookenv import (
     config,
 )
+
+from charmhelpers.core.host import (
+    mkdir
+)
 from charmhelpers.contrib.storage.linux.ceph import (
     CephBrokerRq,
 )
 
-_radosgw_keyring = "/etc/ceph/keyring.rados.gateway"
+CEPH_DIR = '/etc/ceph'
+CEPH_RADOSGW_DIR = '/var/lib/ceph/radosgw'
+_radosgw_keyring = "keyring.rados.gateway"
 
 
-def import_radosgw_key(key):
+def import_radosgw_key(key, name=None):
-    if not os.path.exists(_radosgw_keyring):
+    if name:
+        keyring_path = os.path.join(CEPH_RADOSGW_DIR,
+                                    'ceph-{}'.format(name),
+                                    'keyring')
+        owner = group = 'ceph'
+    else:
+        keyring_path = os.path.join(CEPH_DIR, _radosgw_keyring)
+        owner = group = 'root'
+
+    if not os.path.exists(keyring_path):
+        mkdir(path=os.path.dirname(keyring_path),
+              owner=owner, group=group, perms=0o750)
         cmd = [
             'ceph-authtool',
-            _radosgw_keyring,
+            keyring_path,
             '--create-keyring',
-            '--name=client.radosgw.gateway',
+            '--name=client.{}'.format(
+                name or 'radosgw.gateway'
+            ),
             '--add-key={}'.format(key)
         ]
         subprocess.check_call(cmd)
+        cmd = [
+            'chown',
+            '{}:{}'.format(owner, group),
+            keyring_path
+        ]
+        subprocess.check_call(cmd)
+        return True
+
+    return False
 
 
 def get_create_rgw_pools_rq(prefix=None):

hooks/ceph_radosgw_context.py

@@ -144,6 +144,10 @@ class MonContext(context.CephContext):
     def __call__(self):
         if not relation_ids('mon'):
             return {}
+
+        host = socket.gethostname()
+        systemd_rgw = False
+
         mon_hosts = []
         auths = []
 
@@ -161,6 +165,8 @@ class MonContext(context.CephContext):
                 ceph_addr = format_ipv6_addr(ceph_addr) or ceph_addr
                 if ceph_addr:
                     mon_hosts.append(ceph_addr)
+                if relation_get('rgw.{}_key'.format(host), rid=rid, unit=unit):
+                    systemd_rgw = True
 
         if len(set(auths)) != 1:
             e = ("Inconsistent or absent auth returned by mon units. Setting "
@@ -172,7 +178,6 @@ class MonContext(context.CephContext):
 
         # /etc/init.d/radosgw mandates that a dns name is used for this
         # parameter so ensure that address is resolvable
-        host = socket.gethostname()
         if config('prefer-ipv6'):
             ensure_host_resolvable_v6(host)
 
@@ -186,6 +191,7 @@ class MonContext(context.CephContext):
             'mon_hosts': ' '.join(mon_hosts),
             'hostname': host,
             'old_auth': cmp_pkgrevno('radosgw', "0.51") < 0,
+            'systemd_rgw': systemd_rgw,
             'use_syslog': str(config('use-syslog')).lower(),
             'loglevel': config('loglevel'),
             'port': port,

hooks/charmhelpers/contrib/storage/linux/ceph.py

@@ -856,12 +856,22 @@ def _keyring_path(service):
     return KEYRING.format(service)
 
 
-def create_keyring(service, key):
+def add_key(service, key):
-    """Create a new Ceph keyring containing key."""
+    """
+    Add a key to a keyring.
+
+    Creates the keyring if it doesn't already exist.
+
+    Logs and returns if the key is already in the keyring.
+    """
     keyring = _keyring_path(service)
     if os.path.exists(keyring):
-        log('Ceph keyring exists at %s.' % keyring, level=WARNING)
+        with open(keyring, 'r') as ring:
+            if key in ring.read():
+                log('Ceph keyring exists at %s and has not changed.' % keyring,
+                    level=DEBUG)
                 return
+            log('Updating existing keyring %s.' % keyring, level=DEBUG)
 
     cmd = ['ceph-authtool', keyring, '--create-keyring',
            '--name=client.{}'.format(service), '--add-key={}'.format(key)]
@@ -869,6 +879,11 @@ def create_keyring(service, key):
     log('Created new ceph keyring at %s.' % keyring, level=DEBUG)
 
 
+def create_keyring(service, key):
+    """Deprecated. Please use the more accurately named 'add_key'"""
+    return add_key(service, key)
+
+
 def delete_keyring(service):
     """Delete an existing Ceph keyring."""
     keyring = _keyring_path(service)
@@ -905,7 +920,7 @@ def get_ceph_nodes(relation='ceph'):
 
 def configure(service, key, auth, use_syslog):
     """Perform basic configuration of Ceph."""
-    create_keyring(service, key)
+    add_key(service, key)
     create_key_file(service, key)
     hosts = get_ceph_nodes()
     with open('/etc/ceph/ceph.conf', 'w') as ceph_conf:
@@ -1068,7 +1083,7 @@ def ensure_ceph_keyring(service, user=None, group=None,
     if not key:
         return False
 
-    create_keyring(service=service, key=key)
+    add_key(service=service, key=key)
     keyring = _keyring_path(service)
     if user and group:
         check_call(['chown', '%s.%s' % (user, group), keyring])

hooks/hooks.py

@@ -17,6 +17,7 @@
 import os
 import subprocess
 import sys
+import socket
 
 import ceph
 
@@ -45,6 +46,9 @@ from charmhelpers.core.host import (
     cmp_pkgrevno,
     is_container,
     service_reload,
+    service_restart,
+    service_stop,
+    service,
 )
 from charmhelpers.contrib.network.ip import (
     get_relation_ip,
@@ -84,6 +88,10 @@ from utils import (
     disable_unused_apache_sites,
     pause_unit_helper,
     resume_unit_helper,
+    restart_map,
+    service_name,
+    systemd_based_radosgw,
+    request_per_unit_key,
 )
 from charmhelpers.contrib.charmsupport import nrpe
 from charmhelpers.contrib.hardening.harden import harden
@@ -137,10 +145,10 @@ def install():
 
 
 @hooks.hook('config-changed')
-@restart_on_change({'/etc/ceph/ceph.conf': ['radosgw'],
-                    '/etc/haproxy/haproxy.cfg': ['haproxy']})
 @harden()
 def config_changed():
+    @restart_on_change(restart_map())
+    def _config_changed():
         # if we are paused, delay doing any config changed hooks.
         # It is forced on the resume.
         if is_unit_paused_set():
@@ -171,24 +179,49 @@ def config_changed():
 
         update_nrpe_config()
 
+        open_port(port=config('port'))
+    _config_changed()
+
 
 @hooks.hook('mon-relation-departed',
             'mon-relation-changed')
-@restart_on_change({'/etc/ceph/ceph.conf': ['radosgw']})
 def mon_relation(rid=None, unit=None):
+    @restart_on_change(restart_map())
+    def _mon_relation():
+        key_name = 'rgw.{}'.format(socket.gethostname())
+        if request_per_unit_key():
+            relation_set(relation_id=rid,
+                         key_name=key_name)
         rq = ceph.get_create_rgw_pools_rq(
             prefix=config('pool-prefix'))
         if is_request_complete(rq, relation='mon'):
             log('Broker request complete', level=DEBUG)
             CONFIGS.write_all()
+            # New style per unit keys
+            key = relation_get(attribute='{}_key'.format(key_name),
+                               rid=rid, unit=unit)
+            if not key:
+                # Fallback to old style global key
                 key = relation_get(attribute='radosgw_key',
                                    rid=rid, unit=unit)
+                key_name = None
+
             if key:
-            ceph.import_radosgw_key(key)
+                new_keyring = ceph.import_radosgw_key(key,
-            if not is_unit_paused_set():
+                                                      name=key_name)
-                restart()  # TODO figure out a better way todo this
+                # NOTE(jamespage):
+                # Deal with switch from radosgw init script to
+                # systemd named units for radosgw instances by
+                # stopping and disabling the radosgw unit
+                if systemd_based_radosgw():
+                    service_stop('radosgw')
+                    service('disable', 'radosgw')
+                if not is_unit_paused_set() and new_keyring:
+                    service('enable', service_name())
+                    service_restart(service_name())
         else:
             send_request_if_needed(rq, relation='mon')
+    _mon_relation()
 
 
 @hooks.hook('gateway-relation-joined')
@@ -197,21 +230,6 @@ def gateway_relation():
                  port=config('port'))
 
 
-def start():
-    subprocess.call(['service', 'radosgw', 'start'])
-    open_port(port=config('port'))
-
-
-def stop():
-    subprocess.call(['service', 'radosgw', 'stop'])
-    open_port(port=config('port'))
-
-
-def restart():
-    subprocess.call(['service', 'radosgw', 'restart'])
-    open_port(port=config('port'))
-
-
 @hooks.hook('identity-service-relation-joined')
 def identity_joined(relid=None):
     if cmp_pkgrevno('radosgw', '0.55') < 0:
@@ -233,18 +251,19 @@ def identity_joined(relid=None):
 
 
 @hooks.hook('identity-service-relation-changed')
-@restart_on_change({'/etc/ceph/ceph.conf': ['radosgw']})
 def identity_changed(relid=None):
+    @restart_on_change(restart_map())
+    def _identity_changed():
         identity_joined(relid)
         CONFIGS.write_all()
-    if not is_unit_paused_set():
-        restart()
         configure_https()
+    _identity_changed()
 
 
 @hooks.hook('cluster-relation-joined')
-@restart_on_change({'/etc/haproxy/haproxy.cfg': ['haproxy']})
 def cluster_joined(rid=None):
+    @restart_on_change(restart_map())
+    def _cluster_joined():
         settings = {}
 
         for addr_type in ADDRESS_TYPES:
@@ -257,14 +276,17 @@ def cluster_joined(rid=None):
         settings['private-address'] = get_relation_ip('cluster')
 
         relation_set(relation_id=rid, relation_settings=settings)
+    _cluster_joined()
 
 
 @hooks.hook('cluster-relation-changed')
-@restart_on_change({'/etc/haproxy/haproxy.cfg': ['haproxy']})
 def cluster_changed():
+    @restart_on_change(restart_map())
+    def _cluster_changed():
         CONFIGS.write_all()
         for r_id in relation_ids('identity-service'):
             identity_joined(relid=r_id)
+    _cluster_changed()
 
 
 @hooks.hook('ha-relation-joined')

hooks/utils.py

@@ -14,6 +14,7 @@
 
 import os
 import re
+import socket
 import subprocess
 import sys
 
@@ -57,6 +58,7 @@ from charmhelpers.core.host import (
     lsb_release,
     mkdir,
     CompareHostReleases,
+    init_is_systemd,
 )
 from charmhelpers.fetch import (
     apt_cache,
@@ -124,7 +126,7 @@ BASE_RESOURCE_MAP = OrderedDict([
     }),
     (CEPH_CONF, {
         'contexts': [ceph_radosgw_context.MonContext()],
-        'services': ['radosgw'],
+        'services': [],
     }),
     (APACHE_SITE_CONF, {
         'contexts': [ceph_radosgw_context.ApacheSSLContext()],
@@ -152,14 +154,25 @@ def resource_map():
     """
     resource_map = deepcopy(BASE_RESOURCE_MAP)
 
+    if not https():
+        resource_map.pop(APACHE_SITE_CONF)
+        resource_map.pop(APACHE_SITE_24_CONF)
+    else:
         if os.path.exists('/etc/apache2/conf-available'):
             resource_map.pop(APACHE_SITE_CONF)
         else:
             resource_map.pop(APACHE_SITE_24_CONF)
 
+    resource_map[CEPH_CONF]['services'] = [service_name()]
     return resource_map
 
 
+def restart_map():
+    return OrderedDict([(cfg, v['services'])
+                        for cfg, v in resource_map().items()
+                        if v['services']])
+
+
 # Hardcoded to icehouse to enable use of charmhelper templating/context tools
 # Ideally these function would support non-OpenStack services
 def register_configs(release='icehouse'):
@@ -180,12 +193,9 @@ def register_configs(release='icehouse'):
 def services():
     """Returns a list of services associate with this charm."""
     _services = []
-    for v in BASE_RESOURCE_MAP.values():
+    for v in resource_map().values():
         _services.extend(v.get('services', []))
-    _set_services = set(_services)
+    return list(set(_services))
-    if not https():
-        _set_services.remove('apache2')
-    return list(_set_services)
 
 
 def enable_pocket(pocket):
@@ -560,3 +570,26 @@ def disable_unused_apache_sites():
 
     with open(APACHE_PORTS_FILE, 'w') as ports:
         ports.write("")
+
+
+def systemd_based_radosgw():
+    """Determine if install should use systemd based radosgw instances"""
+    host = socket.gethostname()
+    for rid in relation_ids('mon'):
+        for unit in related_units(rid):
+            if relation_get('rgw.{}_key'.format(host), rid=rid, unit=unit):
+                return True
+    return False
+
+
+def request_per_unit_key():
+    """Determine if a per-unit cephx key should be requested"""
+    return (cmp_pkgrevno('radosgw', '12.2.0') >= 0 and init_is_systemd())
+
+
+def service_name():
+    """Determine the name of the RADOS Gateway service"""
+    if systemd_based_radosgw():
+        return 'ceph-radosgw@rgw.{}'.format(socket.gethostname())
+    else:
+        return 'radosgw'

templates/ceph.conf

@@ -22,12 +22,18 @@ ms bind ipv6 = true
 {% endfor %}
 {% endif %}
 
-[client.radosgw.gateway]
+{% if systemd_rgw -%}
+[client.rgw.{{ hostname }}]
 host = {{ hostname }}
-rgw init timeout = 1200
+{% else -%}
+[client.radosgw.gateway]
 keyring = /etc/ceph/keyring.rados.gateway
+host = {{ hostname }}
 rgw socket path = /tmp/radosgw.sock
 log file = /var/log/ceph/radosgw.log
+{% endif %}
+
+rgw init timeout = 1200
 rgw frontends = civetweb port={{ port }}
 {% if auth_type == 'keystone' %}
 rgw keystone url = {{ auth_protocol }}://{{ auth_host }}:{{ auth_port }}/

tests/basic_deployment.py

@@ -329,7 +329,6 @@ class CephRadosGwBasicDeployment(OpenStackAmuletDeployment):
         relation = ['radosgw', 'ceph-radosgw:mon']
         expected = {
             'private-address': u.valid_ip,
-            'radosgw_key': u.not_null,
             'auth': 'none',
             'ceph-public-address': u.valid_ip,
         }
@@ -394,10 +393,6 @@ class CephRadosGwBasicDeployment(OpenStackAmuletDeployment):
         u.log.debug('Checking ceph config file data...')
         unit = self.ceph_radosgw_sentry
         conf = '/etc/ceph/ceph.conf'
-        keystone_sentry = self.keystone_sentry
-        relation = keystone_sentry.relation('identity-service',
-                                            'ceph-radosgw:identity-service')
-        keystone_ip = relation['auth_host']
         expected = {
             'global': {
                 'auth cluster required': 'none',
@@ -407,26 +402,7 @@ class CephRadosGwBasicDeployment(OpenStackAmuletDeployment):
                 'err to syslog': 'false',
                 'clog to syslog': 'false'
             },
-            'client.radosgw.gateway': {
-                'keyring': '/etc/ceph/keyring.rados.gateway',
-                'rgw socket path': '/tmp/radosgw.sock',
-                'log file': '/var/log/ceph/radosgw.log',
-                'rgw keystone url': 'http://{}:35357/'.format(keystone_ip),
-                'rgw keystone accepted roles': 'Member,Admin',
-                'rgw keystone token cache size': '500',
-                'rgw keystone revocation interval': '600',
-                'rgw frontends': 'civetweb port=70',
-            },
         }
-        if self._get_openstack_release() >= self.xenial_queens:
-            expected['client.radosgw.gateway']['rgw keystone admin domain'] = (
-                'service_domain')
-            (expected['client.radosgw.gateway']
-                ['rgw keystone admin project']) = 'services'
-        else:
-            expected['client.radosgw.gateway']['rgw keystone admin token'] = (
-                'ubuntutesting')
-
         for section, pairs in expected.items():
             ret = u.validate_config_data(unit, conf, section, pairs)
             if ret:

unit_tests/test_ceph.py

@@ -32,6 +32,7 @@ TO_PATCH = [
     'config',
     'os',
     'subprocess',
+    'mkdir',
 ]
 
 
@@ -42,6 +43,7 @@ class CephRadosGWCephTests(CharmTestCase):
 
     def test_import_radosgw_key(self):
         self.os.path.exists.return_value = False
+        self.os.path.join.return_value = '/etc/ceph/keyring.rados.gateway'
         ceph.import_radosgw_key('mykey')
         cmd = [
             'ceph-authtool',
@@ -50,7 +52,11 @@ class CephRadosGWCephTests(CharmTestCase):
             '--name=client.radosgw.gateway',
             '--add-key=mykey'
         ]
-        self.subprocess.check_call.assert_called_with(cmd)
+        self.subprocess.check_call.assert_has_calls([
+            call(cmd),
+            call(['chown', 'root:root',
+                  '/etc/ceph/keyring.rados.gateway'])
+        ])
 
     @patch('charmhelpers.contrib.storage.linux.ceph.CephBrokerRq'
            '.add_op_create_pool')

unit_tests/test_ceph_radosgw_context.py

@@ -29,7 +29,8 @@ TO_PATCH = [
     'cmp_pkgrevno',
     'socket',
     'unit_public_ip',
-    'determine_api_port'
+    'determine_api_port',
+    'cmp_pkgrevno',
 ]
 
 
@@ -312,6 +313,8 @@ class MonContextTest(CharmTestCase):
                 return addresses.pop()
             elif attr == 'auth':
                 return 'cephx'
+            elif attr == 'rgw.testhost_key':
+                return 'testkey'
 
         self.relation_get.side_effect = _relation_get
         self.relation_ids.return_value = ['mon:6']
@@ -322,6 +325,7 @@ class MonContextTest(CharmTestCase):
             'hostname': 'testhost',
             'mon_hosts': '10.5.4.1 10.5.4.2 10.5.4.3',
             'old_auth': False,
+            'systemd_rgw': True,
             'unit_public_ip': '10.255.255.255',
             'use_syslog': 'false',
             'loglevel': 1,
@@ -346,12 +350,15 @@ class MonContextTest(CharmTestCase):
         self.socket.gethostname.return_value = 'testhost'
         mon_ctxt = context.MonContext()
         addresses = ['10.5.4.1 10.5.4.2 10.5.4.3']
+        self.cmp_pkgrevno.return_value = 1
 
         def _relation_get(attr, unit, rid):
             if attr == 'ceph-public-address':
                 return addresses.pop()
             elif attr == 'auth':
                 return 'cephx'
+            elif attr == 'rgw.testhost_key':
+                return 'testkey'
 
         self.relation_get.side_effect = _relation_get
         self.relation_ids.return_value = ['mon:6']
@@ -362,6 +369,7 @@ class MonContextTest(CharmTestCase):
             'hostname': 'testhost',
             'mon_hosts': '10.5.4.1 10.5.4.2 10.5.4.3',
             'old_auth': False,
+            'systemd_rgw': True,
             'unit_public_ip': '10.255.255.255',
             'use_syslog': 'false',
             'loglevel': 1,
@@ -402,6 +410,9 @@ class MonContextTest(CharmTestCase):
                 return addresses.pop()
             elif attr == 'auth':
                 return auths.pop()
+            elif attr == 'rgw.testhost_key':
+                return 'testkey'
+
         self.relation_get.side_effect = _relation_get
         self.relation_ids.return_value = ['mon:6']
         self.related_units.return_value = ['ceph/0', 'ceph/1', 'ceph/2']
@@ -411,6 +422,7 @@ class MonContextTest(CharmTestCase):
             'hostname': 'testhost',
             'mon_hosts': '10.5.4.1 10.5.4.2 10.5.4.3',
             'old_auth': False,
+            'systemd_rgw': True,
             'unit_public_ip': '10.255.255.255',
             'use_syslog': 'false',
             'loglevel': 1,
@@ -433,6 +445,9 @@ class MonContextTest(CharmTestCase):
                 return addresses.pop()
             elif attr == 'auth':
                 return auths.pop()
+            elif attr == 'rgw.testhost_key':
+                return 'testkey'
+
         self.relation_get.side_effect = _relation_get
         self.relation_ids.return_value = ['mon:6']
         self.related_units.return_value = ['ceph/0', 'ceph/1', 'ceph/2']
@@ -442,6 +457,7 @@ class MonContextTest(CharmTestCase):
             'hostname': 'testhost',
             'mon_hosts': '10.5.4.1 10.5.4.2 10.5.4.3',
             'old_auth': False,
+            'systemd_rgw': True,
             'unit_public_ip': '10.255.255.255',
             'use_syslog': 'false',
             'loglevel': 1,

unit_tests/test_ceph_radosgw_utils.py

@@ -28,6 +28,12 @@ TO_PATCH = [
     'get_upstream_version',
     'format_endpoint',
     'https',
+    'relation_ids',
+    'relation_get',
+    'related_units',
+    'socket',
+    'cmp_pkgrevno',
+    'init_is_systemd',
 ]
 
 
@@ -35,6 +41,7 @@ class CephRadosGWUtilTests(CharmTestCase):
     def setUp(self):
         super(CephRadosGWUtilTests, self).setUp(utils, TO_PATCH)
         self.get_upstream_version.return_value = '10.2.2'
+        self.socket.gethostname.return_value = 'testhost'
 
     def test_assess_status(self):
         with patch.object(utils, 'assess_status_func') as asf:
@@ -219,3 +226,68 @@ class CephRadosGWUtilTests(CharmTestCase):
             c = ['openssl', 'x509', '-in', '/foo/bar/ca.pem',
                  '-pubkey']
             mock_check_output.assert_called_with(c)
+
+    def _setup_relation_data(self, data):
+        self.relation_ids.return_value = data.keys()
+        self.related_units.side_effect = (
+            lambda rid: data[rid].keys()
+        )
+        self.relation_get.side_effect = (
+            lambda attr, rid, unit: data[rid][unit].get(attr)
+        )
+
+    def test_systemd_based_radosgw_old_style(self):
+        _relation_data = {
+            'mon:1': {
+                'ceph-mon/0': {
+                    'radosgw_key': 'testkey',
+                },
+                'ceph-mon/1': {
+                    'radosgw_key': 'testkey',
+                },
+                'ceph-mon/2': {
+                    'radosgw_key': 'testkey',
+                },
+            }
+        }
+        self._setup_relation_data(_relation_data)
+        self.assertFalse(utils.systemd_based_radosgw())
+
+    def test_systemd_based_radosgw_new_style(self):
+        _relation_data = {
+            'mon:1': {
+                'ceph-mon/0': {
+                    'rgw.testhost_key': 'testkey',
+                },
+                'ceph-mon/1': {
+                    'rgw.testhost_key': 'testkey',
+                },
+                'ceph-mon/2': {
+                    'rgw.testhost_key': 'testkey',
+                },
+            }
+        }
+        self._setup_relation_data(_relation_data)
+        self.assertTrue(utils.systemd_based_radosgw())
+
+    def test_request_per_unit_key(self):
+        self.init_is_systemd.return_value = False
+        self.cmp_pkgrevno.return_value = -1
+        self.assertFalse(utils.request_per_unit_key())
+        self.init_is_systemd.return_value = True
+        self.cmp_pkgrevno.return_value = 1
+        self.assertTrue(utils.request_per_unit_key())
+        self.init_is_systemd.return_value = False
+        self.cmp_pkgrevno.return_value = 1
+        self.assertFalse(utils.request_per_unit_key())
+
+        self.cmp_pkgrevno.assert_called_with('radosgw', '12.2.0')
+
+    @patch.object(utils, 'systemd_based_radosgw')
+    def test_service_name(self, mock_systemd_based_radosgw):
+        mock_systemd_based_radosgw.return_value = True
+        self.assertEqual(utils.service_name(),
+                         'ceph-radosgw@rgw.testhost')
+        mock_systemd_based_radosgw.return_value = False
+        self.assertEqual(utils.service_name(),
+                         'radosgw')

unit_tests/test_hooks.py

@@ -55,7 +55,15 @@ TO_PATCH = [
     'get_relation_ip',
     'disable_unused_apache_sites',
     'service_reload',
+    'service_stop',
+    'service_restart',
+    'service',
     'setup_keystone_certs',
+    'service_name',
+    'socket',
+    'restart_map',
+    'systemd_based_radosgw',
+    'request_per_unit_key',
 ]
 
 
@@ -68,6 +76,9 @@ class CephRadosGWTests(CharmTestCase):
         self.test_config.set('key', 'secretkey')
         self.test_config.set('use-syslog', False)
         self.cmp_pkgrevno.return_value = 0
+        self.service_name.return_value = 'radosgw'
+        self.request_per_unit_key.return_value = False
+        self.systemd_based_radosgw.return_value = False
 
     def test_install_packages(self):
         ceph_hooks.install_packages()
@@ -95,22 +106,46 @@ class CephRadosGWTests(CharmTestCase):
                   lambda *args, **kwargs: True)
     def test_mon_relation(self):
         _ceph = self.patch('ceph')
-        _restart = self.patch('restart')
+        _ceph.import_radosgw_key.return_value = True
         self.relation_get.return_value = 'seckey'
+        self.socket.gethostname.return_value = 'testinghostname'
         ceph_hooks.mon_relation()
-        self.assertTrue(_restart.called)
+        self.relation_set.assert_not_called()
-        _ceph.import_radosgw_key.assert_called_with('seckey')
+        self.service_restart.assert_called_once_with('radosgw')
+        self.service.assert_called_once_with('enable', 'radosgw')
+        _ceph.import_radosgw_key.assert_called_with('seckey',
+                                                    name='rgw.testinghostname')
+        self.CONFIGS.write_all.assert_called_with()
+
+    @patch.object(ceph_hooks, 'is_request_complete',
+                  lambda *args, **kwargs: True)
+    def test_mon_relation_request_key(self):
+        _ceph = self.patch('ceph')
+        _ceph.import_radosgw_key.return_value = True
+        self.relation_get.return_value = 'seckey'
+        self.socket.gethostname.return_value = 'testinghostname'
+        self.request_per_unit_key.return_value = True
+        ceph_hooks.mon_relation()
+        self.relation_set.assert_called_with(
+            relation_id=None,
+            key_name='rgw.testinghostname'
+        )
+        self.service_restart.assert_called_once_with('radosgw')
+        self.service.assert_called_once_with('enable', 'radosgw')
+        _ceph.import_radosgw_key.assert_called_with('seckey',
+                                                    name='rgw.testinghostname')
         self.CONFIGS.write_all.assert_called_with()
 
     @patch.object(ceph_hooks, 'is_request_complete',
                   lambda *args, **kwargs: True)
     def test_mon_relation_nokey(self):
         _ceph = self.patch('ceph')
-        _restart = self.patch('restart')
+        _ceph.import_radosgw_key.return_value = False
         self.relation_get.return_value = None
         ceph_hooks.mon_relation()
         self.assertFalse(_ceph.import_radosgw_key.called)
-        self.assertFalse(_restart.called)
+        self.service_restart.assert_not_called()
+        self.service.assert_not_called()
         self.CONFIGS.write_all.assert_called_with()
 
     @patch.object(ceph_hooks, 'send_request_if_needed')
@@ -119,10 +154,11 @@ class CephRadosGWTests(CharmTestCase):
     def test_mon_relation_send_broker_request(self,
                                               mock_send_request_if_needed):
         _ceph = self.patch('ceph')
-        _restart = self.patch('restart')
+        _ceph.import_radosgw_key.return_value = False
         self.relation_get.return_value = 'seckey'
         ceph_hooks.mon_relation()
-        self.assertFalse(_restart.called)
+        self.service_restart.assert_not_called()
+        self.service.assert_not_called()
         self.assertFalse(_ceph.import_radosgw_key.called)
         self.assertFalse(self.CONFIGS.called)
         self.assertTrue(mock_send_request_if_needed.called)
@@ -132,21 +168,6 @@ class CephRadosGWTests(CharmTestCase):
         ceph_hooks.gateway_relation()
         self.relation_set.assert_called_with(hostname='10.0.0.1', port=80)
 
-    def test_start(self):
-        ceph_hooks.start()
-        cmd = ['service', 'radosgw', 'start']
-        self.subprocess.call.assert_called_with(cmd)
-
-    def test_stop(self):
-        ceph_hooks.stop()
-        cmd = ['service', 'radosgw', 'stop']
-        self.subprocess.call.assert_called_with(cmd)
-
-    def test_restart(self):
-        ceph_hooks.restart()
-        cmd = ['service', 'radosgw', 'restart']
-        self.subprocess.call.assert_called_with(cmd)
-
     @patch('charmhelpers.contrib.openstack.ip.service_name',
            lambda *args: 'ceph-radosgw')
     @patch('charmhelpers.contrib.openstack.ip.config')
@@ -200,10 +221,8 @@ class CephRadosGWTests(CharmTestCase):
 
     @patch.object(ceph_hooks, 'identity_joined')
     def test_identity_changed(self, mock_identity_joined):
-        _restart = self.patch('restart')
         ceph_hooks.identity_changed()
         self.CONFIGS.write_all.assert_called_with()
-        self.assertTrue(_restart.called)
         self.assertTrue(mock_identity_joined.called)
 
     @patch('charmhelpers.contrib.openstack.ip.is_clustered')