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

Proposed by Adam Collard on 2015-09-02
Status: Merged
Merged at revision: 80
Proposed branch: lp:~adam-collard/charms/trusty/swift-storage/guard-paused-unit-service-restarts
Merge into: lp:~openstack-charmers-archive/charms/trusty/swift-storage/next
Prerequisite: lp:~adam-collard/charms/trusty/swift-storage/fix-unconditional-service-restart
Diff against target: 158 lines (+75/-5)
4 files modified
hooks/swift_storage_hooks.py (+5/-3)
lib/misc_utils.py (+18/-0)
lib/swift_storage_utils.py (+4/-2)
tests/basic_deployment.py (+48/-0)
To merge this branch: bzr merge lp:~adam-collard/charms/trusty/swift-storage/guard-paused-unit-service-restarts
Reviewer Review Type Date Requested Status
Liam Young 2015-09-02 Approve on 2015-09-07
Alberto Donato (community) 2015-09-02 Approve on 2015-09-02
Review via email: mp+269860@code.launchpad.net

This proposal supersedes a proposal from 2015-08-28.

Description of the change

This is a follow up branch to https://code.launchpad.net/~adam-collard/charms/trusty/swift-storage/add-pause-resume-actions/+merge/268233 to prevent config-changed 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.

To post a comment you must log in.
uosci-testing-bot (uosci-testing-bot) wrote : Posted in a previous version of this proposal

charm_lint_check #8938 swift-storage-next for adam-collard mp269528
    LINT OK: passed

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

uosci-testing-bot (uosci-testing-bot) wrote : Posted in a previous version of this proposal

charm_unit_test #8258 swift-storage-next for adam-collard mp269528
    UNIT OK: passed

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

uosci-testing-bot (uosci-testing-bot) wrote : Posted in a previous version of this proposal

charm_amulet_test #6082 swift-storage-next for adam-collard mp269528
    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/12215822/
Build: http://10.245.162.77:8080/job/charm_amulet_test/6082/

uosci-testing-bot (uosci-testing-bot) wrote : Posted in a previous version of this proposal

charm_lint_check #8939 swift-storage-next for adam-collard mp269528
    LINT OK: passed

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

uosci-testing-bot (uosci-testing-bot) wrote : Posted in a previous version of this proposal

charm_unit_test #8259 swift-storage-next for adam-collard mp269528
    UNIT OK: passed

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

uosci-testing-bot (uosci-testing-bot) wrote : Posted in a previous version of this proposal

charm_amulet_test #6083 swift-storage-next for adam-collard mp269528
    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/12216361/
Build: http://10.245.162.77:8080/job/charm_amulet_test/6083/

charm_lint_check #9227 swift-storage-next for adam-collard mp269860
    LINT OK: passed

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

charm_unit_test #8528 swift-storage-next for adam-collard mp269860
    UNIT OK: passed

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

charm_amulet_test #6189 swift-storage-next for adam-collard mp269860
    AMULET OK: passed

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

Alberto Donato (ack) wrote :

Looks good, +1

one minor comment inline.

review: Approve
97. By Adam Collard on 2015-09-02

