Merge lp:~jimbaker/pyjuju/rel-mgmt-add-relation-simple into lp:pyjuju

[Jim Baker]

This is substantially Kapil's patch (https://gist.github.com/727049). I have changed it so that add_relation_state still takes a relation type and kept assign_service, both for backwards compatibility. Otherwise, the scope of changes to the current tests would have been much larger, at least as I was able to determine. The remaining changes then are in the tests, mostly with with respect to consuming the tuple returned by add_relation_state, which is the RelationState plus any additional ServiceRelationStates, created.

This branch is of course much simpler than what I originally proposed for a command pattern approach to composable changes in https://bugs.launchpad.net/ensemble/+bug/684465

[Gustavo Niemeyer (niemeyer)]

[1]

Jim, as Kapil stated and as I reminded over our call, Kapil's code was a quick hack put
in place to demonstrate the concept. There are error situations which should be verified
and tested within that logic. Assigning a service to the same relation twice shouldn't
be possible, for instance, and InternalTopologyErrors should never surface out of the
state code, so there are checks which must be made and appropriate errors raised in
those cases.

Please look at existing code similar to that logic to identify the pattern.

review: Needs Fixing

[Jim Baker (jimbaker)]

On Mon, Dec 6, 2010 at 1:13 PM, Gustavo Niemeyer <email address hidden>wrote:

> Review: Needs Fixing
>
> [1]
>
> Jim, as Kapil stated and as I reminded over our call, Kapil's code was a
> quick hack put
> in place to demonstrate the concept. There are error situations which
> should be verified
> and tested within that logic. Assigning a service to the same relation
> twice shouldn't
> be possible, for instance, and InternalTopologyErrors should never surface
> out of the
> state code, so there are checks which must be made and appropriate errors
> raised in
> those cases.
>
> Please look at existing code similar to that logic to identify the pattern.
>

ack

[lp:~jimbaker/pyjuju/rel-mgmt-add-relation-simple]

111. By Jim Baker on 2010-12-07

Added unit tests for duplicate assignments and missing services in add_relation_state

112. By Jim Baker on 2010-12-07

Added unit tests for duplicate assignments and missing services in add_relation_state

113. By Jim Baker on 2010-12-07

More unit tests around add_relation_state to enusre InternalTopologyError exceptions do not leak

114. By Jim Baker on 2010-12-07

PEP8

[Jim Baker (jimbaker)]

Please take a look at the latest version of the branch that I've pushed.

On Mon, Dec 6, 2010 at 1:58 PM, Jim Baker <email address hidden> wrote:

> On Mon, Dec 6, 2010 at 1:13 PM, Gustavo Niemeyer <email address hidden>wrote:
>
>> Review: Needs Fixing
>>
>> [1]
>>
>> Jim, as Kapil stated and as I reminded over our call, Kapil's code was a
>> quick hack put
>> in place to demonstrate the concept. There are error situations which
>> should be verified
>> and tested within that logic. Assigning a service to the same relation
>> twice shouldn't
>> be possible, for instance, and InternalTopologyErrors should never surface
>> out of the
>> state code, so there are checks which must be made and appropriate errors
>> raised in
>> those cases.
>>
>> Please look at existing code similar to that logic to identify the
>> pattern.
>>
>
> ack
>

Additional checks added, along with unit tests to verify that appropriate
errors are raised (presumably ServiceRelationAlreadyAssigned) instead of
InternalTopologyError.

[Gustavo Niemeyer (niemeyer)]

[1]

Is the error checking really doing the right thing? It looks like it's verifying that
the services were not yet added to the relation which was just created, which will of
course always be false.

Am I misunderstanding it?

[Gustavo Niemeyer (niemeyer)]

As an additional aid, please note that the intention is preventing that the same
relation *endpoints* are not connected twice between the same two services. It's
fine to have different endpoints connected, though.

[Gustavo Niemeyer (niemeyer)]

[2]

One more point, I believe there's no need to have relation_type in the method signature,
since all of the endpoints must necessarily have that type as well (and they must be
the same, which is another interesting situation to catch).

[Gustavo Niemeyer (niemeyer)]

[3]

379 + def relation_has_role(self, relation_id, role):
380 + """Return true if this `role` is already assigned to `relation_id`.
381 + """

That's missing unittests.

[Gustavo Niemeyer (niemeyer)]

[4]

RelationState.assign_service() is probably a red herring, and should be removed so that we can
focus on the real issues. We have no plans of introducing a way to link additional *services*
into the same *relation*.

[Jim Baker (jimbaker)]

On Tue, Dec 7, 2010 at 1:41 PM, Gustavo Niemeyer <email address hidden>wrote:

> [4]
>
> RelationState.assign_service() is probably a red herring, and should be
> removed so that we can
> focus on the real issues. We have no plans of introducing a way to link
> additional *services*
> into the same *relation*.
>

agreed!

[Jim Baker (jimbaker)]

On Tue, Dec 7, 2010 at 1:38 PM, Gustavo Niemeyer <email address hidden>wrote:

> [3]
>
> 379 + def relation_has_role(self, relation_id, role):
> 380 + """Return true if this `role` is already assigned to
> `relation_id`.
> 381 + """
>
> That's missing unittests.
>

ack

[Jim Baker (jimbaker)]

On Tue, Dec 7, 2010 at 1:25 PM, Gustavo Niemeyer <email address hidden>wrote:

> [2]
>
> One more point, I believe there's no need to have relation_type in the
> method signature,
> since all of the endpoints must necessarily have that type as well (and
> they must be
> the same, which is another interesting situation to catch).

We should assert the endpoints share a relation_type. In addition we now
must have either one or two endpoints passed in, since we will eliminate the
relation_type (and vestigial assign_service).

[Jim Baker (jimbaker)]

On Tue, Dec 7, 2010 at 1:20 PM, Gustavo Niemeyer <email address hidden>wrote:

>
> [1]
>
> Is the error checking really doing the right thing? It looks like it's
> verifying that
> the services were not yet added to the relation which was just created,
> which will of
> course always be false.
>
> Am I misunderstanding it?

This is not clear to me - what error checking are you referring specifically
to?

[Gustavo Niemeyer (niemeyer)]

[1]

Of course. I'm referring to the error checking you introduced in the branch as
a side effect of point 1 above. E.g.:

55 + if (topology.relation_has_service(
56 + service_relation.internal_relation_id,
57 + service_relation.internal_service_id) or
58 + topology.relation_has_role(
59 + service_relation.internal_relation_id,
60 + service_relation.relation_role)):
61 + raise ServiceRelationAlreadyAssigned(
62 + topology.get_service_name(
63 + service_relation.internal_service_id),
64 + service_relation.relation_name,
65 + service_relation.relation_role)

[2]

> We should assert the endpoints share a relation_type. In addition we now
> must have either one or two endpoints passed in, since we will eliminate the
> relation_type (and vestigial assign_service).

Sounds good. It doesn't ever make sense to create a relation without services, and
most probably, as we discussed yesterday, it doesn't make sense to assign new
services to an existing relation either.

[lp:~jimbaker/pyjuju/rel-mgmt-add-relation-simple]

115. By Jim Baker on 2010-12-07

Merge trunk

116. By Jim Baker on 2010-12-08

Transitional code to remove support for assign_service/unassign_service

117. By Jim Baker on 2010-12-08

Fix ensemble.state.tests.test_hook

118. By Jim Baker on 2010-12-08

Fix ensemble.hooks.tests.test_invoker

119. By Jim Baker on 2010-12-08

Fix test_watch_relations_when_being_created

120. By Jim Baker on 2010-12-08

Change add_relation_state so that it requires 1 or 2 endpoints, which share a relation type

121. By Jim Baker on 2010-12-08

Removed relation_type from add_relation_state signature

122. By Jim Baker on 2010-12-08

Cleanup

123. By Jim Baker on 2010-12-08

Update docs and use better method names

124. By Jim Baker on 2010-12-08

Merged trunk

125. By Jim Baker on 2010-12-08

Test topology.relation_has_role

126. By Jim Baker on 2010-12-08

Verify compatibility of endpoints in add_relation_state, so we can remove topology checking logic

127. By Jim Baker on 2010-12-08

PEP8

[Jim Baker (jimbaker)]

On Wed, Dec 8, 2010 at 6:07 AM, Gustavo Niemeyer <email address hidden>wrote:

> [1]
>
> Of course. I'm referring to the error checking you introduced in the branch
> as
> a side effect of point 1 above. E.g.:
>
> 55 + if (topology.relation_has_service(
> 56 + service_relation.internal_relation_id,
> 57 + service_relation.internal_service_id) or
> 58 + topology.relation_has_role(
> 59 + service_relation.internal_relation_id,
> 60 + service_relation.relation_role)):
> 61 + raise ServiceRelationAlreadyAssigned(
> 62 + topology.get_service_name(
> 63 + service_relation.internal_service_id),
> 64 + service_relation.relation_name,
> 65 + service_relation.relation_role)
>
>
Removed error checking in the topology test in favor of checking the
endpoints for compatibility. As a side effect, I also did the pending
revision of provides -> server, requires -> client, and peers -> peer to
standardize role names. Note that since we use other role names in testing,
the endpoint verification is weaker than it might be.

> [2]
>
> > We should assert the endpoints share a relation_type. In addition we now
> > must have either one or two endpoints passed in, since we will eliminate
> the
> > relation_type (and vestigial assign_service).
>
> Sounds good. It doesn't ever make sense to create a relation without
> services, and
> most probably, as we discussed yesterday, it doesn't make sense to assign
> new
> services to an existing relation either.
>
>
Done

[Jim Baker (jimbaker)]

On Tue, Dec 7, 2010 at 1:25 PM, Gustavo Niemeyer <email address hidden>wrote:

> [2]
>
> One more point, I believe there's no need to have relation_type in the
> method signature,
> since all of the endpoints must necessarily have that type as well (and
> they must be
> the same, which is another interesting situation to catch).

Removed, and asserted they must be the same in the endpoints, with tests.

[lp:~jimbaker/pyjuju/rel-mgmt-add-relation-simple]

128. By Jim Baker on 2010-12-09

Added implementation, testing, and use of InternalTopology.has_relation_between_endpoints to prevent adding the same relation for endpoints twice (peer or binary)

[Gustavo Niemeyer (niemeyer)]

[5]

Here is a simpler version of has_relation_between_endpoints:

        service_ids = dict((e, self.find_service_with_name(e.service_name))
                           for e in endpoints)
        relations = self._state.get("relations", self._nil_dict)
        for _, services in relations.itervalues():
            for endpoint in endpoints:
                service = services.get(service_ids[endpoint])
                if not service or service["name"] != endpoint.relation_name:
                    break
            else:
                return True
        return False

