Merge lp:~tribaal/charms/trusty/rabbitmq-server/add-pause-resume-actions into lp:~openstack-charmers-archive/charms/trusty/rabbitmq-server/next

[Chris Glass]

This branch adds two unit-level actions, "pause" and "resume" to the charm, that users can execute to pause and respectively resume a particular unit of a rabbitmq-server cluster, similar to the way other charms implement the same feature (ceilometer, swift*).

A unit that is "paused" should not have any responsability anymore and should be allowed to be taken offline without further warning. In Rabbitmq's case this is pretty simple, since the application itself allows for this to happen by default. Pausing the unit will simply stop the rabbitmq service on the given unit. Other charms need to make sure no data is lost.

One small non-obvious change here: the @restart_on_change decorator is further wrapped to make it "pause aware" otherwise some service-level configuration changes would restart the rabbitmq services on paused units. Again, this is not critical in rabbitmq's case since the application logic should handle a killed service gracefully, but was added anyway for consistency's sake.

[Geoff Teale (tealeg)]

+1 Approve.

review: Approve

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

charm_unit_test #12768 rabbitmq-server-next for tribaal mp277418
    UNIT FAIL: unit-test failed

UNIT Results (max last 2 lines):
make: *** [test] Error 1
ERROR:root:Make target returned non-zero.

Full unit test output: http://paste.ubuntu.com/13246423/
Build: http://10.245.162.77:8080/job/charm_unit_test/12768/

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

charm_lint_check #13698 rabbitmq-server-next for tribaal mp277418
    LINT FAIL: lint-test failed

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

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

[James Page (james-page)]

I think we need to avoid calling status_set directly in the actions, and move to updating the assess_status function in the main relations file to be pause aware; this will ensure that the next 'update-status' hook execution (which happens periodically in 1.25) will set the correct status of 'paused' rather than determining that rabbitmq is not running and setting 'blocked'

review: Needs Fixing

[Chris Glass (tribaal)]

> I think we need to avoid calling status_set directly in the actions, and move
> to updating the assess_status function in the main relations file to be pause
> aware; this will ensure that the next 'update-status' hook execution (which
> happens periodically in 1.25) will set the correct status of 'paused' rather
> than determining that rabbitmq is not running and setting 'blocked'

Excellent point. Changed the "status_set" to be in assess_status() as you suggested.

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

charm_amulet_test #7855 rabbitmq-server-next for tribaal mp277418
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
make: *** [functional_test] Error 1
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/13246587/
Build: http://10.245.162.77:8080/job/charm_amulet_test/7855/

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

charm_unit_test #12769 rabbitmq-server-next for tribaal mp277418
    UNIT OK: passed

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

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

charm_lint_check #13699 rabbitmq-server-next for tribaal mp277418
    LINT OK: passed

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

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

charm_amulet_test #7856 rabbitmq-server-next for tribaal mp277418
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
make: *** [functional_test] Error 1
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/13246796/
Build: http://10.245.162.77:8080/job/charm_amulet_test/7856/

[James Page (james-page)]

Hmm - that amulet test look suspicious as the rabbitmq cluster is not clustering correctly

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

charm_amulet_test #7859 rabbitmq-server-next for tribaal mp277418
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
2015-11-13 18:31:20,197 publish_amqp_message_by_unit DEBUG: Closing connection...
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/13249544/
Build: http://10.245.162.77:8080/job/charm_amulet_test/7859/

[Ryan Beisner (1chb1n)]

Please also add lint coverage for the new actions dir via the Makefile's lint target. Thank you.

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

charm_amulet_test #7860 rabbitmq-server-next for tribaal mp277418
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
MESSAGE 1@172.17.107.152 [4B29C6B3-6F9A-4DF2-9A09-3675F6DE0EEA-1447444725.03]
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/13250328/
Build: http://10.245.162.77:8080/job/charm_amulet_test/7860/

[Chris Glass (tribaal)]

> Please also add lint coverage for the new actions dir via the Makefile's lint
> target. Thank you.

Good catch! Done.

Thanks!

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

charm_unit_test #12924 rabbitmq-server-next for tribaal mp277418
    UNIT OK: passed

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

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

charm_lint_check #13863 rabbitmq-server-next for tribaal mp277418
    LINT OK: passed

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

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

