Merge lp:~hazmat/pyjuju/sane-relation-ids into lp:pyjuju

[Kapil Thangavelu]

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://codereview.appspot.com/7241062/

[Kapil Thangavelu (hazmat)]

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:
   A [revision details]
   M juju/hooks/invoker.py
   M juju/hooks/protocol.py
   M juju/hooks/tests/test_invoker.py
   M juju/state/errors.py

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
=== modified file 'juju/hooks/invoker.py'
--- juju/hooks/invoker.py 2013-01-15 19:53:21 +0000
+++ juju/hooks/invoker.py 2013-02-01 03:35:02 +0000
@@ -6,7 +6,7 @@
  from twisted.python.failure import Failure

  from juju import errors
-from juju.state.errors import RelationStateNotFound,
InvalidRelationIdentity
+from juju.state.errors import RelationIdentNotFound,
InvalidRelationIdentity
  from juju.state.hook import RelationHookContext

@@ -172,7 +172,8 @@
      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(None)))
+ relation_idents = set((yield
self._context.get_relation_idents(None)))
+ #relation_idents = set((yield self.get_relation_idents(None)))
          if isinstance(self._context, RelationHookContext):
              # Exclude the parent context for being looked up as a child
              relation_idents.discard(self._context.relation_ident)
@@ -244,10 +245,14 @@
              if len(parts) != 2 or not parts[1].isdigit():
                  raise InvalidRelationIdentity(relation_ident)
              else:
- raise RelationStateNotFound()
+ raise RelationIdentNotFound(relation_ident)

+ @inlineCallbacks
      def get_relation_idents(self, relation_name):
- return self._context.get_relation_idents(relation_name)
+ """Returns valid relation instances for the given name."""
+ idents = yield self._context.get_relation_idents(relation_name)
+ returnValue(
+ list(set(idents).intersection(set(self._relation_contexts))))

      def validate_hook(self, hook_filename):
          """Verify that the hook_filename exists and is executable. """

Index: juju/hooks/protocol.py
=== modified file 'juju/hooks/protocol.py'
--- juju/hooks/protocol.py 2012-10-16 23:19:55 +0...

[lp:~hazmat/pyjuju/sane-relation-ids]

615. By Kapil Thangavelu on 2013-02-01

a rel-id cache coherence test

[Kapil Thangavelu (hazmat)]

Please take a look.

https://codereview.appspot.com/7241062/

[Benjamin Saller (bcsaller)]

LGTM, subtle issue you're fixing there.

https://codereview.appspot.com/7241062/diff/3001/juju/hooks/invoker.py
File juju/hooks/invoker.py (right):

https://codereview.appspot.com/7241062/diff/3001/juju/hooks/invoker.py#newcode255
juju/hooks/invoker.py:255:
list(set(idents).intersection(set(self._relation_contexts))))
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.

https://codereview.appspot.com/7241062/

[Kapil Thangavelu (hazmat)]

thanks for the review

https://codereview.appspot.com/7241062/diff/3001/juju/hooks/invoker.py
File juju/hooks/invoker.py (right):

https://codereview.appspot.com/7241062/diff/3001/juju/hooks/invoker.py#newcode255
juju/hooks/invoker.py:255:
list(set(idents).intersection(set(self._relation_contexts))))
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).

https://codereview.appspot.com/7241062/

[Kapil Thangavelu (hazmat)]

*** 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://codereview.appspot.com/7241062

https://codereview.appspot.com/7241062/

=== modified file 'juju/hooks/invoker.py'
--- juju/hooks/invoker.py	2013-01-15 19:53:21 +0000
+++ juju/hooks/invoker.py	2013-02-01 14:18:29 +0000
@@ -6,7 +6,7 @@
 from twisted.python.failure import Failure
 
 from juju import errors
-from juju.state.errors import RelationStateNotFound, InvalidRelationIdentity
+from juju.state.errors import RelationIdentNotFound, InvalidRelationIdentity
 from juju.state.hook import RelationHookContext
 
 