[6]

1450 + def test_has_relation_between_dyadic_endpoints_wrong_role(self):

This test is verifying a situation we don't ever want to allow, and if the situation
does to through as a bug, we shouldn't allow a new relation to be created among the
same endpoints. In other words, (service1:a, service2:b) shouldn't *ever* be ambiguous.

[7]

+ # TODO: consider adding this assertion, but it will break some
+ # test code (example: some tests use the role name of
+ # ``role``). It may also make sense to have other types of
+ # role names outside of testing (such as ``dev`` or ``prod``).

The assertion seems sensible. The alternative relation roles don't seem to make sense, though.

[8]

+ def add_relation(topology):
+ if topology.has_relation_between_endpoints(endpoints):
+ raise RelationAlreadyExists(endpoints)

Besides doing this inside the change method, I suggest redundantly doing that
upfront before the relation state is created, so that we avoid garbage which
we know won't be usable.

[9]

+ if len(endpoints) == 2:
+ assert endpoints[0].relation_role != "peer", \
+ "Duplicate peer endpoint"
+ if (endpoints[0].relation_role in ("client", "server") or
+ endpoints[1].relation_role in ("client", "server")):
+ assert endpoints[0].may_relate_to(endpoints[1]), \
+ "Endpoints are not compatible: %r" % (endpoints,)

These assertions should be proper well-tested errors, rather than AssertionErrors,
since they are acceptable situations within the application which need proper
handling and proper notifying.

Also, note that the nested "if" isn't really necessary. If there are two endpoints,
they must necessarily be compatible, so you can safely enforce endpoints[0].may_relate_to(endpoints[1]) at all times.

[10]

+ self.topology._relation_matches_endpoints(
+ "mysql",
+ ("mysql", {"s-0": {"name": "mysql", "role": "client"}}),
+ {"s-0": blog_ep, "s-1": mysql_ep}),
+ "Missing assigned service")

Unless really necessary, we try to avoid whitebox testing, since it binds the test to
the specifics of the implementation. That case is even more obvious given how convoluted
the signature of this method is. Just looking at this method, for instance, I have no
idea about what these arguments mean.

Luckily, given [5], this should easily go away.

review: Needs Fixing

[Jim Baker (jimbaker)]

Pushed the latest revision, so please do what I hope is one last review on
that.

On Thu, Dec 9, 2010 at 8:36 AM, Gustavo Niemeyer <email address hidden>wrote:

> Review: Needs Fixing
> [5]
>
> Here is a simpler version of has_relation_between_endpoints:
>
> service_ids = dict((e, self.find_service_with_name(e.service_name))
> for e in endpoints)
> relations = self._state.get("relations", self._nil_dict)
> for _, services in relations.itervalues():
> for endpoint in endpoints:
> service = services.get(service_ids[endpoint])
> if not service or service["name"] != endpoint.relation_name:
> break
> else:
> return True
> return False
>
>
Implemented, thanks!

> [6]
>
> 1450 + def
> test_has_relation_between_dyadic_endpoints_wrong_role(self):
>
> This test is verifying a situation we don't ever want to allow, and if the
> situation
> does to through as a bug, we shouldn't allow a new relation to be created
> among the
> same endpoints. In other words, (service1:a, service2:b) shouldn't *ever*
> be ambiguous.
>
>
Removed

> [7]
>
> + # TODO: consider adding this assertion, but it will break some
> + # test code (example: some tests use the role name of
> + # ``role``). It may also make sense to have other types of
> + # role names outside of testing (such as ``dev`` or ``prod``).
>
> The assertion seems sensible. The alternative relation roles don't seem to
> make sense, though.
>

Fixed the TODO to document the intention for a future branch, where we will
change the tests accordingly so they don't use invalid role names.

> [8]
>
> + def add_relation(topology):
> + if topology.has_relation_between_endpoints(endpoints):
> + raise RelationAlreadyExists(endpoints)
>
> Besides doing this inside the change method, I suggest redundantly doing
> that
> upfront before the relation state is created, so that we avoid garbage
> which
> we know won't be usable.
>

Makes sense, added. Ideally this race on adds would be tested via mocker's
patching support, but deferring that for now, since this can be considered
an optimization.

> [9]
>
> + if len(endpoints) == 2:
> + assert endpoints[0].relation_role != "peer", \
> + "Duplicate peer endpoint"
> + if (endpoints[0].relation_role in ("client", "server") or
> + endpoints[1].relation_role in ("client", "server")):
> + assert endpoints[0].may_relate_to(endpoints[1]), \
> + "Endpoints are not compatible: %r" % (endpoints,)
>
> These assertions should be proper well-tested errors, rather than
> AssertionErrors,
> since they are acceptable situations within the application which need
> proper
> handling and proper notifying.
>
> Also, note that the nested "if" isn't really necessary. If there are two
> endpoints,
> they must necessarily be compatible, so you can safely enforce
> endpoints[0].may_relate_to(endpoints[1]) at all times.
>

Changed the errors accordingly and cleaned up the type checking logic. The
n...

[lp:~jimbaker/pyjuju/rel-mgmt-add-relation-simple]

129. By Jim Baker on 2010-12-10

Rewritten InternalTopology.has_relation_between_endpoints, per review

130. By Jim Baker on 2010-12-10

Resolved conflicts with trunk merge

131. By Jim Baker on 2010-12-10

Fixes per review comments

[Gustavo Niemeyer (niemeyer)]

Thanks for the changes. Just one follow up:

[9]

> Changed the errors accordingly and cleaned up the type checking logic. The
> nested-if is still required until we update the tests, per [7].

No, it's not required. As mentioned, the only chance of may_relate_to() being true
in that location is them being of the correct roles and types, and if it is false
the endpoints are necessarily incompatible.

Considering that, +1!

review: Approve

=== modified file 'ensemble/hooks/tests/test_invoker.py'
--- ensemble/hooks/tests/test_invoker.py	2010-12-07 19:05:09 +0000
+++ ensemble/hooks/tests/test_invoker.py	2010-12-10 23:06:27 +0000
@@ -11,6 +11,7 @@
 from ensemble.hooks import invoker
 from ensemble.hooks.protocol import UnitSettingsFactory
 from ensemble.state import hook
+from ensemble.state.endpoint import RelationEndpoint
 from ensemble.state.tests.test_relation import RelationTestBase
 
 
@@ -104,11 +105,14 @@
 
     @defer.inlineCallbacks
     def _default_relations(self):
-        self.mysql_states = yield self.add_relation_service_unit(
-            "mysql", "mysql", "", "server")
-        self.wordpress_states = yield self.add_relation_service_unit(
-            "mysql", "wordpress", "", "client",
-            relation_state=self.mysql_states["relation"])
+        wordpress_ep = RelationEndpoint(
+            "wordpress", "client-server", "", "client")
+        mysql_ep = RelationEndpoint(
+            "mysql", "client-server", "", "server")
+        self.wordpress_states = yield self.add_relation_service_unit_from_endpoints(
+            wordpress_ep, mysql_ep)
+        self.mysql_states = yield self.add_opposite_service_unit(
+            self.wordpress_states)
         self.relation = self.mysql_states["unit_relation"]
 
     def _manage_path(self):

=== modified file 'ensemble/state/endpoint.py'
--- ensemble/state/endpoint.py	2010-12-03 03:36:30 +0000
+++ ensemble/state/endpoint.py	2010-12-10 23:06:27 +0000
@@ -33,9 +33,9 @@
         if not isinstance(other, RelationEndpoint):
             raise TypeError("Not a RelationEndpoint", other)
         return (self.relation_type == other.relation_type and
-                ((self.relation_role == "provides" and
-                  other.relation_role == "requires") or
-                 (self.relation_role == "requires" and
-                  other.relation_role == "provides") or
-                 (self.relation_role == "peers" and
-                  other.relation_role == "peers")))
+                ((self.relation_role == "server" and
+                  other.relation_role == "client") or
+                 (self.relation_role == "client" and
+                  other.relation_role == "server") or
+                 (self.relation_role == "peer" and
+                  other.relation_role == "peer")))

=== modified file 'ensemble/state/errors.py'
--- ensemble/state/errors.py	2010-12-10 04:55:47 +0000
+++ ensemble/state/errors.py	2010-12-10 23:06:27 +0000
@@ -94,18 +94,13 @@
                self.unit_name
 
 
-class ServiceRelationAlreadyAssigned(StateError):
+class RelationAlreadyExists(StateError):
 
-    def __init__(self, service_name, relation_name, relation_role):
-        self.service_name = service_name
-        self.relation_name = relation_name
-        self.relation_role = relation_role
+    def __init__(self, *endpoints):
+        self.endpoints = endpoints
 
     def __str__(self):
-        return (
-            "Service %r is already in relation with different name %r "
-            "or role %r.") % (
-            self.service_name, self.relation_name, self.relation_role)
+        return "Relation already exists %r" % (self.endpoints,)
 
 
 class UnitRelationStateAlreadyAssigned(StateError):
@@ -156,7 +151,7 @@
             self.relation_role, self.service_name)
 
 
-class BadDescriptor(StateError):
+class BadDescriptor(ValueError, EnsembleError):
     """Descriptor is not valid.
 
     A descriptor must be of the form <service name>[:<relation name>].
@@ -169,3 +164,22 @@
 
     def __str__(self):
         return "Bad descriptor: %r" % (self.descriptor,)
+
+
+class DuplicateEndpoints(StateError):
+    """Endpoints cannot be duplicate."""
+
+    def __init__(self, *endpoints):
+        self.endpoints = endpoints
+
+    def __str__(self):
+        return "Duplicate endpoints: %r" % (self.endpoints,)
+
+class IncompatibleEndpoints(StateError):
+    """Endpoints are incompatible."""
+
+    def __init__(self, *endpoints):
+        self.endpoints = endpoints
+
+    def __str__(self):
+        return "Incompatible endpoints: %r" % (self.endpoints,)

=== modified file 'ensemble/state/relation.py'
--- ensemble/state/relation.py	2010-12-07 21:49:45 +0000
+++ ensemble/state/relation.py	2010-12-10 23:06:27 +0000
@@ -9,37 +9,107 @@
 
 from ensemble.state.base import StateBase
 from ensemble.state.errors import (
-    StateChanged, ServiceRelationAlreadyAssigned, UnitRelationStateNotFound,
-    UnitRelationStateAlreadyAssigned, UnknownRelationRole)
+    DuplicateEndpoints, IncompatibleEndpoints, RelationAlreadyExists,
+    StateChanged, UnitRelationStateAlreadyAssigned, UnitRelationStateNotFound,
+    UnknownRelationRole)
 
 
 class RelationStateManager(StateBase):
     """Manages the state of relations in an environment."""
 
     @inlineCallbacks
-    def add_relation_state(self, relation_type):
-        """Add a new relation state. The relation is of the given type.
+    def add_relation_state(self, *endpoints):
+        """Add new relation state with the common relation type of `endpoints`.
 
-        Type is used to distinguish the type of the relation, ie.
-        'postgres', or 'varnish'.
+        There must be one or two endpoints specified, with the same
+        `relation_type`. Their corresponding services will be assigned
+        atomically.
         """
