Implement child relation hook context with lookup by relation id

https://code.launchpad.net/~jimbaker/juju/relation-hook-context/+merge/99149

Requires: https://code.launchpad.net/~jimbaker/juju/relation-id/+merge/99148

(do not edit description out of merge proposal)

[jimbaker, 2012-03-26 01:49:32]

Please take a look.

[hazmat, 2012-03-27 22:27:51]

mostly the concern here is how this is trying to achieve a consistent state.

https://codereview.appspot.com/5909056/diff/1/juju/hooks/invoker.py
File juju/hooks/invoker.py (right):

https://codereview.appspot.com/5909056/diff/1/juju/hooks/invoker.py#newcode129
juju/hooks/invoker.py:129: self._relation_contexts_flush_order = []
why are we tracking the context order here? the changes being made are to a
resource (unit-settings) that is effectively locked for the duration of the hook
execution.

https://codereview.appspot.com/5909056/diff/1/juju/hooks/invoker.py#newcode285
juju/hooks/invoker.py:285: changes = yield context.flush()
for context in self._relation_context.values() given the above comment.

https://codereview.appspot.com/5909056/diff/1/juju/state/hook.py
File juju/state/hook.py (right):

https://codereview.appspot.com/5909056/diff/1/juju/state/hook.py#newcode127
juju/state/hook.py:127: 
we discussed this in our call, but fwiw.

its not clear that hook context should be responsible for this, as it manages a
single execution context. definitely passing in a topology to a state manager
raises a red flag.

there's inherently a race here, thats currently being avoided by passing around
the topology. afaics a cleaner way to do and avoid the topology passing is 1)
calculate the full rel set upfront, 2) this lets the individual hook contexts
continue to manage their own consistency on a per relation basis.

https://codereview.appspot.com/5909056/diff/1/juju/state/hook.py#newcode162
juju/state/hook.py:162: getattr(self, "_change", EMPTY_CHANGE),
empty change should go into the spec. charm authors would need to be aware of
this. several charms dispatch on change type. for non implicit hook contexts, i
think the change should always be none, else its propagating change information
out of context.

https://codereview.appspot.com/5909056/diff/1/juju/state/hook.py#newcode197
juju/state/hook.py:197: self._relation_id = relation_id if relation_id else
change.relation_id
why would it be passed in sometimes and on the change at others.

it feels like the construction should be uniform here, the caller is making the
distinction between implicit/implied contexts.

ie. relation_id is required.

[jimbaker, 2012-03-30 05:30:27]

Please take a look.

[jimbaker, 2012-03-30 06:10:57]

Please take a look at the latest proposal

https://codereview.appspot.com/5909056/diff/1/juju/hooks/invoker.py
File juju/hooks/invoker.py (right):

https://codereview.appspot.com/5909056/diff/1/juju/hooks/invoker.py#newcode129
juju/hooks/invoker.py:129: self._relation_contexts_flush_order = []
Removed per discussion.

https://codereview.appspot.com/5909056/diff/1/juju/hooks/invoker.py#newcode285
juju/hooks/invoker.py:285: changes = yield context.flush()
Removed

https://codereview.appspot.com/5909056/diff/1/juju/state/hook.py
File juju/state/hook.py (right):

https://codereview.appspot.com/5909056/diff/1/juju/state/hook.py#newcode127
juju/state/hook.py:127: 
No longer passing in the topology to the state mgr

https://codereview.appspot.com/5909056/diff/1/juju/state/hook.py#newcode197
juju/state/hook.py:197: self._relation_id = relation_id if relation_id else
change.relation_id
Now required

[hazmat, 2012-04-02 15:16:37]

lgtm.

please add a bug for the inconsistent memberships across relations within a
hook.

also add a test for a relation removed during execution (post invoker start).

https://codereview.appspot.com/5909056/diff/6001/juju/hooks/invoker.py
File juju/hooks/invoker.py (right):

