add RelationUnit type

add RelationUnit type

A RelationUnit maintains a unit's presence in a relation, and watches
changes to other RelationUnits running in other unit agents.

https://code.launchpad.net/~fwereade/juju-core/relation-unit/+merge/115030

Requires: https://code.launchpad.net/~fwereade/juju-core/relation-units-watcher/+merge/115001

(do not edit description out of merge proposal)

[fwereade, 2012-07-15 15:28:50]

Please take a look.

[dave_cheney.net, 2012-07-16 10:03:12]

https://codereview.appspot.com/6402048/diff/1/state/confignode.go
File state/confignode.go (right):

https://codereview.appspot.com/6402048/diff/1/state/confignode.go#newcode177
state/confignode.go:177: // lp:~fwereade/juju-core/config-node-bug; drop this
note
Please address the TODO

https://codereview.appspot.com/6402048/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6402048/diff/1/state/relation.go#newcode148
state/relation.go:148: settingsPath string
nit: s/settingsP/p/

[fwereade, 2012-07-16 12:22:16]

Please take a look.

[niemeyer, 2012-07-18 21:54:40]

This is looking *great*. Seems very close to one of the trickiest pieces in the
unit agent, and feels like we're almost there.

I have a suggestion for a small twist on the API, which shouldn't really be too
painful or even change the implementation too much at all.

In the current incarnation, that one code path via AgentJoin handles presence
signaling, observation of settings, observation of presence, and settings access
for changes. This is a lot of goodness, that unfortunately nobody can touch
except the unit agent since it's all hidden behind an API that only really make
sense there.

The fundamental ground of the proposal is attempting to turn the state logic
into something that makes sense not just for that one single unit agent, but for
anyone that wants to tap into these concepts.

I think that's doable with very minimal changes, just by twisting a little bit
the exact API you've put in place.

My proposal for the API is the following.

  func (s *State) RelationHandler(endpoints ...RelationEndpoint)
(*RelationHandler, error)
  
  type RelationHandler struct { ... }
  
  func (h *RelationHandler) Join(u *Unit) (*presence.Pinger, error)
  
  func (h *RelationHandler) Settings(u *Unit) (*ConfigNode, error)
  
  func (h *RelationHandler) Watch(ignore *Unit) (<-chan RelationUnitsChange,
error)

That's it.

Interestingly, despite this being more flexible since it gives us a way to
observe and change a relation no matter if we're sitting within the unit agent
or not, this is actually simpler! No need for Depart or Abandon, since Join
returns the Pinger, for example.

The ignore parameter in Watch should be optional eventually, so that we can
easily monitor all units in that relation when desired. It's cool to support
that later, though, if that turns out to be tricky.

Thoughts?

https://codereview.appspot.com/6402048/diff/6001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6402048/diff/6001/state/unit.go#newcode537
state/unit.go:537: func (u *Unit) AgentJoin(r *Relation) (*RelationUnit, error)
{
Some thinking should probably be put into the error reporting in these
functions. Right now it returns everything as-is, and it might get confusing in
some cases to tell what's actually failing.

https://codereview.appspot.com/6402048/diff/6001/state/unit.go#newcode551
state/unit.go:551: pkey, err := topo.UnitPrincipalKey(u.key)
We used to have the unit's principal key in u.principalKey, and we have it at
hand when we create the unit. It's extremely cheap and simplifies this logic, so
I suggest putting it back (and keeping isPrincipal as-is) so we can just use it
here and drop the topology-reading entirely.

https://codereview.appspot.com/6402048/diff/6001/state/unit.go#newcode565
state/unit.go:565: // will know it's time to add it.
I suggest cutting down the comment to:

// The unit agent should have set the private address by now.

The rest is timing-dependent information that will become out-of-date very soon,
and doesn't add to the above.

[fwereade, 2012-07-19 15:39:26]

I'm going to reject this CL and build the suggested RelationHandler methods into
Relation itself in new branches.

https://codereview.appspot.com/6402048/diff/6001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6402048/diff/6001/state/unit.go#newcode537
state/unit.go:537: func (u *Unit) AgentJoin(r *Relation) (*RelationUnit, error)
{
On 2012/07/18 21:54:40, niemeyer wrote:
> Some thinking should probably be put into the error reporting in these
> functions. Right now it returns everything as-is, and it might get confusing
in
> some cases to tell what's actually failing.

Good point, thanks.

https://codereview.appspot.com/6402048/diff/6001/state/unit.go#newcode551
state/unit.go:551: pkey, err := topo.UnitPrincipalKey(u.key)
On 2012/07/18 21:54:40, niemeyer wrote:
> We used to have the unit's principal key in u.principalKey, and we have it at
> hand when we create the unit. It's extremely cheap and simplifies this logic,
so
> I suggest putting it back (and keeping isPrincipal as-is) so we can just use
it
> here and drop the topology-reading entirely.

Addressed in https://codereview.appspot.com/6421049

https://codereview.appspot.com/6402048/diff/6001/state/unit.go#newcode565
state/unit.go:565: // will know it's time to add it.
On 2012/07/18 21:54:40, niemeyer wrote:
> I suggest cutting down the comment to:
> 
> // The unit agent should have set the private address by now.
> 
> The rest is timing-dependent information that will become out-of-date very
soon,
> and doesn't add to the above.

Cheers.