+
+        # the TOODs in the following comments in this function are for
+        # type checking to be implemented ASAP. However, this will
+        # require some nontrivial test modification, so it's best to
+        # be done in a future branch. This is because these tests use
+        # such invalid role names as ``role``, ``dev``, and ``prod``;
+        # or they add non-peer relations of only one endpoint.
+
+        if len(endpoints) == 1:
+            # TODO verify that the endpoint is a peer endpoint only
+            pass
+        elif len(endpoints) == 2:
+            if endpoints[0] == endpoints[1]:
+                raise DuplicateEndpoints(endpoints)
+
+            # TODO verify that the relation roles are client or server
+            # only
+            if (endpoints[0].relation_role in ("client", "server") or
+                endpoints[1].relation_role in ("client", "server")):
+                if not endpoints[0].may_relate_to(endpoints[1]):
+                    raise IncompatibleEndpoints(endpoints)
+        else:
+            raise TypeError("Requires 1 or 2 endpoints, %d given" % \
+                            len(endpoints))
+
+        # first check so as to prevent unnecessary garbage in ZK in
+        # case this relation has been previously added
+        topology = yield self._read_topology()
+        if topology.has_relation_between_endpoints(endpoints):
+            raise RelationAlreadyExists(endpoints)
+
+        relation_type = endpoints[0].relation_type
+        relation_id = yield self._add_relation_state(relation_type)
+        services = []
+        for endpoint in endpoints:
+            service_id = topology.find_service_with_name(endpoint.service_name)
+            yield self._add_service_relation_state(
+                relation_id, service_id, endpoint)
+            services.append(ServiceRelationState(self._client,
+                                                 service_id,
+                                                 relation_id,
+                                                 endpoint.relation_role,
+                                                 endpoint.relation_name))
+
+        def add_relation(topology):
+            if topology.has_relation_between_endpoints(endpoints):
+                raise RelationAlreadyExists(endpoints)
+            topology.add_relation(relation_id, relation_type)
+            for service_relation in services:
+                if not topology.has_service(service_id):
+                    raise StateChanged()
+                topology.assign_service_to_relation(
+                    relation_id,
+                    service_relation.internal_service_id,
+                    service_relation.relation_name,
+                    service_relation.relation_role)
+        yield self._retry_topology_change(add_relation)
+
+        returnValue((RelationState(self._client, relation_id), services))
+
+    @inlineCallbacks
+    def _add_relation_state(self, relation_type):
         path = yield self._client.create(
             "/relations/relation-", flags=zookeeper.SEQUENCE)
-
         internal_id = basename(path)
-
         # create the settings container, for individual units settings.
         yield self._client.create(path + "/settings")
-
-        # Not clear if we really need to update the topology till the relation
-        # is in use. Topology updates are broadcasting to most connected
-        # clients. Perhaps a separate batch api would be useful to minimize
-        # broadcast events on the topology.
-        def add_relation(topology):
-            topology.add_relation(internal_id, relation_type)
-        yield self._retry_topology_change(add_relation)
-
-        returnValue(RelationState(self._client, internal_id))
+        returnValue(internal_id)
+
+    @inlineCallbacks
+    def _add_service_relation_state(
+        self, relation_id, service_id, endpoint):
+        """Add a service relation state.
+        """
+        # Add service container in relation.
+        node_data = yaml.safe_dump(
+            {"name": endpoint.relation_name, "role": endpoint.relation_role})
+        path = "/relations/%s/%s" % (relation_id, endpoint.relation_role)
+
+        try:
+            yield self._client.create(path, node_data)
+        except zookeeper.NodeExistsException:
+            # If its not, then update the node, and continue.
+            yield retry_change(
+                self._client, path, lambda content, stat: node_data)
 
     @inlineCallbacks
     def remove_relation_state(self, relation_state):
@@ -86,89 +156,6 @@
     def internal_id(self):
         return self._internal_id
 
-    @inlineCallbacks
-    def unassign_service(self, service_state):
-        """Remove a service from the relation.
-
-        Any removed service needs to have its zookeeper state garbage collected
-        external to this api. The individual units should remove their
-        contained nodes, upon notification but even then the service container
-        node within the relation remains to be garbage collected.
-        """
-
-        def remove_service(topology):
-            # Verify our relation and service still exist.
-            if not topology.has_relation(self._internal_id) or \
-               not topology.has_service(service_state.internal_id):
-                raise StateChanged()
-
-            # Ignore concurrent requests that achieve the same state.
-            if not topology.relation_has_service(
-                self._internal_id, service_state.internal_id):
-                return
-
-            topology.unassign_service_from_relation(
-                self._internal_id, service_state.internal_id)
-
-        yield self._retry_topology_change(remove_service)
-
-    @inlineCallbacks
-    def assign_service(self, service_state, relation_name, relation_role):
-        """Add a service to the relation.
-
-        @param service_state - The service to be added to the relation.
-        @param name - The service's name for this relation.
-        @param role - The service's role within this relation.
-        """
-        # Add service container in relation.
-        node_data = yaml.safe_dump(
-            {"name": relation_name, "role": relation_role})
-        path = "/relations/%s/%s" % (self.internal_id, relation_role)
-
-        try:
-            yield self._client.create(path, node_data)
-        except zookeeper.NodeExistsException:
-            # because we don't garbage collect, we need to ascertain
-            # if the service node is actually in use.
-            topology = yield self._read_topology()
-
-            # If it is assigned, its an error.
-            # There's a small chance for a race condition between the check
-            # and set.
-            if topology.relation_has_service(
-                self._internal_id, service_state.internal_id):
-                raise ServiceRelationAlreadyAssigned(
-                    service_state.service_name, relation_name, relation_role)
-
-            # If its not, then update the node, and continue.
-            yield retry_change(
-                self._client, path, lambda content, stat: node_data)
-
-        # Associate service to relation in the topology.
-        def add_service(topology):
-            # Verify our service and relation exists
-            if not topology.has_relation(self._internal_id) or \
-               not topology.has_service(service_state.internal_id):
-                raise StateChanged()
-
-            # Add it the topology if its not there.
-            if not topology.relation_has_service(
-                self._internal_id, service_state.internal_id):
-                topology.assign_service_to_relation(
-                    self.internal_id,
-                    service_state.internal_id,
-                    relation_name,
-                    relation_role)
-
-
-        yield self._retry_topology_change(add_service)
-
-        returnValue(ServiceRelationState(self._client,
-                                         service_state.internal_id,
-                                         self.internal_id,
-                                         relation_role,
-                                         relation_name))
-
 
 class ServiceRelationState(object):
     """A state representative of a relation between one or more services."""
