Merge lp:~thedac/charms/trusty/ceph-radosgw/stop-start into lp:~openstack-charmers-archive/charms/trusty/ceph-radosgw/next

[David Ames]

Fixes Bug #1508551
Check radosgw is running for status
stop/start rather than restart
Remove superfluous restart() call

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #12291 ceph-radosgw-next for thedac mp275225
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/12291/

[Ryan Beisner (1chb1n)]

FYI, it's safe to ignore any amulet tests on this MP.

I've pulled these fixes into my WIP MP for updated tests @:
https://code.launchpad.net/~1chb1n/charms/trusty/ceph-radosgw/next-amulet-15.10/+merge/275107

[lp:~thedac/charms/trusty/ceph-radosgw/stop-start]

51. By David Ames on 2015-10-22

Allow time between start and stop

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #12327 ceph-radosgw-next for thedac mp275225
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/12327/

[lp:~thedac/charms/trusty/ceph-radosgw/stop-start]

52. By David Ames on 2015-10-22

Sleep on restart() also

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #12332 ceph-radosgw-next for thedac mp275225
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/12332/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #11438 ceph-radosgw-next for thedac mp275225
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/11438/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #7498 ceph-radosgw-next for thedac mp275225
    AMULET OK: passed

Build: http://10.245.162.77:8080/job/charm_amulet_test/7498/

[Ryan Beisner (1chb1n)]

I'd recommend that we wait for the underlying init script bug fix to make its way into the releases by way of the updated ceph package, then re-visit this.

https://bugs.launchpad.net/charms/+source/ceph-radosgw/+bug/1477225

https://launchpad.net/ubuntu/+source/ceph/0.94.3-0ubuntu0.15.04.1

review: Needs Information

[Ryan Beisner (1chb1n)]

Perhaps we should go forward with radosgw process check and corresponding status message, though. That is helpful, and creates a more accurate readiness indicator in the charm.

[Liam Young (gnuoy)]

I agree with Ryan that it would be useful to have the status info but the init script fix in the package should negate the need for the restart changes

review: Needs Fixing

Unmerged revisions

52. By David Ames on 2015-10-22

Sleep on restart() also

51. By David Ames on 2015-10-22

Allow time between start and stop

50. By David Ames on 2015-10-21

Pull in CH message fix

49. By David Ames on 2015-10-21

Check radosgw is running for status. stop/start rather than restart. Remove superfluous restart() call

=== modified file 'hooks/charmhelpers/contrib/openstack/utils.py'
--- hooks/charmhelpers/contrib/openstack/utils.py	2015-10-01 20:02:04 +0000
+++ hooks/charmhelpers/contrib/openstack/utils.py	2015-10-22 04:50:31 +0000
@@ -858,7 +858,9 @@
         if charm_state != 'active' and charm_state != 'unknown':
             state = workload_state_compare(state, charm_state)
             if message:
-                message = "{} {}".format(message, charm_message)
+                charm_message = charm_message.replace("Incomplete relations: ",
+                                                      "")
+                message = "{}, {}".format(message, charm_message)
             else:
                 message = charm_message
 

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2015-10-08 12:13:08 +0000
+++ hooks/hooks.py	2015-10-22 04:50:31 +0000
@@ -13,6 +13,7 @@
 import glob
 import os
 import ceph
+import time
 from charmhelpers.core.hookenv import (
     relation_get,
     relation_ids,
@@ -32,7 +33,6 @@
 )
 from charmhelpers.core.host import (
     lsb_release,
-    restart_on_change,
 )
 from utils import (
     render_template,
@@ -42,6 +42,7 @@
     register_configs,
     REQUIRED_INTERFACES,
     check_optional_relations,
+    restart_on_change,
 )
 
 from charmhelpers.payload.execd import execd_preinstall
@@ -158,7 +159,8 @@
 @hooks.hook('upgrade-charm',
             'config-changed')
 @restart_on_change({'/etc/ceph/ceph.conf': ['radosgw'],
-                    '/etc/haproxy/haproxy.cfg': ['haproxy']})
+                    '/etc/haproxy/haproxy.cfg': ['haproxy']},
+                   stopstart=True)
 def config_changed():
     install_packages()
     CONFIGS.write_all()
@@ -176,7 +178,8 @@
 
 @hooks.hook('mon-relation-departed',
             'mon-relation-changed')
-@restart_on_change({'/etc/ceph/ceph.conf': ['radosgw']})
+@restart_on_change({'/etc/ceph/ceph.conf': ['radosgw']},
+                   stopstart=True)
 def mon_relation():
     CONFIGS.write_all()
     key = relation_get('radosgw_key')
@@ -202,7 +205,9 @@
 
 
 def restart():
