Merge lp:~adam-collard/charms/trusty/swift-proxy/add-pause-resume-actions into lp:~openstack-charmers-archive/charms/trusty/swift-proxy/next

[Adam Collard]

This branch adds actions for pausing and resuming services on swift-storage units.

In addition to just stopping the services, the actions also set the status of the unit to be "maintenance" with a message on how to resume.

Further refinements to the charm to support pause mode will be coming in follow-up branches, notably guarding calls to service_start and service_restart to prevent config-changed, *-relation-changed and other hooks from (re)-starting a unit which should be paused.

It's a mirror of https://code.launchpad.net/~adam-collard/charms/trusty/swift-storage/add-pause-resume-actions/+merge/268233

Note that this depends on charm-helpers from https://code.launchpad.net/~tealeg/charm-helpers/add-amulet-utils-status-get/+merge/270311

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

charm_lint_check #9595 swift-proxy-next for adam-collard mp270407
    LINT OK: passed

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

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

charm_unit_test #8833 swift-proxy-next for adam-collard mp270407
    UNIT OK: passed

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

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

charm_amulet_test #6319 swift-proxy-next for adam-collard mp270407
    AMULET OK: passed

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

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

charm_unit_test #8834 swift-proxy-next for adam-collard mp270407
    UNIT OK: passed

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

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

charm_lint_check #9596 swift-proxy-next for adam-collard mp270407
    LINT OK: passed

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

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

charm_amulet_test #6320 swift-proxy-next for adam-collard mp270407
    AMULET OK: passed

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

[Geoff Teale (tealeg)]

+1 Sometimes the unit-test + functional-test feels a bit belt-and-braces, but I suppose that's a good thing.

I'd like to see the status get tested in the functional side (As per the in line comment) but that can wait until it's all landed in charmhelpers.

review: Approve

[lp:~adam-collard/charms/trusty/swift-proxy/add-pause-resume-actions]

114. By Adam Collard on 2015-09-09

Sync from tealeg's charm-helpers branch, add tests for status (tealeg's review)

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

charm_lint_check #9708 swift-proxy-next for adam-collard mp270407
    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/12328244/
Build: http://10.245.162.77:8080/job/charm_lint_check/9708/

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

charm_unit_test #8937 swift-proxy-next for adam-collard mp270407
    UNIT OK: passed

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

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

charm_amulet_test #6333 swift-proxy-next for adam-collard mp270407
    AMULET OK: passed

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

[lp:~adam-collard/charms/trusty/swift-proxy/add-pause-resume-actions]

115. By Adam Collard on 2015-09-10

Rename categories to tags (please the linter)

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

charm_lint_check #9715 swift-proxy-next for adam-collard mp270407
    LINT OK: passed

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

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

charm_unit_test #8948 swift-proxy-next for adam-collard mp270407
    UNIT OK: passed

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

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

charm_amulet_test #6346 swift-proxy-next for adam-collard mp270407
    AMULET OK: passed

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

[Chris Glass (tribaal)]

Looks good! +1

review: Approve

=== added directory 'actions'
=== added file 'actions.yaml'
--- actions.yaml	1970-01-01 00:00:00 +0000
+++ actions.yaml	2015-09-10 14:11:09 +0000
@@ -0,0 +1,11 @@
+pause:
+  description: |
+    Pause swift-proxy services.
+    If the swift-proxy deployment is clustered using the hacluster charm, the
+    corresponding hacluster unit on the node must first be paused as well.
+    Not doing so may lead to an interruption of service.
+resume:
+  description: |
+    Resume swift-proxy services.
+    If the swift-proxy deployment is clustered using the hacluster charm, the
+    corresponding hacluster unit on the node must be resumed as well.
 
