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 +0000
+++ juju/hooks/protocol.py	2013-02-01 03:35:02 +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"
          }




Index: juju/state/errors.py
=== modified file 'juju/state/errors.py'
--- juju/state/errors.py	2012-04-11 17:48:26 +0000
+++ juju/state/errors.py	2013-02-01 03:35:02 +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."""



Index: juju/hooks/tests/test_invoker.py
=== 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 03:35:02 +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,7 +1156,7 @@
              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
@@ -1294,9 +1294,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 +1323,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 +1400,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 +1493,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"