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-08-28
Status: Superseded
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
Diff against target: 270 lines (+100/-31)
7 files modified
charmhelpers/contrib/network/ip.py (+5/-1)
charmhelpers/contrib/openstack/utils.py (+7/-5)
charmhelpers/core/hookenv.py (+11/-9)
hooks/swift_storage_hooks.py (+5/-3)
lib/misc_utils.py (+18/-0)
lib/swift_storage_utils.py (+4/-2)
tests/basic_deployment.py (+50/-11)
To merge this branch: bzr merge lp:~adam-collard/charms/trusty/swift-storage/guard-paused-unit-service-restarts
Reviewer Review Type Date Requested Status
Landscape 2015-08-28 Pending
OpenStack Charmers 2015-08-28 Pending
Review via email: mp+269528@code.launchpad.net

This proposal has been superseded by a proposal from 2015-09-02.

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.

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/

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/

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/

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/

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/

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/

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)

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'charmhelpers/contrib/network/ip.py'
2--- charmhelpers/contrib/network/ip.py 2015-03-04 09:54:59 +0000
3+++ charmhelpers/contrib/network/ip.py 2015-09-01 16:09:33 +0000
4@@ -435,8 +435,12 @@
5
6 rev = dns.reversename.from_address(address)
7 result = ns_query(rev)
8+
9 if not result:
10- return None
11+ try:
12+ result = socket.gethostbyaddr(address)[0]
13+ except:
14+ return None
15 else:
16 result = address
17
18
19=== modified file 'charmhelpers/contrib/openstack/utils.py'
20--- charmhelpers/contrib/openstack/utils.py 2015-08-17 13:44:42 +0000
21+++ charmhelpers/contrib/openstack/utils.py 2015-09-01 16:09:33 +0000
22@@ -1,5 +1,3 @@
23-#!/usr/bin/python
24-
25 # Copyright 2014-2015 Canonical Limited.
26 #
27 # This file is part of charm-helpers.
28@@ -195,9 +193,9 @@
29 error_out(e)
30
31
32-def get_os_version_codename(codename):
33+def get_os_version_codename(codename, version_map=OPENSTACK_CODENAMES):
34 '''Determine OpenStack version number from codename.'''
35- for k, v in six.iteritems(OPENSTACK_CODENAMES):
36+ for k, v in six.iteritems(version_map):
37 if v == codename:
38 return k
39 e = 'Could not derive OpenStack version for '\
40@@ -429,7 +427,11 @@
41 import apt_pkg as apt
42 src = config('openstack-origin')
43 cur_vers = get_os_version_package(package)
44- available_vers = get_os_version_install_source(src)
45+ if "swift" in package:
46+ codename = get_os_codename_install_source(src)
47+ available_vers = get_os_version_codename(codename, SWIFT_CODENAMES)
48+ else:
49+ available_vers = get_os_version_install_source(src)
50 apt.init()
51 return apt.version_compare(available_vers, cur_vers) == 1
52
53
54=== modified file 'charmhelpers/core/hookenv.py'
55--- charmhelpers/core/hookenv.py 2015-08-17 13:44:42 +0000
56+++ charmhelpers/core/hookenv.py 2015-09-01 16:09:33 +0000
57@@ -767,21 +767,23 @@
58
59
60 def status_get():
61- """Retrieve the previously set juju workload state
62-
63- If the status-set command is not found then assume this is juju < 1.23 and
64- return 'unknown'
65+ """Retrieve the previously set juju workload state and message
66+
67+ If the status-get command is not found then assume this is juju < 1.23 and
68+ return 'unknown', ""
69+
70 """
71- cmd = ['status-get']
72+ cmd = ['status-get', "--format=json", "--include-data"]
73 try:
74- raw_status = subprocess.check_output(cmd, universal_newlines=True)
75- status = raw_status.rstrip()
76- return status
77+ raw_status = subprocess.check_output(cmd)
78 except OSError as e:
79 if e.errno == errno.ENOENT:
80- return 'unknown'
81+ return ('unknown', "")
82 else:
83 raise
84+ else:
85+ status = json.loads(raw_status.decode("UTF-8"))
86+ return (status["status"], status["message"])
87
88
89 def translate_exc(from_exc, to_exc):
90
91=== modified file 'hooks/swift_storage_hooks.py'
92--- hooks/swift_storage_hooks.py 2015-07-17 09:57:19 +0000
93+++ hooks/swift_storage_hooks.py 2015-09-01 16:09:33 +0000
94@@ -18,6 +18,8 @@
95 setup_rsync,
96 )
97
98+from lib.misc_utils import pause_aware_restart_on_change
99+
100 from charmhelpers.core.hookenv import (
101 Hooks, UnregisteredHookError,
102 config,
103@@ -32,7 +34,7 @@
104 apt_update,
105 filter_installed_packages
106 )
107-from charmhelpers.core.host import restart_on_change, rsync
108+from charmhelpers.core.host import rsync
109 from charmhelpers.payload.execd import execd_preinstall
110
111 from charmhelpers.contrib.openstack.utils import (
112@@ -63,7 +65,7 @@
113
114
115 @hooks.hook('config-changed')
116-@restart_on_change(RESTART_MAP)
117+@pause_aware_restart_on_change(RESTART_MAP)
118 def config_changed():
119 if config('prefer-ipv6'):
120 assert_charm_supports_ipv6()
121@@ -104,7 +106,7 @@
122
123
124 @hooks.hook('swift-storage-relation-changed')
125-@restart_on_change(RESTART_MAP)
126+@pause_aware_restart_on_change(RESTART_MAP)
127 def swift_storage_relation_changed():
128 rings_url = relation_get('rings_url')
129 swift_hash = relation_get('swift_hash')
130
131=== modified file 'lib/misc_utils.py'
132--- lib/misc_utils.py 2015-07-17 10:51:25 +0000
133+++ lib/misc_utils.py 2015-09-01 16:09:33 +0000
134@@ -16,12 +16,14 @@
135 from charmhelpers.core.host import (
136 mounts,
137 umount,
138+ restart_on_change,
139 )
140
141 from charmhelpers.core.hookenv import (
142 log,
143 INFO,
144 ERROR,
145+ status_get,
146 )
147
148 DEFAULT_LOOPBACK_SIZE = '5G'
149@@ -83,3 +85,19 @@
150 remove_lvm_physical_volume(block_device)
151 else:
152 zap_disk(block_device)
153+
154+
155+def is_paused(status_get=status_get):
156+ """Is the unit paused?"""
157+ status, message = status_get()
158+ return status == "maintenance" and message.startswith("Paused")
159+
160+
161+def pause_aware_restart_on_change(restart_map):
162+ """Avoids restarting services if config changes when unit is paused."""
163+ def wrapper(f):
164+ if is_paused():
165+ return f
166+ else:
167+ return restart_on_change(restart_map)(f)
168+ return wrapper
169
170=== modified file 'lib/swift_storage_utils.py'
171--- lib/swift_storage_utils.py 2015-08-11 13:59:31 +0000
172+++ lib/swift_storage_utils.py 2015-09-01 16:09:33 +0000
173@@ -10,6 +10,7 @@
174 from misc_utils import (
175 ensure_block_device,
176 clean_storage,
177+ is_paused
178 )
179
180 from swift_storage_context import (
181@@ -151,8 +152,9 @@
182 apt_upgrade(options=dpkg_opts, fatal=True, dist=True)
183 configs.set_release(openstack_release=new_os_rel)
184 configs.write_all()
185- [service_restart(svc) for svc in
186- (ACCOUNT_SVCS + CONTAINER_SVCS + OBJECT_SVCS)]
187+ if not is_paused():
188+ [service_restart(svc) for svc in
189+ (ACCOUNT_SVCS + CONTAINER_SVCS + OBJECT_SVCS)]
190
191
192 def _is_storage_ready(partition):
193
194=== modified file 'tests/basic_deployment.py'
195--- tests/basic_deployment.py 2015-08-22 04:12:14 +0000
196+++ tests/basic_deployment.py 2015-09-01 16:09:33 +0000
197@@ -429,19 +429,10 @@
198
199 # Config file affected by juju set config change, and
200 # services which are expected to restart upon config change
201- services = {'swift-account-server': 'account-server.conf',
202- 'swift-account-auditor': 'account-server.conf',
203- 'swift-account-reaper': 'account-server.conf',
204- 'swift-account-replicator': 'account-server.conf',
205- 'swift-container-server': 'container-server.conf',
206- 'swift-container-auditor': 'container-server.conf',
207- 'swift-container-replicator': 'container-server.conf',
208- 'swift-container-updater': 'container-server.conf',
209- 'swift-object-server': 'object-server.conf',
210+ services = {'swift-object-server': 'object-server.conf',
211 'swift-object-auditor': 'object-server.conf',
212 'swift-object-replicator': 'object-server.conf',
213- 'swift-object-updater': 'object-server.conf',
214- 'swift-container-sync': 'container-server.conf'}
215+ 'swift-object-updater': 'object-server.conf'}
216
217 # Make config change, check for service restarts
218 u.log.debug('Making config change on {}...'.format(juju_service))
219@@ -510,3 +501,51 @@
220 u.log.debug('Checking pause/resume actions...')
221 self._test_pause()
222 self._test_resume()
223+
224+ def test_920_no_restart_on_config_change_when_paused(self):
225+ """Verify that the specified services are not restarted when the config
226+ is changed and the unit is paused."""
227+ u.log.info('Checking that system services do not get restarted '
228+ 'when charm config changes but unit is paused...')
229+ sentry = self.swift_storage_sentry
230+ juju_service = 'swift-storage'
231+
232+ # Expected default and alternate values
233+ set_default = {'object-server-threads-per-disk': '4'}
234+ set_alternate = {'object-server-threads-per-disk': '2'}
235+
236+ services = ['swift-account-server',
237+ 'swift-account-auditor',
238+ 'swift-account-reaper',
239+ 'swift-account-replicator',
240+ 'swift-container-server',
241+ 'swift-container-auditor',
242+ 'swift-container-replicator',
243+ 'swift-container-updater',
244+ 'swift-object-server',
245+ 'swift-object-auditor',
246+ 'swift-object-replicator',
247+ 'swift-object-updater',
248+ 'swift-container-sync']
249+ if self._get_openstack_release() < self.precise_icehouse:
250+ services.remove('swift-container-sync')
251+
252+ # Pause the unit
253+ u.log.debug('Pausing the unit...')
254+ pause_action_id = u.run_action(sentry, "pause")
255+ assert u.wait_on_action(pause_action_id), "Pause action failed."
256+
257+ # Make config change, check for service restarts
258+ u.log.debug('Making config change on {}...'.format(juju_service))
259+ self.d.configure(juju_service, set_alternate)
260+
261+ for service in services:
262+ u.log.debug("Checking that service didn't start while "
263+ "paused: {}".format(service))
264+ # No explicit assert because get_process_id_list will do it for us
265+ u.get_process_id_list(
266+ sentry, service, expect_success=False)
267+
268+ self.d.configure(juju_service, set_default)
269+ resume_action_id = u.run_action(sentry, "resume")
270+ assert u.wait_on_action(resume_action_id), "Resume action failed."

Subscribers

People subscribed via source and target branches