=== added file 'actions/__init__.py'
=== added file 'actions/actions.py'
--- actions/actions.py	1970-01-01 00:00:00 +0000
+++ actions/actions.py	2015-09-10 14:11:09 +0000
@@ -0,0 +1,83 @@
+#!/usr/bin/python
+
+import argparse
+import os
+import sys
+import yaml
+
+from charmhelpers.core.host import service_pause, service_resume
+from charmhelpers.core.hookenv import action_fail, status_set
+
+from lib.swift_utils import services
+
+
+def get_action_parser(actions_yaml_path, action_name,
+                      get_services=services):
+    """Make an argparse.ArgumentParser seeded from actions.yaml definitions."""
+    with open(actions_yaml_path) as fh:
+        doc = yaml.load(fh)[action_name]["description"]
+    parser = argparse.ArgumentParser(description=doc)
+    parser.add_argument("--services", default=get_services())
+    # TODO: Add arguments for params defined in the actions.yaml
+    return parser
+
+
+def pause(args):
+    """Pause all the swift services.
+
+    @raises Exception if any services fail to stop
+    """
+    for service in args.services:
+        stopped = service_pause(service)
+        if not stopped:
+            raise Exception("{} didn't stop cleanly.".format(service))
+    status_set(
+        "maintenance", "Paused. Use 'resume' action to resume normal service.")
+
+
+def resume(args):
+    """Resume all the swift services.
+
+    @raises Exception if any services fail to start
+    """
+    for service in args.services:
+        started = service_resume(service)
+        if not started:
+            raise Exception("{} didn't start cleanly.".format(service))
+    status_set("active", "")
+
+
+# A dictionary of all the defined actions to callables (which take
+# parsed arguments).
+ACTIONS = {"pause": pause, "resume": resume}
+
+
+def main(argv):
+    action_name = _get_action_name()
+    actions_yaml_path = _get_actions_yaml_path()
+    parser = get_action_parser(actions_yaml_path, action_name)
+    args = parser.parse_args(argv)
+    try:
+        action = ACTIONS[action_name]
+    except KeyError:
+        return "Action %s undefined" % action_name
+    else:
+        try:
+            action(args)
+        except Exception as e:
+            action_fail(str(e))
+
+
+def _get_action_name():
+    """Return the name of the action."""
+    return os.path.basename(__file__)
+
+
+def _get_actions_yaml_path():
+    """Return the path to actions.yaml"""
+    cwd = os.path.dirname(__file__)
+    return os.path.join(cwd, "..", "actions.yaml")
+
+
+if __name__ == "__main__":
+    sys.exit(main(sys.argv[1:]))
 
=== added symlink 'actions/charmhelpers'
=== target is u'../charmhelpers/'
=== added symlink 'actions/lib'
=== target is u'../lib/'
=== added symlink 'actions/pause'
=== target is u'actions.py'
=== added symlink 'actions/resume'
=== target is u'actions.py'
=== modified file 'charmhelpers/core/host.py'
--- charmhelpers/core/host.py	2015-08-19 13:49:37 +0000
+++ charmhelpers/core/host.py	2015-09-10 14:11:09 +0000
@@ -63,32 +63,48 @@
     return service_result
 
 
-def service_pause(service_name, init_dir=None):
+def service_pause(service_name, init_dir="/etc/init", initd_dir="/etc/init.d"):
     """Pause a system service.
 
     Stop it, and prevent it from starting again at boot."""
-    if init_dir is None:
-        init_dir = "/etc/init"
     stopped = service_stop(service_name)
-    # XXX: Support systemd too
-    override_path = os.path.join(
-        init_dir, '{}.override'.format(service_name))
-    with open(override_path, 'w') as fh:
-        fh.write("manual\n")
+    upstart_file = os.path.join(init_dir, "{}.conf".format(service_name))
+    sysv_file = os.path.join(initd_dir, service_name)
+    if os.path.exists(upstart_file):
+        override_path = os.path.join(
+            init_dir, '{}.override'.format(service_name))
+        with open(override_path, 'w') as fh:
+            fh.write("manual\n")
+    elif os.path.exists(sysv_file):
+        subprocess.check_call(["update-rc.d", service_name, "disable"])
+    else:
+        # XXX: Support SystemD too
+        raise ValueError(
+            "Unable to detect {0} as either Upstart {1} or SysV {2}".format(
+                service_name, upstart_file, sysv_file))
     return stopped
 
 
