Merge lp:~ack/charms/trusty/keystone/pause-and-resume into lp:~openstack-charmers-archive/charms/trusty/keystone/next
| Status: | Merged |
|---|---|
| Merged at revision: | 173 |
| Proposed branch: | lp:~ack/charms/trusty/keystone/pause-and-resume |
| Merge into: | lp:~openstack-charmers-archive/charms/trusty/keystone/next |
| Diff against target: |
584 lines (+300/-48) 12 files modified
.coveragerc (+1/-0) actions.yaml (+11/-0) actions/actions.py (+56/-0) actions/git_reinstall.py (+3/-5) charm-helpers-hooks.yaml (+1/-1) hooks/keystone_utils.py (+11/-22) tests/basic_deployment.py (+33/-0) unit_tests/test_actions.py (+150/-0) unit_tests/test_actions_git_reinstall.py (+6/-4) unit_tests/test_keystone_contexts.py (+16/-6) unit_tests/test_keystone_hooks.py (+9/-9) unit_tests/test_keystone_utils.py (+3/-1) |
| To merge this branch: | bzr merge lp:~ack/charms/trusty/keystone/pause-and-resume |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Billy Olsen | 2015-08-13 | Approve on 2015-08-31 | |
| Adam Collard (community) | Approve on 2015-08-24 | ||
| Ryan Beisner | Approve on 2015-08-21 | ||
| Geoff Teale (community) | 2015-08-13 | Approve on 2015-08-13 | |
| OpenStack Charmers | 2015-08-26 | Pending | |
|
Review via email:
|
|||
Description of the Change
This branch adds pause/resume actions that can be used to pause the service for maintenance on a unit.
| Alberto Donato (ack) wrote : | # |
| Adam Collard (adam-collard) wrote : | # |
You should stop all of the services that are started by the charm - namely apache2 and haproxy - look at BASE_RESOURCE_MAP in keystone_utils.py
charm_lint_check #7984 keystone-next for ack mp267931
LINT OK: passed
charm_unit_test #7398 keystone-next for ack mp267931
UNIT OK: passed
charm_amulet_test #5776 keystone-next for ack mp267931
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://
Build: http://
charm_lint_check #7986 keystone-next for ack mp267931
LINT OK: passed
charm_unit_test #7400 keystone-next for ack mp267931
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://
Build: http://
charm_amulet_test #5778 keystone-next for ack mp267931
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://
Build: http://
charm_unit_test #7402 keystone-next for ack mp267931
UNIT OK: passed
charm_lint_check #7989 keystone-next for ack mp267931
LINT OK: passed
charm_lint_check #8040 keystone-next for ack mp267931
LINT OK: passed
charm_unit_test #7451 keystone-next for ack mp267931
UNIT OK: passed
charm_amulet_test #5784 keystone-next for ack mp267931
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://
Build: http://
charm_lint_check #8041 keystone-next for ack mp267931
LINT OK: passed
charm_unit_test #7452 keystone-next for ack mp267931
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://
Build: http://
| Alberto Donato (ack) wrote : | # |
> You should stop all of the services that are started by the charm - namely
> apache2 and haproxy - look at BASE_RESOURCE_MAP in keystone_utils.py
Good catch, it's fixed now.
charm_lint_check #8043 keystone-next for ack mp267931
LINT OK: passed
charm_unit_test #7454 keystone-next for ack mp267931
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://
Build: http://
charm_amulet_test #5787 keystone-next for ack mp267931
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://
Build: http://
charm_amulet_test #5785 keystone-next for ack mp267931
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://
Build: http://
| Ryan Beisner (1chb1n) wrote : | # |
Thank you for your work on this. These will be great test additions.
I'd like to see the added amulet tests and helpers land in basic_deployment.py (or charmhelpers where appropriate) so that we can maintain consistency across the os-charms in the way that we iterate ubuntu/openstack series/release.
This means adding new test_ methods in basic_deploymen
If there are OpenStack-specific, amulet-specific helpers which are useful in other charm tests, please land those in charmhelpers/
If there are non-OpenStack-
Often times, during test dev, I'll keep all of my helpers as local helpers in basic_deploymen
Feel free to holler with any questions. Thanks again!
charm_lint_check #8192 keystone-next for ack mp267931
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://
Build: http://
charm_unit_test #7594 keystone-next for ack mp267931
UNIT OK: passed
- 195. By Alberto Donato on 2015-08-17
-
Lint.
charm_lint_check #8193 keystone-next for ack mp267931
LINT OK: passed
charm_unit_test #7595 keystone-next for ack mp267931
UNIT OK: passed
charm_amulet_test #5838 keystone-next for ack mp267931
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://
Build: http://
charm_amulet_test #5839 keystone-next for ack mp267931
AMULET OK: passed
Build: http://
- 196. By Alberto Donato on 2015-08-19
-
Move actions to action.py
charm_lint_check #8338 keystone-next for ack mp267931
LINT OK: passed
charm_unit_test #7735 keystone-next for ack mp267931
UNIT OK: passed
charm_amulet_test #5881 keystone-next for ack mp267931
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://
Build: http://
- 197. By Alberto Donato on 2015-08-19
-
Fix mocking, again.
- 198. By Alberto Donato on 2015-08-19
-
Add missing files,
charm_lint_check #8339 keystone-next for ack mp267931
LINT OK: passed
charm_unit_test #7736 keystone-next for ack mp267931
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://
Build: http://
- 199. By Alberto Donato on 2015-08-19
-
Fix unittest.
charm_amulet_test #5882 keystone-next for ack mp267931
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://
Build: http://
charm_lint_check #8340 keystone-next for ack mp267931
LINT OK: passed
charm_unit_test #7737 keystone-next for ack mp267931
UNIT OK: passed
charm_amulet_test #5883 keystone-next for ack mp267931
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://
Build: http://
- 200. By Alberto Donato on 2015-08-19
-
Fix docstrings.
charm_lint_check #8341 keystone-next for ack mp267931
LINT OK: passed
charm_unit_test #7738 keystone-next for ack mp267931
UNIT OK: passed
- 201. By Alberto Donato on 2015-08-19
-
Add missing shebang.
charm_amulet_test #5884 keystone-next for ack mp267931
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://
Build: http://
charm_lint_check #8342 keystone-next for ack mp267931
LINT OK: passed
charm_unit_test #7739 keystone-next for ack mp267931
UNIT OK: passed
charm_amulet_test #5885 keystone-next for ack mp267931
AMULET OK: passed
Build: http://
| Alberto Donato (ack) wrote : | # |
@Ryan, thanks for the feedback. I've aligned the changes in this branch with the swift-storage one.
It should be good for another round of review.
| Adam Collard (adam-collard) wrote : | # |
5 inline comments below :)
- 202. By Alberto Donato on 2015-08-19
-
Merge from -next.
- 203. By Alberto Donato on 2015-08-19
-
Undo charm sync.
- 204. By Alberto Donato on 2015-08-19
-
Address review comments.
- 205. By Alberto Donato on 2015-08-19
-
Use right executable names.
charm_lint_check #8367 keystone-next for ack mp267931
LINT OK: passed
charm_unit_test #7764 keystone-next for ack mp267931
UNIT OK: passed
| Alberto Donato (ack) wrote : | # |
> 5 inline comments below :)
All comments addressed, thanks
charm_lint_check #8368 keystone-next for ack mp267931
LINT OK: passed
charm_unit_test #7765 keystone-next for ack mp267931
UNIT OK: passed
charm_amulet_test #5910 keystone-next for ack mp267931
AMULET OK: passed
Build: http://
| Ryan Beisner (1chb1n) wrote : | # |
RE: Amulet tests
Thank you for your work on this.
Please do another charm-helper sync to pull in sparkiegeek's tests/charmhelpers/ system service status check and py3 drive-by fixes.
Otherwise, looks good to me.
- 206. By Alberto Donato on 2015-08-21
-
charmhelpers sync.
charm_lint_check #8483 keystone-next for ack mp267931
LINT OK: passed
charm_unit_test #7874 keystone-next for ack mp267931
UNIT OK: passed
| Ryan Beisner (1chb1n) wrote : | # |
Although the amulet job isn't done, I checked its status as of 019 vivid-kilo, and all passed.
All clear to merge as far as test perspective.
charm_amulet_test #5942 keystone-next for ack mp267931
AMULET OK: passed
Build: http://
- 207. By Alberto Donato on 2015-08-24
-
Merge from next, fix conflict.
charm_lint_check #8647 keystone-next for ack mp267931
LINT OK: passed
charm_unit_test #7984 keystone-next for ack mp267931
UNIT OK: passed
| Adam Collard (adam-collard) wrote : | # |
Thanks for your patience and work on getting this to match style of swift-storage. Looks good! +1
charm_amulet_test #6012 keystone-next for ack mp267931
AMULET OK: passed
Build: http://
| Billy Olsen (billy-olsen) wrote : | # |
Thanks for the submission! I think this generally looks good and would be quite pleased to approve it for a single unit operation. However, in the case where keystone is scaled out and paired with the hacluster charm I think we want to do some cluster appropriate operations first (e.g. move resources off-node pre-emptively).
That's probably common code to be shared amongst other charms, but I think we definitely need to consider the scenario in which a service is paused/resumed in a cluster. If its paused and resources are moved away, does it need to be able to ensure that resources cannot be moved back to the node which may cause confusion.
Will mark the review as Needs Information for now while this is sorted.
| Billy Olsen (billy-olsen) wrote : | # |
After discussion, the hacluster charm can handle the move of the VIP - which is probably an appropriate place to put it. It needs to move the vip off of this node and mark the cluster in maintenance mode so that services aren't moved around. That can be handled by the hacluster charm itself.
However, this would introduce a 2 step process for the user to be able to do things without service disruption. It would require they first run the pause action against the hacluster charm and then run the pause action against the keystone charm. It would be ideal if the charm could enforce this or invoke the hacluster pause action for the node, however that might be over complicating things for the basic building blocks here.
At a minimum I think we need some better docs describing this behavior in the actions.yaml file and possibly the consequences of not performing the node movement.
Thinking something along the lines of (though I'm sure there's better wordsmithing available):
Note: when pausing the keystone services when keystone is clustered using the hacluster charm, the hacluster unit on this node must first be paused as well. Not doing so may lead to an interruption of service.
- 208. By Alberto Donato on 2015-08-28
-
Fix actions description.
| Alberto Donato (ack) wrote : | # |
@Billy thanks for the review and suggestion, I've expanded the actions description.
- 209. By Alberto Donato on 2015-08-28
-
Lint.
charm_lint_check #8929 keystone-next for ack mp267931
LINT OK: passed
charm_unit_test #8252 keystone-next for ack mp267931
UNIT OK: passed
charm_amulet_test #6078 keystone-next for ack mp267931
AMULET OK: passed
Build: http://
| Billy Olsen (billy-olsen) wrote : | # |
On further inspection, its not clear why the charmhelpers were moved out of the hooks directory to the root directory of the charm but the amulet tests continues to have its own charmhelpers installation. I'm not holding this merge proposal up based on this comment, but I also noticed the same in the swift storage charm (and likely glance, though I haven't looked there yet).
Thanks for the contribution Alberto!

There's also a charmhelpers sync to pull fixes for service_ pause/resume.