Code review comment for lp:~hazmat/pyjuju/sane-relation-ids

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"