Merge lp:~hazmat/juju/status-w-peers into lp:juju

Proposed by Kapil Thangavelu on 2011-07-01
Status: Merged
Approved by: Gustavo Niemeyer on 2011-07-06
Approved revision: 264
Merged at revision: 265
Proposed branch: lp:~hazmat/juju/status-w-peers
Merge into: lp:juju
Diff against target: 147 lines (+61/-10) 2 files modified
To merge this branch: bzr merge lp:~hazmat/juju/status-w-peers
Reviewer Review Type Date Requested Status
Gustavo Niemeyer 2011-07-01 Approve on 2011-07-06
Review via email: mp+66618@code.launchpad.net

Description of the Change

Fixes ensemble status to work with peer relations.

To post a comment you must log in.
Benjamin Saller (bcsaller) wrote :

[1]

        for relation in relations:
            rel_services = yield relation.get_service_states()
            # filter out 'self' if its nots a peer relation
            if len(rel_services) > 1:
                rel_services = [
                    rsn for rsn in rel_services if rsn.service_name !=
                    service.service_name]
            if len(rel_services) > 1:
                raise ValueError("Unexpected relationship with more "
                                 "than 2 endpoints")

This does fix it, I'd like to expand the comment such that its a bit more obvious whats happening there. To be clear this is my fault not yours, but a series of len(x) > 1 checks don't really related to how that handles peer relations or not. Something like:

# Singular rel_services imply a peer relation. Two rel_services imply a provides/requires relationship. In the later case we omit the local size of the relation in reporting.

Gustavo Niemeyer (niemeyer) wrote :

Looks good, thanks!

[1]

22 + rel_services = [
23 + rsn for rsn in rel_services if rsn.service_name !=
24 + service.service_name]

The way this is formatted feels more unreadable than it ought to be.

Here is a suggested style:

                rel_services = [rsn for rsn in rel_services
                                if rsn.service_name != service.service_name]

review: Approve

Preview Diff