charm_lint_check #13866 rabbitmq-server-next for tribaal mp277418
    LINT OK: passed

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

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

charm_unit_test #12926 rabbitmq-server-next for tribaal mp277418
    UNIT OK: passed

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

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

charm_amulet_test #7900 rabbitmq-server-next for tribaal mp277418
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
2015-11-16 22:33:08,393 _test_rmq_amqp_messages_all_units DEBUG: Publish message to: 172.17.105.97 (rabbitmq-server/0 juju-osci-sv05-machine-4)
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/13303433/
Build: http://10.245.162.77:8080/job/charm_amulet_test/7900/

[David Ames (thedac)]

Tests need fixing:
Traceback (most recent call last):
  File "tests/014-basic-precise-icehouse", line 11, in <module>
    deployment.run_tests()
  File "/var/lib/jenkins/checkout/rabbitmq-server/tests/charmhelpers/contrib/amulet/deployment.py", line 95, in run_tests
    getattr(self, test)()
  File "/var/lib/jenkins/checkout/rabbitmq-server/tests/basic_deployment.py", line 587, in test_416_pause_resume_actions
    self._test_pause()
  File "/var/lib/jenkins/checkout/rabbitmq-server/tests/basic_deployment.py", line 551, in _test_pause
    self._assert_services(should_run=True)
AttributeError: 'RmqBasicDeployment' object has no attribute '_assert_services'

review: Needs Fixing

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

charm_unit_test #12978 rabbitmq-server-next for tribaal mp277418
    UNIT OK: passed

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

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

charm_lint_check #13923 rabbitmq-server-next for tribaal mp277418
    LINT OK: passed

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

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

charm_amulet_test #7905 rabbitmq-server-next for tribaal mp277418
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
2015-11-17 14:35:37,575 connect_amqp_by_unit DEBUG: Connecting to amqp on 172.17.107.199:5671 (rabbitmq-server/2) as testuser1...
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/13313632/
Build: http://10.245.162.77:8080/job/charm_amulet_test/7905/

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

charm_unit_test #12981 rabbitmq-server-next for tribaal mp277418
    UNIT OK: passed

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

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

charm_lint_check #13925 rabbitmq-server-next for tribaal mp277418
    LINT OK: passed

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

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

charm_amulet_test #7908 rabbitmq-server-next for tribaal mp277418
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
2015-11-17 16:00:50,154 connect_amqp_by_unit DEBUG: Connecting to amqp on 172.17.106.18:5671 (rabbitmq-server/1) as testuser1...
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/13314342/
Build: http://10.245.162.77:8080/job/charm_amulet_test/7908/

[Ryan Beisner (1chb1n)]

hi @tribaal

It's hard to say why the test failed on the pause action. I'd suggest adding a check + debug output when it fails the == check, before returning from wait_on_action() in charmhelpers/contrib/amulet/utils.py.

Unmerged revisions

134. By Chris Glass on 2015-11-17

This should hopefully fix tests once and for all.

133. By Chris Glass on 2015-11-17

Added missing method (straight copy from the swift-proxy charm).

132. By Chris Glass on 2015-11-16

Re-added actions directory to linting after commit revert.

131. By Chris Glass on 2015-11-16

Reverting last change - unintended charmhelpers sync was pulled in as well.

130. By Chris Glass on 2015-11-16

Added new actions directory to lint makefile target.

129. By Chris Glass on 2015-11-13

Fixed missing mock for tests.

128. By Chris Glass on 2015-11-13

Lint fixes.

127. By Chris Glass on 2015-11-13

