Rename ServiceRelation to RelatedService

Rename ServiceRelation to RelatedService

Hopefully the name betrays intent a little more clearly; the type represents
a specific service in a relation from the perspective of another service in
the relation (or at least from a unit eligible to join that relation, which
may be a better description in a peer relation...). role/Role field/method
were also changed to refer to the "remote" service rather than the "local"
service.

Also added RelationRole.RelatedRole() to replace most of
RelationEndpoint.CanRelateTo and a small part of the relation
sanity-checking in topology.

https://code.launchpad.net/~fwereade/juju-core/related-service/+merge/110578

Requires: https://code.launchpad.net/~fwereade/juju-core/redundant-relation-prefixes/+merge/110504

(do not edit description out of merge proposal)

[fwereade, 2012-06-15 17:28:03]

Please take a look.

[niemeyer, 2012-06-15 20:49:33]

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

https://codereview.appspot.com/6307099/diff/1/state/relation.go#newcode17
state/relation.go:17: func (r RelationRole) RelatedRole() RelationRole {

s/Related/Counterpart/

I'm not sure this is a good idea, since there isn't really such a need to have a
single sensible counterpart role for every other role, but I don't mind for the
moment.

https://codereview.appspot.com/6307099/diff/1/state/relation.go#newcode63
state/relation.go:63: // RelatedService represents a "remote" service
participating in a relation,

This is constraining the API of the state package to the specific use case of
sitting within a unit agent. If I say:

    st, err := state.Open(...)

That "st" I get is the *environment state*. It is not local, and it is not
remote. It is *the environment*. The code within the unit agent can make more
assumptions about what is local and what is remote. The code within this package
can't make that kind of assumption.

Another way to explain it. We have this logic in place today:

// AddRelation creates a new relation with the given endpoints.  
func (s *State) AddRelation(endpoints ...RelationEndpoint) (*Relation,
[]*ServiceRelation, error) {

We get back all the ServiceRelations when we add the relation. Which one of them
is the "remote" service and which of them is the "local" service? It just
doesn't make sense.

[niemeyer, 2012-06-15 21:19:13]

Sorry, after thinking further about this, and realizing that you have already
changed AddRelation so that it doesn't return ServiceRelation anymore. Perhaps
the direction you're taking this may actually fine, 
but please help me understanding where you're trying to take this.

You've dropped Relation, which identified a relation. You've also dropped
ServiceRelation, which identified a relation from the perspective of the system,
without assuming a local endpoint.

How do we talk about a relation now?  If I want to remove a relation, or see
which services are participating in a given relation, how do I do that?

Also, if I have a service, that has a peer relation, this is the *only* service
in that relation. It doesn't make sense to return the service itself when
calling RelatedServices, nor does it make sense to return the local relation
endpoint when I call RelatedServices() on a requirer service (the requirer is
definitely not *relating* to itself).

I still have some of the sentiment that we're replacing a good model by
something that puts us into trouble, but I'm not sure. Would you mind to
brainstorm with me to see where we can take this?

[fwereade, 2012-06-18 10:38:45]

On 2012/06/15 21:19:13, niemeyer wrote:
> Sorry, after thinking further about this, and realizing that you have already
> changed AddRelation so that it doesn't return ServiceRelation anymore. Perhaps
> the direction you're taking this may actually fine, 
> but please help me understanding where you're trying to take this.

I hope the following makes sense to you :).

> You've dropped Relation, which identified a relation. You've also dropped
> ServiceRelation, which identified a relation from the perspective of the
system,
> without assuming a local endpoint.

I dropped Relation, because it was essentially unused. We don't have any way to
list all relations, even in python, because there's no point in doing so; and,
while we do use the Relation type, the only thing we do is effectively
rsm.remove_relation(rsm.get_relation_state(*endpoints)), which is IMO far better
expressed as rsm.remove_relation(*endpoints); less code, fewer types, more
expressive. I don't see a downside here.

More on ServiceRelation to follow...

> How do we talk about a relation now?  If I want to remove a relation, or see
> which services are participating in a given relation, how do I do that?

We identify a relation to remove by looking at the endpoints, as we always did;
we've just dropped an obfuscatory extra step. AFAICT, we don't ever need to know
what services are participating in a particular relation; but, given a service,
we do need to know what it relates to, and this seems to me to apply both when
running a unit agent and when displaying status. What use cases have I missed
here?

Assuming I haven't, I think that RelatedService collects all the information
it's useful for us to know. In each of the above contexts, we want to do
something in response to the fact that a "local" service is related to a bunch
of other services; in every case, this involves knowing what the "local" service
thinks the relation is called (ie we always want to somehow pull in information
about the "local" endpoint).

I'm not saying we couldn't take a perspective-free ServiceRelation and figure
this out; I'm saying that, because we always end up wanting to use information
about each end of the relation, a perspective-free ServiceRelation is of limited
utility.

> Also, if I have a service, that has a peer relation, this is the *only*
service
> in that relation. It doesn't make sense to return the service itself when
> calling RelatedServices,

In both the unit agent and the status command, a service needs to deal with
itself-within-a-peer-relation in the same way it needs to deal with
another-service-in-pro-req-relation. We *could* do as we do, and get a bunch of
ServiceRelations from which we get all involved services and then strip out the
"local" service except in the case of a peer relation... or we could just grab
the related "remote" services and work with them directly.

> nor does it make sense to return the local relation
> endpoint when I call RelatedServices() on a requirer service (the requirer is
> definitely not *relating* to itself).

Quite so. A service-which-is-only-a-requirer's RelatedServices will include
providers only; but they'll be annotated with the "local" relation name, because
that *is* the relation name in every context in which they're used.

> I still have some of the sentiment that we're replacing a good model by
> something that puts us into trouble, but I'm not sure. Would you mind to
> brainstorm with me to see where we can take this?

I'm not sure it actually is a good model for us to use in the code; it seems to
me that recasting the situation in the terms I'm proposing allows us to discard
a lot of (IMO) confusing and superfluous code, both in the status command and in
the unit agent; and I don't *think* I'm making our lives any harder in any other
situations. (Counterexamples here will be the quickest way of convincing me of
crackfulness...) Medium-term, I'm aiming for something a bit like:

// For use by status
(*Service) RelatedServices() ([]RelatedService, error)

// For use by the unit agent (at the top level)
(*Service) WatchRelatedServices() (*RelatedServicesWatcher, error)
(*RelatedServicesWatcher) Changes() <-chan RelatedServicesChange

// For use by the unit agent (per-relation)
(*Unit) AgentJoin(*RelatedService) (*AgentRelation, error)
(*AgentRelation) Changes() <-chan RelatedUnitsChange
(*AgentRelation) Settings() (*ConfigNode, error)
(*AgentRelation) Depart() error
(*AgentRelation) Abscond() error

Note in particular that we end up with a very helpful symetry on AgentRelation
-- an AgentJoin will lead to a -joined on the counterpart, a write to Settings
will lead to a -changed, and a call to Depart will lead to a -departed. (A call
to Abscond will eventually lead to a -departed, but the purpose of that method
is to sneak away without detection in the hope that the agent will replace
itself before anyone else notices it's gone).

None of this includes unit state or unit relation state, which I think are
separate problems, and which I don't think are directly related here.