|
|
|
Agent lifecycle suport for subordinates
Includes changes to the unit lifecycle to support deployment
of subordinates in reaction to relation events.
https://code.launchpad.net/~bcsaller/juju/subordinate-unit-agent-deploy/+merge/101475
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 15
Patch Set 2 : Agent lifecycle suport for subordinates #
Total comments: 6
Patch Set 3 : Agent lifecycle suport for subordinates #
Total comments: 10
Patch Set 4 : Agent lifecycle suport for subordinates #Patch Set 5 : Agent lifecycle suport for subordinates #
Total messages: 11
|
bcsaller
Please take a look.
|
13 years, 9 months ago (2012年03月23日 18:51:45 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
just some initial feedback, although the add_subordinate_unit would more properly be applied to rel-type branch. The pre-req status branch will need to be rebased on trunk (status-changes merge). make check output.. juju/control/tests/test_deploy.py:527:80: E501 line too long (101 characters) juju/control/tests/test_deploy.py:531:80: E501 line too long (85 characters) juju/control/tests/test_remove_unit.py:160:5: E303 too many blank lines (2) juju/control/tests/test_status.py:76:80: E501 line too long (81 characters) juju/control/tests/test_status.py:98:80: E501 line too long (96 characters) juju/control/tests/test_status.py:173:9: E303 too many blank lines (2) juju/control/tests/test_status.py:260:80: E501 line too long (80 characters) juju/control/tests/test_status.py:671:80: E501 line too long (136 characters) juju/control/tests/test_status.py:674:80: E501 line too long (136 characters) juju/control/tests/test_status.py:679:80: E501 line too long (144 characters) juju/control/tests/test_status.py:681:80: E501 line too long (143 characters) juju/control/tests/test_status.py:681:136: E231 missing whitespace after ',' juju/control/tests/test_status.py:683:80: E501 line too long (138 characters) juju/control/tests/test_status.py:729:80: E501 line too long (84 characters) juju/control/tests/test_status.py:730:80: E501 line too long (95 characters) juju/control/tests/test_status.py:731:80: E501 line too long (101 characters) juju/control/tests/test_status.py:733:80: E501 line too long (91 characters) juju/control/tests/test_status.py:735:80: E501 line too long (91 characters) juju/control/tests/test_status.py:743:80: E501 line too long (80 characters) juju/control/tests/test_status.py:744:80: E501 line too long (80 characters) juju/control/tests/test_status.py:745:80: E501 line too long (80 characters) juju/lib/tests/test_pick.py:26:80: E501 line too long (81 characters) juju/lib/tests/test_pick.py:44:80: E501 line too long (83 characters) juju/state/tests/test_charm.py:9:80: E501 line too long (82 characters) juju/state/tests/test_charm.py:129:5: E303 too many blank lines (2) juju/state/tests/test_errors.py:236:80: E501 line too long (98 characters) juju/state/tests/test_errors.py:242:80: E501 line too long (86 characters) juju/state/tests/test_relation.py:476:80: E501 line too long (88 characters) juju/state/tests/test_relation.py:493:80: E501 line too long (88 characters) juju/state/tests/test_relation.py:507:80: E501 line too long (88 characters) juju/state/tests/test_relation.py:698:80: E501 line too long (88 characters) juju/state/tests/test_service.py:398:5: E303 too many blank lines (2) juju/state/tests/test_service.py:1675:80: E501 line too long (81 characters) juju/state/tests/test_service.py:1688:80: E501 line too long (82 characters) juju/state/tests/test_service.py:1694:80: E501 line too long (81 characters) juju/state/tests/test_service.py:1727:80: E501 line too long (81 characters) juju/state/tests/test_service.py:2254:80: E501 line too long (81 characters) juju/state/tests/test_service.py:2300:80: E501 line too long (81 characters) juju/state/tests/test_service.py:2698:80: E501 line too long (83 characters) juju/state/tests/test_topology.py:226:80: E501 line too long (89 characters) juju/state/tests/test_topology.py:228:80: E501 line too long (90 characters) juju/state/tests/test_topology.py:240:80: E501 line too long (89 characters) juju/state/tests/test_topology.py:243:80: E501 line too long (85 characters) juju/state/tests/test_topology.py:244:80: E501 line too long (85 characters) juju/state/tests/test_topology.py:247:80: E501 line too long (81 characters) juju/state/tests/test_topology.py:253:80: E501 line too long (83 characters) juju/state/tests/test_topology.py:254:80: E501 line too long (81 characters) juju/state/tests/test_topology.py:255:80: E501 line too long (81 characters) juju/state/tests/test_topology.py:1007:80: E501 line too long (88 characters) juju/state/tests/test_topology.py:1008:80: E501 line too long (88 characters) juju/unit/tests/test_workflow.py:533:5: E303 too many blank lines (2) juju/unit/tests/test_workflow.py:697:13: E303 too many blank lines (2) make: *** [check] Error 123 https://codereview.appspot.com/5892046/diff/1/juju/agents/unit.py File juju/agents/unit.py (right): https://codereview.appspot.com/5892046/diff/1/juju/agents/unit.py#newcode40 juju/agents/unit.py:40: container_name = os.environ.get("JUJU_CONTAINER_NAME", "") Why? https://codereview.appspot.com/5892046/diff/1/juju/lib/lxc/__init__.py File juju/lib/lxc/__init__.py (right): https://codereview.appspot.com/5892046/diff/1/juju/lib/lxc/__init__.py#newcode48 juju/lib/lxc/__init__.py:48: container_name, template="ubuntu", config_file=None, release="precise"): template and release should be required. Considering how far its buried, its rather non obvious that this string needs changing per release. That choice needs to be made at a higher level, both _lxc_create and LXContainer should require the release. https://codereview.appspot.com/5892046/diff/1/juju/state/relation.py File juju/state/relation.py (left): https://codereview.appspot.com/5892046/diff/1/juju/state/relation.py#oldcode213 juju/state/relation.py:213: self, client, service_id, relation_id, relation_scope, role, name): I'd prefer this style more, given a doc string. https://codereview.appspot.com/5892046/diff/1/juju/unit/deploy.py File juju/unit/deploy.py (right): https://codereview.appspot.com/5892046/diff/1/juju/unit/deploy.py#newcode33 juju/unit/deploy.py:33: #assert isinstance(machine_id, str) the assertion should probably remain, and callers fixed. https://codereview.appspot.com/5892046/diff/1/juju/unit/deploy.py#newcode40 juju/unit/deploy.py:40: def start(self, provider_type=None): make defaults to get alternate behavior is rather implicit and adds complexity. provider type here is universal, better to just make a subordinate unit deploy subclass that has the logic you want. https://codereview.appspot.com/5892046/diff/1/juju/unit/lifecycle.py File juju/unit/lifecycle.py (right): https://codereview.appspot.com/5892046/diff/1/juju/unit/lifecycle.py#newcode396 juju/unit/lifecycle.py:396: if (is_subordinate is False and i've commented on this on the type previously i believe. but this logic is rather hard to read.. if is_subordinate is False: deploy_subordinate. Shouldn't be subordinate be true here. https://codereview.appspot.com/5892046/diff/1/juju/unit/lifecycle.py#newcode426 juju/unit/lifecycle.py:426: with (yield workflow.lock()): why is this synchronizing? https://codereview.appspot.com/5892046/diff/1/juju/unit/lifecycle.py#newcode432 juju/unit/lifecycle.py:432: # inline deployer is not being used. https://codereview.appspot.com/5892046/diff/1/juju/unit/lifecycle.py#newcode443 juju/unit/lifecycle.py:443: sub_unit = yield sub_service.add_unit_state(container=self._unit) I'm starting to think this api would be cleaner as add_subordinate_unit. https://codereview.appspot.com/5892046/diff/1/juju/unit/tests/test_lifecycle.py File juju/unit/tests/test_lifecycle.py (right): https://codereview.appspot.com/5892046/diff/1/juju/unit/tests/test_lifecycle.... juju/unit/tests/test_lifecycle.py:1316: @self.addCleanup cleanup should be setup earlier. https://codereview.appspot.com/5892046/diff/1/juju/unit/tests/test_lifecycle.... juju/unit/tests/test_lifecycle.py:1359: def test_subordinate_lifecycle_start(self): definitely needs some more testing around this. unit deployment is a potentially 'long' activity. what if the relation departs mid processing of new relations as an example? thanks for setting up a testing method, it still feels a bit large though to be reuseable though.
Please take a look.
[WIP] I spent some time merging and retesting this branch. I didn't properly respond to comments from before but will include remaining changes tomorrow. As of now this properly executes the follow test resulting in the expected log output in the contains. I'll repush again with the still relevant comments
The previously mentioned script ------------------ #!/bin/bash set -x ./bin/juju destroy-environment ./bin/juju bootstrap ./bin/juju deploy --repository examples/ local:mysql ./bin/juju add-unit mysql ./bin/juju deploy --repository examples/ local:recorder ./bin/juju add-relation mysql recorder ./bin/juju deploy --repository examples local:wordpress ./bin/juju add-relation mysql wordpress ./bin/juju add-relation recorder wordpress
outside of the lack of test/info around subordinate removal this looks good. http://codereview.appspot.com/5892046/diff/7001/juju/agents/unit.py File juju/agents/unit.py (right): http://codereview.appspot.com/5892046/diff/7001/juju/agents/unit.py#newcode40 juju/agents/unit.py:40: container_name = os.environ.get("JUJU_CONTAINER_NAME", "") this naming is unfortunate. container has a well defined meaning.. lxc, this is not lxc. a unit's container isn't about subordinates its about where/how its deployed. every unit has a container regardless of being subordinate. where does this value get used? afaics its dead code, no tests & no callers. http://codereview.appspot.com/5892046/diff/7001/juju/unit/deploy.py File juju/unit/deploy.py (right): http://codereview.appspot.com/5892046/diff/7001/juju/unit/deploy.py#newcode33 juju/unit/deploy.py:33: #assert isinstance(machine_id, str) leftover http://codereview.appspot.com/5892046/diff/7001/juju/unit/lifecycle.py File juju/unit/lifecycle.py (right): http://codereview.appspot.com/5892046/diff/7001/juju/unit/lifecycle.py#newcod... juju/unit/lifecycle.py:435: def _do_unit_deploy(self, unit_name, machine_id, charm_dir, deployer="machine"): is it useful to have this parametized. there is only one mode here that is functional. http://codereview.appspot.com/5892046/diff/7001/juju/unit/tests/test_lifecycl... File juju/unit/tests/test_lifecycle.py (right): http://codereview.appspot.com/5892046/diff/7001/juju/unit/tests/test_lifecycl... juju/unit/tests/test_lifecycle.py:1355: def test_subordinate_lifecycle_start(self): what's the current behavior for remove of a subordinate via breaking the relation or destroying the primary unit? That should be documented somewhere, but i don't see any tests around it, so what happens? http://codereview.appspot.com/5892046/diff/7001/juju/unit/tests/test_lifecycl... juju/unit/tests/test_lifecycle.py:1376: #self.capture_output() leftover
https://codereview.appspot.com/5892046/diff/1/juju/agents/unit.py File juju/agents/unit.py (right): https://codereview.appspot.com/5892046/diff/1/juju/agents/unit.py#newcode40 juju/agents/unit.py:40: container_name = os.environ.get("JUJU_CONTAINER_NAME", "") On 2012年03月25日 21:12:41, hazmat wrote: > Why? removed https://codereview.appspot.com/5892046/diff/1/juju/lib/lxc/__init__.py File juju/lib/lxc/__init__.py (right): https://codereview.appspot.com/5892046/diff/1/juju/lib/lxc/__init__.py#newcode48 juju/lib/lxc/__init__.py:48: container_name, template="ubuntu", config_file=None, release="precise"): On 2012年03月25日 21:12:41, hazmat wrote: > template and release should be required. Considering how far its buried, its > rather non obvious that this string needs changing per release. That choice > needs to be made at a higher level, both _lxc_create and LXContainer should > require the release. I create a defaults file (in environment/defaults.py) and import the symbol from there. In places that we encode a default series I've updated with a reference. https://codereview.appspot.com/5892046/diff/1/juju/state/relation.py File juju/state/relation.py (left): https://codereview.appspot.com/5892046/diff/1/juju/state/relation.py#oldcode213 juju/state/relation.py:213: self, client, service_id, relation_id, relation_scope, role, name): On 2012年03月25日 21:12:41, hazmat wrote: > I'd prefer this style more, given a doc string. Done. https://codereview.appspot.com/5892046/diff/1/juju/unit/lifecycle.py File juju/unit/lifecycle.py (right): https://codereview.appspot.com/5892046/diff/1/juju/unit/lifecycle.py#newcode443 juju/unit/lifecycle.py:443: sub_unit = yield sub_service.add_unit_state(container=self._unit) On 2012年03月25日 21:12:41, hazmat wrote: > I'm starting to think this api would be cleaner as add_subordinate_unit. I reordered things a little bit and was able to make subordinate deployment appear a bit less special. Hopefully this eases the concern. https://codereview.appspot.com/5892046/diff/7001/juju/unit/tests/test_lifecyc... File juju/unit/tests/test_lifecycle.py (right): https://codereview.appspot.com/5892046/diff/7001/juju/unit/tests/test_lifecyc... juju/unit/tests/test_lifecycle.py:1355: def test_subordinate_lifecycle_start(self): On 2012年04月04日 16:38:51, hazmat wrote: > what's the current behavior for remove of a subordinate via breaking the > relation or destroying the primary unit? That should be documented somewhere, > but i don't see any tests around it, so what happens? It was previously undefined :( I've added some behaviour around this and will push it again. This needs more testing but I'd like to see if the approach is agreeable.
So based on our conversation, and the desire to land this functionality soon, we should table removal and put in place the control guards about subordinate removals. https://codereview.appspot.com/5892046/diff/15001/juju/control/destroy_servic... File juju/control/destroy_service.py (right): https://codereview.appspot.com/5892046/diff/15001/juju/control/destroy_servic... juju/control/destroy_service.py:41: # We can destroy the service if it has not relations if it doesn't have https://codereview.appspot.com/5892046/diff/15001/juju/control/destroy_servic... juju/control/destroy_service.py:48: if relations: this should filter for primary relations, a non subordinate relation is still fine to tear down the subordinate. https://codereview.appspot.com/5892046/diff/15001/juju/control/tests/test_des... File juju/control/tests/test_destroy_service.py (right): https://codereview.appspot.com/5892046/diff/15001/juju/control/tests/test_des... juju/control/tests/test_destroy_service.py:89: "Illegal attempt to destroy subordinate service 'logging'.", just a note on wording.. illegal is a bit strong here.. i'd go with invalid, as well saying why its invalid, ie what system state prevents the behavior, in this case the existenc eof a subordinate relation. https://codereview.appspot.com/5892046/diff/15001/juju/environment/defaults.py File juju/environment/defaults.py (right): https://codereview.appspot.com/5892046/diff/15001/juju/environment/defaults.p... juju/environment/defaults.py:6: DEFAULT_SERIES="precise" default-series is required in config, the important bit here is to force the local provider to actually use the provided value, instead of just moving the constant around. this is also completely orthogonal to the functionality in this branch though. https://codereview.appspot.com/5892046/diff/15001/juju/unit/lifecycle.py File juju/unit/lifecycle.py (right): https://codereview.appspot.com/5892046/diff/15001/juju/unit/lifecycle.py#newc... juju/unit/lifecycle.py:380: # could this service be a principal container We've been trying to use proper punctuation on comments per evidence in the rest of this module. ie. Caps and periods. https://codereview.appspot.com/5892046/diff/15001/juju/unit/lifecycle.py#newc... juju/unit/lifecycle.py:381: principal = not (yield self._service.is_subordinate()) For booleans its better to be explicit, is_principal would, else its ambigious looking at it later below. https://codereview.appspot.com/5892046/diff/15001/juju/unit/lifecycle.py#newc... juju/unit/lifecycle.py:409: service_relation = old_relations[relation_id] This inverts our common usage of supervision trees for any resource entity and leaves it subject to breakage if the managed resource is a runaway, albeit in this context its more of a sibling resource. ie. the principal unit should be removing the subordinate, not the subordinate removing itself. https://codereview.appspot.com/5892046/diff/15001/juju/unit/lifecycle.py#newc... juju/unit/lifecycle.py:420: if (principal and service_relation.relation_scope == "container"): what if we have multiple container scope'd relations to the same service? https://codereview.appspot.com/5892046/diff/15001/juju/unit/lifecycle.py#newc... juju/unit/lifecycle.py:451: self._remote_service[service_relation.internal_relation_id] = remote_service This seems fragile, think of a process restart, what happens when we go to undeploy? the cache need is a bit unclear as well... we have access to the remote endpoint even post topology removal from its tree state. https://codereview.appspot.com/5892046/diff/15001/juju/unit/lifecycle.py#newc... juju/unit/lifecycle.py:459: So based on our conversation, and the desire to land this functionality soon, we should table removal and put in place the control guards about subrodinate removals.
Please take a look.
Please take a look.
As has been noted a few times, this can't be reviewed till the diff actually looks sane. -k On Tue, Apr 10, 2012 at 9:23 PM, <bcsaller@gmail.com> wrote: > Please take a look. > > https://codereview.appspot.**com/5892046/<https://codereview.appspot.com/5892... >