-def service_resume(service_name, init_dir=None):
+def service_resume(service_name, init_dir="/etc/init",
+                   initd_dir="/etc/init.d"):
     """Resume a system service.
 
     Reenable starting again at boot. Start the service"""
-    # XXX: Support systemd too
-    if init_dir is None:
-        init_dir = "/etc/init"
-    override_path = os.path.join(
-        init_dir, '{}.override'.format(service_name))
-    if os.path.exists(override_path):
-        os.unlink(override_path)
+    upstart_file = os.path.join(init_dir, "{}.conf".format(service_name))
+    sysv_file = os.path.join(initd_dir, service_name)
+    if os.path.exists(upstart_file):
+        override_path = os.path.join(
+            init_dir, '{}.override'.format(service_name))
+        if os.path.exists(override_path):
+            os.unlink(override_path)
+    elif os.path.exists(sysv_file):
+        subprocess.check_call(["update-rc.d", service_name, "enable"])
+    else:
+        # XXX: Support SystemD too
+        raise ValueError(
+            "Unable to detect {0} as either Upstart {1} or SysV {2}".format(
+                service_name, upstart_file, sysv_file))
+
     started = service_start(service_name)
     return started
 
 
=== modified file 'metadata.yaml'
--- metadata.yaml	2014-10-30 03:30:36 +0000
+++ metadata.yaml	2015-09-10 14:11:09 +0000
@@ -4,7 +4,8 @@
 description: |
   Swift is a distributed virtual object store. This formula deploys the proxy node
   to be related to storage nodes.
-categories:
+tags:
+    - openstack
     - cache-proxy
 provides:
   nrpe-external-master:
 
=== modified file 'tests/basic_deployment.py'
--- tests/basic_deployment.py	2015-04-20 11:17:45 +0000
+++ tests/basic_deployment.py	2015-09-10 14:11:09 +0000
@@ -1,5 +1,3 @@
-#!/usr/bin/python
-
 import amulet
 import swiftclient
 
@@ -335,6 +333,60 @@
 
         self.d.configure('swift-proxy', {'node-timeout': '60'})
 
+    def _assert_services(self, should_run):
+        swift_proxy_services = ['swift-proxy-server',
+                                'haproxy',
+                                'apache2',
+                                'memcached']
+        u.get_unit_process_ids(
+            {self.swift_proxy_sentry: swift_proxy_services},
+            expect_success=should_run)
+        # No point using validate_unit_process_ids, since we don't
+        # care about how many PIDs, merely that they're running, so
+        # would populate expected with either True or False. This
+        # validation is already performed in get_process_id_list
+
+    def _test_pause(self):
+        u.log.info("Testing pause action")
+        self._assert_services(should_run=True)
+        pause_action_id = u.run_action(self.swift_proxy_sentry, "pause")
+        assert u.wait_on_action(pause_action_id), "Pause action failed."
+
+        self._assert_services(should_run=False)
+        status, message = u.status_get(self.swift_proxy_sentry)
+        if status != "maintenance":
+            msg = ("Pause action failed to move unit to maintenance "
+                   "status (got {} instead)".format(status))
+            amulet.raise_status(amulet.FAIL, msg=msg)
+        if message != "Paused. Use 'resume' action to resume normal service.":
+            msg = ("Pause action failed to set message"
+                   " (got {} instead)".format(message))
+            amulet.raise_status(amulet.FAIL, msg=msg)
+
+    def _test_resume(self):
+        u.log.info("Testing resume action")
+        # service is left paused by _test_pause
+        self._assert_services(should_run=False)
+        resume_action_id = u.run_action(self.swift_proxy_sentry, "resume")
+        assert u.wait_on_action(resume_action_id), "Resume action failed."
+
+        self._assert_services(should_run=True)
+        status, message = u.status_get(self.swift_proxy_sentry)
+        if status != "active":
+            msg = ("Resume action failed to move unit to active "
+                   "status (got {} instead)".format(status))
+            amulet.raise_status(amulet.FAIL, msg=msg)
+        if message != "":
+            msg = ("Resume action failed to clear message"
+                   " (got {} instead)".format(message))
+            amulet.raise_status(amulet.FAIL, msg=msg)
+
+    def test_z_pause_resume_actions(self):
+        """Pause and then resume swift-proxy."""
+        u.log.debug('Checking pause/resume actions...')
+        self._test_pause()
+        self._test_resume()
+
     def test_swift_config(self):
         """Verify the data in the swift config file."""
         unit = self.swift_proxy_sentry
 
