relation-ids lists only complete relations

relation-ids lists only complete relations

Fixes a problem where the relation-ids hook command, as supported by `HookContext.get_relations`, would return relation ids for relations not fully constructed, that is missing a `UnitRelationState`. Also fixed corresponding test setups, to ensure that these states are in ZK, so as to correspond to what ZK looks like in an actual scenario. Also renamed `Invoker.get_relation_hook_context` to `Invoker.get_cached_relation_hook_context` and changed clients to not yield on it, since it's not actually returning a Deferred. In practice, this seemed to change the behavior such that it went from occasionally failing for a test stack using memcached to *always* failing, before the fix to `get_relations`, at which point it now has succeeded every time.

The stack test I used for verifying this works in practice:

juju deploy --num-units=2 mediawiki
juju deploy mysql
juju deploy memcached
juju add-relation mediawiki:db mysql:db
juju add-relation memcached mediawiki
juju expose mediawiki

jitsu watch --failfast \
    mysql --state=started -r "mediawiki:db mysql:db" --setting=database \
    memcached --state=started -r "mediawiki memcached" --setting=host \
    mediawiki --num-units=2 --state=started --open-port=80

https://code.launchpad.net/~jimbaker/juju/relation-hook-cache-race/+merge/130720

(do not edit description out of merge proposal)

[jimbaker, 2012-10-22 03:01:19]

Please take a look.

[jameinel, 2012-10-29 15:00:53]

Martin worked with me to review this code. Overall the code changes look good,
just some small polish to make the code a bit clearer as to what is actually
going on.

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

https://codereview.appspot.com/6749057/diff/1/juju/state/hook.py#newcode118
juju/state/hook.py:118: yield relation.get_unit_state(unit_state)
It seems a bit odd that you pass 'get_unit_state' a 'unit_state'. Offhand I
would have expected something more like:
unit_state = yield relation.get_unit_state(...)

This could be correct usage of the api, but the variable naming makes it hard to
be sure of that.

https://codereview.appspot.com/6749057/diff/1/juju/state/hook.py#newcode121
juju/state/hook.py:121: pass  # ignore for now, this has not yet been
constructed
Would it make sense to have a debug log message here, rather than a silent
success?

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

https://codereview.appspot.com/6749057/diff/1/juju/state/tests/test_hook.py#n...
juju/state/tests/test_hook.py:106: blog_name, "client-server", "app",
"client"))))
This seems like it would be easier to read as:
mysql_ep = RelationEndpoint(...)
other_ep = yield self.add_relation_service_unit_to_another_endpoint(...)
blog_states = yield self.add_opposite_service_unit(...)

The nesting makes it hard to parse out what steps are executed in what order.

https://codereview.appspot.com/6749057/diff/1/juju/state/tests/test_hook.py#n...
juju/state/tests/test_hook.py:221: set((yield
context.get_relation_idents("not-a-relation"))),
It isn't clear that 'not-a-relation' is related to the partial update in (1). It
looks like this test would have passed without your change because
'not-a-relation' wouldn't have been found as a partial relationship.
Is the partial bit because the 'db' list only has 2 items instead of 3, and this
bit is just to make sure we don't crash when handed a relationship id that
doesn't exist?

[jimbaker, 2012-10-29 20:54:44]

Please look again, I made the suggested fixes.

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

https://codereview.appspot.com/6749057/diff/1/juju/state/hook.py#newcode118
juju/state/hook.py:118: yield relation.get_unit_state(unit_state)
On 2012/10/29 15:00:53, john.meinel wrote:
> It seems a bit odd that you pass 'get_unit_state' a 'unit_state'. Offhand I
> would have expected something more like:
> unit_state = yield relation.get_unit_state(...)
> 
> This could be correct usage of the api, but the variable naming makes it hard
to
> be sure of that.

From the docs of ServiceRelationState.get_unit_state (which takes as parameter,
`unit_state` :),

"""Given a service unit state, return its unit relation state."""

So just two types of unit state here! I changed it accordingly so that the
variable passed in is `service_unit_state`.

https://codereview.appspot.com/6749057/diff/1/juju/state/hook.py#newcode121
juju/state/hook.py:121: pass  # ignore for now, this has not yet been
constructed
On 2012/10/29 15:00:53, john.meinel wrote:
> Would it make sense to have a debug log message here, rather than a silent
> success?

It's normal to have this happen, but it is also potentially interesting to log.
Also logging allows us to further verify this passing over actually happened. So
I made the suggested change and added a test assertion.

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

https://codereview.appspot.com/6749057/diff/1/juju/state/tests/test_hook.py#n...
juju/state/tests/test_hook.py:106: blog_name, "client-server", "app",
"client"))))
On 2012/10/29 15:00:53, john.meinel wrote:
> This seems like it would be easier to read as:
> mysql_ep = RelationEndpoint(...)
> other_ep = yield self.add_relation_service_unit_to_another_endpoint(...)
> blog_states = yield self.add_opposite_service_unit(...)
> 
> The nesting makes it hard to parse out what steps are executed in what order.

That's quite reasonable, especially since the names you suggest are not quite
right, thus indicating it's not at easy to parse. Done.

https://codereview.appspot.com/6749057/diff/1/juju/state/tests/test_hook.py#n...
juju/state/tests/test_hook.py:221: set((yield
context.get_relation_idents("not-a-relation"))),
On 2012/10/29 15:00:53, john.meinel wrote:
> It isn't clear that 'not-a-relation' is related to the partial update in (1).
It
> looks like this test would have passed without your change because
> 'not-a-relation' wouldn't have been found as a partial relationship.
> Is the partial bit because the 'db' list only has 2 items instead of 3, and
this
> bit is just to make sure we don't crash when handed a relationship id that
> doesn't exist?

Correct, we are just asserting the expected behavior that get_relation_idents
returns an empty list if provided a nonexistent relation name, just like the
previous test (test_get_relation_idents).

Fixed.

[jimbaker, 2012-10-29 20:55:40]

Please take a look.