1=== modified file 'ensemble/control/status.py'
2--- ensemble/control/status.py 2011-06-28 09:40:28 +0000
3+++ ensemble/control/status.py 2011-07-01 14:41:25 +0000
4@@ -192,7 +192,7 @@
5 # but can happen in test circumstances. In
6 # particular, it will happen with a misconfigured
7 # provider, which exercises this codepath.
8- continue # should not occur, but status should not fail
9+ continue # should not occur, but status should not fail
10 relation_workflow_client = WorkflowStateClient(
11 client, relation_unit)
12 relation_workflow_state = \
13@@ -209,9 +209,11 @@
14
15 for relation in relations:
16 rel_services = yield relation.get_service_states()
17- # filter out 'self'
18- rel_services = [rsn for rsn in rel_services if rsn.service_name !=
19- service.service_name]
20+ # filter out 'self' if its nots a peer relation
21+ if len(rel_services) > 1:
22+ rel_services = [
23+ rsn for rsn in rel_services if rsn.service_name !=
24+ service.service_name]
25 if len(rel_services) > 1:
26 raise ValueError("Unexpected relationship with more "
27 "than 2 endpoints")
28@@ -295,7 +297,10 @@
29 """
30 return name.replace("-", "_")
31
32-def render_dot(data, filelike, environment, format="dot", style=DEFAULT_STYLE):
33+def render_dot(
34+ data, filelike, environment, format="dot", style=DEFAULT_STYLE):
35+ """Render a graphiz output of the status information.
36+ """
37 try:
38 import pydot
39 except ImportError:
40
41=== modified file 'ensemble/control/tests/test_status.py'
42--- ensemble/control/tests/test_status.py 2011-06-28 09:40:28 +0000
43+++ ensemble/control/tests/test_status.py 2011-07-01 14:41:25 +0000
44@@ -19,6 +19,7 @@
45 sample_path = os.path.join(tests_path, "sample_cluster.yaml")
46 sample_cluster = yaml.load(open(sample_path, "r"))
47
48+
49 def dump_stringio(stringio, filename):
50 """Debug utility to dump a StringIO to a filename."""
51 fp = open(filename, "w")
52@@ -61,7 +62,8 @@
53 for unit_state, state in zip(unit_states, states):
54 relation_unit_state = yield relation_state.add_unit_state(
55 unit_state)
56- workflow_client = ZookeeperWorkflowState(self.client, relation_unit_state)
57+ workflow_client = ZookeeperWorkflowState(
58+ self.client, relation_unit_state)
59 yield workflow_client.set_state(state)
60
61 @inlineCallbacks
62@@ -97,7 +99,6 @@
63 m7 = yield self.machine_state_manager.add_machine_state()
64
65 # inform the provider about the machine
66-
67 yield self.provider.start_machine({"machine-id": 0,
68 "dns-name": "steamcloud-1.com"})
69 yield self.provider.start_machine({"machine-id": 1,
70@@ -135,7 +136,7 @@
71 state["services"] = dict(wordpress=wordpress, mysql=mysql,
72 varnish=varnish, memcache=memcache)
73
74- # add unit states to services and assign to machines
75+ # add unit states to services and assign to machines
76 wpu = yield wordpress.add_unit_state()
77 yield wpu.assign_to_machine(m1)
78
79@@ -207,6 +208,49 @@
80
81
82 class StatusTest(StatusTestBase):
83+
84+ @inlineCallbacks
85+ def test_peer_relation(self):
86+ """Verify status works with peer relations.
87+ """
88+ m1 = yield self.machine_state_manager.add_machine_state()
89+ m2 = yield self.machine_state_manager.add_machine_state()
90+ yield self.provider.start_machine({"machine-id": 0,
91+ "dns-name": "steamcloud-1.com"})
92+ yield self.provider.start_machine({"machine-id": 1,
93+ "dns-name": "steamcloud-2.com"})
94+ yield m1.set_instance_id(0)
95+ yield m2.set_instance_id(1)
96+
97+ riak = yield self.add_service_from_formula("riak")
98+ riak_u1 = yield riak.add_unit_state()
99+ riak_u2 = yield riak.add_unit_state()
100+
101+ yield riak_u1.assign_to_machine(m1)
102+ yield riak_u2.assign_to_machine(m2)
103+ yield self.set_unit_state(riak_u1, "started")
104+ yield self.set_unit_state(riak_u2, "started")
105+
106+ _, (peer_rel,) = yield self.relation_state_manager.add_relation_state(
107+ RelationEndpoint("riak", "peer", "ring", "peer"))
108+
109+ yield ZookeeperWorkflowState(
110+ self.client,
111+ (yield peer_rel.add_unit_state(riak_u1))).set_state("up")
112+ yield peer_rel.add_unit_state(riak_u2)
113+
114+ state = yield status.collect("riak", self.provider, self.client, None)
115+ self.assertEqual(
116+ state["services"]["riak"],
117+ {'formula': 'namespace:riak-7',
118+ 'relations': {'ring': 'riak'},
119+ 'units': {'riak/0': {'machine': 0,
120+ 'relations': {'ring': {'state': 'up'}},
121+ 'state': 'started'},
122+ 'riak/1': {'machine': 1,
123+ 'relations': {'ring': {'state': None}},
124+ 'state': 'started'}}})
125+
126 @inlineCallbacks
127 def test_collect(self):
128 yield self.build_topology()
129@@ -254,7 +298,8 @@
130 yield self.build_topology()
131
132 # collect by service name
133- state = yield status.collect("wordpress", self.provider, self.client, None)
134+ state = yield status.collect(
135+ "wordpress", self.provider, self.client, None)
136 # Validate that only the expected service is present
137 # in the state
138 self.assertEqual(state["machines"].keys(), [0])
139@@ -299,7 +344,8 @@
140 "relations": {"proxy": {"state": "up"}}}}}})
141
142 # filter a missing service
143- state = yield status.collect("cluehammer", self.provider, self.client, None)
144+ state = yield status.collect(
145+ "cluehammer", self.provider, self.client, None)
146 self.assertEqual(set(state["machines"].keys()), set([]))
147 self.assertEqual(set(state["services"].keys()), set([]))
148

Subscribers

People subscribed via source and target branches

to status/vote changes: