Merge lp:~adam-collard/charms/trusty/swift-proxy/guard-paused-unit-service-restarts into lp:~openstack-charmers-archive/charms/trusty/swift-proxy/next

[Adam Collard]

This is a follow up branch to https://code.launchpad.net/~adam-collard/charms/trusty/swift-proxy/add-pause-resume-actions/+merge/270407 to prevent config-changed and other hooks from blindly (re)starting services when the unit is paused.

Normal flow is pause(), (optionally) change config -> changes should be persisted on disk, but nothing started. resume() -> services started.

It's a mirror of https://code.launchpad.net/~adam-collard/charms/trusty/swift-storage/guard-paused-unit-service-restarts/+merge/269860

[lp:~adam-collard/charms/trusty/swift-proxy/guard-paused-unit-service-restarts]

114. By Adam Collard on 2015-09-10

Back out accidental commit of pointer to tealeg's charm-helpers branch

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

charm_lint_check #9702 swift-proxy-next for adam-collard mp270641
    LINT FAIL: lint-test failed
    LINT FAIL: charm-proof failed

LINT Results (max last 2 lines):
make: *** [lint] Error 100
ERROR:root:Make target returned non-zero.

Full lint test output: http://paste.ubuntu.com/12328231/
Build: http://10.245.162.77:8080/job/charm_lint_check/9702/

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

charm_unit_test #8938 swift-proxy-next for adam-collard mp270641
    UNIT OK: passed

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

[lp:~adam-collard/charms/trusty/swift-proxy/guard-paused-unit-service-restarts]

115. By Adam Collard on 2015-09-10

Merge from ../add-pause-resume-actions/

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

charm_lint_check #9716 swift-proxy-next for adam-collard mp270641
    LINT OK: passed

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

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

charm_unit_test #8947 swift-proxy-next for adam-collard mp270641
    UNIT OK: passed

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

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

charm_amulet_test #6342 swift-proxy-next for adam-collard mp270641
    AMULET OK: passed

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

[Geoff Teale (tealeg)]

+1

review: Approve

[Chris Glass (tribaal)]

+1, Merged.

review: Approve

=== modified file 'hooks/swift_hooks.py'
--- hooks/swift_hooks.py	2015-09-07 13:44:04 +0000
+++ hooks/swift_hooks.py	2015-09-10 14:15:17 +0000
@@ -32,6 +32,8 @@
     get_first_available_value,
     all_responses_equal,
     ensure_www_dir_permissions,
+    is_paused,
+    pause_aware_restart_on_change
 )
 
 import charmhelpers.contrib.openstack.utils as openstack
@@ -60,7 +62,6 @@
     service_restart,
     service_stop,
     service_start,
-    restart_on_change,
 )
 from charmhelpers.fetch import (
     apt_install,
@@ -121,7 +122,7 @@
 
 
 @hooks.hook('config-changed')
-@restart_on_change(restart_map())
+@pause_aware_restart_on_change(restart_map())
 def config_changed():
     if config('prefer-ipv6'):
         setup_ipv6()
@@ -161,7 +162,7 @@
 
 
 @hooks.hook('identity-service-relation-changed')
-@restart_on_change(restart_map())
+@pause_aware_restart_on_change(restart_map())
 def keystone_changed():
     configure_https()
 
@@ -181,7 +182,7 @@
 
 
 @hooks.hook('swift-storage-relation-changed')
-@restart_on_change(restart_map())
+@pause_aware_restart_on_change(restart_map())
 def storage_changed():
     """Storage relation.
 
@@ -231,13 +232,14 @@
         node_settings['devices'] = devs.split(':')
 
     update_rings(node_settings)
-    # Restart proxy here in case no config changes made (so
-    # restart_on_change() ineffective).
-    service_restart('swift-proxy')
+    if not is_paused():
+        # Restart proxy here in case no config changes made (so
+        # pause_aware_restart_on_change() ineffective).
+        service_restart('swift-proxy')
 
 
 @hooks.hook('swift-storage-relation-broken')
-@restart_on_change(restart_map())
+@pause_aware_restart_on_change(restart_map())
 def storage_broken():
     CONFIGS.write_all()
 
@@ -358,7 +360,8 @@
     broker = settings.get('builder-broker', None)
     if not broker:
         log("No update available", level=DEBUG)
-        service_start('swift-proxy')
+        if not is_paused():
+            service_start('swift-proxy')
         return
 
     builders_only = int(settings.get('sync-only-builders', 0))
@@ -375,14 +378,15 @@
     if fully_synced():
         log("Ring builders synced - starting proxy", level=INFO)
         CONFIGS.write_all()
-        service_start('swift-proxy')
+        if not is_paused():
+            service_start('swift-proxy')
     else:
         log("Not all builders and rings synced yet - waiting for peer sync "
             "before starting proxy", level=INFO)
 
 
 @hooks.hook('cluster-relation-changed')
-@restart_on_change(restart_map())
+@pause_aware_restart_on_change(restart_map())
 def cluster_changed():
     key = SwiftProxyClusterRPC.KEY_NOTIFY_LEADER_CHANGED
     leader_changed = relation_get(attribute=key)
@@ -481,9 +485,10 @@
     if os.path.exists('/usr/sbin/a2enconf'):
         check_call(['a2enconf', 'swift-rings'])
 
-    # TODO: improve this by checking if local CN certs are available
-    # first then checking reload status (see LP #1433114).
-    service_reload('apache2', restart_on_failure=True)
+    if not is_paused():
+        # TODO: improve this by checking if local CN certs are available
+        # first then checking reload status (see LP #1433114).
+        service_reload('apache2', restart_on_failure=True)
 
     for rid in relation_ids('identity-service'):
         keystone_joined(relid=rid)

=== modified file 'lib/swift_utils.py'
--- lib/swift_utils.py	2015-09-07 13:44:04 +0000
+++ lib/swift_utils.py	2015-09-10 14:15:17 +0000
@@ -42,6 +42,7 @@
     relation_set,
     relation_ids,
     related_units,
+    status_get
 )
 from charmhelpers.fetch import (
     apt_update,
@@ -50,7 +51,8 @@
     add_source
 )
 from charmhelpers.core.host import (
-    lsb_release
+    lsb_release,
+    restart_on_change,
 )
 from charmhelpers.contrib.network.ip import (
     format_ipv6_addr,
@@ -994,3 +996,19 @@
         return get_ipv6_addr(exc_list=[config('vip')])[0]
 
     return unit_get('private-address')
+
+
+def is_paused(status_get=status_get):
+    """Is the unit paused?"""
+    status, message = status_get()
+    return status == "maintenance" and message.startswith("Paused")
+
+
+def pause_aware_restart_on_change(restart_map):
+    """Avoids restarting services if config changes when unit is paused."""
+    def wrapper(f):
+        if is_paused():
+            return f
+        else:
+            return restart_on_change(restart_map)(f)
+    return wrapper

=== modified file 'tests/basic_deployment.py'
--- tests/basic_deployment.py	2015-09-10 14:15:17 +0000
+++ tests/basic_deployment.py	2015-09-10 14:15:17 +0000
@@ -333,6 +333,39 @@
 
         self.d.configure('swift-proxy', {'node-timeout': '60'})
 
+    def test_z_no_restart_on_config_change_when_paused(self):
+        """Verify that the specified services are not restarted when the config
+           is changed and the unit is paused."""
+        u.log.info('Checking that system services do not get restarted  '
+                   'when charm config changes but unit is paused...')
+        sentry = self.swift_proxy_sentry
+        juju_service = 'swift-proxy'
+
+        # Expected default and alternate values
+        set_default = {'node-timeout': '60'}
+        set_alternate = {'node-timeout': '90'}
+
+        services = ['swift-proxy', 'haproxy', 'apache2', 'memcached']
+
+        # Pause the unit
+        u.log.debug('Pausing the unit...')
+        pause_action_id = u.run_action(sentry, "pause")
+        assert u.wait_on_action(pause_action_id), "Resume action failed."
+        # Make config change, check for service restarts
+        u.log.debug('Making config change on {}...'.format(juju_service))
+        self.d.configure(juju_service, set_alternate)
+
+        for service in services:
+            u.log.debug("Checking that service didn't start while "
+                        "paused: {}".format(service))
+            # No explicit assert because get_process_id_list will do it for us
+            u.get_process_id_list(
+                sentry, service, expect_success=False)
+
+        self.d.configure(juju_service, set_default)
+        resume_action_id = u.run_action(sentry, "resume")
+        assert u.wait_on_action(resume_action_id), "Resume action failed."
+
     def _assert_services(self, should_run):
         swift_proxy_services = ['swift-proxy-server',
                                 'haproxy',

=== modified file 'unit_tests/test_swift_utils.py'
--- unit_tests/test_swift_utils.py	2015-09-07 13:44:04 +0000
+++ unit_tests/test_swift_utils.py	2015-09-10 14:15:17 +0000
@@ -222,3 +222,15 @@
 
         rsps = []
         self.assertIsNone(swift_utils.get_first_available_value(rsps, 'key3'))
+
+    def test_is_paused_unknown(self):
+        fake_status_get = lambda: ("unknown", "")
+        self.assertFalse(swift_utils.is_paused(status_get=fake_status_get))
+
+    def test_is_paused_paused(self):
+        fake_status_get = lambda: ("maintenance", "Paused")
+        self.assertTrue(swift_utils.is_paused(status_get=fake_status_get))
+
+    def test_is_paused_other_maintenance(self):
+        fake_status_get = lambda: ("maintenance", "Hook")
+        self.assertFalse(swift_utils.is_paused(status_get=fake_status_get))