@@ -695,4 +682,3 @@
         """
         return [unit_id for unit_id in units \
                 if unit_id != self._watcher_unit.internal_unit_id]
-

=== modified file 'ensemble/state/service.py'
--- ensemble/state/service.py	2010-12-10 04:59:54 +0000
+++ ensemble/state/service.py	2010-12-10 23:06:27 +0000
@@ -88,8 +88,11 @@
             formula_id)
         formula_metadata = yield formula_state.get_metadata()
         endpoints = set()
-        for relation_role in ("peers", "provides", "requires"):
-            relations = getattr(formula_metadata, relation_role)
+        relation_role_map = {
+            "peer": "peers", "client": "requires", "server": "provides"}
+        for relation_role in ("peer", "client", "server"):
+            relations = getattr(
+                formula_metadata, relation_role_map[relation_role])
             if relations:
                 for relation_name, spec in relations.iteritems():
                     if (query_relation_name is None or

=== modified file 'ensemble/state/tests/test_endpoint.py'
--- ensemble/state/tests/test_endpoint.py	2010-11-25 05:28:27 +0000
+++ ensemble/state/tests/test_endpoint.py	2010-12-10 23:06:27 +0000
@@ -4,9 +4,9 @@
 
 class RelationEndpointTest(TestCase):
     def test_may_relate_to(self):
-        mysql_ep = RelationEndpoint("mysqldb", "mysql", "db", "provides")
-        blog_ep = RelationEndpoint("blog", "mysql", "mysql", "requires")
-        pg_ep = RelationEndpoint("postgres", "postgres", "db", "provides")
+        mysql_ep = RelationEndpoint("mysqldb", "mysql", "db", "server")
+        blog_ep = RelationEndpoint("blog", "mysql", "mysql", "client")
+        pg_ep = RelationEndpoint("postgres", "postgres", "db", "server")
 
         self.assertRaises(TypeError, mysql_ep.may_relate_to, 42)
 
@@ -21,15 +21,15 @@
         # antireflexive om relation_role -
         # must be consumer AND provider or vice versa
         self.assertFalse(blog_ep.may_relate_to(
-            RelationEndpoint("foo", "mysql", "db", "requires")))
+            RelationEndpoint("foo", "mysql", "db", "client")))
         self.assertFalse(mysql_ep.may_relate_to(
-            RelationEndpoint("foo", "mysql", "db", "provides")))
+            RelationEndpoint("foo", "mysql", "db", "server")))
 
-        # irreflexive for provides/requires
+        # irreflexive for server/client
         self.assertFalse(mysql_ep.may_relate_to(mysql_ep))
         self.assertFalse(blog_ep.may_relate_to(blog_ep))
         self.assertFalse(pg_ep.may_relate_to(pg_ep))
 
-        # but reflexive for peers
-        riak_ep = RelationEndpoint("riak", "riak", "riak", "peers")
+        # but reflexive for peer
+        riak_ep = RelationEndpoint("riak", "riak", "riak", "peer")
         self.assert_(riak_ep.may_relate_to(riak_ep))

=== modified file 'ensemble/state/tests/test_errors.py'
--- ensemble/state/tests/test_errors.py	2010-11-04 03:41:11 +0000
+++ ensemble/state/tests/test_errors.py	2010-12-10 23:06:27 +0000
@@ -1,12 +1,14 @@
 from ensemble.lib.testing import TestCase
 
+from ensemble.state.endpoint import RelationEndpoint
 from ensemble.state.errors import (
     EnsembleError, StateError, StateChanged, FormulaStateNotFound,
     ServiceStateNotFound, ServiceUnitStateNotFound, MachineStateNotFound,
     ServiceUnitStateMachineAlreadyAssigned, ServiceStateNameInUse,
     BadServiceStateName, EnvironmentStateNotFound,
-    ServiceRelationAlreadyAssigned, UnitRelationStateNotFound,
-    UnitRelationStateAlreadyAssigned, UnknownRelationRole)
+    RelationAlreadyExists, UnitRelationStateNotFound,
+    UnitRelationStateAlreadyAssigned, UnknownRelationRole,
+    BadDescriptor, DuplicateEndpoints, IncompatibleEndpoints)
 
 
 class StateErrorsTest(TestCase):
@@ -76,13 +78,13 @@
         self.assertEquals(str(error),
                           "Environment state was not found")
 
-    def test_service_relation_already_assigned(self):
-        error = ServiceRelationAlreadyAssigned(
-            "wordpress", "mysql", "client")
+    def test_relation_already_exists(self):
+        error = RelationAlreadyExists(
+            RelationEndpoint("wordpress", "mysql", "mysql", "client"),
+            RelationEndpoint("mysql", "mysql", "db", "server"))
         self.assertIsStateError(error)
-        msg = ("Service 'wordpress' is already in relation with different "
-               "name 'mysql' or role 'client'.")
-        self.assertEquals(str(error), msg)
+        self.assertTrue("wordpress" in str(error))
+        self.assertTrue("mysql" in str(error))
 
     def test_unit_relation_state_not_found(self):
         error = UnitRelationStateNotFound(
@@ -103,3 +105,24 @@
         self.assertIsStateError(error)
         msg = "Unknown relation role 'server2' for service 'service-name'"
         self.assertEquals(str(error), msg)
+
+    def test_bad_descriptor(self):
+        error = BadDescriptor("a:b:c")
+        self.assertTrue(isinstance(error, EnsembleError))
+        msg = "Bad descriptor: 'a:b:c'"
+        self.assertEquals(str(error), msg)
+
+    def test_duplicate_endpoints(self):
+        riak_ep = RelationEndpoint("riak", "riak", "ring", "peer")
+        error = DuplicateEndpoints(riak_ep, riak_ep)
+        self.assertIsStateError(error)
+        self.assertTrue("riak" in str(error))
+
+    def test_incompatible_endpoints(self):
+        error = IncompatibleEndpoints(
+            RelationEndpoint("mysql", "mysql", "db", "server"),
+            RelationEndpoint("riak", "riak", "ring", "peer"))
+        self.assertIsStateError(error)
+        self.assertTrue("mysql" in str(error))
+        self.assertTrue("riak" in str(error))
+

=== modified file 'ensemble/state/tests/test_hook.py'
--- ensemble/state/tests/test_hook.py	2010-12-03 00:13:01 +0000
+++ ensemble/state/tests/test_hook.py	2010-12-10 23:06:27 +0000
@@ -3,6 +3,7 @@
 from twisted.internet.defer import inlineCallbacks
 from ensemble.lib.testing import TestCase
 
+from ensemble.state.endpoint import RelationEndpoint
 from ensemble.state.hook import RelationChange, HookContext
 from ensemble.state.errors import UnitRelationStateNotFound, StateError
 from ensemble.state.tests.test_relation import RelationTestBase
@@ -23,11 +24,14 @@
     @inlineCallbacks
     def setUp(self):
         yield super(ExecutionContextTest, self).setUp()
-        self.mysql_states = yield self.add_relation_service_unit(
-            "mysql", "mysql", "", "server")
-        self.wordpress_states = yield self.add_relation_service_unit(
-            "mysql", "wordpress", "", "client",
-            relation_state=self.mysql_states["relation"])
+        wordpress_ep = RelationEndpoint(
+            "wordpress", "client-server", "", "client")
+        mysql_ep = RelationEndpoint(
+            "mysql", "client-server", "", "server")
+        self.wordpress_states = yield self.add_relation_service_unit_from_endpoints(
+            wordpress_ep, mysql_ep)
+        self.mysql_states = yield self.add_opposite_service_unit(
+            self.wordpress_states)
         self.relation = self.mysql_states["relation"]
 
     def get_execution_context(self, states, change_type, unit_name):

=== modified file 'ensemble/state/tests/test_relation.py'
--- ensemble/state/tests/test_relation.py	2010-12-07 00:22:20 +0000
+++ ensemble/state/tests/test_relation.py	2010-12-10 23:06:27 +0000
@@ -8,11 +8,12 @@
     inlineCallbacks, returnValue, Deferred, fail, succeed)
 from txzookeeper import ZookeeperClient
 
+from ensemble.state.endpoint import RelationEndpoint
 from ensemble.state.formula import FormulaStateManager
 from ensemble.state.errors import (
-    StateChanged, ServiceRelationAlreadyAssigned,
-    UnitRelationStateNotFound, UnitRelationStateAlreadyAssigned,
-    UnknownRelationRole)
+    DuplicateEndpoints, IncompatibleEndpoints, RelationAlreadyExists,
+    StateChanged, UnitRelationStateAlreadyAssigned,
+    UnitRelationStateNotFound, UnknownRelationRole)
 from ensemble.state.relation import (
     RelationStateManager, ServiceRelationState, UnitRelationState)
 from ensemble.state.service import ServiceStateManager
@@ -40,13 +41,95 @@
 
     @inlineCallbacks
     def add_relation(self, relation_type, *services):
-        relation_state = yield self.relation_manager.add_relation_state(
-            relation_type)
+        """Support older tests that don't use `RelationEndpoint`s"""
+        endpoints = []
         for service_meta in services:
             service_state, relation_name, relation_role = service_meta
-            yield relation_state.assign_service(
-                service_state, relation_name, relation_role)
-        returnValue(relation_state)
+            endpoints.append(RelationEndpoint(
+                service_state.service_name,
+                relation_type,
+                relation_name,
+                relation_role))
+        relation_state = yield self.relation_manager.add_relation_state(
+            *endpoints)
+        returnValue(relation_state[0])
+
+    @inlineCallbacks
+    def add_relation_service_unit_from_endpoints(self, *endpoints):
+        """Build the relation and add one service unit to the first endpoint.
+
+        This method is used to migrate older tests that would create
+        the relation, assign one service, add a service unit, AND then
+        assign a service. However, service assignment is now done all
+        at once with the relation creation. Because we are interested
+        in testing what happens with the changes to the service units,
+        such tests remain valid.
+
+        Returns a dict to collect together the various state objects
+        being created. This is created from the perspective of the
+        first endpoint, but the states of all of the endpoints are
+        also captured, so it can be worked with from the opposite
+        endpoint, as seen in :func:`add_opposite_service_unit`.
+        """
+        # 1. Setup all service states
+        service_states = []
+        for endpoint in endpoints:
+            service_state = yield self.add_service(endpoint.service_name)
+            service_states.append(service_state)
+
+        # 2. And join together in a relation
+        relation_state, service_relation_states = \
+                        yield self.relation_manager.add_relation_state(
+            *endpoints)
+        
+        # 3. Add a service unit to only the first endpoint - we need
+        #    to test what happens when service units are added to the
+        #    other service state (if any), so do so separately
+        unit_state = yield service_states[0].add_unit_state()
+        relation_unit_state = yield service_relation_states[0].add_unit_state(
+            unit_state)
+
+        returnValue({
+            "service": service_states[0],
+            "services": service_states,
+            "unit": unit_state,
+            "relation": relation_state,
+            "service_relation": service_relation_states[0],
+            "unit_relation": relation_unit_state,
+            "service_relations": service_relation_states})
+
+    @inlineCallbacks
+    def add_opposite_service_unit(self, other_states):
+        """Given `other_states`, add a service unit to the opposite endpoint.
+
+        Like :func:`add_relation_service_unit_from_endpoints`, this is
+        used to support older tests. Although it's slightly awkward to
+        use because of attempt to be backwards compatible, it does
+        enable the testing of a typical case: we are now bringing
+        online a service unit on the opposite side of a relation
+        endpoint pairing.
+
+        TODO: there's probably a better name for this method.
+        """
+        
+        assert len(other_states["services"]) == 2
+        unit_state = yield other_states["services"][1].add_unit_state()
+        relation_unit_state = yield other_states["service_relations"][1].\
+                              add_unit_state(unit_state)
+
+        def rotate(X):
+            rotated = X[1:]
+            rotated.append(X[0])
+            return rotated
+
+        returnValue({
+            "service": other_states["services"][1],
+            "services": rotate(other_states["services"]),
+            "unit": unit_state,
+            "relation": other_states["relation"],
+            "service_relation": other_states["service_relations"][1],
+            "unit_relation": relation_unit_state,
+            "service_relations": rotate(other_states["service_relations"])})
 
     @inlineCallbacks
     def add_relation_service_unit(self,
@@ -54,7 +137,6 @@
                                   service_name,
                                   relation_name="name",
                                   relation_role="role",
-                                  relation_state=None,
                                   client=None):
         """
         Create a relation, service, and service unit, with the
@@ -63,21 +145,11 @@
         the service relation and the service relation state will
         utilize that as their zookeeper client.
         """
-        # Add the service
+        # Add the service, relation, unit states
         service_state = yield self.add_service(service_name)
-
-        if relation_state is None:
-            # If no relation is passed, create the relation
-            # and associate the service to it.
-            relation_state = yield self.add_relation(
-                relation_type,
-                (service_state, relation_name, relation_role))
-        else:
-            # If the relation exists, assign the service to it.
-            yield relation_state.assign_service(
-                service_state, relation_name, relation_role)
-
-        # add a unit of the service
+        relation_state = yield self.add_relation(
+            relation_type,
+            (service_state, relation_name, relation_role))
         unit_state = yield service_state.add_unit_state()
 
         # Get the service relation.
@@ -132,11 +204,11 @@
 
     @inlineCallbacks
     def test_add_relation_state(self):