@@ -172,7 +172,8 @@
     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(None)))
+        relation_idents = set((yield self._context.get_relation_idents(None)))
+
         if isinstance(self._context, RelationHookContext):
             # Exclude the parent context for being looked up as a child
             relation_idents.discard(self._context.relation_ident)
@@ -185,8 +186,8 @@
                 relation_ident)
             self._relation_contexts[relation_ident] = child
         self._log.debug("Cached relation hook contexts%s: %r" % (
-                display_parent_relation_ident,
-                sorted(relation_idents)))
+            display_parent_relation_ident,
+            sorted(relation_idents)))
 
         service = yield self._context.get_local_service()
         charm = yield service.get_charm_state()
@@ -244,10 +245,14 @@
             if len(parts) != 2 or not parts[1].isdigit():
                 raise InvalidRelationIdentity(relation_ident)
             else:
-                raise RelationStateNotFound()
+                raise RelationIdentNotFound(relation_ident)
 
+    @inlineCallbacks
     def get_relation_idents(self, relation_name):
-        return self._context.get_relation_idents(relation_name)
+        """Returns valid relation instances for the given name."""
+        idents = yield self._context.get_relation_idents(relation_name)
+        returnValue(
+            list(set(idents).intersection(set(self._relation_contexts))))
 
     def validate_hook(self, hook_filename):
         """Verify that the hook_filename exists and is executable. """

=== modified file 'juju/hooks/protocol.py'
--- juju/hooks/protocol.py	2012-10-16 23:19:55 +0000
+++ juju/hooks/protocol.py	2013-02-01 14:18:29 +0000
@@ -54,7 +54,8 @@
 from juju.lib import serializer
 from juju.lib.format import get_charm_formatter, get_charm_formatter_from_env
 from juju.state.errors import (
-    InvalidRelationIdentity,  RelationStateNotFound, UnitRelationStateNotFound)
+    InvalidRelationIdentity,  RelationStateNotFound, UnitRelationStateNotFound,
+    RelationIdentNotFound)
 from juju.state.hook import RelationHookContext
 
 
@@ -92,7 +93,8 @@
         UnitRelationStateNotFound: "UnitRelationStateNotFound",
         MustSpecifyRelationName: "MustSpecifyRelationName",
         InvalidRelationIdentity: "InvalidRelationIdentity",
-        RelationStateNotFound: "RelationStateNotFound"
+        RelationStateNotFound: "RelationStateNotFound",
+        RelationIdentNotFound: "RelationIdentNotFound"
         }
 
 

=== modified file 'juju/hooks/tests/test_invoker.py'
--- juju/hooks/tests/test_invoker.py	2013-01-15 19:53:21 +0000
+++ juju/hooks/tests/test_invoker.py	2013-02-01 14:18:29 +0000
@@ -24,7 +24,7 @@
 from juju.lib.twistutils import get_module_directory
 from juju.state import hook
 from juju.state.endpoint import RelationEndpoint
-from juju.state.errors import RelationStateNotFound
+from juju.state.errors import RelationIdentNotFound
 from juju.state.relation import RelationStateManager
 from juju.state.tests.test_relation import RelationTestBase
 
@@ -1046,12 +1046,12 @@
 
         # Not yet visible relation
         self.assertRaises(
-            RelationStateNotFound,
+            RelationIdentNotFound,
             exe.get_cached_relation_hook_context, "db:2")
 
         # Nonexistent relation idents
         self.assertRaises(
-            RelationStateNotFound,
+            RelationIdentNotFound,
             exe.get_cached_relation_hook_context, "db:12345")
 
         # Reread parent and child contexts
@@ -1156,13 +1156,13 @@
             set((yield exe.get_context().get_relation_idents("db"))),
             set(["db:0", "db:2"]))
         yield self.assertRaises(
-            RelationStateNotFound,
+            RelationIdentNotFound,
             exe.get_cached_relation_hook_context, "db:1")
 
     @defer.inlineCallbacks
     def test_relation_ids(self):
         """Verify `relation-ids` command returns ids separated by newlines."""
