Merge lp:~hazmat/pyjuju/relation-state into lp:pyjuju

[Kapil Thangavelu]

relation state management, lacking watch api at the moment which is for a separate branch, also i'm using service id instead of service role name atm, but that's a fairly minor change. I mostly want to get some eyes on this code to make sure its going in a good direction.

[Gustavo Niemeyer (niemeyer)]

The overall feeling of the work so far is very good. Feels generally clean, and understandable.

Some specific points:

[1]

+ def add_relation(self, relation_id):
+ """Add a relation with given id between the given services.
(...)
+ def has_relation(self, relation_id):
+ """Return True if service_id was previously added to this topology.

These docstrings aren't reflecting the implementation.

[2]

+ def has_relation_service(self, relation_id, service_id):

relation_has_service might read better here.

[3]

+ def unassign_service_to_relation(self, relation_id, service_id):
+ """Disassociate service to relation.

Similarly, shouldn't that be "_from_relation" and "from relation"?

[4]

All of the date for the relation is missing from the topology (relation name, relation type, relation role). Without at least some of these in the topology, we can't do the mapping of a user provided name to the relation id without iterating through the nodes.

[5]

+ Type is used to distinguish the type of the relation, ie.
+ 'database.postgres', or 'http.cache.varnish'.

We haven't discussed/specified the namespacing of the types. We have to talk about "http.cache.varnish" vs. "varnish".

[6]

+ node_data = {"type": relation_type}
+ path = yield self._client.create("/relations/relation-",
+ yaml.safe_dump(node_data),

I'm wondering a bit about where to store the information for the relation. It feels a bit clumsy to have the relation type inside the relation node itself, as a first gut feeling. Imagine what it'd take to implement the matching of relation types to resolve dependencies. We'd basically have to look at the topology and then iterate over every node we have to discover which of them is responsible for which relation type/name/role.

[7]

+ internal_id = path.rsplit("/", 1)[1]

FWIW, that's os.path.basename(path)

[8]

+ # 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.

Yeah, it's a good question. I guess it's alright for now, since the model feels good. Adding brand new relations should be an operation generally done after a user action, anyway, so this is not a fast path we'll have to worry about hopefully.

[9]

+ @param name - The service's name for this relation.
+ @param role - The service's role within this relation.

I've mentioned this a few times, and we really have to define how to call these. I've been calling these "relation role" and "relation name". These terms are unique for the relation, and can be used by themselves. We cannot use "service role" and "service name" by itself, though. We'll always have to say "the service role in this relation" and "the service name in this relation" instead, due to the fact that a service has many relations and thus many names and roles.

We have to make a decision regarding this nomenclature over UDS and stop using mixed terms to avoid confus...

[Kapil Thangavelu (hazmat)]

On Tue, 26 Oct 2010 11:32:44 -0400, Gustavo Niemeyer
<email address hidden> wrote:

> The overall feeling of the work so far is very good. Feels generally
> clean, and understandable.

Cool, thanks for the review, replies inline.

>
> Some specific points:
>
>
> [1]
>
> + def add_relation(self, relation_id):
> + """Add a relation with given id between the given services.
> (...)
> + def has_relation(self, relation_id):
> + """Return True if service_id was previously added to this
> topology.
>
> These docstrings aren't reflecting the implementation.

Yeah.. they where prior to some discussions we had, when we discussed
separating out add relation from associating a service to it. I'll update
them.

>
> [2]
>
> + def has_relation_service(self, relation_id, service_id):
>
> relation_has_service might read better here.

agreed.

>
> [3]
>
> + def unassign_service_to_relation(self, relation_id, service_id):
> + """Disassociate service to relation.
>
> Similarly, shouldn't that be "_from_relation" and "from relation"?
>

sounds good as well.

> [4]
>
> All of the date for the relation is missing from the topology (relation
> name, relation type, relation role). Without at least some of these in
> the topology, we can't do the mapping of a user provided name to the
> relation id without iterating through the nodes.
>

The formula author aka 'user' name for a relation is specific to a
service, its not global to the relation. We can lookup this meta
information when we retrieve the services for a relation, we don't have to
iterate all relation nodes, just the ones associated to a service.
Additionally, we don't want to bloat the limited space we have in the
topology node for non coordination data, all of this additional metadata
is effectively static, formula derived metadata.

> [5]
>
> + Type is used to distinguish the type of the relation, ie.
> + 'database.postgres', or 'http.cache.varnish'.
>
> We haven't discussed/specified the namespacing of the types. We have to
> talk about "http.cache.varnish" vs. "varnish".
>

agreed, i can remove the docstring namespaced type reference, as its not
germane to the implementation at this point.

> [6]
>
>
> + node_data = {"type": relation_type}
> + path = yield self._client.create("/relations/relation-",
> + yaml.safe_dump(node_data),
>
> I'm wondering a bit about where to store the information for the
> relation. It feels a bit clumsy to have the relation type inside the
> relation node itself, as a first gut feeling. Imagine what it'd take to
> implement the matching of relation types to resolve dependencies. We'd
> basically have to look at the topology and then iterate over every node
> we have to discover which of them is responsible for which relation
> type/name/role.
>

We haven't really discussed dependency resolution using existing services.
I think we can construct a secondary index (potentially multi-node) of the
relation data to do resolutions, but unless we want the index to be
primary storage, i think there is still value to having this ...

[Gustavo Niemeyer (niemeyer)]

> > [4]
> >
> > All of the date for the relation is missing from the topology (relation
> > name, relation type, relation role). Without at least some of these in
> > the topology, we can't do the mapping of a user provided name to the
> > relation id without iterating through the nodes.
>
> The formula author aka 'user' name for a relation is specific to a
> service, its not global to the relation.

Sure, (relation name, relation type, relation role) are all equally associated
to the relation endpoint from the service.

> We can lookup this meta
> information when we retrieve the services for a relation, we don't have to
> iterate all relation nodes, just the ones associated to a service.
> Additionally, we don't want to bloat the limited space we have in the
> topology node for non coordination data, all of this additional metadata
> is effectively static, formula derived metadata.

The additional complexity that this will introduce feels a bit like an early
optimization, even more considering that we'd be storing that data per service
rather than per unit, but ok, let's see how that turns out then.

> > [6]
>
> We haven't really discussed dependency resolution using existing services.
> I think we can construct a secondary index (potentially multi-node) of the
> relation data to do resolutions, but unless we want the index to be
> primary storage, i think there is still value to having this 'identity'
> information stored on the node.

Cool, same case as above.

> noted, fwiw i copied it from the service state impl ;-)

Hah. Self-note taken! ;-)

> > [9]
(...)
> We should agree on terminology. In both cases we're referring to a
> service's role within a relationship. As long as we define and use the
> term consistently the ambiguity can be minimized. I'm okay with either.
> relation name and relation role are fine, but we need to stress in
> definition that while its target/context is a relation, its subject is a
> service.

Agreed. It's the "service relation role", but I propose that when shortened we call it "relation role", rather than "service role", to avoid the misconception that the service has a single role for all relations.

> > [11]
(...)
> i used a shortcut so i could it to review, as the branch was getting
> larged, as noted in the merge request. i'll fix that up.

Thank you!

[Kapil Thangavelu (hazmat)]

On Tue, 26 Oct 2010 17:57:45 -0400, Gustavo Niemeyer
<email address hidden> wrote:

>
>> > [4]
>> >
>> > All of the date for the relation is missing from the topology
>> (relation
>> > name, relation type, relation role). Without at least some of these
>> in
>> > the topology, we can't do the mapping of a user provided name to the
>> > relation id without iterating through the nodes.
>>
>> The formula author aka 'user' name for a relation is specific to a
>> service, its not global to the relation.
>
> Sure, (relation name, relation type, relation role) are all equally
> associated
> to the relation endpoint from the service.
>
>> We can lookup this meta
>> information when we retrieve the services for a relation, we don't have
>> to
>> iterate all relation nodes, just the ones associated to a service.
>> Additionally, we don't want to bloat the limited space we have in the
>> topology node for non coordination data, all of this additional metadata
>> is effectively static, formula derived metadata.
>
> The additional complexity that this will introduce feels a bit like an
> early
> optimization, even more considering that we'd be storing that data per
> service
> rather than per unit, but ok, let's see how that turns out then.

I'm going to have backtrack on this, as i'll need it to maintain the
current relation manager get relations for service api with a
relation-role container. I'll need to add the role to topology, so i might
as well add the relation type as well.

[lp:~hazmat/pyjuju/relation-state]

97. By Kapil Thangavelu on 2010-10-27

store relation info in topology

98. By Kapil Thangavelu on 2010-10-27

update relation model with topology stored rel info

99. By Kapil Thangavelu on 2010-10-27

doc string updates

100. By Kapil Thangavelu on 2010-10-27

separate out api for service retrieval from relation type retrieval

[Kapil Thangavelu (hazmat)]

all review comments, addressed. re the docstring in #9, its not clear that its different then what we agreed on re relation-role usage.

[lp:~hazmat/pyjuju/relation-state]

101. By Kapil Thangavelu on 2010-10-27

address some review comments

[Gustavo Niemeyer (niemeyer)]

Thanks for the changes, this is looking good. I'm raising some further points and issues for discussion below, but considering these the branch looks good for committing.

[9]

Ok, given that we went back and forth a few times on this, let's use the following terminology for clarity and convention alignment:

Long description:

- Service relation role
- Service relation name
- Service relation type

Short description:

- Relation role
- Relation name
- Relation type

Variable names:

- relation_role
- relation_name
- relation_type

[12]

+ internal_id = os.path.basename(path)#.rsplit("/", 1)[1]

Please use one or the other. No benefit in having both joined by a comment like this.

[13]

+ node_data = {"type": relation_type}

This node data doesn't seem to be used, and is redundant with the topology now.

[14]

+class RelationState(StateBase):
+
+ def __init__(self, client, internal_relation_id):
+ self._client = client

This should use the StateBase constructor for the client assignment. So far, this is the only thing done, but it may do additional tasks in the future and in such case this will break given that it's ignoring the base class.

[15]

+ node_data = yaml.safe_dump(dict(name = name, role = role))

As per PEP8, in these cases let's use spacing like dict(name=name, role=role).

[16]

+ service_id = service_state.internal_id

Would you mind to use the name "internal_service_id" for this variable and respective attribute in classes? I've been trying to be explicit about this to avoid surfacing this id outside of the state API (we have the service_state.name for that).

[17]

+ # previous persistent settings are not an error.

This raises some concerns. It's *adding* the unit to the relation, and even then any previous settings are blindly accepted. I'd guess that user hooks fired when adding the unit to the relation will not expect a previous dirty state. What would be the reasoning behind this behavior, and how do you imagine users will relate to it?

[18]

+ def __init__(self, client, service_id, unit_id, relation_id):
+ self._client = client
+ self._service_id = service_id
+ self._unit_id = unit_id
+ self._relation_id = relation_id

Would be nice to have these exported as properties and tested for equality in this round, just to ensure that the logic being put in place thus far is correct (parameter swapping is a common issue).

review: Approve

[Kapil Thangavelu (hazmat)]

On Thu, 28 Oct 2010 09:49:41 -0400, Gustavo Niemeyer
<email address hidden> wrote:

> Review: Approve
> Thanks for the changes, this is looking good. I'm raising some further
> points and issues for discussion below, but considering these the branch
> looks good for committing.
>
> [9]
>
> Ok, given that we went back and forth a few times on this, let's use the
> following terminology for clarity and convention alignment:
>
<...>
Sounds good.

[12-16, 18] In progress

> [17]
>
> + # previous persistent settings are not an error.
>
> This raises some concerns. It's *adding* the unit to the relation, and
> even then any previous settings are blindly accepted. I'd guess that
> user hooks fired when adding the unit to the relation will not expect a
> previous dirty state. What would be the reasoning behind this behavior,
> and how do you imagine users will relate to it?
>

This behavior is to allow units that have had transient disconnection or
errors, the ability to resume their work with the relation without loss of
configuration state. Take a mysql/wordpress relation as an example. If
mysql creates a database for wordpress and stores the db info its relation
unit settings, and subsequently the wordpress unit becomes unavailable
temporarily, when it reconnects we don't want it to create a new database
for wordpress, we want it to resume its role with its previous settings.
The main point for formula authors, is that unit relation settings are
persistent. Indeed as we now only utilize a relation-changed hook, there
is little distinction for formula authors from new/old, they must always
update their settings based on the current state of the world. If the
relation is explicitly broken the participating units agents will be
responsible for removing/cleaning up their settings.

Thanks for the review.

Kapil

[lp:~hazmat/pyjuju/relation-state]

102. By Kapil Thangavelu on 2010-10-28

address some review comments re style/naming

103. By Kapil Thangavelu on 2010-10-29

use internal prefix on ids, relation prefix on role/name

104. By Kapil Thangavelu on 2010-10-29

unit rel state property tests, and get/set settings data

105. By Kapil Thangavelu on 2010-10-29

align the assign service to relation signature between topology and relation state

=== modified file 'ensemble/errors.py'
--- ensemble/errors.py	2010-09-21 19:03:46 +0000
+++ ensemble/errors.py	2010-10-29 18:34:42 +0000
@@ -6,7 +6,7 @@
 
 class EnsembleError(Exception):
     """All errors in Ensemble are subclasses of this.
- 
+
     This error should not be raised by itself, though, since it means
     pretty much nothing.  It's useful mostly as something to catch instead.
     """

=== modified file 'ensemble/state/errors.py'
--- ensemble/state/errors.py	2010-09-16 20:13:37 +0000
+++ ensemble/state/errors.py	2010-10-29 18:34:42 +0000
@@ -94,6 +94,48 @@
                self.unit_name
 
 
+class ServiceRelationAlreadyAssigned(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 __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)
+
+
+class UnitRelationStateAlreadyAssigned(StateError):
+    """The unit already exists in the relation."""
+
+    def __init__(self, relation_id, relation_name, unit_name):
+        self.relation_id = relation_id
+        self.relation_name = relation_name
+        self.unit_name = unit_name
+
+    def __str__(self):
+        return "The relation %r already contains a unit for %r" % (
+            self.relation_name,
+            self.unit_name)
+
+
+class UnitRelationStateNotFound(StateError):
+    """The unit does not exist in the relation."""
+
+    def __init__(self, relation_id, relation_name, unit_name):
+        self.relation_id = relation_id
+        self.relation_name = relation_name
+        self.unit_name = unit_name
+
+    def __str__(self):
+        return "The relation %r has no unit state for %r" % (
+            self.relation_name,
+            self.unit_name)
+
+
 class EnvironmentStateNotFound(StateError):
     """Environment state was not found."""
 