-        """
-        Adding a relation will create a relation node and update the topology.
-        """
-        relation_state = yield self.relation_manager.add_relation_state(
-            "mysql")
+        """Adding a relation will create a relation node and update topology."""
+        mysql_ep = RelationEndpoint("mysql", "mysql", "db", "server")
+        yield self.add_service("mysql")
+        relation_state = (yield self.relation_manager.add_relation_state(
+            mysql_ep))[0]
         topology = yield self.get_topology()
         self.assertTrue(topology.has_relation(relation_state.internal_id))
         exists = yield self.client.exists(
@@ -147,12 +219,118 @@
         self.assertTrue(exists)
 
     @inlineCallbacks
+    def test_add_relation_state_to_missing_service(self):
+        """Test adding a relation to a nonexistent service"""
+        mysql_ep = RelationEndpoint("mysql", "mysql", "db", "server")
+        blog_ep = RelationEndpoint("wordpress", "mysql", "mysql", "client")
+        yield self.add_service("mysql")
+        # but didn't create the service for wordpress
+        yield self.assertFailure(
+            self.relation_manager.add_relation_state(
+                mysql_ep, blog_ep),
+            StateChanged)
+
+    @inlineCallbacks
+    def test_add_relation_state_bad_relation_role(self):
+        """Test adding a relation with a bad role when is one is well defined (client or server)"""
+        blog_ep = RelationEndpoint("wordpress", "mysql", "mysql", "client")
+        mysql_ep = RelationEndpoint("mysql", "mysql", "db", "server")
+        bad_mysql_ep = RelationEndpoint("mysql", "mysql", "db", "bad-server-role")
+        bad_blog_ep = RelationEndpoint("wordpress", "mysql", "mysql", "bad-client-role")
+        yield self.add_service("mysql")
+        yield self.add_service("wordpress")      
+        yield self.assertFailure(
+            self.relation_manager.add_relation_state(
+                bad_mysql_ep, blog_ep),
+            IncompatibleEndpoints)
+        yield self.assertFailure(
+            self.relation_manager.add_relation_state(
+                bad_blog_ep, mysql_ep),
+            IncompatibleEndpoints)
+        # TODO in future branch referenced in relation, also test
+        # bad_blog_ep *and* bad_mysql_ep
+        
+    @inlineCallbacks
+    def test_add_binary_relation_state_twice(self):
+        """Test adding the same relation twice"""
+        blog_ep = RelationEndpoint("wordpress", "mysql", "mysql", "client")
+        mysql_ep = RelationEndpoint("mysql", "mysql", "db", "server")
+        yield self.add_service("mysql")
+        yield self.add_service("wordpress")      
+        yield self.relation_manager.add_relation_state(mysql_ep, blog_ep)
+        yield self.assertFailure(
+            self.relation_manager.add_relation_state(blog_ep, mysql_ep),
+            RelationAlreadyExists)
+        yield self.assertFailure(
+            self.relation_manager.add_relation_state(mysql_ep, blog_ep),
+            RelationAlreadyExists)
+
+    @inlineCallbacks
+    def test_add_peer_relation_state_twice(self):
+        """Test adding the same relation twice"""
+        riak_ep = RelationEndpoint("riak", "riak", "ring", "peer")
+        yield self.add_service("riak")
+        yield self.relation_manager.add_relation_state(riak_ep)
+        yield self.assertFailure(
+            self.relation_manager.add_relation_state(riak_ep),
+            RelationAlreadyExists)
+
+    @inlineCallbacks
+    def test_add_relation_state_no_endpoints(self):
+        """Test adding a relation with no endpoints (no longer allowed)"""
+        yield self.assertFailure(
+            self.relation_manager.add_relation_state(),
+            TypeError)
+        
+    @inlineCallbacks
+    def test_add_relation_state_relation_type_unshared(self):
+        """Test adding a relation with endpoints not sharing a relation type"""
+        pg_ep = RelationEndpoint("pg", "postgres", "db", "server")
+        blog_ep = RelationEndpoint("wordpress", "mysql", "mysql", "client")
+        yield self.assertFailure(
+            self.relation_manager.add_relation_state(pg_ep, blog_ep),
+            IncompatibleEndpoints)
+
+    @inlineCallbacks
+    def test_add_relation_state_too_many_endpoints(self):
+        """Test adding a relation between too many endpoints (> 2)"""
+        mysql_ep = RelationEndpoint("mysql", "mysql", "db", "server")
+        blog_ep = RelationEndpoint("wordpress", "mysql", "mysql", "client")
+        yield self.add_service("mysql")
+        yield self.add_service("wordpress")
+        yield self.assertFailure(
+            self.relation_manager.add_relation_state(
+                mysql_ep, blog_ep, mysql_ep),
+            TypeError)
+
+    @inlineCallbacks
+    def test_add_relation_state_duplicate_peer_endpoints(self):
+        """Test adding a relation between duplicate peer endpoints"""
+        riak_ep = RelationEndpoint("riak", "riak", "ring", "peer")
+        yield self.add_service("riak")
+        yield self.assertFailure(
+            self.relation_manager.add_relation_state(riak_ep, riak_ep),
+            DuplicateEndpoints)
+
+    @inlineCallbacks
+    def test_add_relation_state_endpoints_duplicate_role(self):
+        """Test adding a relation with services overlapped by duplicate role"""
+        mysql_ep = RelationEndpoint("mysql", "mysql", "db", "server")
+        drizzle_ep = RelationEndpoint("drizzle", "mysql", "db", "server")
+        yield self.add_service("mysql")
+        yield self.add_service("drizzle")
+        yield self.assertFailure(
+            self.relation_manager.add_relation_state(mysql_ep, drizzle_ep),
+            IncompatibleEndpoints)
+
+    @inlineCallbacks
     def test_remove_relation_state(self):
-        """Removing a relation will remove it from the topology.
-        """
+        """Removing a relation will remove it from the topology."""
         # Simulate add and remove.
-        relation_state = yield self.relation_manager.add_relation_state(
-            "varnish")
+        varnish_ep = RelationEndpoint("varnish", "webcache", "cache", "server")
+        yield self.add_service("varnish")
+        relation_state = (yield self.relation_manager.add_relation_state(
+            varnish_ep))[0]
         topology = yield self.get_topology()
         self.assertTrue(topology.has_relation(relation_state.internal_id))
         yield self.relation_manager.remove_relation_state(relation_state)
@@ -164,8 +342,10 @@
     @inlineCallbacks
     def test_remove_relation_with_changing_state(self):
         # Simulate add and remove.
-        relation_state = yield self.relation_manager.add_relation_state(
-            "varnish")
+        varnish_ep = RelationEndpoint("varnish", "webcache", "cache", "server")
+        yield self.add_service("varnish")
+        relation_state = (yield self.relation_manager.add_relation_state(
+            varnish_ep))[0]
         topology = yield self.get_topology()
         self.assertTrue(topology.has_relation(relation_state.internal_id))
         yield self.relation_manager.remove_relation_state(relation_state)
@@ -218,8 +398,8 @@
     def setUp(self):
         yield super(RelationStateTest, self).setUp()
 
-        self.relation_state = yield self.relation_manager.add_relation_state(
-            "rel-type")
+        self.relation_state = (yield self.relation_manager.add_relation_state(
+            "rel-type"))[0]
         self.formula_state = yield self.formula_manager.add_formula_state(
             "namespace", self.formula)
         self.service_state1 = yield self.service_manager.add_service_state(
@@ -227,106 +407,6 @@
         self.service_state2 = yield self.service_manager.add_service_state(
             "wordpress-dev", self.formula_state)
 
-    @inlineCallbacks
-    def test_assign_service(self):
-        yield self.relation_state.assign_service(
-            self.service_state1,
-            "dev-instance",
-            "prod")
-
-        yield self.relation_state.assign_service(
-            self.service_state2,
-            "prod-instance",
-            "dev")
-
-        topology = yield self.get_topology()
-
-        self.assertEqual(
-            topology.get_relations_for_service(
-                self.service_state1.internal_id),
-            [(self.relation_state.internal_id,
-              "rel-type",
-              {"name": "dev-instance", "role": "prod"})])
-
-        self.assertEqual(
-            topology.get_relation_services(self.relation_state.internal_id),
-            {'service-0000000000': {'name': 'dev-instance', 'role': 'prod'},
-             'service-0000000001': {'name': 'prod-instance', 'role': 'dev'}})
-
-    @inlineCallbacks
-    def test_assign_service_already_assigned(self):
-        """
-        Attempting to add a service to a relation multiple times
-        will raise an error if its already there.
-        """
-        # Assign and verify
-        yield self.relation_state.assign_service(self.service_state1,
-                                                 "dev-instance",
-                                                 "prod")
-        topology = yield self.get_topology()
-        self.assertTrue(topology.relation_has_service(
-            self.relation_state.internal_id, self.service_state1.internal_id))
-
-        # If we attempt to add again, we'll get an error.
-        yield self.assertFailure(
-            self.relation_state.assign_service(self.service_state1,
-                                               "dev-instance",
-                                               "prod"),
-            ServiceRelationAlreadyAssigned)
-
-        # If we unassign and assign again, the service container state updates.
-        yield self.relation_state.unassign_service(self.service_state1)
-        yield self.relation_state.assign_service(self.service_state1,
-                                                 "dev-instance2",
-                                                 "prod")
-        content, stat = yield self.client.get(
-            "/relations/%s/%s" % (
-                self.relation_state.internal_id, "prod"))
-
-        node_data = yaml.load(content)
-        self.assertEqual(node_data["name"], "dev-instance2")
-
-        # Attempting to assign when the relation or service has gone away
-        # raises a state changed error.
-        yield self.relation_manager.remove_relation_state(self.relation_state)
-        topology = yield self.get_topology()
-        self.assertFalse(
-            topology.has_relation(self.relation_state.internal_id))
-
-        yield self.assertFailure(
-            self.relation_state.assign_service(self.service_state1,
-                                               "dev-instance2",
-                                               "prod2"),
-            StateChanged)
-
-    @inlineCallbacks
-    def test_unassign_service(self):
-        """A service can be disassociated from a relation.
-        """
-        yield self.relation_state.assign_service(self.service_state1,
-                                                 "dev-instance",
-                                                 "prod")
-        topology = yield self.get_topology()
-        self.assertTrue(topology.relation_has_service(
-            self.relation_state.internal_id,
-            self.service_state1.internal_id))
-
-        # Remove and verify
-        yield self.relation_state.unassign_service(self.service_state1)
-        topology = yield self.get_topology()
-        self.assertFalse(topology.relation_has_service(
-            self.relation_state.internal_id,
-            self.service_state1.internal_id))
-
-        # if we remove again, we'll succed as the final state is the same.
-        yield self.relation_state.unassign_service(self.service_state1)
-
-        # Removing again, without the relation state, errors on state change.
-        yield self.relation_manager.remove_relation_state(self.relation_state)
-        yield self.assertFailure(
-            self.relation_state.unassign_service(self.service_state1),
-            StateChanged)
-
 
 class ServiceRelationStateTest(RelationTestBase):
 
@@ -596,53 +676,16 @@
         self.assertEqual(path, container_path)
 
     @inlineCallbacks
-    def test_watch_start_existing_service(self):
-        """Invoking watcher.start returns a deferred that only fires
-        after watch on the container is in place. In the case of an
-        existing service, this is after a child watch is established.
-        """
-        wordpress_states = yield self.add_relation_service_unit(
-            "client-server", "wordpress", "", "client")
-
-        mysql_states = yield self.add_relation_service_unit(
-            "client-server", "mysql", "", "server",
-            relation_state = wordpress_states["relation"])
-
-        results = []
-        wait_callback = [Deferred() for i in range(5)]
-
-        def watch_related(old_units=None, new_units=None, modified=None):
-            results.append((old_units, new_units, modified))
-            wait_callback[len(results)-1].callback(True)
-
-        def invoked(*args, **kw):
-            # sleep to make sure that things haven't fired till the watch is
-            # in place.
-            time.sleep(0.1)
-            self.assertFalse(results)
-
-        mock_client = self.mocker.patch(self.client)
-        mock_client.get_children_and_watch("/relations/%s/server" % (
-            wordpress_states["relation"].internal_id))
-
-        self.mocker.call(invoked)
-        self.mocker.passthrough()
-        self.mocker.replay()
-
-        watcher = yield wordpress_states["unit_relation"].watch_related_units(
-            watch_related)
-        yield watcher.start()
-        yield wait_callback[0]
-
-    @inlineCallbacks
     def test_watch_start_new_service(self):
         """Invoking watcher.start returns a deferred that only fires
         after watch on the containr is in place. In the case of a new
         service this after an existance watch is established on the
         container.
         """
-        wordpress_states = yield self.add_relation_service_unit(
-            "client-server", "wordpress", "", "client")
+        wordpress_ep = RelationEndpoint(
+            "wordpress", "client-server", "", "client")
+        wordpress_states = yield self.add_relation_service_unit_from_endpoints(
+            wordpress_ep)
 
         results = []
 
@@ -673,8 +716,12 @@
         first within relation, and start to monitor the server service
         as it joins the relation, adds a unit, modifies, the unit.
         """
-        wordpress_states = yield self.add_relation_service_unit(
-            "client-server", "wordpress", "", "client")
+        wordpress_ep = RelationEndpoint(
+            "wordpress", "client-server", "", "client")
+        mysql_ep = RelationEndpoint(
+            "mysql", "client-server", "", "server")
+        wordpress_states = yield self.add_relation_service_unit_from_endpoints(
+            wordpress_ep, mysql_ep)
 
         # setup watch callbacks, and start watching.
         wait_callback = [Deferred() for i in range(5)]
@@ -700,9 +747,8 @@
         self.assertFalse(results)
 
         # add the server service and a unit of that
-        mysql_states = yield self.add_relation_service_unit(
-            "client-server", "mysql", "", "server",
-            relation_state=wordpress_states["relation"])
+        mysql_states = yield self.add_opposite_service_unit(
+            wordpress_states)
 
         topology = yield self.get_topology()
         # assert the relation is established correctly
@@ -734,17 +780,18 @@
     @inlineCallbacks
     def test_watch_client_server_with_existing_service(self):
         """We simulate a scenario where the client and server are both
-        in place before the client begins observing. The server subequently,
+        in place before the client begins observing. The server subsequently
         modifies, and remove its unit from the relation.
         """
         # add the client service and a unit of that
-        wordpress_states = yield self.add_relation_service_unit(
-            "client-server", "wordpress", "", "client")
-
-        # add the server service and a unit of that
-        mysql_states = yield self.add_relation_service_unit(
-            "client-server", "mysql", "", "server",
-            relation_state=wordpress_states["relation"])
+        wordpress_ep = RelationEndpoint(
+            "wordpress", "client-server", "", "client")
+        mysql_ep = RelationEndpoint(
+            "mysql", "client-server", "", "server")
+        wordpress_states = yield self.add_relation_service_unit_from_endpoints(
+            wordpress_ep, mysql_ep)
+        mysql_states = yield self.add_opposite_service_unit(
+            wordpress_states)
 
         # setup callbacks and start observing.
         wait_callback = [Deferred() for i in range(5)]
@@ -784,9 +831,13 @@
     def test_watch_server_client_with_new_service(self):
         """We simulate a server watching a client.
         """
+        wordpress_ep = RelationEndpoint(
+            "wordpress", "client-server", "", "client")
+        mysql_ep = RelationEndpoint(
+            "mysql", "client-server", "", "server")
         # add the server service and a unit of that
-        mysql_states = yield self.add_relation_service_unit(
-            "client-server", "mysql", "", "server")
+        mysql_states = yield self.add_relation_service_unit_from_endpoints(
+            mysql_ep, wordpress_ep)
 
         # setup callbacks and start observing.
         wait_callback = [Deferred() for i in range(5)]
@@ -804,9 +855,8 @@
         self.assertFalse(results)
 
         # add the client service and a unit of that
-        wordpress_states = yield self.add_relation_service_unit(
-            "client-server", "wordpress", "", "client",
-            relation_state=mysql_states["relation"])
+        wordpress_states = yield self.add_opposite_service_unit(
+            mysql_states)
 
         yield wait_callback[0]
         self.verify_unit_watch_result(
@@ -817,10 +867,9 @@
         """Peer relations always watch the peer container.
         """
         # add the peer relation and two unit of the service.
-        relation_state = yield self.relation_manager.add_relation_state("peer")
-        riak_states = yield self.add_relation_service_unit(
-            "peer", "riak", "riak-db", "peer",
-            relation_state=relation_state)
+        riak_ep = RelationEndpoint("riak", "peer", "riak-db", "peer")
+        riak_states = yield self.add_relation_service_unit_from_endpoints(
+            riak_ep)
 
         riak2_unit = yield riak_states["service"].add_unit_state()
         yield riak_states["service_relation"].add_unit_state(riak2_unit)
@@ -878,13 +927,15 @@
         created container is detected correctly and the observation
         works immediately.
         """
-        # Add the server service and a service unit.
-        mysql_states = yield self.add_relation_service_unit(
-            "client-server", "mysql", "", "server")
-
-        wordpress_states = yield self.add_relation_service_unit(
-            "client-server", "wordpress", "", "client",
-            relation_state=mysql_states["relation"])
+        # Add the relation, services, and related units.
+        wordpress_ep = RelationEndpoint(
+            "wordpress", "client-server", "", "client")
+        mysql_ep = RelationEndpoint(
+            "mysql", "client-server", "", "server")
+        wordpress_states = yield self.add_relation_service_unit_from_endpoints(
+            wordpress_ep, mysql_ep)
+        mysql_states = yield self.add_opposite_service_unit(
+            wordpress_states)
 
         container_path = "/relations/%s/client" % (
             mysql_states["relation"].internal_id)
@@ -929,12 +980,14 @@
         notification.
         """
         # Add the relation, services, and related units.
-        mysql_states = yield self.add_relation_service_unit(
-            "client-server", "mysql", "", "server")
-
-        wordpress_states = yield self.add_relation_service_unit(
-            "client-server", "wordpress", "", "client",
-            relation_state=mysql_states["relation"])
+        wordpress_ep = RelationEndpoint(
+            "wordpress", "client-server", "", "client")
+        mysql_ep = RelationEndpoint(
+            "mysql", "client-server", "", "server")
+        wordpress_states = yield self.add_relation_service_unit_from_endpoints(
+            wordpress_ep, mysql_ep)
+        mysql_states = yield self.add_opposite_service_unit(
+            wordpress_states)
 
         # setup callbacks and start observing.
         wait_callback = [Deferred() for i in range(5)]
@@ -981,12 +1034,14 @@
         the unit is being observed, the watcher will ignore the deletion.
         """
         # Add the relation, services, and related units.
-        mysql_states = yield self.add_relation_service_unit(
-            "client-server", "mysql", "", "server")
-
-        wordpress_states = yield self.add_relation_service_unit(
-            "client-server", "wordpress", "", "client",
-            relation_state=mysql_states["relation"])
+        wordpress_ep = RelationEndpoint(
+            "wordpress", "client-server", "", "client")
+        mysql_ep = RelationEndpoint(
+            "mysql", "client-server", "", "server")
+        wordpress_states = yield self.add_relation_service_unit_from_endpoints(
+            wordpress_ep, mysql_ep)
+        mysql_states = yield self.add_opposite_service_unit(
+            wordpress_states)
 
         # setup callbacks and start observing.
         wait_callback = [Deferred() for i in range(5)]
@@ -1039,12 +1094,14 @@
         without additional callbacks.
         """
         # Add the relation, services, and related units.
-        mysql_states = yield self.add_relation_service_unit(
-            "client-server", "mysql", "", "server")
-
-        wordpress_states = yield self.add_relation_service_unit(
-            "client-server", "wordpress", "", "client",
-            relation_state=mysql_states["relation"])
+        wordpress_ep = RelationEndpoint(
+            "wordpress", "client-server", "", "client")
+        mysql_ep = RelationEndpoint(
+            "mysql", "client-server", "", "server")
+        wordpress_states = yield self.add_relation_service_unit_from_endpoints(
+            wordpress_ep, mysql_ep)
+        mysql_states = yield self.add_opposite_service_unit(
+            wordpress_states)
 
         # setup callbacks and start observing.
         wait_callback = [Deferred() for i in range(5)]
@@ -1099,8 +1156,12 @@
         without additional callbacks.
         """
         # Add the relation, services, and related units.
-        mysql_states = yield self.add_relation_service_unit(
-            "client-server", "mysql", "", "server")
+        wordpress_ep = RelationEndpoint(
+            "wordpress", "client-server", "", "client")
+        mysql_ep = RelationEndpoint(
+            "mysql", "client-server", "", "server")
+        mysql_states = yield self.add_relation_service_unit_from_endpoints(
+            mysql_ep, wordpress_ep)
 
         # setup callbacks and start observing.
         wait_callback = [Deferred() for i in range(5)]
@@ -1119,9 +1180,8 @@
         watcher.stop()
 
         # Add the new service and a unit
-        wordpress_states = yield self.add_relation_service_unit(
-            "client-server", "wordpress", "", "client",
-            relation_state=mysql_states["relation"])
+        wordpress_states = yield self.add_opposite_service_unit(
+            mysql_states)
 
         # Add another unit
         wordpress2_states = yield self.add_related_service_unit(
@@ -1314,12 +1374,14 @@
         watcher_callback = ParallelWatcherCallback()
 
         # Add the relation, services, and related units.
-        mysql_states = yield self.add_relation_service_unit(
-            "client-server", "mysql", "", "server")
-
-        wordpress_states = yield self.add_relation_service_unit(
-            "client-server", "wordpress", "", "client",
-            relation_state=mysql_states["relation"])
+        wordpress_ep = RelationEndpoint(
+            "wordpress", "client-server", "", "client")
+        mysql_ep = RelationEndpoint(
+            "mysql", "client-server", "", "server")
+        wordpress_states = yield self.add_relation_service_unit_from_endpoints(
+            wordpress_ep, mysql_ep)
+        mysql_states = yield self.add_opposite_service_unit(
+            wordpress_states)
 
         # Start watching, and give a moment to establish
         watcher = yield mysql_states["unit_relation"].watch_related_units(

=== modified file 'ensemble/state/tests/test_service.py'
--- ensemble/state/tests/test_service.py	2010-12-10 04:59:54 +0000
+++ ensemble/state/tests/test_service.py	2010-12-10 23:06:27 +0000
@@ -27,8 +27,8 @@
         self.formula_state_manager = FormulaStateManager(self.client)
         self.service_state_manager = ServiceStateManager(self.client)
         self.machine_state_manager = MachineStateManager(self.client)
-        self.formula_state = yield self.formula_state_manager.add_formula_state(
-            "namespace", self.formula)
+        self.formula_state = yield self.formula_state_manager.\
+                             add_formula_state("namespace", self.formula)
         self.relation_state_manager = RelationStateManager(self.client)
 
     @inlineCallbacks
@@ -39,13 +39,17 @@
 
     @inlineCallbacks
     def add_relation(self, relation_type, *services):
-        relation_state = yield self.relation_state_manager.add_relation_state(
-            relation_type)
+        endpoints = []
         for service_meta in services:
             service_state, relation_name, relation_role = service_meta
-            yield relation_state.assign_service(
-                service_state, relation_name, relation_role)
-        returnValue(relation_state)
+            endpoints.append(RelationEndpoint(
+                service_state.service_name,
+                relation_type,
+                relation_name,
+                relation_role))
+        relation_state = yield self.relation_state_manager.add_relation_state(
+            *endpoints)
+        returnValue(relation_state[0])
 
     @inlineCallbacks
     def remove_service(self, internal_service_id):
@@ -435,55 +439,12 @@
         yield self.assertFailure(d, StateChanged)
 
     @inlineCallbacks
-    def test_watch_relations_with_changing_topology(self):
-        """
-        If the topology changes in an unrelated way, the
-        relations watch callback should not be called.
-        """
-        service_state = yield self.add_service("wordpress")
-        relation_state = yield self.add_relation("rel-type")
-
-        wait_callback = [Deferred() for i in range(5)]
-        calls = []
-
-        def watch_relations(old_relations, new_relations):
-            calls.append((old_relations, new_relations))
-            wait_callback[len(calls)-1].callback(True)
-
-        # Start watching
-        service_state.watch_relation_states(watch_relations)
-
-        # Callback is still untouched.
-        self.assertEquals(calls, [])
-
-        # assign service to relation, and wait for callback
-        yield relation_state.assign_service(service_state, "name", "role")
-        yield wait_callback[0]
-        yield self.sleep(0.1)
-        self.assertEqual(len(calls), 2)
-        self.assertEquals(calls[0], (None, []))
-        self.assertEquals(calls[1][0], [])
-        self.assertEquals(
-            calls[1][1][0].internal_service_id, service_state.internal_id)
-        self.assertEquals(
-            calls[1][1][0].internal_relation_id, relation_state.internal_id)
-
-        # Change the topology in an unrelated way, and give it some time
-        # in case a callback happens.
-        yield self.add_service("s-1")
-        yield self.add_relation("r-2-type")
-        yield self.sleep(0.2)
-
-        # Assert no changes observed.
-        self.assertEquals(len(calls), 2)
-
-    @inlineCallbacks
     def test_watch_relations_when_being_created(self):
         """
         We can watch relations before we have any.
         """
         service_state = yield self.add_service("wordpress")
-        relation_state = yield self.add_relation("rel-type")
+
 
         wait_callback = [Deferred() for i in range(5)]
         calls = []
@@ -499,10 +460,11 @@
         self.assertEquals(calls, [])
 
         # add a service relation and wait for the callback
-        yield relation_state.assign_service(service_state, "name", "role")
+        relation_state = yield self.add_relation(
+            "rel-type", [service_state, "name", "role"])
         yield wait_callback[1]
 
-        # verify the result
+        # # verify the result
         self.assertEquals(len(calls), 2)
         old_relations, new_relations = calls[1]
         self.assertFalse(old_relations)
@@ -511,7 +473,6 @@
         # add a new relation with the service assigned to it.
         relation_state2 = yield self.add_relation(
             "rel-type2", [service_state, "app", "server"])
-
         yield wait_callback[2]
 
         self.assertEquals(len(calls), 3)
@@ -555,7 +516,7 @@
 
         # Assign to another relation.
         yield self.add_relation("rel-type",
-                          (service_state, "name", "role"))
+                          (service_state, "name2", "role"))
 
         # Give a chance for something bad to happen.
         yield self.sleep(0.3)
@@ -573,61 +534,6 @@
         self.assertEquals(len(calls), 2)
 
     @inlineCallbacks
-    def test_watch_relation_assign_and_unassign_at_once(self):
-        """
-        If a service is quickly assigned and unassigned to a relation, the
-        change maybe be observed, as a single modificaiton in state.
-        """
-        wait_callback = [Deferred() for i in range(10)]
-
-        calls = []
-
-        def watch_units(old_units, new_units):
-            calls.append((old_units, new_units))
-            wait_callback[len(calls)-1].callback(True)
-
-        # Create the service
-        service_state = yield self.add_service("s-1")
-
-        # Create a relation with the service assigned.
-        relation_state1 = yield self.add_relation(
-            "rel-type", (service_state, "name", "role"))
-        relation_state2 = yield self.add_relation("rel-type")
-
-        # Start watching
-        yield service_state.watch_relation_states(watch_units)
-
-        yield wait_callback[0]
-        self.assertEquals(len(calls), 1)
-        old_relations, new_relations = calls[0]
-        self.assertEqual(old_relations, None)
-
-        # Modify the topology by hand so we get notification of multiple
-        # things simultaneously.
-        topology = yield self.get_topology()
-        topology.unassign_service_from_relation(
-            relation_state1.internal_id,
-            service_state.internal_id)
-
-        topology.assign_service_to_relation(
-            relation_state2.internal_id,
-            service_state.internal_id,
-            "role", "name")
-
-        yield self.set_topology(topology)
-
-        yield wait_callback[1]
-        self.assertEqual(len(calls), 2)
-        old_relations, new_relations = calls[1]
-
-        # Verify the changes seen.
-        self.assertEqual([r.internal_relation_id for r in old_relations],
-                         [relation_state1.internal_id])
-
-        self.assertEqual([r.internal_relation_id for r in new_relations],
-                         [relation_state2.internal_id])
-
-    @inlineCallbacks
     def add_service_from_formula(self, service_name):
         formula_dir = FormulaDirectory(os.path.join(
             test_repository_path, service_name))
@@ -644,17 +550,17 @@
         self.assertEqual(
             (yield self.service_state_manager.get_relation_endpoints(
             "wordpress")),
-            set([RelationEndpoint("wordpress", "http", "url", "provides"),
-                 RelationEndpoint("wordpress", "mysql", "db", "requires"),
+            set([RelationEndpoint("wordpress", "http", "url", "server"),
+                 RelationEndpoint("wordpress", "mysql", "db", "client"),
                  RelationEndpoint(
-                     "wordpress", "varnish", "cache", "requires")]))
+                     "wordpress", "varnish", "cache", "client")]))
         yield self.add_service_from_formula("riak")
         self.assertEqual(
             (yield self.service_state_manager.get_relation_endpoints(
             "riak")),
-            set([RelationEndpoint("riak", "http", "admin", "provides"),
-                 RelationEndpoint("riak", "http", "endpoint", "provides"),
-                 RelationEndpoint("riak", "riak", "ring", "peers")]))
+            set([RelationEndpoint("riak", "http", "admin", "server"),
+                 RelationEndpoint("riak", "http", "endpoint", "server"),
+                 RelationEndpoint("riak", "riak", "ring", "peer")]))
         
     @inlineCallbacks
     def test_get_relation_endpoints_service_name_relation_name(self):
