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

Proposed by Adam Collard
Status: Merged
Merged at revision: 109
Proposed branch: lp:~adam-collard/charms/trusty/swift-proxy/add-pause-resume-actions
Merge into: lp:~openstack-charmers-archive/charms/trusty/swift-proxy/next
Diff against target: 493 lines (+395/-19)
7 files modified
actions.yaml (+11/-0)
actions/actions.py (+83/-0)
charmhelpers/core/host.py (+32/-16)
metadata.yaml (+2/-1)
tests/basic_deployment.py (+54/-2)
tests/charmhelpers/contrib/amulet/utils.py (+9/-0)
unit_tests/test_actions.py (+204/-0)
To merge this branch: bzr merge lp:~adam-collard/charms/trusty/swift-proxy/add-pause-resume-actions
Reviewer Review Type Date Requested Status
Chris Glass (community) Approve
Geoff Teale (community) Approve
Review via email: mp+270407@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

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/

Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

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/

Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

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/

Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

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/

Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

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/

Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

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/

Revision history for this message
Geoff Teale (tealeg) wrote :

+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
114. By Adam Collard

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

Revision history for this message
Adam Collard (adam-collard) :
Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

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/

Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

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/

Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

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/

115. By Adam Collard

Rename categories to tags (please the linter)

Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

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/

Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

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/

Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

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/

Revision history for this message
Chris Glass (tribaal) wrote :

Looks good! +1

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added directory 'actions'
2=== added file 'actions.yaml'
3--- actions.yaml 1970-01-01 00:00:00 +0000
4+++ actions.yaml 2015-09-10 14:11:09 +0000
5@@ -0,0 +1,11 @@
6+pause:
7+ description: |
8+ Pause swift-proxy services.
9+ If the swift-proxy deployment is clustered using the hacluster charm, the
10+ corresponding hacluster unit on the node must first be paused as well.
11+ Not doing so may lead to an interruption of service.
12+resume:
13+ description: |
14+ Resume swift-proxy services.
15+ If the swift-proxy deployment is clustered using the hacluster charm, the
16+ corresponding hacluster unit on the node must be resumed as well.
17
18=== added file 'actions/__init__.py'
19=== added file 'actions/actions.py'
20--- actions/actions.py 1970-01-01 00:00:00 +0000
21+++ actions/actions.py 2015-09-10 14:11:09 +0000
22@@ -0,0 +1,83 @@
23+#!/usr/bin/python
24+
25+import argparse
26+import os
27+import sys
28+import yaml
29+
30+from charmhelpers.core.host import service_pause, service_resume
31+from charmhelpers.core.hookenv import action_fail, status_set
32+
33+from lib.swift_utils import services
34+
35+
36+def get_action_parser(actions_yaml_path, action_name,
37+ get_services=services):
38+ """Make an argparse.ArgumentParser seeded from actions.yaml definitions."""
39+ with open(actions_yaml_path) as fh:
40+ doc = yaml.load(fh)[action_name]["description"]
41+ parser = argparse.ArgumentParser(description=doc)
42+ parser.add_argument("--services", default=get_services())
43+ # TODO: Add arguments for params defined in the actions.yaml
44+ return parser
45+
46+
47+def pause(args):
48+ """Pause all the swift services.
49+
50+ @raises Exception if any services fail to stop
51+ """
52+ for service in args.services:
53+ stopped = service_pause(service)
54+ if not stopped:
55+ raise Exception("{} didn't stop cleanly.".format(service))
56+ status_set(
57+ "maintenance", "Paused. Use 'resume' action to resume normal service.")
58+
59+
60+def resume(args):
61+ """Resume all the swift services.
62+
63+ @raises Exception if any services fail to start
64+ """
65+ for service in args.services:
66+ started = service_resume(service)
67+ if not started:
68+ raise Exception("{} didn't start cleanly.".format(service))
69+ status_set("active", "")
70+
71+
72+# A dictionary of all the defined actions to callables (which take
73+# parsed arguments).
74+ACTIONS = {"pause": pause, "resume": resume}
75+
76+
77+def main(argv):
78+ action_name = _get_action_name()
79+ actions_yaml_path = _get_actions_yaml_path()
80+ parser = get_action_parser(actions_yaml_path, action_name)
81+ args = parser.parse_args(argv)
82+ try:
83+ action = ACTIONS[action_name]
84+ except KeyError:
85+ return "Action %s undefined" % action_name
86+ else:
87+ try:
88+ action(args)
89+ except Exception as e:
90+ action_fail(str(e))
91+
92+
93+def _get_action_name():
94+ """Return the name of the action."""
95+ return os.path.basename(__file__)
96+
97+
98+def _get_actions_yaml_path():
99+ """Return the path to actions.yaml"""
100+ cwd = os.path.dirname(__file__)
101+ return os.path.join(cwd, "..", "actions.yaml")
102+
103+
104+if __name__ == "__main__":
105+ sys.exit(main(sys.argv[1:]))
106
107=== added symlink 'actions/charmhelpers'
108=== target is u'../charmhelpers/'
109=== added symlink 'actions/lib'
110=== target is u'../lib/'
111=== added symlink 'actions/pause'
112=== target is u'actions.py'
113=== added symlink 'actions/resume'
114=== target is u'actions.py'
115=== modified file 'charmhelpers/core/host.py'
116--- charmhelpers/core/host.py 2015-08-19 13:49:37 +0000
117+++ charmhelpers/core/host.py 2015-09-10 14:11:09 +0000
118@@ -63,32 +63,48 @@
119 return service_result
120
121
122-def service_pause(service_name, init_dir=None):
123+def service_pause(service_name, init_dir="/etc/init", initd_dir="/etc/init.d"):
124 """Pause a system service.
125
126 Stop it, and prevent it from starting again at boot."""
127- if init_dir is None:
128- init_dir = "/etc/init"
129 stopped = service_stop(service_name)
130- # XXX: Support systemd too
131- override_path = os.path.join(
132- init_dir, '{}.override'.format(service_name))
133- with open(override_path, 'w') as fh:
134- fh.write("manual\n")
135+ upstart_file = os.path.join(init_dir, "{}.conf".format(service_name))
136+ sysv_file = os.path.join(initd_dir, service_name)
137+ if os.path.exists(upstart_file):
138+ override_path = os.path.join(
139+ init_dir, '{}.override'.format(service_name))
140+ with open(override_path, 'w') as fh:
141+ fh.write("manual\n")
142+ elif os.path.exists(sysv_file):
143+ subprocess.check_call(["update-rc.d", service_name, "disable"])
144+ else:
145+ # XXX: Support SystemD too
146+ raise ValueError(
147+ "Unable to detect {0} as either Upstart {1} or SysV {2}".format(
148+ service_name, upstart_file, sysv_file))
149 return stopped
150
151
152-def service_resume(service_name, init_dir=None):
153+def service_resume(service_name, init_dir="/etc/init",
154+ initd_dir="/etc/init.d"):
155 """Resume a system service.
156
157 Reenable starting again at boot. Start the service"""
158- # XXX: Support systemd too
159- if init_dir is None:
160- init_dir = "/etc/init"
161- override_path = os.path.join(
162- init_dir, '{}.override'.format(service_name))
163- if os.path.exists(override_path):
164- os.unlink(override_path)
165+ upstart_file = os.path.join(init_dir, "{}.conf".format(service_name))
166+ sysv_file = os.path.join(initd_dir, service_name)
167+ if os.path.exists(upstart_file):
168+ override_path = os.path.join(
169+ init_dir, '{}.override'.format(service_name))
170+ if os.path.exists(override_path):
171+ os.unlink(override_path)
172+ elif os.path.exists(sysv_file):
173+ subprocess.check_call(["update-rc.d", service_name, "enable"])
174+ else:
175+ # XXX: Support SystemD too
176+ raise ValueError(
177+ "Unable to detect {0} as either Upstart {1} or SysV {2}".format(
178+ service_name, upstart_file, sysv_file))
179+
180 started = service_start(service_name)
181 return started
182
183
184=== modified file 'metadata.yaml'
185--- metadata.yaml 2014-10-30 03:30:36 +0000
186+++ metadata.yaml 2015-09-10 14:11:09 +0000
187@@ -4,7 +4,8 @@
188 description: |
189 Swift is a distributed virtual object store. This formula deploys the proxy node
190 to be related to storage nodes.
191-categories:
192+tags:
193+ - openstack
194 - cache-proxy
195 provides:
196 nrpe-external-master:
197
198=== modified file 'tests/basic_deployment.py'
199--- tests/basic_deployment.py 2015-04-20 11:17:45 +0000
200+++ tests/basic_deployment.py 2015-09-10 14:11:09 +0000
201@@ -1,5 +1,3 @@
202-#!/usr/bin/python
203-
204 import amulet
205 import swiftclient
206
207@@ -335,6 +333,60 @@
208
209 self.d.configure('swift-proxy', {'node-timeout': '60'})
210
211+ def _assert_services(self, should_run):
212+ swift_proxy_services = ['swift-proxy-server',
213+ 'haproxy',
214+ 'apache2',
215+ 'memcached']
216+ u.get_unit_process_ids(
217+ {self.swift_proxy_sentry: swift_proxy_services},
218+ expect_success=should_run)
219+ # No point using validate_unit_process_ids, since we don't
220+ # care about how many PIDs, merely that they're running, so
221+ # would populate expected with either True or False. This
222+ # validation is already performed in get_process_id_list
223+
224+ def _test_pause(self):
225+ u.log.info("Testing pause action")
226+ self._assert_services(should_run=True)
227+ pause_action_id = u.run_action(self.swift_proxy_sentry, "pause")
228+ assert u.wait_on_action(pause_action_id), "Pause action failed."
229+
230+ self._assert_services(should_run=False)
231+ status, message = u.status_get(self.swift_proxy_sentry)
232+ if status != "maintenance":
233+ msg = ("Pause action failed to move unit to maintenance "
234+ "status (got {} instead)".format(status))
235+ amulet.raise_status(amulet.FAIL, msg=msg)
236+ if message != "Paused. Use 'resume' action to resume normal service.":
237+ msg = ("Pause action failed to set message"
238+ " (got {} instead)".format(message))
239+ amulet.raise_status(amulet.FAIL, msg=msg)
240+
241+ def _test_resume(self):
242+ u.log.info("Testing resume action")
243+ # service is left paused by _test_pause
244+ self._assert_services(should_run=False)
245+ resume_action_id = u.run_action(self.swift_proxy_sentry, "resume")
246+ assert u.wait_on_action(resume_action_id), "Resume action failed."
247+
248+ self._assert_services(should_run=True)
249+ status, message = u.status_get(self.swift_proxy_sentry)
250+ if status != "active":
251+ msg = ("Resume action failed to move unit to active "
252+ "status (got {} instead)".format(status))
253+ amulet.raise_status(amulet.FAIL, msg=msg)
254+ if message != "":
255+ msg = ("Resume action failed to clear message"
256+ " (got {} instead)".format(message))
257+ amulet.raise_status(amulet.FAIL, msg=msg)
258+
259+ def test_z_pause_resume_actions(self):
260+ """Pause and then resume swift-proxy."""
261+ u.log.debug('Checking pause/resume actions...')
262+ self._test_pause()
263+ self._test_resume()
264+
265 def test_swift_config(self):
266 """Verify the data in the swift config file."""
267 unit = self.swift_proxy_sentry
268
269=== modified file 'tests/charmhelpers/contrib/amulet/utils.py'
270--- tests/charmhelpers/contrib/amulet/utils.py 2015-08-18 17:34:36 +0000
271+++ tests/charmhelpers/contrib/amulet/utils.py 2015-09-10 14:11:09 +0000
272@@ -594,3 +594,12 @@
273 output = _check_output(command, universal_newlines=True)
274 data = json.loads(output)
275 return data.get(u"status") == "completed"
276+
277+ def status_get(self, unit):
278+ """Return the current service status of this unit."""
279+ raw_status, return_code = unit.run(
280+ "status-get --format=json --include-data")
281+ if return_code != 0:
282+ return ("unknown", "")
283+ status = json.loads(raw_status)
284+ return (status["status"], status["message"])
285
286=== added file 'unit_tests/test_actions.py'
287--- unit_tests/test_actions.py 1970-01-01 00:00:00 +0000
288+++ unit_tests/test_actions.py 2015-09-10 14:11:09 +0000
289@@ -0,0 +1,204 @@
290+import argparse
291+import tempfile
292+import unittest
293+
294+import mock
295+import yaml
296+
297+import actions.actions
298+
299+
300+class CharmTestCase(unittest.TestCase):
301+
302+ def setUp(self, obj, patches):
303+ super(CharmTestCase, self).setUp()
304+ self.patches = patches
305+ self.obj = obj
306+ self.patch_all()
307+
308+ def patch(self, method):
309+ _m = mock.patch.object(self.obj, method)
310+ mocked = _m.start()
311+ self.addCleanup(_m.stop)
312+ return mocked
313+
314+ def patch_all(self):
315+ for method in self.patches:
316+ setattr(self, method, self.patch(method))
317+
318+
319+class PauseTestCase(CharmTestCase):
320+
321+ def setUp(self):
322+ super(PauseTestCase, self).setUp(
323+ actions.actions, ["service_pause", "status_set"])
324+
325+ class FakeArgs(object):
326+ services = ['swift-proxy', 'haproxy', 'memcached', 'apache2']
327+ self.args = FakeArgs()
328+
329+ def test_pauses_services(self):
330+ """Pause action pauses all of the Swift services."""
331+ pause_calls = []
332+
333+ def fake_service_pause(svc):
334+ pause_calls.append(svc)
335+ return True
336+
337+ self.service_pause.side_effect = fake_service_pause
338+
339+ actions.actions.pause(self.args)
340+ self.assertEqual(
341+ pause_calls, ['swift-proxy', 'haproxy', 'memcached', 'apache2'])
342+
343+ def test_bails_out_early_on_error(self):
344+ """Pause action fails early if there are errors stopping a service."""
345+ pause_calls = []
346+
347+ def maybe_kill(svc):
348+ if svc == "haproxy":
349+ return False
350+ else:
351+ pause_calls.append(svc)
352+ return True
353+
354+ self.service_pause.side_effect = maybe_kill
355+ self.assertRaisesRegexp(
356+ Exception, "haproxy didn't stop cleanly.",
357+ actions.actions.pause, self.args)
358+ self.assertEqual(pause_calls, ["swift-proxy"])
359+
360+ def test_status_mode(self):
361+ """Pause action sets the status to maintenance."""
362+ status_calls = []
363+ self.status_set.side_effect = lambda state, msg: status_calls.append(
364+ state)
365+
366+ actions.actions.pause(self.args)
367+ self.assertEqual(status_calls, ["maintenance"])
368+
369+ def test_status_message(self):
370+ """Pause action sets a status message reflecting that it's paused."""
371+ status_calls = []
372+ self.status_set.side_effect = lambda state, msg: status_calls.append(
373+ msg)
374+
375+ actions.actions.pause(self.args)
376+ self.assertEqual(
377+ status_calls, ["Paused. "
378+ "Use 'resume' action to resume normal service."])
379+
380+
381+class ResumeTestCase(CharmTestCase):
382+
383+ def setUp(self):
384+ super(ResumeTestCase, self).setUp(
385+ actions.actions, ["service_resume", "status_set"])
386+
387+ class FakeArgs(object):
388+ services = ['swift-proxy', 'haproxy', 'memcached', 'apache2']
389+ self.args = FakeArgs()
390+
391+ def test_resumes_services(self):
392+ """Resume action resumes all of the Swift services."""
393+ resume_calls = []
394+
395+ def fake_service_resume(svc):
396+ resume_calls.append(svc)
397+ return True
398+
399+ self.service_resume.side_effect = fake_service_resume
400+ actions.actions.resume(self.args)
401+ self.assertEqual(
402+ resume_calls, ['swift-proxy', 'haproxy', 'memcached', 'apache2'])
403+
404+ def test_bails_out_early_on_error(self):
405+ """Resume action fails early if there are errors starting a service."""
406+ resume_calls = []
407+
408+ def maybe_kill(svc):
409+ if svc == "haproxy":
410+ return False
411+ else:
412+ resume_calls.append(svc)
413+ return True
414+
415+ self.service_resume.side_effect = maybe_kill
416+ self.assertRaisesRegexp(
417+ Exception, "haproxy didn't start cleanly.",
418+ actions.actions.resume, self.args)
419+ self.assertEqual(resume_calls, ['swift-proxy'])
420+
421+ def test_status_mode(self):
422+ """Resume action sets the status to maintenance."""
423+ status_calls = []
424+ self.status_set.side_effect = lambda state, msg: status_calls.append(
425+ state)
426+
427+ actions.actions.resume(self.args)
428+ self.assertEqual(status_calls, ["active"])
429+
430+ def test_status_message(self):
431+ """Resume action sets an empty status message."""
432+ status_calls = []
433+ self.status_set.side_effect = lambda state, msg: status_calls.append(
434+ msg)
435+
436+ actions.actions.resume(self.args)
437+ self.assertEqual(status_calls, [""])
438+
439+
440+class GetActionParserTestCase(unittest.TestCase):
441+
442+ def test_definition_from_yaml(self):
443+ """ArgumentParser is seeded from actions.yaml."""
444+ actions_yaml = tempfile.NamedTemporaryFile(
445+ prefix="GetActionParserTestCase", suffix="yaml")
446+ actions_yaml.write(yaml.dump({"foo": {"description": "Foo is bar"}}))
447+ actions_yaml.seek(0)
448+ parser = actions.actions.get_action_parser(actions_yaml.name, "foo",
449+ get_services=lambda: [])
450+ self.assertEqual(parser.description, 'Foo is bar')
451+
452+
453+class MainTestCase(CharmTestCase):
454+
455+ def setUp(self):
456+ super(MainTestCase, self).setUp(
457+ actions.actions, ["_get_action_name",
458+ "get_action_parser",
459+ "action_fail"])
460+
461+ def test_invokes_pause(self):
462+ dummy_calls = []
463+
464+ def dummy_action(args):
465+ dummy_calls.append(True)
466+
467+ self._get_action_name.side_effect = lambda: "foo"
468+ self.get_action_parser = lambda: argparse.ArgumentParser()
469+ with mock.patch.dict(actions.actions.ACTIONS, {"foo": dummy_action}):
470+ actions.actions.main([])
471+ self.assertEqual(dummy_calls, [True])
472+
473+ def test_unknown_action(self):
474+ """Unknown actions aren't a traceback."""
475+ self._get_action_name.side_effect = lambda: "foo"
476+ self.get_action_parser = lambda: argparse.ArgumentParser()
477+ exit_string = actions.actions.main([])
478+ self.assertEqual("Action foo undefined", exit_string)
479+
480+ def test_failing_action(self):
481+ """Actions which traceback trigger action_fail() calls."""
482+ dummy_calls = []
483+
484+ self.action_fail.side_effect = dummy_calls.append
485+ self._get_action_name.side_effect = lambda: "foo"
486+
487+ def dummy_action(args):
488+ raise ValueError("uh oh")
489+
490+ self.get_action_parser = lambda: argparse.ArgumentParser()
491+ with mock.patch.dict(actions.actions.ACTIONS, {"foo": dummy_action}):
492+ actions.actions.main([])
493+ self.assertEqual(dummy_calls, ["uh oh"])

Subscribers

People subscribed via source and target branches