=== modified file 'tests/charmhelpers/contrib/amulet/utils.py'
--- tests/charmhelpers/contrib/amulet/utils.py	2015-08-18 17:34:36 +0000
+++ tests/charmhelpers/contrib/amulet/utils.py	2015-09-10 14:11:09 +0000
@@ -594,3 +594,12 @@
         output = _check_output(command, universal_newlines=True)
         data = json.loads(output)
         return data.get(u"status") == "completed"
+
+    def status_get(self, unit):
+        """Return the current service status of this unit."""
+        raw_status, return_code = unit.run(
+            "status-get --format=json --include-data")
+        if return_code != 0:
+            return ("unknown", "")
+        status = json.loads(raw_status)
+        return (status["status"], status["message"])
 
=== added file 'unit_tests/test_actions.py'
--- unit_tests/test_actions.py	1970-01-01 00:00:00 +0000
+++ unit_tests/test_actions.py	2015-09-10 14:11:09 +0000
@@ -0,0 +1,204 @@
+import argparse
+import tempfile
+import unittest
+
+import mock
+import yaml
+
+import actions.actions
+
+
+class CharmTestCase(unittest.TestCase):
+
+    def setUp(self, obj, patches):
+        super(CharmTestCase, self).setUp()
+        self.patches = patches
+        self.obj = obj
+        self.patch_all()
+
+    def patch(self, method):
+        _m = mock.patch.object(self.obj, method)
+        mocked = _m.start()
+        self.addCleanup(_m.stop)
+        return mocked
+
+    def patch_all(self):
+        for method in self.patches:
+            setattr(self, method, self.patch(method))
+
+
+class PauseTestCase(CharmTestCase):
+
+    def setUp(self):
+        super(PauseTestCase, self).setUp(
+            actions.actions, ["service_pause", "status_set"])
+
+        class FakeArgs(object):
+            services = ['swift-proxy', 'haproxy', 'memcached', 'apache2']
+        self.args = FakeArgs()
+
+    def test_pauses_services(self):
+        """Pause action pauses all of the Swift services."""
+        pause_calls = []
+
+        def fake_service_pause(svc):
+            pause_calls.append(svc)
+            return True
+
+        self.service_pause.side_effect = fake_service_pause
+
+        actions.actions.pause(self.args)
+        self.assertEqual(
+            pause_calls, ['swift-proxy', 'haproxy', 'memcached', 'apache2'])
+
+    def test_bails_out_early_on_error(self):
+        """Pause action fails early if there are errors stopping a service."""
+        pause_calls = []
+
+        def maybe_kill(svc):
+            if svc == "haproxy":
+                return False
+            else:
+                pause_calls.append(svc)
+                return True
+
+        self.service_pause.side_effect = maybe_kill
+        self.assertRaisesRegexp(
+            Exception, "haproxy didn't stop cleanly.",
+            actions.actions.pause, self.args)
+        self.assertEqual(pause_calls, ["swift-proxy"])
+
+    def test_status_mode(self):
+        """Pause action sets the status to maintenance."""
+        status_calls = []
+        self.status_set.side_effect = lambda state, msg: status_calls.append(
+            state)
+
+        actions.actions.pause(self.args)
+        self.assertEqual(status_calls, ["maintenance"])
+
+    def test_status_message(self):
+        """Pause action sets a status message reflecting that it's paused."""
+        status_calls = []
+        self.status_set.side_effect = lambda state, msg: status_calls.append(
+            msg)
+
+        actions.actions.pause(self.args)
+        self.assertEqual(
+            status_calls, ["Paused. "
+                           "Use 'resume' action to resume normal service."])
+
+
+class ResumeTestCase(CharmTestCase):
+
+    def setUp(self):
+        super(ResumeTestCase, self).setUp(
+            actions.actions, ["service_resume", "status_set"])
+
+        class FakeArgs(object):
+            services = ['swift-proxy', 'haproxy', 'memcached', 'apache2']
+        self.args = FakeArgs()
+
+    def test_resumes_services(self):
+        """Resume action resumes all of the Swift services."""
+        resume_calls = []
+
+        def fake_service_resume(svc):
+            resume_calls.append(svc)
+            return True
+
+        self.service_resume.side_effect = fake_service_resume
+        actions.actions.resume(self.args)
+        self.assertEqual(
+            resume_calls, ['swift-proxy', 'haproxy', 'memcached', 'apache2'])
+
+    def test_bails_out_early_on_error(self):
+        """Resume action fails early if there are errors starting a service."""
+        resume_calls = []
+
+        def maybe_kill(svc):
+            if svc == "haproxy":
+                return False
+            else:
+                resume_calls.append(svc)
+                return True
+
+        self.service_resume.side_effect = maybe_kill
+        self.assertRaisesRegexp(
+            Exception, "haproxy didn't start cleanly.",
+            actions.actions.resume, self.args)
+        self.assertEqual(resume_calls, ['swift-proxy'])
+
+    def test_status_mode(self):
+        """Resume action sets the status to maintenance."""
+        status_calls = []
+        self.status_set.side_effect = lambda state, msg: status_calls.append(
+            state)
+
+        actions.actions.resume(self.args)
+        self.assertEqual(status_calls, ["active"])
+
+    def test_status_message(self):
+        """Resume action sets an empty status message."""
+        status_calls = []
+        self.status_set.side_effect = lambda state, msg: status_calls.append(
+            msg)
+
+        actions.actions.resume(self.args)
+        self.assertEqual(status_calls, [""])
+
+
+class GetActionParserTestCase(unittest.TestCase):
+
+    def test_definition_from_yaml(self):
+        """ArgumentParser is seeded from actions.yaml."""
+        actions_yaml = tempfile.NamedTemporaryFile(
+            prefix="GetActionParserTestCase", suffix="yaml")
+        actions_yaml.write(yaml.dump({"foo": {"description": "Foo is bar"}}))
+        actions_yaml.seek(0)
+        parser = actions.actions.get_action_parser(actions_yaml.name, "foo",
+                                                   get_services=lambda: [])
+        self.assertEqual(parser.description, 'Foo is bar')
+
+
+class MainTestCase(CharmTestCase):
+
+    def setUp(self):
+        super(MainTestCase, self).setUp(
+            actions.actions, ["_get_action_name",
+                              "get_action_parser",
+                              "action_fail"])
+
+    def test_invokes_pause(self):
+        dummy_calls = []
+
+        def dummy_action(args):
+            dummy_calls.append(True)
+
+        self._get_action_name.side_effect = lambda: "foo"
+        self.get_action_parser = lambda: argparse.ArgumentParser()
+        with mock.patch.dict(actions.actions.ACTIONS, {"foo": dummy_action}):
+            actions.actions.main([])
+        self.assertEqual(dummy_calls, [True])
+
+    def test_unknown_action(self):
+        """Unknown actions aren't a traceback."""
+        self._get_action_name.side_effect = lambda: "foo"
+        self.get_action_parser = lambda: argparse.ArgumentParser()
+        exit_string = actions.actions.main([])
+        self.assertEqual("Action foo undefined", exit_string)
+
+    def test_failing_action(self):
+        """Actions which traceback trigger action_fail() calls."""
+        dummy_calls = []
+
+        self.action_fail.side_effect = dummy_calls.append
+        self._get_action_name.side_effect = lambda: "foo"
+
+        def dummy_action(args):
+            raise ValueError("uh oh")
+
+        self.get_action_parser = lambda: argparse.ArgumentParser()
+        with mock.patch.dict(actions.actions.ACTIONS, {"foo": dummy_action}):
+            actions.actions.main([])
+        self.assertEqual(dummy_calls, ["uh oh"])