@@ -663,21 +569,21 @@
         self.assertEqual(
             (yield self.service_state_manager.get_relation_endpoints(
             "wordpress:url")),
-            set([RelationEndpoint("wordpress", "http", "url", "provides")]))
+            set([RelationEndpoint("wordpress", "http", "url", "server")]))
         self.assertEqual(
             (yield self.service_state_manager.get_relation_endpoints(
             "wordpress:db")),
-            set([RelationEndpoint("wordpress", "mysql", "db", "requires")]))
+            set([RelationEndpoint("wordpress", "mysql", "db", "client")]))
         self.assertEqual(
             (yield self.service_state_manager.get_relation_endpoints(
             "wordpress:cache")),
              set([RelationEndpoint(
-                "wordpress", "varnish", "cache", "requires")]))
+                "wordpress", "varnish", "cache", "client")]))
         yield self.add_service_from_formula("riak")
         self.assertEqual(
             (yield self.service_state_manager.get_relation_endpoints(
             "riak:ring")),
-            set([RelationEndpoint("riak", "riak", "ring", "peers")]))
+            set([RelationEndpoint("riak", "riak", "ring", "peer")]))
 
     @inlineCallbacks
     def test_descriptor_for_services_without_formulas(self):
@@ -718,20 +624,20 @@
         self.assertEqual(
             (yield self.service_state_manager.join_descriptors(
                 "wordpress", "mysql")),
-            [(RelationEndpoint("wordpress", "mysql", "db", "requires"),
-              RelationEndpoint("mysql", "mysql", "server", "provides"))])
+            [(RelationEndpoint("wordpress", "mysql", "db", "client"),
+              RelationEndpoint("mysql", "mysql", "server", "server"))])
         # symmetric - note the pair has rotated
         self.assertEqual(
             (yield self.service_state_manager.join_descriptors(
                 "mysql", "wordpress")),
-            [(RelationEndpoint("mysql", "mysql", "server", "provides"),
-              RelationEndpoint("wordpress", "mysql", "db", "requires"))])
+            [(RelationEndpoint("mysql", "mysql", "server", "server"),
+              RelationEndpoint("wordpress", "mysql", "db", "client"))])
         yield self.add_service_from_formula("varnish")
         self.assertEqual(
             (yield self.service_state_manager.join_descriptors(
                 "wordpress", "varnish")),
-            [(RelationEndpoint("wordpress", "varnish", "cache", "requires"),
-              RelationEndpoint("varnish", "varnish", "webcache", "provides"))])
+            [(RelationEndpoint("wordpress", "varnish", "cache", "client"),
+              RelationEndpoint("varnish", "varnish", "webcache", "server"))])
 
     @inlineCallbacks
     def test_join_descriptors_service_name_relation_name(self):