=== added file 'ensemble/state/relation.py'
--- ensemble/state/relation.py	1970-01-01 00:00:00 +0000
+++ ensemble/state/relation.py	2010-10-29 18:34:42 +0000
@@ -0,0 +1,289 @@
+import yaml
+import os
+import zookeeper
+
+from twisted.internet.defer import inlineCallbacks, returnValue
+from txzookeeper.utils import retry_change
+
+from ensemble.state.base import StateBase
+from ensemble.state.errors import (
+    StateChanged, ServiceRelationAlreadyAssigned, UnitRelationStateNotFound,
+    UnitRelationStateAlreadyAssigned)
+
+
+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.
+
+        Type is used to distinguish the type of the relation, ie.
+        'postgres', or 'varnish'.
+        """
+        path = yield self._client.create(
+            "/relations/relation-", flags=zookeeper.SEQUENCE)
+
+        internal_id = os.path.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))
+
+    @inlineCallbacks
+    def remove_relation_state(self, relation_state):
+        """Remove the relation of the given id.
+
+        The relation is removed from the topology, however its container
+        node is not removed, as associated units will still be processing
+        its removal.
+        """
+
+        def remove_relation(topology):
+            if not topology.has_relation(relation_state.internal_id):
+                raise StateChanged()
+
+            topology.remove_relation(relation_state.internal_id)
+
+        yield self._retry_topology_change(remove_relation)
+
+    @inlineCallbacks
+    def get_relations_for_service(self, service_state):
+        """Get the relations associated to the service.
+        """
+        relations = []
+        internal_service_id = service_state.internal_id
+        topology = yield self._read_topology()
+        for info in topology.get_relations_for_service(internal_service_id):
+            relation_id, relation_type, service_info = info
+            relations.append(
+                ServiceRelationState(
+                    self._client,
+                    internal_service_id,
+                    relation_id,
+                    **service_info))
+        returnValue(relations)
+
+
+class RelationState(StateBase):
+
+    def __init__(self, client, internal_relation_id):
+        super(RelationState, self).__init__(client)
+        self._internal_id = internal_relation_id
+
+    @property
+    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."""
+
+    def __init__(self, client, service_id, relation_id, role, name):
+        self._client = client
+        self._service_id = service_id
+        self._relation_id = relation_id
+        self._role = role
+        self._name = name
+
+    @property
+    def internal_relation_id(self):
+        return self._relation_id
+
+    @property
+    def internal_service_id(self):
+        return self._service_id
+
+    @property
+    def relation_name(self):
+        """The service's name for the relation."""
+        return self._name
+
+    @property
+    def relation_role(self):
+        """The service's role within the relation."""
+        return self._role
+
+    @inlineCallbacks
+    def add_unit_state(self, unit_state):
+        """Add a unit to the service relation.
+
+        returns a unit relation state.
+        """
+        settings_path = "/relations/%s/settings/%s" % (
+            self._relation_id, unit_state.internal_id)
+
+        # We create settings node first, so that presence node events
+        # have a chance to inspect state.
+        try:
+            yield self._client.create(settings_path)
+        except zookeeper.NodeExistsException:
+            # previous persistent settings are not an error.
+            pass
+
+        alive_path = "/relations/%s/%s/%s" % (
+            self._relation_id, self._role, unit_state.internal_id)
+
+        try:
+            yield self._client.create(alive_path, flags=zookeeper.EPHEMERAL)
+        except zookeeper.NodeExistsException:
+            raise UnitRelationStateAlreadyAssigned(
+                self._relation_id, self._name, unit_state.unit_name)
+
+        returnValue(
+            UnitRelationState(
+                self._client,
+                self._service_id,
+                unit_state.internal_id,
+                self._relation_id))
+
+    @inlineCallbacks
+    def get_unit_state(self, unit_state):
+        """Given a service unit state, return its unit relation state."""
+        alive_path = "/relations/%s/%s/%s" % (
+            self._relation_id, self._role, unit_state.internal_id)
+        stat = yield self._client.exists(alive_path)
+
+        if not stat:
+            raise UnitRelationStateNotFound(
+                self._relation_id, self._name, unit_state.unit_name)
+
+        returnValue(
+            UnitRelationState(
+                self._client,
+                self._service_id,
+                unit_state.internal_id,
+                self._relation_id))
+
+
+class UnitRelationState(object):
+    """A service unit's relation state.
+    """
+
+    def __init__(self, client, service_id, unit_id, relation_id):
+        self._client = client
+        self._service_id = service_id
+        self._unit_id = unit_id
+        self._relation_id = relation_id
+
+    @property
+    def internal_service_id(self):
+        return self._service_id
+
+    @property
+    def internal_unit_id(self):
+        return self._unit_id
+
+    @property
+    def internal_relation_id(self):
+        return self._relation_id
+
+    @inlineCallbacks
+    def set_data(self, data):
+        """Set the relation local configuration data for a unit.
+        """
+        unit_settings_path = "/relations/%s/settings/%s" % (
+            self._relation_id, self._unit_id)
+        yield retry_change(
+            self._client, unit_settings_path, lambda content, stat: data)
+
+    @inlineCallbacks
+    def get_data(self):
+        """Get the relation local configuration data for a unit.
+        """
+        data, stat = yield self._client.get(
+            "/relations/%s/settings/%s" % (
+                self._relation_id, self._unit_id))
+        returnValue(data)

=== modified file 'ensemble/state/tests/common.py'
--- ensemble/state/tests/common.py	2010-09-20 19:53:57 +0000
+++ ensemble/state/tests/common.py	2010-10-29 18:34:42 +0000
@@ -25,6 +25,7 @@
         yield self.client.create("/machines")
         yield self.client.create("/services")
         yield self.client.create("/units")
+        yield self.client.create("/relations")
 
     @inlineCallbacks
     def tearDown(self):

=== modified file 'ensemble/state/tests/test_errors.py'
--- ensemble/state/tests/test_errors.py	2010-09-16 20:13:37 +0000
+++ ensemble/state/tests/test_errors.py	2010-10-29 18:34:42 +0000
@@ -4,7 +4,9 @@
     EnsembleError, StateError, StateChanged, FormulaStateNotFound,
     ServiceStateNotFound, ServiceUnitStateNotFound, MachineStateNotFound,
     ServiceUnitStateMachineAlreadyAssigned, ServiceStateNameInUse,
-    BadServiceStateName, EnvironmentStateNotFound)
+    BadServiceStateName, EnvironmentStateNotFound,
+    ServiceRelationAlreadyAssigned, UnitRelationStateNotFound,
+    UnitRelationStateAlreadyAssigned)
 
 
 class StateErrorsTest(TestCase):
@@ -73,3 +75,25 @@
         self.assertIsStateError(error)
         self.assertEquals(str(error),
                           "Environment state was not found")
+
+    def test_service_relation_already_assigned(self):
+        error = ServiceRelationAlreadyAssigned(
+            "wordpress", "mysql", "client")
+        self.assertIsStateError(error)
+        msg = ("Service 'wordpress' is already in relation with different "
+               "name 'mysql' or role 'client'.")
+        self.assertEquals(str(error), msg)
+
+    def test_unit_relation_state_not_found(self):
+        error = UnitRelationStateNotFound(
+            "rel-1", "rel-client", "mysql/0")
+        self.assertIsStateError(error)
+        msg = "The relation 'rel-client' has no unit state for 'mysql/0'"
+        self.assertEquals(str(error), msg)
+
+    def test_unit_relation_state_exists(self):
+        error = UnitRelationStateAlreadyAssigned(
+            "rel-id", "rel-client", "mysql/0")
+        self.assertIsStateError(error)
+        msg = "The relation 'rel-client' already contains a unit for 'mysql/0'"
+        self.assertEquals(str(error), msg)