https://codereview.appspot.com/5909056/diff/6001/juju/hooks/invoker.py#newcod...
juju/hooks/invoker.py:151: # flushed since it could be a nonrelational context
drop everything after the semicolon, as the rest isn't true, the primary context
is always tracked as _context.

https://codereview.appspot.com/5909056/diff/6001/juju/hooks/invoker.py#newcod...
juju/hooks/invoker.py:158: relation_idents = set((yield
self.get_relation_idents(None)))
could use an inline comment, about what its doing (ie. None ==all)

https://codereview.appspot.com/5909056/diff/6001/juju/hooks/tests/test_invoke...
File juju/hooks/tests/test_invoker.py (right):

https://codereview.appspot.com/5909056/diff/6001/juju/hooks/tests/test_invoke...
juju/hooks/tests/test_invoker.py:988: "    Setting changed: 'a'='42' (was
unset)",
changes need to be qualified with relation ident now that they can refer to any
of a unit's relation settings.

https://codereview.appspot.com/5909056/diff/6001/juju/state/hook.py
File juju/state/hook.py (right):

https://codereview.appspot.com/5909056/diff/6001/juju/state/hook.py#newcode18
juju/state/hook.py:18: "relation_ident change_type unit_name")):
nice

https://codereview.appspot.com/5909056/diff/6001/juju/state/tests/test_hook.py
File juju/state/tests/test_hook.py (right):

https://codereview.appspot.com/5909056/diff/6001/juju/state/tests/test_hook.p...
juju/state/tests/test_hook.py:452: ValueError)
these would be better as invalidrelationidentity

[jimbaker, 2012-04-04 00:58:14]

Please take a look.

https://codereview.appspot.com/5909056/diff/6001/juju/state/tests/test_hook.py
File juju/state/tests/test_hook.py (right):

https://codereview.appspot.com/5909056/diff/6001/juju/state/tests/test_hook.p...
juju/state/tests/test_hook.py:452: ValueError)
Changed to InvalidRelationIdentity.

[jimbaker, 2012-04-04 01:04:56]

This also addresses the following concerns:

Relation members will now be empty if the relation has been removed, paralleling
what happens in a DepartedRelationHookContext; formerly an InternalTopologyError
was actually raised. This also required fixing code in juju.state.relation.

Removing relations is now tested, both in test_hook and test_invoker.

https://codereview.appspot.com/5909056/diff/6001/juju/hooks/invoker.py
File juju/hooks/invoker.py (right):

https://codereview.appspot.com/5909056/diff/6001/juju/hooks/invoker.py#newcod...
juju/hooks/invoker.py:151: # flushed since it could be a nonrelational context
That was part of an obsolete comment; removed

https://codereview.appspot.com/5909056/diff/6001/juju/hooks/invoker.py#newcod...
juju/hooks/invoker.py:158: relation_idents = set((yield
self.get_relation_idents(None)))
On 2012/04/02 15:16:37, hazmat wrote:
> could use an inline comment, about what its doing (ie. None ==all)

Done.

https://codereview.appspot.com/5909056/diff/6001/juju/hooks/tests/test_invoke...
File juju/hooks/tests/test_invoker.py (right):

https://codereview.appspot.com/5909056/diff/6001/juju/hooks/tests/test_invoke...
juju/hooks/tests/test_invoker.py:988: "    Setting changed: 'a'='42' (was
unset)",
Relation settings on relations other than the parent are now qualified.

[jimbaker, 2012-04-04 01:17:10]

Please take a look.

[hazmat, 2012-04-04 01:32:03]

looks good, nice work. one minor discussed on irc.

https://codereview.appspot.com/5909056/diff/8212/juju/hooks/invoker.py
File juju/hooks/invoker.py (right):

https://codereview.appspot.com/5909056/diff/8212/juju/hooks/invoker.py#newcod...
juju/hooks/invoker.py:308: if context is self._context:
it feels like we should always have rel id reported on change instead of
omitting it