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
=== modified file 'hooks/swift_storage_hooks.py'
--- hooks/swift_storage_hooks.py 2015-07-17 09:57:19 +0000
+++ hooks/swift_storage_hooks.py 2015-09-02 10:52:42 +0000
@@ -18,6 +18,8 @@
18 setup_rsync,18 setup_rsync,
19)19)
2020
21from lib.misc_utils import pause_aware_restart_on_change
22
21from charmhelpers.core.hookenv import (23from charmhelpers.core.hookenv import (
22 Hooks, UnregisteredHookError,24 Hooks, UnregisteredHookError,
23 config,25 config,
@@ -32,7 +34,7 @@
32 apt_update,34 apt_update,
33 filter_installed_packages35 filter_installed_packages
34)36)
35from charmhelpers.core.host import restart_on_change, rsync37from charmhelpers.core.host import rsync
36from charmhelpers.payload.execd import execd_preinstall38from charmhelpers.payload.execd import execd_preinstall
3739
38from charmhelpers.contrib.openstack.utils import (40from charmhelpers.contrib.openstack.utils import (
@@ -63,7 +65,7 @@
6365
6466
65@hooks.hook('config-changed')67@hooks.hook('config-changed')
66@restart_on_change(RESTART_MAP)68@pause_aware_restart_on_change(RESTART_MAP)
67def config_changed():69def config_changed():
68 if config('prefer-ipv6'):70 if config('prefer-ipv6'):
69 assert_charm_supports_ipv6()71 assert_charm_supports_ipv6()
@@ -104,7 +106,7 @@
104106
105107
106@hooks.hook('swift-storage-relation-changed')108@hooks.hook('swift-storage-relation-changed')
107@restart_on_change(RESTART_MAP)109@pause_aware_restart_on_change(RESTART_MAP)
108def swift_storage_relation_changed():110def swift_storage_relation_changed():
109 rings_url = relation_get('rings_url')111 rings_url = relation_get('rings_url')
110 swift_hash = relation_get('swift_hash')112 swift_hash = relation_get('swift_hash')
111113
=== modified file 'lib/misc_utils.py'
--- lib/misc_utils.py 2015-07-17 10:51:25 +0000
+++ lib/misc_utils.py 2015-09-02 10:52:42 +0000
@@ -16,12 +16,14 @@
16from charmhelpers.core.host import (16from charmhelpers.core.host import (
17 mounts,17 mounts,
18 umount,18 umount,
19 restart_on_change,
19)20)
2021
21from charmhelpers.core.hookenv import (22from charmhelpers.core.hookenv import (
22 log,23 log,
23 INFO,24 INFO,
24 ERROR,25 ERROR,
26 status_get,
25)27)
2628
27DEFAULT_LOOPBACK_SIZE = '5G'29DEFAULT_LOOPBACK_SIZE = '5G'
@@ -83,3 +85,19 @@
83 remove_lvm_physical_volume(block_device)85 remove_lvm_physical_volume(block_device)
84 else:86 else:
85 zap_disk(block_device)87 zap_disk(block_device)
88
89
90def is_paused(status_get=status_get):
91 """Is the unit paused?"""
92 status, message = status_get()
93 return status == "maintenance" and message.startswith("Paused")
94
95
96def pause_aware_restart_on_change(restart_map):
97 """Avoids restarting services if config changes when unit is paused."""
98 def wrapper(f):
99 if is_paused():
100 return f
101 else:
102 return restart_on_change(restart_map)(f)
103 return wrapper
86104
=== modified file 'lib/swift_storage_utils.py'
--- lib/swift_storage_utils.py 2015-08-11 13:59:31 +0000
+++ lib/swift_storage_utils.py 2015-09-02 10:52:42 +0000
@@ -10,6 +10,7 @@
10from misc_utils import (10from misc_utils import (
11 ensure_block_device,11 ensure_block_device,
12 clean_storage,12 clean_storage,
13 is_paused
13)14)
1415
15from swift_storage_context import (16from swift_storage_context import (
@@ -151,8 +152,9 @@
151 apt_upgrade(options=dpkg_opts, fatal=True, dist=True)152 apt_upgrade(options=dpkg_opts, fatal=True, dist=True)
152 configs.set_release(openstack_release=new_os_rel)153 configs.set_release(openstack_release=new_os_rel)
153 configs.write_all()154 configs.write_all()
154 [service_restart(svc) for svc in155 if not is_paused():
155 (ACCOUNT_SVCS + CONTAINER_SVCS + OBJECT_SVCS)]156 for service in SWIFT_SVCS:
157 service_restart(service)
156158
157159
158def _is_storage_ready(partition):160def _is_storage_ready(partition):
159161
=== modified file 'tests/basic_deployment.py'
--- tests/basic_deployment.py 2015-09-02 10:52:42 +0000
+++ tests/basic_deployment.py 2015-09-02 10:52:42 +0000
@@ -501,3 +501,51 @@
501 u.log.debug('Checking pause/resume actions...')501 u.log.debug('Checking pause/resume actions...')
502 self._test_pause()502 self._test_pause()
503 self._test_resume()503 self._test_resume()
504
505 def test_920_no_restart_on_config_change_when_paused(self):
506 """Verify that the specified services are not restarted when the config
507 is changed and the unit is paused."""
508 u.log.info('Checking that system services do not get restarted '
509 'when charm config changes but unit is paused...')
510 sentry = self.swift_storage_sentry
511 juju_service = 'swift-storage'
512
513 # Expected default and alternate values
514 set_default = {'object-server-threads-per-disk': '4'}
515 set_alternate = {'object-server-threads-per-disk': '2'}
516
517 services = ['swift-account-server',
518 'swift-account-auditor',
519 'swift-account-reaper',
520 'swift-account-replicator',
521 'swift-container-server',
522 'swift-container-auditor',
523 'swift-container-replicator',
524 'swift-container-updater',
525 'swift-object-server',
526 'swift-object-auditor',
527 'swift-object-replicator',
528 'swift-object-updater',
529 'swift-container-sync']
530 if self._get_openstack_release() < self.precise_icehouse:
531 services.remove('swift-container-sync')
532
533 # Pause the unit
534 u.log.debug('Pausing the unit...')
535 pause_action_id = u.run_action(sentry, "pause")
536 assert u.wait_on_action(pause_action_id), "Pause action failed."
537
538 # Make config change, check for service restarts
539 u.log.debug('Making config change on {}...'.format(juju_service))
540 self.d.configure(juju_service, set_alternate)
541
542 for service in services:
543 u.log.debug("Checking that service didn't start while "
544 "paused: {}".format(service))
545 # No explicit assert because get_process_id_list will do it for us
546 u.get_process_id_list(
547 sentry, service, expect_success=False)
548
549 self.d.configure(juju_service, set_default)
550 resume_action_id = u.run_action(sentry, "resume")
551 assert u.wait_on_action(resume_action_id), "Resume action failed."

Subscribers

People subscribed via source and target branches