Agent lifecycle suport for subordinates

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)

[bcsaller, 2012-03-23 18:51:45]

Please take a look.

[hazmat, 2012-03-25 21:12:41]

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.

[bcsaller, 2012-04-03 06:52:03]

Please take a look.

[bcsaller, 2012-04-03 07:03:41]

[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

[bcsaller, 2012-04-03 07:07:09]

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

[hazmat, 2012-04-04 16:38:51]

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

[bcsaller, 2012-04-09 06:00:04]

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.

[hazmat, 2012-04-09 23:02:22]

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.

[bcsaller, 2012-04-10 23:01:30]

Please take a look.

[bcsaller, 2012-04-11 01:23:52]

Please take a look.

[hazmat, 2012-04-11 09:40:12]

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...
>