-    subprocess.call(['service', 'radosgw', 'restart'])
+    stop()
+    time.sleep(5)
+    start()
     open_port(port=80)
 
 
@@ -227,15 +232,16 @@
 
 
 @hooks.hook('identity-service-relation-changed')
-@restart_on_change({'/etc/ceph/ceph.conf': ['radosgw']})
+@restart_on_change({'/etc/ceph/ceph.conf': ['radosgw']},
+                   stopstart=True)
 def identity_changed():
     CONFIGS.write_all()
-    restart()
 
 
 @hooks.hook('cluster-relation-changed',
             'cluster-relation-joined')
-@restart_on_change({'/etc/haproxy/haproxy.cfg': ['haproxy']})
+@restart_on_change({'/etc/haproxy/haproxy.cfg': ['haproxy']},
+                   stopstart=True)
 def cluster_changed():
     CONFIGS.write_all()
     for r_id in relation_ids('identity-service'):

=== modified file 'hooks/utils.py'
--- hooks/utils.py	2015-10-12 10:56:01 +0000
+++ hooks/utils.py	2015-10-22 04:50:31 +0000
@@ -11,13 +11,15 @@
 import os
 import dns.resolver
 import jinja2
+import time
 from copy import deepcopy
 from collections import OrderedDict
 from charmhelpers.core.hookenv import unit_get, relation_ids, status_get
 from charmhelpers.contrib.openstack import context, templating
 from charmhelpers.contrib.openstack.utils import set_os_workload_status
 from charmhelpers.contrib.hahelpers.cluster import get_hacluster_config
-from charmhelpers.core.host import cmp_pkgrevno
+from charmhelpers.core.hookenv import status_set
+from charmhelpers.core.host import cmp_pkgrevno, path_hash, service
 from charmhelpers.fetch import filter_installed_packages
 
 import ceph_radosgw_context
@@ -45,6 +47,8 @@
     }),
 ])
 
+SOCKET_PATH = '/var/run/ceph/ceph-client.radosgw.gateway.asok'
+
 
 def resource_map():
     '''
@@ -115,6 +119,10 @@
 
 
 def check_optional_relations(configs):
+    # Check if radosgw is running by verifying its socket file
+    if not os.path.exists(SOCKET_PATH):
+        return ("blocked",
+                "radosgw is not running")
     required_interfaces = {}
     if relation_ids('ha'):
         required_interfaces['ha'] = ['cluster']
@@ -132,3 +140,45 @@
         return status_get()
     else:
         return 'unknown', 'No optional relations'
+
+
+def restart_on_change(restart_map, stopstart=False):
+    """Restart services based on configuration files changing
+
+    This function is used a decorator, for example::
+
+        @restart_on_change({
+            '/etc/ceph/ceph.conf': [ 'cinder-api', 'cinder-volume' ]
+            '/etc/apache/sites-enabled/*': [ 'apache2' ]
+            })
+        def config_changed():
+            pass  # your code here
+
+    In this example, the cinder-api and cinder-volume services
+    would be restarted if /etc/ceph/ceph.conf is changed by the
+    ceph_client_changed function. The apache2 service would be
+    restarted if any file matching the pattern got changed, created
+    or removed. Standard wildcards are supported, see documentation
+    for the 'glob' module for more information.
+    """
+    def wrap(f):
+        def wrapped_f(*args, **kwargs):
+            checksums = {path: path_hash(path) for path in restart_map}
+            f(*args, **kwargs)
+            restarts = []
+            for path in restart_map:
+                if path_hash(path) != checksums[path]:
+                    restarts += restart_map[path]
+            services_list = list(OrderedDict.fromkeys(restarts))
+            if not stopstart:
+                for service_name in services_list:
+                    service('restart', service_name)
+            else:
+                for action in ['stop', 'start']:
+                    for service_name in services_list:
+                        status_set("maintenance", "{}ing {}"
+                                   "".format(action, service_name))
+                        service(action, service_name)
+                        time.sleep(5)
+        return wrapped_f
+    return wrap

=== modified file 'unit_tests/test_hooks.py'
--- unit_tests/test_hooks.py	2015-10-08 13:17:51 +0000
+++ unit_tests/test_hooks.py	2015-10-22 04:50:31 +0000
@@ -218,8 +218,11 @@
 
     def test_restart(self):
         ceph_hooks.restart()
-        cmd = ['service', 'radosgw', 'restart']
-        self.subprocess.call.assert_called_with(cmd)
+        start = ['service', 'radosgw', 'stop']
+        stop = ['service', 'radosgw', 'start']
+        self.subprocess.call.assert_any_call(start)
+        self.subprocess.call.assert_any_call(stop)
+        assert 2 == self.subprocess.call.call_count
 
     @patch('charmhelpers.contrib.openstack.ip.service_name',
            lambda *args: 'ceph-radosgw')