@@ -741,30 +647,30 @@
         self.assertEqual(
             (yield self.service_state_manager.join_descriptors(
                 "wordpress:db", "mysql")),
-            [(RelationEndpoint("wordpress", "mysql", "db", "requires"),
-              RelationEndpoint("mysql", "mysql", "server", "provides"))])
+            [(RelationEndpoint("wordpress", "mysql", "db", "client"),
+              RelationEndpoint("mysql", "mysql", "server", "server"))])
         self.assertEqual(
             (yield self.service_state_manager.join_descriptors(
                 "mysql:server", "wordpress")),
-            [(RelationEndpoint("mysql", "mysql", "server", "provides"),
-              RelationEndpoint("wordpress", "mysql", "db", "requires"))])
+            [(RelationEndpoint("mysql", "mysql", "server", "server"),
+              RelationEndpoint("wordpress", "mysql", "db", "client"))])
         self.assertEqual(
             (yield self.service_state_manager.join_descriptors(
                 "mysql:server", "wordpress:db")),
-            [(RelationEndpoint("mysql", "mysql", "server", "provides"),
-              RelationEndpoint("wordpress", "mysql", "db", "requires"))])
+            [(RelationEndpoint("mysql", "mysql", "server", "server"),
+              RelationEndpoint("wordpress", "mysql", "db", "client"))])
 
         yield self.add_service_from_formula("varnish")
         self.assertEqual(
             (yield self.service_state_manager.join_descriptors(
                 "wordpress:cache", "varnish")),
-            [(RelationEndpoint("wordpress", "varnish", "cache", "requires"),
-              RelationEndpoint("varnish", "varnish", "webcache", "provides"))])
+            [(RelationEndpoint("wordpress", "varnish", "cache", "client"),
+              RelationEndpoint("varnish", "varnish", "webcache", "server"))])
         self.assertEqual(
             (yield self.service_state_manager.join_descriptors(
                 "wordpress:cache", "varnish:webcache")),
-            [(RelationEndpoint("wordpress", "varnish", "cache", "requires"),
-              RelationEndpoint("varnish", "varnish", "webcache", "provides"))])
+            [(RelationEndpoint("wordpress", "varnish", "cache", "client"),
+              RelationEndpoint("varnish", "varnish", "webcache", "server"))])
 
     @inlineCallbacks
     def test_join_peer_descriptors(self):