=== added file 'ensemble/state/tests/test_relation.py'
--- ensemble/state/tests/test_relation.py	1970-01-01 00:00:00 +0000
+++ ensemble/state/tests/test_relation.py	2010-10-29 18:34:42 +0000
@@ -0,0 +1,471 @@
+import yaml
+
+from twisted.internet.defer import inlineCallbacks, returnValue, Deferred
+
+from txzookeeper import ZookeeperClient
+
+from ensemble.state.formula import FormulaStateManager
+from ensemble.state.errors import (
+    StateChanged, ServiceRelationAlreadyAssigned,
+    UnitRelationStateNotFound, UnitRelationStateAlreadyAssigned)
+from ensemble.state.relation import (
+    RelationStateManager, ServiceRelationState, UnitRelationState)
+from ensemble.state.service import ServiceStateManager
+from ensemble.state.tests.common import StateTestBase
+
+
+class RelationTestBase(StateTestBase):
+
+    @inlineCallbacks
+    def setUp(self):
+        yield super(RelationTestBase, self).setUp()
+        self.relation_manager = RelationStateManager(self.client)
+        self.formula_manager = FormulaStateManager(self.client)
+        self.service_manager = ServiceStateManager(self.client)
+        self.formula_state = None
+
+    @inlineCallbacks
+    def add_service(self, name):
+        if not self.formula_state:
+            self.formula_state = yield self.formula_manager.add_formula_state(
+                "namespace", self.formula)
+        service_state = yield self.service_manager.add_service_state(
+            name, self.formula_state)
+        returnValue(service_state)
+
+    @inlineCallbacks
+    def add_relation(self, relation_type, *services):
+        relation_state = yield self.relation_manager.add_relation_state(
+            relation_type)
+        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)
+
+
+class RelationStateManagerTest(RelationTestBase):
+
+    @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")
+        topology = yield self.get_topology()
+        self.assertTrue(topology.has_relation(relation_state.internal_id))
+        exists = yield self.client.exists(
+            "/relations/%s" % relation_state.internal_id)
+        self.assertTrue(exists)
+        exists = yield self.client.get(
+            "/relations/%s/settings" % relation_state.internal_id)
+        self.assertTrue(exists)
+
+    @inlineCallbacks
+    def test_remove_relation_state(self):
+        """Removing a relation will remove it from the topology.
+        """
+        # Simulate add and remove.
+        relation_state = yield self.relation_manager.add_relation_state(
+            "varnish")
+        topology = yield self.get_topology()
+        self.assertTrue(topology.has_relation(relation_state.internal_id))
+        yield self.relation_manager.remove_relation_state(relation_state)
+
+        # Verify removal.
+        topology = yield self.get_topology()
+        self.assertFalse(topology.has_relation(relation_state.internal_id))
+
+    @inlineCallbacks
+    def test_remove_relation_with_changing_state(self):
+        # Simulate add and remove.
+        relation_state = yield self.relation_manager.add_relation_state(
+            "varnish")
+        topology = yield self.get_topology()
+        self.assertTrue(topology.has_relation(relation_state.internal_id))
+        yield self.relation_manager.remove_relation_state(relation_state)
+
+        topology = yield self.get_topology()
+        self.assertFalse(topology.has_relation(relation_state.internal_id))
+
+        # try to remove again, should get state change error.
+        yield self.assertFailure(
+            self.relation_manager.remove_relation_state(relation_state),
+            StateChanged)
+
+    @inlineCallbacks
+    def test_get_relations_for_service(self):
+        # Create some services and relations
+        service1 = yield self.add_service("database")
+        service2 = yield self.add_service("application")
+        service3 = yield self.add_service("cache")
+
+        relation1 = yield self.add_relation(
+            "database",
+            (service1, "client", "server"), (service2, "db", "client"))
+
+        relation2 = yield self.add_relation(
+            "cache",
+            (service3, "app", "server"), (service2, "cache", "client"))
+
+        relations = yield self.relation_manager.get_relations_for_service(
+            service2)
+        rel_ids = [r.internal_relation_id for r in relations]
+        self.assertEqual(sorted(rel_ids),
+                         [relation1.internal_id, relation2.internal_id])
+
+        relations = yield self.relation_manager.get_relations_for_service(
+            service1)
+        rel_ids = [r.internal_relation_id for r in relations]
+        self.assertEqual(sorted(rel_ids), [relation1.internal_id])
+
+    def test_get_relations_for_invalid_service(self):
+        pass
+
+    @inlineCallbacks
+    def test_get_relations_for_service_with_none(self):
+        service1 = yield self.add_service("database")
+        yield self.relation_manager.get_relations_for_service(service1)
+
+
+class RelationStateTest(RelationTestBase):
+
+    @inlineCallbacks
+    def setUp(self):
+        yield super(RelationStateTest, self).setUp()
+
+        self.relation_state = yield self.relation_manager.add_relation_state(
+            "rel-type")
+        self.formula_state = yield self.formula_manager.add_formula_state(
+            "namespace", self.formula)
+        self.service_state1 = yield self.service_manager.add_service_state(
+            "wordpress-prod", self.formula_state)
+        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):
+
+    @inlineCallbacks
+    def setUp(self):
+        yield super(ServiceRelationStateTest, self).setUp()
+
+        self.service_state1 = yield self.add_service("wordpress-prod")
+        self.service_state2 = yield self.add_service("wordpress-dev")
+        self.relation_state = yield self.add_relation(
+            "riak",
+            (self.service_state1, "dev-connect", "prod"),
+            (self.service_state2, "prod-connect", "dev"))
+
+        relations = yield self.relation_manager.get_relations_for_service(
+            self.service_state1)
+        self.service1_relation = relations.pop()
+
+    def get_presence_path(self, relation_state, relation_role, unit_state):
+        presence_path = "/relations/%s/%s/%s" % (
+            relation_state.internal_id,
+            relation_role,
+            unit_state.internal_id)
+        return presence_path
+
+    def test_property_internal_service_id(self):
+        self.assertEqual(self.service1_relation.internal_service_id,
+                         self.service_state1.internal_id)
+
+    def test_property_internal_relation_id(self):
+        self.assertEqual(self.service1_relation.internal_relation_id,
+                         self.relation_state.internal_id)
+
+    def test_property_relation_role(self):
+        self.assertEqual(self.service1_relation.relation_role, "prod")
+
+    def test_property_relation_name(self):
+        """
+        The service's name from the relation is accessible from the service
+        relation state.
+        """
+        self.assertEqual(self.service1_relation.relation_name, "dev-connect")
+
+    @inlineCallbacks
+    def test_add_unit_state(self):
+        """The service state is used to create units in the relation."""
+        unit_state = yield self.service_state1.add_unit_state()
+
+        # set some watches to verify the order things are created.
+        state_created = Deferred()
+        creation_order = []
+        presence_path = self.get_presence_path(
+            self.relation_state, "prod", unit_state)
+
+        def append_event(name, event):
+            creation_order.append((name, event))
+            if len(creation_order) == 2:
+                state_created.callback(creation_order)
+
+        self.client.exists_and_watch(presence_path)[1].addCallback(
+            lambda result: append_event("presence", result))
+
+        settings_path = "/relations/%s/settings/%s" % (
+            self.relation_state.internal_id,
+            unit_state.internal_id)
+
+        self.client.exists_and_watch(settings_path)[1].addCallback(
+            lambda result: append_event("settings", result))
+
+        # add the unit agent
+        yield self.service1_relation.add_unit_state(unit_state)
+
+        # wait for the watches
+        yield state_created
+
+        # Verify order of creation, settings first, then presence.
+        self.assertEqual(creation_order[0][0], "settings")
+        self.assertEqual(creation_order[0][1].type_name, "created")
+        self.assertEqual(creation_order[1][0], "presence")
+        self.assertEqual(creation_order[1][1].type_name, "created")
+
+    @inlineCallbacks
+    def test_presence_node_is_ephemeral(self):
+        """
+        A unit relation state is composed of two nodes, an ephemeral
+        presence node, and a persistent settings node. Verify that
+        the presence node is ephemeral.
+        """
+        unit_state = yield self.service_state1.add_unit_state()
+
+        # manually construct a unit relation state using a separate
+        # connection.
+        client2 = ZookeeperClient(self.client.servers)
+        yield client2.connect()
+        service_relation = ServiceRelationState(
+            client2,
+            self.service_state1.internal_id,
+            self.relation_state.internal_id,
+            "prod",
+            "name")
+        yield service_relation.add_unit_state(unit_state)
+
+        presence_path = self.get_presence_path(
+            self.relation_state, "prod", unit_state)
+
+        exists_d, watch_d = self.client.exists_and_watch(presence_path)
+        exists = yield exists_d
+
+        self.assertTrue(exists)
+
+        yield client2.close()
+        event = yield watch_d
+        self.assertEquals(event.type_name, "deleted")
+
+    @inlineCallbacks
+    def test_add_unit_state_with_preexisting_presence(self):
+        """
+        If a unit relation presence node exists, attempting
+        to add it again raises an error.
+        """
+        unit_state = yield self.service_state1.add_unit_state()
+        presence_path = self.get_presence_path(
+            self.relation_state, "prod", unit_state)
+        yield self.client.create(presence_path)
+
+        yield self.assertFailure(
+            self.service1_relation.add_unit_state(unit_state),
+            UnitRelationStateAlreadyAssigned)
+
+    @inlineCallbacks
+    def test_add_unit_state_with_preexisting_settings(self):
+        """A unit coming backup retians its existing settings."""
+        unit_state = yield self.service_state1.add_unit_state()
+        settings_path = "/relations/%s/settings/%s" % (
+            self.relation_state.internal_id,
+            unit_state.internal_id)
+
+        data = yaml.dump({"hello": "world"})
+        yield self.client.create(settings_path, data)
+        yield self.service1_relation.add_unit_state(unit_state)
+
+        node_data, stat = yield self.client.get(settings_path)
+        self.assertEqual(node_data, data)
+
+    @inlineCallbacks
+    def test_get_unit_state(self):
+        unit_state = yield self.service_state1.add_unit_state()
+        unit_relation_state = yield self.service1_relation.add_unit_state(
+            unit_state)
+        self.assertTrue(isinstance(unit_relation_state, UnitRelationState))
+
+    @inlineCallbacks
+    def test_get_unit_state_nonexistant(self):
+        unit_state = yield self.service_state1.add_unit_state()
+        yield self.assertFailure(
+            self.service1_relation.get_unit_state(unit_state),
+            UnitRelationStateNotFound)
+
+
+class UnitRelationStateTest(RelationTestBase):
+
+    @inlineCallbacks
+    def add_relation_service_unit(self, relation_type, service_name):
+        service_state = yield self.add_service(service_name)
+        relation_state = yield self.add_relation(
+            relation_type,
+            (service_state, "name", "role"))
+        unit_state = yield service_state.add_unit_state()
+        relations = yield self.relation_manager.get_relations_for_service(
+            service_state)
+
+        service_relation = relations.pop()
+        relation_unit_state = yield service_relation.add_unit_state(
+            unit_state)
+
+        returnValue({
+            "service": service_state,
+            "unit": unit_state,
+            "relation": relation_state,
+            "service_relation": service_relation,
+            "unit_relation": relation_unit_state})
+
+    def get_unit_settings_path(self, state_dict):
+        unit_relation_path = "/relations/%s/settings/%s" % (
+            state_dict["relation"].internal_id,
+            state_dict["unit"].internal_id)
+        return unit_relation_path
+
+    @inlineCallbacks
+    def test_properties(self):
+        states = yield self.add_relation_service_unit("webcache", "varnish")
+        unit_relation = states["unit_relation"]
+
+        self.assertEqual(
+            unit_relation.internal_service_id,
+            states["service"].internal_id)
+        self.assertEqual(
+            unit_relation.internal_relation_id,
+            states["relation"].internal_id)
+        self.assertEqual(
+            unit_relation.internal_unit_id,
+            states["unit"].internal_id)
+
+    @inlineCallbacks
+    def test_get_data(self):
+        states = yield self.add_relation_service_unit("webcache", "varnish")
+        unit_relation = states["unit_relation"]
+
+        data = yield unit_relation.get_data()
+        self.assertEqual(data, "")
+
+        unit_relation_path = self.get_unit_settings_path(states)
+        self.client.set(unit_relation_path, "hello world")
+
+        data = yield unit_relation.get_data()
+        self.assertEqual(data, "hello world")
+
+    @inlineCallbacks
+    def test_set_data(self):
+        states = yield self.add_relation_service_unit("webcache", "varnish")
+        unit_relation = states["unit_relation"]
+        unit_relation_path = self.get_unit_settings_path(states)
+        yield unit_relation.set_data("hello world")
+        data, stat = yield self.client.get(unit_relation_path)
+        self.assertEqual(data, "hello world")

=== modified file 'ensemble/state/tests/test_topology.py'
--- ensemble/state/tests/test_topology.py	2010-09-14 13:56:56 +0000
+++ ensemble/state/tests/test_topology.py	2010-10-29 18:34:42 +0000
@@ -57,6 +57,7 @@
         self.topology.add_machine("m-1")
 
         # Add a non-assigned unit, to test that the logic of
+
         # checking for assigned units validates this.
         self.topology.add_service("s-0", "wordpress")
         self.topology.add_service_unit("s-0", "u-0")
@@ -659,3 +660,170 @@
         self.topology.add_machine("m-0")
         self.topology.reset()
         self.assertEquals(self.topology.dump(), empty_data)
+
+    def test_has_relation(self):
+        """Testing if a relation exists should be possible.
+        """
+        self.topology.add_service("s-0", "wordpress")
+        self.assertFalse(self.topology.has_relation("r-1"))
+        self.topology.add_relation("r-1", "type")
+        self.assertTrue(self.topology.has_relation("r-1"))
+
+    def test_add_relation(self):
+        """Add a relation between the given service ids.
+        """
+        self.assertFalse(self.topology.has_relation("r-1"))
+
+        # Verify add relation works correctly.
+        self.topology.add_relation("r-1", "type")
+        self.assertTrue(self.topology.has_relation("r-1"))
+
+        # Attempting to add again raises an exception
+        self.assertRaises(
+            InternalTopologyError,
+            self.topology.add_relation, "r-1", "type")
+
+    def test_assign_service_to_relation(self):
+        """A service can be associated to a relation.
+        """
+        # Both service and relation must be valid.
+        self.assertRaises(InternalTopologyError,
+                          self.topology.assign_service_to_relation,
+                          "r-1",
+                          "s-0",
+                          "name",
+                          "role")
+        self.topology.add_relation("r-1", "type")
+        self.assertRaises(InternalTopologyError,
+                          self.topology.assign_service_to_relation,
+                          "r-1",
+                          "s-0",
+                          "name",
+                          "role")
+        self.topology.add_service("s-0", "wordpress")
+
+        # The relation can be assigned.
+        self.assertFalse(self.topology.relation_has_service("r-1", "s-0"))
+        self.topology.assign_service_to_relation("r-1", "s-0", "name", "role")
+        self.assertEqual(self.topology.get_relations_for_service("s-0"),
+                         [("r-1", "type", {"name": "name", "role": "role"})])
+
+        # Adding it again raises an error, even with a different name/role
+        self.assertRaises(InternalTopologyError,
+                          self.topology.assign_service_to_relation,
+                          "r-1",
+                          "s-0",
+                          "name2",
+                          "role2")
+
+        # Another service can't provide the same role within a relation.
+        self.topology.add_service("s-1", "database")
+        self.assertRaises(InternalTopologyError,
+                          self.topology.assign_service_to_relation,
+                          "r-1",
+                          "s-1",
+                          "name",
+                          "role")
+
+    def test_unassign_service_from_relation(self):
+        """A service can be disassociated from a relation.
+        """
+        # Both service and relation must be valid.
+        self.assertRaises(InternalTopologyError,
+                          self.topology.unassign_service_from_relation,
+                          "r-1",
+                          "s-0")
+        self.topology.add_relation("r-1", "type")
+        self.assertRaises(InternalTopologyError,
+                          self.topology.unassign_service_from_relation,
+                          "r-1",
+                          "s-0")
+        self.topology.add_service("s-0", "wordpress")
+
+        # If the service is not assigned to the relation, raises an error.
+        self.assertRaises(InternalTopologyError,
+                          self.topology.unassign_service_from_relation,
+                          "r-1",
+                          "s-0")
+
+        self.topology.assign_service_to_relation("r-1", "s-0", "name", "role")
+        self.assertEqual(self.topology.get_relations_for_service("s-0"),
+                         [("r-1", "type", {"name": "name", "role": "role"})])
+
+        self.topology.unassign_service_from_relation("r-1", "s-0")
+        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.
+        """
+        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")
+        self.topology.assign_service_to_relation("r-1", "s-0", "name", "role")
+        self.assertTrue(self.topology.relation_has_service("r-1", "s-0"))
+
+    def test_get_services_for_relations(self):
+        """The services for a given relation can be retrieved."""
+        self.topology.add_relation("r-1", "type")
+        self.topology.add_service("s-0", "wordpress")
+        self.topology.add_service("s-1", "database")
+        self.topology.assign_service_to_relation("r-1", "s-0", "name", "role")
+        self.topology.assign_service_to_relation("r-1", "s-1", "name", "role2")
+        self.assertEqual(
+            self.topology.get_relation_services("r-1"),
+            {'s-1': {'role': 'role2', 'name': 'name'},
+             's-0': {'role': 'role', 'name': 'name'}})
+
+    def test_get_relations_for_service(self):
+        """The relations for a given service can be retrieved.
+        """
+        # Getting relations for unknown service raises a topologyerror.
+        self.assertRaises(InternalTopologyError,
+                          self.topology.get_relations_for_service,
+                          "s-0")
+
+        # A new service has no relations.
+        self.topology.add_service("s-0", "wordpress")
+        self.assertFalse(self.topology.get_relations_for_service("s-0"))
+
+        # Add a relation and fetch it.
+        self.topology.add_relation("r-1", "type")
+        self.topology.assign_service_to_relation("r-1", "s-0", "name", "role")
+        self.topology.add_relation("r-2", "type")
+        self.topology.assign_service_to_relation("r-2", "s-0", "name", "role")
+
+        self.assertEqual(
+            sorted(self.topology.get_relations_for_service("s-0")),
+            [("r-1", "type", {"name": "name", "role": "role"}),
+             ("r-2", "type", {"name": "name", "role": "role"})])
+
+        self.topology.unassign_service_from_relation("r-2", "s-0")
+
+    def test_remove_relation(self):
+        """A relation can be removed.
+        """
+        # Attempting to remove unknown relation raises a topologyerror
+        self.assertRaises(InternalTopologyError,
+                          self.topology.remove_relation,
+                          "r-1")
+
+        # Adding a relation with associated service, and remove it.
+        self.topology.add_service("s-0", "wordpress")
+        self.topology.add_relation("r-1", "type")
+        self.topology.assign_service_to_relation("r-1", "s-0", "name", "role")
+        self.assertTrue(self.topology.has_relation("r-1"))
+        self.topology.remove_relation("r-1")
+        self.assertFalse(self.topology.has_relation("r-1"))
+
+    def test_remove_service_with_relations(self):
+        """
+        Attempting to remove a service that's assigned to relations
+        raises an InternalTopologyError.
+        """
+        self.topology.add_service("s-0", "wordpress")
+        self.topology.add_relation("r-1", "type")
+        self.topology.assign_service_to_relation("r-1", "s-0", "name", "role")
+
+        self.assertRaises(InternalTopologyError,
+                          self.topology.remove_service,
+                          "s-0")

=== modified file 'ensemble/state/topology.py'
--- ensemble/state/topology.py	2010-10-15 20:46:21 +0000
+++ ensemble/state/topology.py	2010-10-29 18:34:42 +0000
@@ -134,6 +134,11 @@
         """Remove service_id from this topology.
         """
         self._assert_service(service_id)
+        relations = self.get_relations_for_service(service_id)
+        if relations:
+            raise InternalTopologyError(
+                "Service %r is associated to relations %s" % (
+                    service_id, relations))
         del self._state["services"][service_id]
 
     def add_service_unit(self, service_id, unit_id):
@@ -259,6 +264,96 @@
                     units.append(unit_id)
         return units
 
+    def add_relation(self, relation_id, relation_type):
+        """Add a relation with given id and of the given type.
+        """
+        relations = self._state.setdefault("relations", {})
+        if relation_id in relations:
+            raise InternalTopologyError(
+                "Relation id %r already in use" % relation_id)
+        relations[relation_id] = (relation_type, {})
+
+    def has_relation(self, relation_id):
+        """Return True if relation with the given id exists in the topology.
+        """
+        return relation_id in self._state.get("relations", self._nil_dict)
+
+    def get_relation_services(self, relation_id):
+        """Get all the services associated to the relation.
+        """
+        self._assert_relation(relation_id)
+        relation_type, services = self._state["relations"][relation_id]
+        return services
+
+    def get_relation_type(self, relation_id):
+        """Get the type of a relation."""
+        self._assert_relation(relation_id)
+        relation_type, services = self._states["relations"][relation_id]
+        return relation_type
+
+    def relation_has_service(self, relation_id, service_id):
+        """Return true 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))
+        return service_id in services
+
+    def remove_relation(self, relation_id):
+        """It should be possible to remove a relation.
+        """
+        self._assert_relation(relation_id)
+        del self._state["relations"][relation_id]
+
+    def assign_service_to_relation(self, relation_id, service_id, name, role):
+        """Associate a service to a relation.
+
+        @param role: The relation role of the service.
+        @param name: The relation name from the service.
+        """
+        self._assert_service(service_id)
+        self._assert_relation(relation_id)
+        relation_type, services = self._state["relations"][relation_id]
+        for sid in services:
+            if sid == service_id:
+                raise InternalTopologyError(
+                    "Service %r is already assigned to relation %r" % (
+                        service_id, relation_id))
+            service_info = services[sid]
+            if service_info["role"] == role:
+                raise InternalTopologyError(
+                    ("Another service %r is already providing %r "
+                     "role in relation") % (sid, service_info["role"]))
+
+        services[service_id] = {"role": role, "name": name}
+
+    def unassign_service_from_relation(self, relation_id, service_id):
+        """Disassociate service to relation.
+        """
+        self._assert_service(service_id)
+        self._assert_relation(relation_id)
+        relation_type, services = self._state["relations"][relation_id]
+        if not service_id in services:
+            raise InternalTopologyError(
+                "Service %r is not assigned to relation %r" % (
+                    service_id, relation_id))
+        del services[service_id]
+
+    def get_relations_for_service(self, service_id):
+        """Given a service id we should be able to retrieve its relations."""
+        self._assert_service(service_id)
+        relations = []
+        for relation_id, (relation_type, services) in self._state.get(
+            "relations", self._nil_dict).items():
+            if service_id in services:
+                relations.append((
+                    relation_id, relation_type, services[service_id]))
+        return relations
+
+    def _assert_relation(self, relation_id):
+        if relation_id not in self._state.get("relations", self._nil_dict):
+            raise InternalTopologyError("Relation not found: %s" % relation_id)
+
     def _assert_machine(self, machine_id):
         if machine_id not in self._state.get("machines", self._nil_dict):
             raise InternalTopologyError("Machine not found: %s" % machine_id)