Merge lp:~hazmat/pyjuju/sane-relation-ids into lp:pyjuju
- sane-relation-ids
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 612 |
Proposed branch: | lp:~hazmat/pyjuju/sane-relation-ids |
Merge into: | lp:pyjuju |
Diff against target: |
206 lines (+44/-21) 4 files modified
juju/hooks/invoker.py (+11/-6) juju/hooks/protocol.py (+4/-2) juju/hooks/tests/test_invoker.py (+17/-13) juju/state/errors.py (+12/-0) |
To merge this branch: | bzr merge lp:~hazmat/pyjuju/sane-relation-ids |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Juju Engineering | Pending | ||
Review via email: mp+146033@code.launchpad.net |
Commit message
Description of the change
Fix relation-ids cache coherency for hooks.
Relation-ids was answering questions about relations
directly from state, whilst other pieces of relation-*
cli was using a hook execution cache. This led to
odd races where a hook would query relation-ids
and then attempt to use relation-list/etc against
that relation resulting in a relation not found error.
This branch also does a drive by to improve the error
reporting for relation not found to include the relation
id where applicable.
Kapil Thangavelu (hazmat) wrote : | # |
- 615. By Kapil Thangavelu
-
a rel-id cache coherence test
Kapil Thangavelu (hazmat) wrote : | # |
Please take a look.
Benjamin Saller (bcsaller) wrote : | # |
LGTM, subtle issue you're fixing there.
https:/
File juju/hooks/
https:/
juju/hooks/
list(set(
It doesn't look like you test this method after its change to be a
generator. You previously returned the generator though it looks like so
it would have been yielded on anyway, I suspect this is still ok.
Kapil Thangavelu (hazmat) wrote : | # |
thanks for the review
https:/
File juju/hooks/
https:/
juju/hooks/
list(set(
On 2013/02/01 16:11:10, bcsaller wrote:
> It doesn't look like you test this method after its change to be a
generator.
> You previously returned the generator though it looks like so it would
have been
> yielded on anyway, I suspect this is still ok.
it was returning a deferred with eventual value of list of strings
idents in either case, so should be fine (and verified end2end by
existing test suite).
Kapil Thangavelu (hazmat) wrote : | # |
*** Submitted:
Fix relation-ids cache coherency for hooks.
Relation-ids was answering questions about relations
directly from state, whilst other pieces of relation-*
cli was using a hook execution cache. This led to
odd races where a hook would query relation-ids
and then attempt to use relation-list/etc against
that relation resulting in a relation not found error.
This branch also does a drive by to improve the error
reporting for relation not found to include the relation
id where applicable.
R=bcsaller
CC=
https:/
Preview Diff
1 | === modified file 'juju/hooks/invoker.py' | |||
2 | --- juju/hooks/invoker.py 2013-01-15 19:53:21 +0000 | |||
3 | +++ juju/hooks/invoker.py 2013-02-01 14:18:29 +0000 | |||
4 | @@ -6,7 +6,7 @@ | |||
5 | 6 | from twisted.python.failure import Failure | 6 | from twisted.python.failure import Failure |
6 | 7 | 7 | ||
7 | 8 | from juju import errors | 8 | from juju import errors |
9 | 9 | from juju.state.errors import RelationStateNotFound, InvalidRelationIdentity | 9 | from juju.state.errors import RelationIdentNotFound, InvalidRelationIdentity |
10 | 10 | from juju.state.hook import RelationHookContext | 10 | from juju.state.hook import RelationHookContext |
11 | 11 | 11 | ||
12 | 12 | 12 | ||
13 | @@ -172,7 +172,8 @@ | |||
14 | 172 | def start(self): | 172 | def start(self): |
15 | 173 | """Cache relation hook contexts for all relation idents.""" | 173 | """Cache relation hook contexts for all relation idents.""" |
16 | 174 | # Get all relation idents (None means "all") | 174 | # Get all relation idents (None means "all") |
18 | 175 | relation_idents = set((yield self.get_relation_idents(None))) | 175 | relation_idents = set((yield self._context.get_relation_idents(None))) |
19 | 176 | |||
20 | 176 | if isinstance(self._context, RelationHookContext): | 177 | if isinstance(self._context, RelationHookContext): |
21 | 177 | # Exclude the parent context for being looked up as a child | 178 | # Exclude the parent context for being looked up as a child |
22 | 178 | relation_idents.discard(self._context.relation_ident) | 179 | relation_idents.discard(self._context.relation_ident) |
23 | @@ -185,8 +186,8 @@ | |||
24 | 185 | relation_ident) | 186 | relation_ident) |
25 | 186 | self._relation_contexts[relation_ident] = child | 187 | self._relation_contexts[relation_ident] = child |
26 | 187 | self._log.debug("Cached relation hook contexts%s: %r" % ( | 188 | self._log.debug("Cached relation hook contexts%s: %r" % ( |
29 | 188 | display_parent_relation_ident, | 189 | display_parent_relation_ident, |
30 | 189 | sorted(relation_idents))) | 190 | sorted(relation_idents))) |
31 | 190 | 191 | ||
32 | 191 | service = yield self._context.get_local_service() | 192 | service = yield self._context.get_local_service() |
33 | 192 | charm = yield service.get_charm_state() | 193 | charm = yield service.get_charm_state() |
34 | @@ -244,10 +245,14 @@ | |||
35 | 244 | if len(parts) != 2 or not parts[1].isdigit(): | 245 | if len(parts) != 2 or not parts[1].isdigit(): |
36 | 245 | raise InvalidRelationIdentity(relation_ident) | 246 | raise InvalidRelationIdentity(relation_ident) |
37 | 246 | else: | 247 | else: |
39 | 247 | raise RelationStateNotFound() | 248 | raise RelationIdentNotFound(relation_ident) |
40 | 248 | 249 | ||
41 | 250 | @inlineCallbacks | ||
42 | 249 | def get_relation_idents(self, relation_name): | 251 | def get_relation_idents(self, relation_name): |
44 | 250 | return self._context.get_relation_idents(relation_name) | 252 | """Returns valid relation instances for the given name.""" |
45 | 253 | idents = yield self._context.get_relation_idents(relation_name) | ||
46 | 254 | returnValue( | ||
47 | 255 | list(set(idents).intersection(set(self._relation_contexts)))) | ||
48 | 251 | 256 | ||
49 | 252 | def validate_hook(self, hook_filename): | 257 | def validate_hook(self, hook_filename): |
50 | 253 | """Verify that the hook_filename exists and is executable. """ | 258 | """Verify that the hook_filename exists and is executable. """ |
51 | 254 | 259 | ||
52 | === modified file 'juju/hooks/protocol.py' | |||
53 | --- juju/hooks/protocol.py 2012-10-16 23:19:55 +0000 | |||
54 | +++ juju/hooks/protocol.py 2013-02-01 14:18:29 +0000 | |||
55 | @@ -54,7 +54,8 @@ | |||
56 | 54 | from juju.lib import serializer | 54 | from juju.lib import serializer |
57 | 55 | from juju.lib.format import get_charm_formatter, get_charm_formatter_from_env | 55 | from juju.lib.format import get_charm_formatter, get_charm_formatter_from_env |
58 | 56 | from juju.state.errors import ( | 56 | from juju.state.errors import ( |
60 | 57 | InvalidRelationIdentity, RelationStateNotFound, UnitRelationStateNotFound) | 57 | InvalidRelationIdentity, RelationStateNotFound, UnitRelationStateNotFound, |
61 | 58 | RelationIdentNotFound) | ||
62 | 58 | from juju.state.hook import RelationHookContext | 59 | from juju.state.hook import RelationHookContext |
63 | 59 | 60 | ||
64 | 60 | 61 | ||
65 | @@ -92,7 +93,8 @@ | |||
66 | 92 | UnitRelationStateNotFound: "UnitRelationStateNotFound", | 93 | UnitRelationStateNotFound: "UnitRelationStateNotFound", |
67 | 93 | MustSpecifyRelationName: "MustSpecifyRelationName", | 94 | MustSpecifyRelationName: "MustSpecifyRelationName", |
68 | 94 | InvalidRelationIdentity: "InvalidRelationIdentity", | 95 | InvalidRelationIdentity: "InvalidRelationIdentity", |
70 | 95 | RelationStateNotFound: "RelationStateNotFound" | 96 | RelationStateNotFound: "RelationStateNotFound", |
71 | 97 | RelationIdentNotFound: "RelationIdentNotFound" | ||
72 | 96 | } | 98 | } |
73 | 97 | 99 | ||
74 | 98 | 100 | ||
75 | 99 | 101 | ||
76 | === modified file 'juju/hooks/tests/test_invoker.py' | |||
77 | --- juju/hooks/tests/test_invoker.py 2013-01-15 19:53:21 +0000 | |||
78 | +++ juju/hooks/tests/test_invoker.py 2013-02-01 14:18:29 +0000 | |||
79 | @@ -24,7 +24,7 @@ | |||
80 | 24 | from juju.lib.twistutils import get_module_directory | 24 | from juju.lib.twistutils import get_module_directory |
81 | 25 | from juju.state import hook | 25 | from juju.state import hook |
82 | 26 | from juju.state.endpoint import RelationEndpoint | 26 | from juju.state.endpoint import RelationEndpoint |
84 | 27 | from juju.state.errors import RelationStateNotFound | 27 | from juju.state.errors import RelationIdentNotFound |
85 | 28 | from juju.state.relation import RelationStateManager | 28 | from juju.state.relation import RelationStateManager |
86 | 29 | from juju.state.tests.test_relation import RelationTestBase | 29 | from juju.state.tests.test_relation import RelationTestBase |
87 | 30 | 30 | ||
88 | @@ -1046,12 +1046,12 @@ | |||
89 | 1046 | 1046 | ||
90 | 1047 | # Not yet visible relation | 1047 | # Not yet visible relation |
91 | 1048 | self.assertRaises( | 1048 | self.assertRaises( |
93 | 1049 | RelationStateNotFound, | 1049 | RelationIdentNotFound, |
94 | 1050 | exe.get_cached_relation_hook_context, "db:2") | 1050 | exe.get_cached_relation_hook_context, "db:2") |
95 | 1051 | 1051 | ||
96 | 1052 | # Nonexistent relation idents | 1052 | # Nonexistent relation idents |
97 | 1053 | self.assertRaises( | 1053 | self.assertRaises( |
99 | 1054 | RelationStateNotFound, | 1054 | RelationIdentNotFound, |
100 | 1055 | exe.get_cached_relation_hook_context, "db:12345") | 1055 | exe.get_cached_relation_hook_context, "db:12345") |
101 | 1056 | 1056 | ||
102 | 1057 | # Reread parent and child contexts | 1057 | # Reread parent and child contexts |
103 | @@ -1156,13 +1156,13 @@ | |||
104 | 1156 | set((yield exe.get_context().get_relation_idents("db"))), | 1156 | set((yield exe.get_context().get_relation_idents("db"))), |
105 | 1157 | set(["db:0", "db:2"])) | 1157 | set(["db:0", "db:2"])) |
106 | 1158 | yield self.assertRaises( | 1158 | yield self.assertRaises( |
108 | 1159 | RelationStateNotFound, | 1159 | RelationIdentNotFound, |
109 | 1160 | exe.get_cached_relation_hook_context, "db:1") | 1160 | exe.get_cached_relation_hook_context, "db:1") |
110 | 1161 | 1161 | ||
111 | 1162 | @defer.inlineCallbacks | 1162 | @defer.inlineCallbacks |
112 | 1163 | def test_relation_ids(self): | 1163 | def test_relation_ids(self): |
113 | 1164 | """Verify `relation-ids` command returns ids separated by newlines.""" | 1164 | """Verify `relation-ids` command returns ids separated by newlines.""" |
115 | 1165 | yield self.add_a_blog("wordpress2") | 1165 | |
116 | 1166 | hook_log = self.capture_logging("hook") | 1166 | hook_log = self.capture_logging("hook") |
117 | 1167 | 1167 | ||
118 | 1168 | # Invoker will be in the context of the mysql/0 service unit | 1168 | # Invoker will be in the context of the mysql/0 service unit |
119 | @@ -1170,6 +1170,10 @@ | |||
120 | 1170 | "db:0", "add", "mysql/0", self.relation, | 1170 | "db:0", "add", "mysql/0", self.relation, |
121 | 1171 | client_id="client_id") | 1171 | client_id="client_id") |
122 | 1172 | 1172 | ||
123 | 1173 | # Invoker has already been started, verify cache coherence by | ||
124 | 1174 | # adding another relation. | ||
125 | 1175 | yield self.add_a_blog("wordpress2") | ||
126 | 1176 | |||
127 | 1173 | # Then verify the hook lists the relation ids corresponding to | 1177 | # Then verify the hook lists the relation ids corresponding to |
128 | 1174 | # the relation name `db` | 1178 | # the relation name `db` |
129 | 1175 | hook = self.create_hook("relation-ids", "db") | 1179 | hook = self.create_hook("relation-ids", "db") |
130 | @@ -1178,12 +1182,12 @@ | |||
131 | 1178 | yield exe.ended | 1182 | yield exe.ended |
132 | 1179 | # Smart formtting outputs one id per line | 1183 | # Smart formtting outputs one id per line |
133 | 1180 | self.assertEqual( | 1184 | self.assertEqual( |
135 | 1181 | hook_log.getvalue(), "db:0\ndb:1\n\n") | 1185 | hook_log.getvalue(), "db:0\n\n") |
136 | 1182 | # But newlines are just whitespace to the shell or to Python, | 1186 | # But newlines are just whitespace to the shell or to Python, |
137 | 1183 | # so they can be split accordingly, adhering to the letter of | 1187 | # so they can be split accordingly, adhering to the letter of |
138 | 1184 | # the spec | 1188 | # the spec |
139 | 1185 | self.assertEqual( | 1189 | self.assertEqual( |
141 | 1186 | hook_log.getvalue().split(), ["db:0", "db:1"]) | 1190 | hook_log.getvalue().split(), ["db:0"]) |
142 | 1187 | 1191 | ||
143 | 1188 | @defer.inlineCallbacks | 1192 | @defer.inlineCallbacks |
144 | 1189 | def test_relation_ids_json_format(self): | 1193 | def test_relation_ids_json_format(self): |
145 | @@ -1294,9 +1298,9 @@ | |||
146 | 1294 | 1298 | ||
147 | 1295 | db1 = exe.get_cached_relation_hook_context("db:1") | 1299 | db1 = exe.get_cached_relation_hook_context("db:1") |
148 | 1296 | yield self.assert_zk_data(db1, { | 1300 | yield self.assert_zk_data(db1, { |
152 | 1297 | "a": "42", | 1301 | "a": "42", |
153 | 1298 | "b": "xyz", | 1302 | "b": "xyz", |
154 | 1299 | "private-address": "mysql-0.example.com"}) | 1303 | "private-address": "mysql-0.example.com"}) |
155 | 1300 | self.assertLogLines( | 1304 | self.assertLogLines( |
156 | 1301 | self.log.getvalue(), | 1305 | self.log.getvalue(), |
157 | 1302 | ["Cached relation hook contexts on 'db:0': ['db:1']", | 1306 | ["Cached relation hook contexts on 'db:0': ['db:1']", |
158 | @@ -1323,7 +1327,7 @@ | |||
159 | 1323 | self.assertEqual(result, 0) | 1327 | self.assertEqual(result, 0) |
160 | 1324 | yield exe.ended | 1328 | yield exe.ended |
161 | 1325 | self.assert_file(args["stdout"], "") | 1329 | self.assert_file(args["stdout"], "") |
163 | 1326 | self.assert_file(args["stderr"], "Relation not found\n") | 1330 | self.assert_file(args["stderr"], "Relation not found for - db:12345\n") |
164 | 1327 | self.assertEqual( | 1331 | self.assertEqual( |
165 | 1328 | hook_log.getvalue(), | 1332 | hook_log.getvalue(), |
166 | 1329 | "Cached relation hook contexts on 'db:0': ['db:1']\n" | 1333 | "Cached relation hook contexts on 'db:0': ['db:1']\n" |
167 | @@ -1400,7 +1404,7 @@ | |||
168 | 1400 | self.assertEqual(result, 0) | 1404 | self.assertEqual(result, 0) |
169 | 1401 | yield exe.ended | 1405 | yield exe.ended |
170 | 1402 | self.assert_file(args["settings"], "") | 1406 | self.assert_file(args["settings"], "") |
172 | 1403 | self.assert_file(args["stderr"], "Relation not found\n") | 1407 | self.assert_file(args["stderr"], "Relation not found for - db:12345\n") |
173 | 1404 | self.assertEqual( | 1408 | self.assertEqual( |
174 | 1405 | hook_log.getvalue(), | 1409 | hook_log.getvalue(), |
175 | 1406 | "Cached relation hook contexts on 'db:0': ['db:1']\n" | 1410 | "Cached relation hook contexts on 'db:0': ['db:1']\n" |
176 | @@ -1493,7 +1497,7 @@ | |||
177 | 1493 | self.assertEqual(result, 0) | 1497 | self.assertEqual(result, 0) |
178 | 1494 | yield exe.ended | 1498 | yield exe.ended |
179 | 1495 | self.assert_file(args["stdout"], "") | 1499 | self.assert_file(args["stdout"], "") |
181 | 1496 | self.assert_file(args["stderr"], "Relation not found\n") | 1500 | self.assert_file(args["stderr"], "Relation not found for - db:12345\n") |
182 | 1497 | self.assertEqual( | 1501 | self.assertEqual( |
183 | 1498 | hook_log.getvalue(), | 1502 | hook_log.getvalue(), |
184 | 1499 | "Cached relation hook contexts on 'db:0': ['db:1']\n" | 1503 | "Cached relation hook contexts on 'db:0': ['db:1']\n" |
185 | 1500 | 1504 | ||
186 | === modified file 'juju/state/errors.py' | |||
187 | --- juju/state/errors.py 2012-04-11 17:48:26 +0000 | |||
188 | +++ juju/state/errors.py 2013-02-01 14:18:29 +0000 | |||
189 | @@ -228,6 +228,18 @@ | |||
190 | 228 | return "Relation not found" | 228 | return "Relation not found" |
191 | 229 | 229 | ||
192 | 230 | 230 | ||
193 | 231 | class RelationIdentNotFound(StateError): | ||
194 | 232 | |||
195 | 233 | def __init__(self, relation_ident): | ||
196 | 234 | # Play nice with amp oddities, where we get our own string repr | ||
197 | 235 | if "-" in relation_ident: | ||
198 | 236 | relation_ident = relation_ident.split("-")[-1].strip() | ||
199 | 237 | self.relation_ident = relation_ident | ||
200 | 238 | |||
201 | 239 | def __str__(self): | ||
202 | 240 | return "Relation not found for - %s" % (self.relation_ident) | ||
203 | 241 | |||
204 | 242 | |||
205 | 231 | class UnitRelationStateAlreadyAssigned(StateError): | 243 | class UnitRelationStateAlreadyAssigned(StateError): |
206 | 232 | """The unit already exists in the relation.""" | 244 | """The unit already exists in the relation.""" |
207 | 233 | 245 |
Reviewers: mp+146033_ code.launchpad. net,
Message:
Please take a look.
Description:
Fix relation-ids cache coherency for hooks.
Relation-ids was answering questions about relations
directly from state, whilst other pieces of relation-*
cli was using a hook execution cache. This led to
odd races where a hook would query relation-ids
and then attempt to use relation-list/etc against
that relation resulting in a relation not found error.
This branch also does a drive by to improve the error
reporting for relation not found to include the relation
id where applicable.
https:/ /code.launchpad .net/~hazmat/ juju/sane- relation- ids/+merge/ 146033
(do not edit description out of merge proposal)
Please review this at https:/ /codereview. appspot. com/7241062/
Affected files: invoker. py protocol. py tests/test_ invoker. py errors. py
A [revision details]
M juju/hooks/
M juju/hooks/
M juju/hooks/
M juju/state/
Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>
Index: juju/hooks/ invoker. py invoker. py' invoker. py 2013-01-15 19:53:21 +0000 invoker. py 2013-02-01 03:35:02 +0000 python. failure import Failure
=== modified file 'juju/hooks/
--- juju/hooks/
+++ juju/hooks/
@@ -6,7 +6,7 @@
from twisted.
from juju import errors tFound, Identity tFound, Identity
-from juju.state.errors import RelationStateNo
InvalidRelation
+from juju.state.errors import RelationIdentNo
InvalidRelation
from juju.state.hook import RelationHookContext
@@ -172,7 +172,8 @@ relation_ idents( None))) get_relation_ idents( None))) relation_ idents( None))) self._context, RelationHookCon text):
relation_ idents. discard( self._context. relation_ ident)
raise InvalidRelation Identity( relation_ ident) tFound( ) tFound( relation_ ident)
def start(self):
"""Cache relation hook contexts for all relation idents."""
# Get all relation idents (None means "all")
- relation_idents = set((yield self.get_
+ relation_idents = set((yield
self._context.
+ #relation_idents = set((yield self.get_
if isinstance(
# Exclude the parent context for being looked up as a child
@@ -244,10 +245,14 @@
if len(parts) != 2 or not parts[1].isdigit():
else:
- raise RelationStateNo
+ raise RelationIdentNo
+ @inlineCallbacks idents( self, relation_name): get_relation_ idents( relation_ name) get_relation_ idents( relation_ name) idents) .intersection( set(self. _relation_ contexts) )))
def get_relation_
- return self._context.
+ """Returns valid relation instances for the given name."""
+ idents = yield self._context.
+ returnValue(
+ list(set(
def validate_hook(self, hook_filename):
"""Verify that the hook_filename exists and is executable. """
Index: juju/hooks/ protocol. py protocol. py' protocol. py 2012-10-16 23:19:55 +0...
=== modified file 'juju/hooks/
--- juju/hooks/