Move the maintenance status set to the assess_status() function. (james's review)

126. By Chris Glass on 2015-11-13

Forgot the tests

125. By Chris Glass on 2015-11-13

First pass to make rabbitmq-server maintenance-aware.

=== modified file 'Makefile'
--- Makefile	2015-10-21 13:36:00 +0000
+++ Makefile	2015-11-17 15:16:48 +0000
@@ -17,7 +17,7 @@
 
 lint: .venv
 	@.venv/bin/flake8 --exclude hooks/charmhelpers,tests/charmhelpers hooks \
-        unit_tests tests
+        unit_tests tests actions
 	@charm proof
 
 bin/charm_helpers_sync.py:

=== added directory 'actions'
=== added file 'actions.yaml'
--- actions.yaml	1970-01-01 00:00:00 +0000
+++ actions.yaml	2015-11-17 15:16:48 +0000
@@ -0,0 +1,4 @@
+pause:
+  description: Pause the RabbitMQ unit.  This action will stop RabbitMQ services.
+resume:
+  descrpition: Resume the RabbitMQ unit.  This action will start RabbitMQ services.

=== added file 'actions/actions.py'
--- actions/actions.py	1970-01-01 00:00:00 +0000
+++ actions/actions.py	2015-11-17 15:16:48 +0000
@@ -0,0 +1,61 @@
+#!/usr/bin/python
+
+import os
+import sys
+
+from charmhelpers.core.host import service_pause, service_resume
+from charmhelpers.core.hookenv import action_fail
+from charmhelpers.core.unitdata import kv
+
+from rabbit_utils import assess_status
+
+
+def pause(args):
+    """Pause the RabbitMQ services.
+
+    @raises Exception should the service fail to stop.
+    """
+    if not service_pause("rabbitmq-server"):
+        raise Exception("Failed to pause rabbitmq-server.")
+
+    db = kv()
+    db.set('unit-paused', True)
+    db.flush()
+
+    assess_status()
+
+
+def resume(args):
+    """Resume the RabbitMQ services.
+
+    @raises Exception should the service fail to start."""
+    if not service_resume("rabbitmq-server"):
+        raise Exception("Failed to resume rabbitmq-server")
+
+    db = kv()
+    db.set('unit-paused', False)
+    db.flush()
+
+    assess_status()
+
+
+# A dictionary of all the defined actions to callables (which take
+# parsed arguments).
+ACTIONS = {"pause": pause, "resume": resume}
+
+
+def main(args):
+    action_name = os.path.basename(args[0])
+    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))
+
+
+if __name__ == "__main__":
+    sys.exit(main(sys.argv))

=== added symlink 'actions/charmhelpers'
=== target is u'../hooks/charmhelpers'
=== added symlink 'actions/pause'
=== target is u'actions.py'
=== added symlink 'actions/rabbit_utils.py'
=== target is u'../hooks/rabbit_utils.py'
=== added symlink 'actions/resume'
=== target is u'actions.py'
=== modified file 'hooks/rabbit_utils.py'
--- hooks/rabbit_utils.py	2015-10-22 02:05:54 +0000
+++ hooks/rabbit_utils.py	2015-11-17 15:16:48 +0000
@@ -27,6 +27,7 @@
     INFO,
     service_name,
     status_set,
+    status_get,
     cached,
     unit_get,
     relation_set,
@@ -49,6 +50,11 @@
 
 from collections import OrderedDict
 
+from charmhelpers.core.unitdata import (
+    HookData,
+    kv,
+)
+
 PACKAGES = ['rabbitmq-server', 'python-amqplib', 'lockfile-progs']
 
 RABBITMQ_CTL = '/usr/sbin/rabbitmqctl'
@@ -691,6 +697,12 @@
     # NOTE: ensure rabbitmq is actually installed before doing
     #       any checks
     if os.path.exists(RABBITMQ_CTL):
+        # Pause check
+        if is_paused():
+            status_set(
+                'maintenance',
+                "Unit paused - use 'resume' action to resume normal service")
+            return
         # Clustering Check
         peer_ids = relation_ids('cluster')
         if peer_ids and len(related_units(peer_ids[0])):
@@ -751,3 +763,22 @@
                         wait_app()
         return wrapped_f
     return wrap
+
+
+def is_paused(status_get=status_get):
+    """Is the unit paused?"""
+    with HookData()():
+        if kv().get('unit-paused'):
+            return True
+        else:
+            return False
+
+
+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 'hooks/rabbitmq_server_relations.py'
--- hooks/rabbitmq_server_relations.py	2015-10-28 11:33:12 +0000
+++ hooks/rabbitmq_server_relations.py	2015-11-17 15:16:48 +0000
@@ -631,7 +631,7 @@
 
 
 @hooks.hook('config-changed')
[e-mail]_on_change(rabbit.restart_map())
[e-mail]_aware_restart_on_change(rabbit.restart_map())
 def config_changed():
 
     if config('prefer-ipv6'):

=== modified file 'tests/basic_deployment.py'
--- tests/basic_deployment.py	2015-10-21 04:54:45 +0000
+++ tests/basic_deployment.py	2015-11-17 15:16:48 +0000
@@ -545,3 +545,128 @@
         u.rmq_wait_for_cluster(self)
 
         u.log.info('OK\n')
+
+    def _assert_services(self, should_run):
+        rabbitmq_services = ['rabbitmq-server']
+
+        sentry_units = self._get_rmq_sentry_units()
+        sentry = sentry_units[0]  # We need to run actions on a single unit
+
+        u.get_unit_process_ids(
+            {sentry: rabbitmq_services}, expect_success=should_run)
+
+    def _test_pause(self):
+        u.log.info("Testing pause action")
+        self._assert_services(should_run=True)
+        sentry_units = self._get_rmq_sentry_units()
+        sentry = sentry_units[0]  # We need to run actions on a single unit
+
+        pause_action_id = u.run_action(sentry, "pause")
+        assert u.wait_on_action(pause_action_id), "Pause action failed."
+
+        self._assert_services(should_run=False)
+        status, message = u.status_get(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)
+        sentry_units = self._get_rmq_sentry_units()
+        sentry = sentry_units[0]  # We need to run actions on a single unit
+
+        resume_action_id = u.run_action(sentry, "resume")
+        assert u.wait_on_action(resume_action_id), "Resume action failed."
+
+        self._assert_services(should_run=True)
+        status, message = u.status_get(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 != "Unit is ready":
+            msg = ("Resume action failed to clear message"
+                   " (got {} instead)".format(message))
+            amulet.raise_status(amulet.FAIL, msg=msg)
+
+    def test_416_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_417_restart_on_config_change(self):
+        """Verify that the specified services are restarted when the config
+           is changed."""
+        u.log.info('Checking that conf files and system services respond '
+                   'to a charm config change...')
+
+        sentry_units = self._get_rmq_sentry_units()
+        sentry = sentry_units[0]  # We need to run actions on a single unit
+        juju_service = 'rabbitmq-server'
+
+        # Process names, corresponding conf files
+        services = {'rabbitmq-server': '/etc/rabbitmq/rabbitmq-env.conf'}
+
+        # Expected default and alternate values
+        set_default = {'ssl': 'off'}
+        set_alternate = {'ssl': 'on'}
+
+        # Make config change, check for service restarts
+        u.log.debug('Making config change on {}...'.format(juju_service))
+        mtime = u.get_sentry_time(sentry)
+        self.d.configure(juju_service, set_alternate)
+
+        sleep_time = 40
+        for s, conf_file in services.iteritems():
+            u.log.debug("Checking that service restarted: {}".format(s))
+            if not u.validate_service_config_changed(sentry, mtime, s,
+                                                     conf_file,
+                                                     sleep_time=sleep_time):
+                self.d.configure(juju_service, set_default)
+                msg = "service {} didn't restart after config change".format(s)
+                amulet.raise_status(amulet.FAIL, msg=msg)
+            sleep_time = 0
+
+        self.d.configure(juju_service, set_default)
+
+    def test_418_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_units = self._get_rmq_sentry_units()
+        sentry = sentry_units[0]  # We need to run actions on a single unit
+        juju_service = 'rabbitmq-server'
+
+        # Expected default and alternate values
+        set_default = {'ssl': 'off'}
+        set_alternate = {'ssl': 'on'}
+
+        services = ['rabbitmq-server']
+
+        # 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."

=== modified file 'unit_tests/test_rabbitmq_server_relations.py'
--- unit_tests/test_rabbitmq_server_relations.py	2015-10-19 03:33:39 +0000
+++ unit_tests/test_rabbitmq_server_relations.py	2015-11-17 15:16:48 +0000
@@ -3,7 +3,8 @@
 from mock import patch, MagicMock
 
 os.environ['JUJU_UNIT_NAME'] = 'UNIT_TEST/0'  # noqa - needed for import
-import rabbitmq_server_relations
+with patch('rabbit_utils.is_paused') as is_paused:
+    import rabbitmq_server_relations
 
 
 class RelationUtil(TestCase):