-        yield self.add_a_blog("wordpress2")
+
         hook_log = self.capture_logging("hook")
 
         # Invoker will be in the context of the mysql/0 service unit
@@ -1170,6 +1170,10 @@
             "db:0", "add", "mysql/0", self.relation,
             client_id="client_id")
 
+        # Invoker has already been started, verify cache coherence by
+        # adding another relation.
+        yield self.add_a_blog("wordpress2")
+
         # Then verify the hook lists the relation ids corresponding to
         # the relation name `db`
         hook = self.create_hook("relation-ids", "db")
@@ -1178,12 +1182,12 @@
         yield exe.ended
         # Smart formtting outputs one id per line
         self.assertEqual(
-            hook_log.getvalue(), "db:0\ndb:1\n\n")
+            hook_log.getvalue(), "db:0\n\n")
         # But newlines are just whitespace to the shell or to Python,
         # so they can be split accordingly, adhering to the letter of
         # the spec
         self.assertEqual(
-            hook_log.getvalue().split(), ["db:0", "db:1"])
+            hook_log.getvalue().split(), ["db:0"])
 
     @defer.inlineCallbacks
     def test_relation_ids_json_format(self):
@@ -1294,9 +1298,9 @@
 
         db1 = exe.get_cached_relation_hook_context("db:1")
         yield self.assert_zk_data(db1, {
-                "a": "42",
-                "b": "xyz",
-                "private-address": "mysql-0.example.com"})
+            "a": "42",
+            "b": "xyz",
+            "private-address": "mysql-0.example.com"})
         self.assertLogLines(
             self.log.getvalue(),
             ["Cached relation hook contexts on 'db:0': ['db:1']",
@@ -1323,7 +1327,7 @@
         self.assertEqual(result, 0)
         yield exe.ended
         self.assert_file(args["stdout"], "")
-        self.assert_file(args["stderr"], "Relation not found\n")
+        self.assert_file(args["stderr"], "Relation not found for - db:12345\n")
         self.assertEqual(
             hook_log.getvalue(),
             "Cached relation hook contexts on 'db:0': ['db:1']\n"
@@ -1400,7 +1404,7 @@
         self.assertEqual(result, 0)
         yield exe.ended
         self.assert_file(args["settings"], "")
-        self.assert_file(args["stderr"], "Relation not found\n")
+        self.assert_file(args["stderr"], "Relation not found for - db:12345\n")
         self.assertEqual(
             hook_log.getvalue(),
             "Cached relation hook contexts on 'db:0': ['db:1']\n"
@@ -1493,7 +1497,7 @@
         self.assertEqual(result, 0)
         yield exe.ended
         self.assert_file(args["stdout"], "")
-        self.assert_file(args["stderr"], "Relation not found\n")
+        self.assert_file(args["stderr"], "Relation not found for - db:12345\n")
         self.assertEqual(
             hook_log.getvalue(),
             "Cached relation hook contexts on 'db:0': ['db:1']\n"

=== modified file 'juju/state/errors.py'
--- juju/state/errors.py	2012-04-11 17:48:26 +0000
+++ juju/state/errors.py	2013-02-01 14:18:29 +0000
@@ -228,6 +228,18 @@
         return "Relation not found"
 
 
+class RelationIdentNotFound(StateError):
+
+    def __init__(self, relation_ident):
+        # Play nice with amp oddities, where we get our own string repr
+        if "-" in relation_ident:
+            relation_ident = relation_ident.split("-")[-1].strip()
+        self.relation_ident = relation_ident
+
+    def __str__(self):
+        return "Relation not found for - %s" % (self.relation_ident)
+
+
 class UnitRelationStateAlreadyAssigned(StateError):
     """The unit already exists in the relation."""