@@ -773,18 +679,18 @@
         self.assertEqual(
             (yield self.service_state_manager.join_descriptors(
                 "riak", "riak")),
-            [(RelationEndpoint("riak", "riak", "ring", "peers"),
-              RelationEndpoint("riak", "riak", "ring", "peers"))])
+            [(RelationEndpoint("riak", "riak", "ring", "peer"),
+              RelationEndpoint("riak", "riak", "ring", "peer"))])
         self.assertEqual(
             (yield self.service_state_manager.join_descriptors(
                 "riak:ring", "riak")),
-            [(RelationEndpoint("riak", "riak", "ring", "peers"),
-              RelationEndpoint("riak", "riak", "ring", "peers"))])
+            [(RelationEndpoint("riak", "riak", "ring", "peer"),
+              RelationEndpoint("riak", "riak", "ring", "peer"))])
         self.assertEqual(
             (yield self.service_state_manager.join_descriptors(
                 "riak:ring", "riak:ring")),
-            [(RelationEndpoint("riak", "riak", "ring", "peers"),
-              RelationEndpoint("riak", "riak", "ring", "peers"))])
+            [(RelationEndpoint("riak", "riak", "ring", "peer"),
+              RelationEndpoint("riak", "riak", "ring", "peer"))])
         self.assertEqual(
             (yield self.service_state_manager.join_descriptors(
                 "riak:no-ring", "riak:ring")),
@@ -828,3 +734,4 @@
         yield self.assertFailure(
             self.service_state_manager.join_descriptors("notyet", "nosuch"),
             ServiceStateNotFound)
+

=== modified file 'ensemble/state/tests/test_topology.py'
--- ensemble/state/tests/test_topology.py	2010-11-17 20:40:28 +0000
+++ ensemble/state/tests/test_topology.py	2010-12-10 23:06:27 +0000
@@ -1,9 +1,8 @@
 import yaml
 
 from ensemble.lib.testing import TestCase
-
-from ensemble.state.topology import (
-    InternalTopology, InternalTopologyError)
+from ensemble.state.endpoint import RelationEndpoint
+from ensemble.state.topology import InternalTopology, InternalTopologyError
 
 
 class InternalTopologyMapTest(TestCase):
@@ -790,8 +789,7 @@
         self.assertFalse(self.topology.get_relations_for_service("s-0"))
 
     def test_relation_has_service(self):
-        """We can test to see if a service is associated to a relation.
-        """
+        """We can test to see if a service is associated to a relation."""
         self.assertFalse(self.topology.relation_has_service("r-1", "s-0"))
         self.topology.add_relation("r-1", "type")
         self.topology.add_service("s-0", "wordpress")
@@ -799,8 +797,7 @@
         self.assertTrue(self.topology.relation_has_service("r-1", "s-0"))
 
     def test_get_relation_service(self):
-        """We can fetch the setting of a service within a relation.
-        """
+        """We can fetch the setting of a service within a relation."""
         # Invalid relations cause an exception.
         self.assertRaises(InternalTopologyError,
                           self.topology.get_relation_service,
@@ -903,3 +900,54 @@
         self.assertRaises(InternalTopologyError,
                           self.topology.remove_service,
                           "s-0")
+
+    def test_has_relation_between_dyadic_endpoints(self):
+        mysql_ep = RelationEndpoint("mysqldb", "mysql", "db", "server")
+        blog_ep = RelationEndpoint("wordpress", "mysql", "mysql", "client")
+        self.topology.add_service("s-0", "wordpress")
+        self.topology.add_service("s-1", "mysqldb")
+        self.topology.add_relation("r-0", "mysql")
+        self.topology.assign_service_to_relation(
+            "r-0", "s-0", "mysql", "client")
+        self.topology.assign_service_to_relation(
+            "r-0", "s-1", "db", "server")
+        self.assertTrue(self.topology.has_relation_between_endpoints([
+            mysql_ep, blog_ep]))
+        self.assertTrue(self.topology.has_relation_between_endpoints([
+            blog_ep, mysql_ep]))
+
+    def test_has_relation_between_dyadic_endpoints_missing_assignment(self):
+        mysql_ep = RelationEndpoint("mysqldb", "mysql", "db", "server")
+        blog_ep = RelationEndpoint("wordpress", "mysql", "mysql", "client")
+        self.topology.add_service("s-0", "wordpress")
+        self.topology.add_service("s-1", "mysqldb")
+        self.topology.add_relation("r-0", "mysql")
+        self.topology.assign_service_to_relation(
+            "r-0", "s-1", "db", "server")
+        self.assertFalse(self.topology.has_relation_between_endpoints([
+            mysql_ep, blog_ep]))
+        self.assertFalse(self.topology.has_relation_between_endpoints([
+            blog_ep, mysql_ep]))
+
+    def test_has_relation_between_dyadic_endpoints_wrong_relation_name(self):
+        mysql_ep = RelationEndpoint("mysqldb", "mysql", "wrong-name", "server")
+        blog_ep = RelationEndpoint("wordpress", "mysql", "mysql", "client")
+        self.topology.add_service("s-0", "wordpress")
+        self.topology.add_service("s-1", "mysqldb")
+        self.topology.add_relation("r-0", "mysql")
+        self.topology.assign_service_to_relation(
+            "r-0", "s-0", "mysql", "client")
+        self.topology.assign_service_to_relation(
+            "r-0", "s-1", "db", "server")
+        self.assertFalse(self.topology.has_relation_between_endpoints([
+            mysql_ep, blog_ep]))
+        self.assertFalse(self.topology.has_relation_between_endpoints([
+            blog_ep, mysql_ep]))
+
+    def test_has_relation_between_monadic_endpoints(self):
+        riak_ep = RelationEndpoint("riak", "riak", "riak", "peer")
+        self.topology.add_service("s-0", "riak")
+        self.topology.add_relation("r-0", "riak")
+        self.topology.assign_service_to_relation("r-0", "s-0", "riak", "peer")
+        self.assertTrue(self.topology.has_relation_between_endpoints(
+            [riak_ep]))

=== modified file 'ensemble/state/topology.py'
--- ensemble/state/topology.py	2010-11-17 20:40:28 +0000
+++ ensemble/state/topology.py	2010-12-10 23:06:27 +0000
@@ -308,8 +308,7 @@
         return relation_type
 
     def relation_has_service(self, relation_id, service_id):
-        """Return true if service_id is assigned to relation_id.
-        """
+        """Return if `service_id` is assigned to `relation_id`."""
         relations = self._state.get("relations", self._nil_dict)
         relation_type, services = relations.get(relation_id,
                                                 (None, self._nil_dict))
@@ -396,3 +395,29 @@
             raise InternalTopologyError(
                 "Service unit %s not found in service %s" % (
                     unit_id, service_id))
+
+    def has_relation_between_endpoints(self, endpoints):
+        """Examine topology to check if relation exists between `endpoints`.
+
+        The relation, with a ``relation type`` common to the
+        endpoints, must exist between all endpoints (presumably one
+        for peer, two for client-server). The topology for the
+        relations looks like the following in YAML::
+
+          relations:
+            relation-0000000000:
+            - mysql
+            - service-0000000000: {name: db, role: client}
+              service-0000000001: {name: server, role: server}
+        """
+        service_ids = dict((e, self.find_service_with_name(e.service_name))
+                           for e in endpoints)
+        relations = self._state.get("relations", self._nil_dict)
+        for _, services in relations.itervalues():
+            for endpoint in endpoints:
+                service = services.get(service_ids[endpoint])
+                if not service or service["name"] != endpoint.relation_name:
+                    break
+            else:
+                return True
+        return False