Expand list-comp to full for-loop, use SWIFT_SVCS instead of ACCOUNT + CONTAINER + OBJECT (ack's review)

charm_unit_test #8531 swift-storage-next for adam-collard mp269860
    UNIT OK: passed

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

charm_lint_check #9230 swift-storage-next for adam-collard mp269860
    LINT OK: passed

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

charm_amulet_test #6192 swift-storage-next for adam-collard mp269860
    AMULET OK: passed

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

Liam Young (gnuoy) wrote :

Approve

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'hooks/swift_storage_hooks.py'
2--- hooks/swift_storage_hooks.py 2015-07-17 09:57:19 +0000
3+++ hooks/swift_storage_hooks.py 2015-09-02 10:52:42 +0000
4@@ -18,6 +18,8 @@
5 setup_rsync,
6 )
7
8+from lib.misc_utils import pause_aware_restart_on_change
9+
10 from charmhelpers.core.hookenv import (
11 Hooks, UnregisteredHookError,
12 config,
13@@ -32,7 +34,7 @@
14 apt_update,
15 filter_installed_packages
16 )
17-from charmhelpers.core.host import restart_on_change, rsync
18+from charmhelpers.core.host import rsync
19 from charmhelpers.payload.execd import execd_preinstall
20
21 from charmhelpers.contrib.openstack.utils import (
22@@ -63,7 +65,7 @@
23
24
25 @hooks.hook('config-changed')
26-@restart_on_change(RESTART_MAP)
27+@pause_aware_restart_on_change(RESTART_MAP)
28 def config_changed():
29 if config('prefer-ipv6'):
30 assert_charm_supports_ipv6()
31@@ -104,7 +106,7 @@
32
33
34 @hooks.hook('swift-storage-relation-changed')
35-@restart_on_change(RESTART_MAP)
36+@pause_aware_restart_on_change(RESTART_MAP)
37 def swift_storage_relation_changed():
38 rings_url = relation_get('rings_url')
39 swift_hash = relation_get('swift_hash')
40
41=== modified file 'lib/misc_utils.py'
42--- lib/misc_utils.py 2015-07-17 10:51:25 +0000
43+++ lib/misc_utils.py 2015-09-02 10:52:42 +0000
44@@ -16,12 +16,14 @@
45 from charmhelpers.core.host import (
46 mounts,
47 umount,
48+ restart_on_change,
49 )
50
51 from charmhelpers.core.hookenv import (
52 log,
53 INFO,
54 ERROR,
55+ status_get,
56 )
57
58 DEFAULT_LOOPBACK_SIZE = '5G'
59@@ -83,3 +85,19 @@
60 remove_lvm_physical_volume(block_device)
61 else:
62 zap_disk(block_device)
63+
64+
65+def is_paused(status_get=status_get):
66+ """Is the unit paused?"""
67+ status, message = status_get()
68+ return status == "maintenance" and message.startswith("Paused")
69+
70+
71+def pause_aware_restart_on_change(restart_map):
72+ """Avoids restarting services if config changes when unit is paused."""
73+ def wrapper(f):
74+ if is_paused():
75+ return f
76+ else:
77+ return restart_on_change(restart_map)(f)
78+ return wrapper
79
80=== modified file 'lib/swift_storage_utils.py'
81--- lib/swift_storage_utils.py 2015-08-11 13:59:31 +0000
82+++ lib/swift_storage_utils.py 2015-09-02 10:52:42 +0000
83@@ -10,6 +10,7 @@
84 from misc_utils import (
85 ensure_block_device,
86 clean_storage,
87+ is_paused
88 )
89
90 from swift_storage_context import (
91@@ -151,8 +152,9 @@
92 apt_upgrade(options=dpkg_opts, fatal=True, dist=True)
93 configs.set_release(openstack_release=new_os_rel)
94 configs.write_all()
95- [service_restart(svc) for svc in
96- (ACCOUNT_SVCS + CONTAINER_SVCS + OBJECT_SVCS)]
97+ if not is_paused():
98+ for service in SWIFT_SVCS:
99+ service_restart(service)
100
101
102 def _is_storage_ready(partition):
103
104=== modified file 'tests/basic_deployment.py'
105--- tests/basic_deployment.py 2015-09-02 10:52:42 +0000
106+++ tests/basic_deployment.py 2015-09-02 10:52:42 +0000
107@@ -501,3 +501,51 @@
108 u.log.debug('Checking pause/resume actions...')
109 self._test_pause()
110 self._test_resume()
111+
112+ def test_920_no_restart_on_config_change_when_paused(self):
113+ """Verify that the specified services are not restarted when the config
114+ is changed and the unit is paused."""
115+ u.log.info('Checking that system services do not get restarted '
116+ 'when charm config changes but unit is paused...')
117+ sentry = self.swift_storage_sentry
118+ juju_service = 'swift-storage'
119+
120+ # Expected default and alternate values
121+ set_default = {'object-server-threads-per-disk': '4'}
122+ set_alternate = {'object-server-threads-per-disk': '2'}
123+
124+ services = ['swift-account-server',
125+ 'swift-account-auditor',
126+ 'swift-account-reaper',
127+ 'swift-account-replicator',
128+ 'swift-container-server',
129+ 'swift-container-auditor',
130+ 'swift-container-replicator',
131+ 'swift-container-updater',
132+ 'swift-object-server',
133+ 'swift-object-auditor',
134+ 'swift-object-replicator',
135+ 'swift-object-updater',
136+ 'swift-container-sync']
137+ if self._get_openstack_release() < self.precise_icehouse:
138+ services.remove('swift-container-sync')
139+
140+ # Pause the unit
141+ u.log.debug('Pausing the unit...')
142+ pause_action_id = u.run_action(sentry, "pause")
143+ assert u.wait_on_action(pause_action_id), "Pause action failed."
144+
145+ # Make config change, check for service restarts
146+ u.log.debug('Making config change on {}...'.format(juju_service))
147+ self.d.configure(juju_service, set_alternate)
148+
149+ for service in services:
150+ u.log.debug("Checking that service didn't start while "
151+ "paused: {}".format(service))
152+ # No explicit assert because get_process_id_list will do it for us
153+ u.get_process_id_list(
154+ sentry, service, expect_success=False)
155+
156+ self.d.configure(juju_service, set_default)
157+ resume_action_id = u.run_action(sentry, "resume")
158+ assert u.wait_on_action(resume_action_id), "Resume action failed."

Subscribers

People subscribed via source and target branches