Merge lp:~jimbaker/pyjuju/hook-command-error-cases into lp:pyjuju

[Jim Baker]

Exceptions for hook commands using relation id

AMP requires that all exceptions that are passed between the hook command (client) and unit agent (server) are registered, otherwise it will raise "twisted ERROR: Unhandled Error". This branch fixes this issue for exceptions introduced by the use of -r to specify a relation id. In addition for completeness, other tests were added around potential error cases.

This branch also resolves bug #1050592, which saw this issue in the use of relation-list with -r.

Note that these fixes continue the current practice that an error report is written to stderr ("Relation not found"), but the exit code of the hook command is still 0.

I have tested the branch with the the debug-charm attached to the bug, lp:~mew/+junk/debug-charm

https://codereview.appspot.com/6589056/

[Clint Byrum (clint-fewbar)]

Hi Jim. This is a pretty big merge, so perhaps next time use lbox propose -cr so we can review w/ rietveld?

Anyway, I think it all LGTM .. one bit of feedback:

99 - raise RelationStateNotFound()
100 + parts = relation_ident.split(":")
101 + if len(parts) != 2 or not parts[1].isdigit():
102 + raise InvalidRelationIdentity(relation_ident)
103 + else:
104 + raise RelationStateNotFound()

I don't really understand checking the format at this point.. we already know its not found, and thats what a user trying to debug their charm would want to know. The format problem would likely be *more* confusing. So I think this check, and the extra test for it, can just be dropped.

review: Needs Fixing

[Jim Baker (jimbaker)]

Reviewers: mp+127188_code.launchpad.net,

Message:
Please take a look.

Description:
Exceptions for hook commands using relation id

AMP requires that all exceptions that are passed between the hook
command (client) and unit agent (server) are registered, otherwise it
will raise "twisted ERROR: Unhandled Error". This branch fixes this
issue for exceptions introduced by the use of -r to specify a relation
id. In addition for completeness, other tests were added around
potential error cases.

This branch also resolves bug #1050592, which saw this issue in the use
of relation-list with -r.

Note that these fixes continue the current practice that an error report
is written to stderr ("Relation not found"), but the exit code of the
hook command is still 0.

I have tested the branch with the the debug-charm attached to the bug,
lp:~mew/+junk/debug-charm

https://code.launchpad.net/~jimbaker/juju/hook-command-error-cases/+merge/127188

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6589056/

Affected files:
   A [revision details]
   M juju/hooks/commands.py
   M juju/hooks/invoker.py
   M juju/hooks/protocol.py
   M juju/hooks/tests/test_invoker.py

[Jim Baker (jimbaker)]

I'm fine with reporting "Relation not found", however, according to the spec, it should report "Not a valid relation id".

Note that the format check was being done on the other get_relation_hook_context (unfortunate naming) in HookContext; however, the invoker always caches associated hook contexts, then looks them up, so this check was never actually being made at a user visible level.

[Clint Byrum (clint-fewbar)]

Ok, well if the spec has it, then I see no reason to delay merging.

review: Approve

=== modified file 'juju/hooks/commands.py'
--- juju/hooks/commands.py	2012-05-03 16:46:40 +0000
+++ juju/hooks/commands.py	2012-10-01 03:59:18 +0000
@@ -9,6 +9,7 @@
 from juju.hooks.cli import (
     CommandLineClient, parse_log_level, parse_port_protocol)
 from juju.hooks.protocol import MustSpecifyRelationName
+from juju.state.errors import InvalidRelationIdentity
 
 
 BAD_CHARS = re.compile("[\-\./:()<>|?*]|(\\\)")
@@ -27,13 +28,22 @@
 
     @inlineCallbacks
     def run(self):
-        # handle settings_name being explictly skipped on the cli
         if self.options.settings_name == "-":
             self.options.settings_name = ""
-        result = yield self.client.relation_get(self.options.client_id,
-                                                self.options.relation_id,
-                                                self.options.unit_name,
-                                                self.options.settings_name)
+        if self.options.unit_name is None:
+            print >>sys.stderr, "Unit name is not defined"
+            return
+        result = None
+        try:
+            result = yield self.client.relation_get(self.options.client_id,
+                                                    self.options.relation_id,
+                                                    self.options.unit_name,
+                                                    self.options.settings_name)
+        except InvalidRelationIdentity, e:
+            # This prevents the exception from getting wrapped by AMP
+            print >>sys.stderr, e.relation_ident
+        except Exception, e:
+            print >>sys.stderr, str(e)
         returnValue(result)
 
     def format_shell(self, result, stream):
@@ -83,10 +93,17 @@
         self.parser.add_argument(
             "-r", dest="relation_id", default="", metavar="RELATION ID")
 
+    @inlineCallbacks
     def run(self):
-        return self.client.relation_set(self.options.client_id,
-                                        self.options.relation_id,
-                                        self.options.keyvalue_pairs)
+        try:
+            yield self.client.relation_set(self.options.client_id,
+                                           self.options.relation_id,
+                                           self.options.keyvalue_pairs)
+        except InvalidRelationIdentity, e:
+            # This prevents the exception from getting wrapped by AMP
+            print >>sys.stderr, e.relation_ident
+        except Exception, e:
+            print >>sys.stderr, str(e)
 
 
 def relation_set():
@@ -135,9 +152,18 @@
         self.parser.add_argument(
             "-r", dest="relation_id", default="", metavar="RELATION ID")
 
+    @inlineCallbacks
     def run(self):
-        return self.client.list_relations(self.options.client_id,
-                                   self.options.relation_id)
+        result = None
+        try:
+            result = yield self.client.list_relations(self.options.client_id,
+                                                      self.options.relation_id)
+        except InvalidRelationIdentity, e:
+            # This prevents the exception from getting wrapped by AMP
+            print >>sys.stderr, e.relation_ident
+        except Exception, e:
+            print >>sys.stderr, str(e)
+        returnValue(result)
 
     def format_eval(self, result, stream):
         """ eval `juju-list` """

=== modified file 'juju/hooks/invoker.py'
--- juju/hooks/invoker.py	2012-07-01 18:38:54 +0000
+++ juju/hooks/invoker.py	2012-10-01 03:59:18 +0000
@@ -6,7 +6,7 @@
 from twisted.python.failure import Failure
 
 from juju import errors
-from juju.state.errors import RelationStateNotFound
+from juju.state.errors import RelationStateNotFound, InvalidRelationIdentity
 from juju.state.hook import RelationHookContext
 
 
@@ -239,7 +239,11 @@
         try:
             return self._relation_contexts[relation_ident]
         except KeyError:
-            raise RelationStateNotFound()
+            parts = relation_ident.split(":")
+            if len(parts) != 2 or not parts[1].isdigit():
+                raise InvalidRelationIdentity(relation_ident)
+            else:
+                raise RelationStateNotFound()
 
     def get_relation_idents(self, relation_name):
         return self._context.get_relation_idents(relation_name)

=== modified file 'juju/hooks/protocol.py'
--- juju/hooks/protocol.py	2012-09-15 14:13:27 +0000
+++ juju/hooks/protocol.py	2012-10-01 03:59:18 +0000
@@ -53,7 +53,8 @@
 from juju.errors import JujuError
 from juju.lib import serializer
 from juju.lib.format import get_charm_formatter, get_charm_formatter_from_env
-from juju.state.errors import UnitRelationStateNotFound
+from juju.state.errors import (
+    InvalidRelationIdentity,  RelationStateNotFound, UnitRelationStateNotFound)
 from juju.state.hook import RelationHookContext
 
 
@@ -84,11 +85,15 @@
 
 
 class BaseCommand(amp.Command):
-    errors = {NoSuchUnit: "NoSuchUnit",
-              NoSuchKey: "NoSuchKey",
-              NotRelationContext: "NotRelationContext",
-              UnitRelationStateNotFound: "UnitRelationStateNotFound",
-              MustSpecifyRelationName: "MustSpecifyRelationName"}
+    errors = {
+        NoSuchUnit: "NoSuchUnit",
+        NoSuchKey: "NoSuchKey",
+        NotRelationContext: "NotRelationContext",
+        UnitRelationStateNotFound: "UnitRelationStateNotFound",
+        MustSpecifyRelationName: "MustSpecifyRelationName",
+        InvalidRelationIdentity: "InvalidRelationIdentity",
+        RelationStateNotFound: "RelationStateNotFound"
+        }
 
 
 # All the commands below this point should be documented in the

=== modified file 'juju/hooks/tests/test_invoker.py'
--- juju/hooks/tests/test_invoker.py	2012-09-15 14:13:27 +0000
+++ juju/hooks/tests/test_invoker.py	2012-10-01 03:59:18 +0000
@@ -123,6 +123,21 @@
             self.socket_path, self.server_factory)
 
 
+def capture_separate_log(name, level):
+    """Support the separate capture of logging at different log levels.
+
+    TestCase.capture_logging only allows one level to be captured at
+    any given time. Given that the hook log captures both data AND
+    traditional logging, it's useful to separate.
+    """
+    logger = logging.getLogger(name)
+    output = StringIO()
+    handler = logging.StreamHandler(output)
+    handler.setLevel(level)
+    logger.addHandler(handler)
+    return output
+
+
 def get_cli_environ_path(*search_path):
     """Construct a path environment variable.
 
@@ -198,6 +213,31 @@
         os.chmod(fn, stat.S_IEXEC | stat.S_IREAD)
         return fn
 
+    def create_capturing_hook(self, hook, files=()):
+        """Create a hook to enable capturing of results into files.
+
+        This method helps test scenarios of bash scripts that depend
+        on exact captures of stdout and stderr as well as set -eu
+        (Juju hook commands do not return nonzero exit codes, except
+        in the case of parse failures).
+
+        bin-path (the path to Juju commands) is always defined, along
+        with paths to temporary files, as keyed to the list of
+        `files`.
+        """
+        args = {"bin-path": os.path.normpath(
+                os.path.join(get_module_directory(juju), "..", "bin"))}
+        for f in files:
+            args[f] = self.makeFile()
+        hook_fn = self.makeFile(hook.format(**args))
+        os.chmod(hook_fn, stat.S_IEXEC | stat.S_IREAD)
+        return hook_fn, args
+
+    def assert_file(self, path, data):
+        """Assert file reached by `path` contains exactly this `data`."""
+        with open(path) as f:
+            self.assertEqual(f.read(), data)
+
 
 class TestCompleteInvoker(InvokerTestBase, StatusTestBase):
 
@@ -1211,6 +1251,29 @@
             hook_log.getvalue())
 
     @defer.inlineCallbacks
+    def test_relation_ids_nonexistent_relation_name(self):
+        """Verify an empty listing is returned if name does not exist"""
+        hook_log = self.capture_logging("hook", level=logging.DEBUG)
+        yield self.add_a_blog("wordpress2")
+        exe = yield self.ua.get_invoker(
+            "db:0", "add", "mysql/0", self.relation, client_id="client_id")
+        hook, args = self.create_capturing_hook(
+            "#!/bin/bash\n"
+            "set -eu\n"
+            "data=$({bin-path}/relation-ids does-not-exist 2> {stderr})\n"
+            "echo -n $data > {stdout}\n",
+            files=["stdout", "stderr"])
+        result = yield exe(hook)
+        self.assertEqual(result, 0)
+        yield exe.ended
+        self.assert_file(args["stdout"], "")
+        self.assert_file(args["stderr"], "")
+        self.assertEqual(
+            hook_log.getvalue(),
+            "Cached relation hook contexts on 'db:0': ['db:1']\n"
+            "hook {} exited, exit code 0.\n".format(os.path.basename(hook)))
+
+    @defer.inlineCallbacks
     def test_relation_set_with_relation_id_option(self):
         """Verify `relation-set` works with -r option."""
         # Invoker will be in the context of the db:0 relation
@@ -1238,6 +1301,57 @@
              "    Setting changed: u'b'=u'xyz' (was unset) on 'db:1'"])
 
     @defer.inlineCallbacks
+    def test_relation_set_with_nonexistent_relation_id(self):
+        """Verify error put on stderr when using nonexistent relation id."""
+        hook_log = self.capture_logging("hook", level=logging.DEBUG)
+        yield self.add_a_blog("wordpress2")
+        exe = yield self.ua.get_invoker(
+            "db:0", "add", "mysql/0", self.relation,
+            client_id="client_id")
+        hook, args = self.create_capturing_hook(
+            "#!/bin/bash\n"
+            "set -eu\n"
+            "data=$({bin-path}/relation-set -r db:12345 "
+            "k1=v1 k2=v2 2> {stderr})\n"
+            "echo -n $data > {stdout}\n",
+            files=["stdout", "stderr"])
+        result = yield exe(hook)
+        self.assertEqual(result, 0)
+        yield exe.ended
+        self.assert_file(args["stdout"], "")
+        self.assert_file(args["stderr"], "Relation not found\n")
+        self.assertEqual(
+            hook_log.getvalue(),
+            "Cached relation hook contexts on 'db:0': ['db:1']\n"
+            "hook {} exited, exit code 0.\n".format(os.path.basename(hook)))
+
+    @defer.inlineCallbacks
+    def test_relation_set_with_invalid_relation_id(self):
+        """Verify message is written to stderr for invalid formatted id."""
+        hook_log = self.capture_logging("hook", level=logging.DEBUG)
+        yield self.add_a_blog("wordpress2")
+        exe = yield self.ua.get_invoker(
+            "db:0", "add", "mysql/0", self.relation, client_id="client_id")
+        exe.environment["JUJU_REMOTE_UNIT"] = "wordpress/0"
+        hook, args = self.create_capturing_hook(
+            "#!/bin/bash\n"
+            "set -eu\n"
+            "data=$({bin-path}/relation-set -r invalid-id:forty-two "
+            "k1=v1 k2=v2 2> {stderr})\n"
+            "echo -n $data > {stdout}\n",
+            files=["stderr", "stdout"])
+        result = yield exe(hook)
+        self.assertEqual(result, 0)
+        yield exe.ended
+        self.assert_file(args["stdout"], "")
+        self.assert_file(args["stderr"],
+                         "Not a valid relation id: 'invalid-id:forty-two'\n")
+        self.assertEqual(
+            hook_log.getvalue(),
+            "Cached relation hook contexts on 'db:0': ['db:1']\n"
+            "hook {} exited, exit code 0.\n".format(os.path.basename(hook)))
+
+    @defer.inlineCallbacks
     def test_relation_get_with_relation_id_option(self):
         """Verify `relation-get` works with -r option."""
         yield self.add_a_blog("wordpress2")
@@ -1264,6 +1378,81 @@
              "name": "whiterabbit"})
 
     @defer.inlineCallbacks
+    def test_relation_get_with_nonexistent_relation_id(self):
+        """Verify error put on stderr when using nonexistent relation id."""
+        hook_log = self.capture_logging("hook", level=logging.DEBUG)
+        yield self.add_a_blog("wordpress2")
+        exe = yield self.ua.get_invoker(
+            "db:0", "add", "mysql/0", self.relation,
+            client_id="client_id")
+        hook, args = self.create_capturing_hook(
+            "#!/bin/bash\n"
+            "set -eu\n"
+            "settings=$({bin-path}/relation-get -r db:12345 - mysql/0 "
+            "2> {stderr})\n"
+            "echo -n $settings > {settings}\n",
+            files=["settings", "stderr"])
+        result = yield exe(hook)
+        self.assertEqual(result, 0)
+        yield exe.ended
+        self.assert_file(args["settings"], "")
+        self.assert_file(args["stderr"], "Relation not found\n")
+        self.assertEqual(
+            hook_log.getvalue(),
+            "Cached relation hook contexts on 'db:0': ['db:1']\n"
+            "hook {} exited, exit code 0.\n".format(os.path.basename(hook)))
+
+    @defer.inlineCallbacks
+    def test_relation_get_with_invalid_relation_id(self):
+        """Verify message is written to stderr for invalid formatted id."""
+        hook_log = self.capture_logging("hook", level=logging.DEBUG)
+        yield self.add_a_blog("wordpress2")
+        exe = yield self.ua.get_invoker(
+            "db:0", "add", "mysql/0", self.relation, client_id="client_id")
+        exe.environment["JUJU_REMOTE_UNIT"] = "wordpress/0"
+        hook, args = self.create_capturing_hook(
+            "#!/bin/bash\n"
+            "set -eu\n"
+            "data=$({bin-path}/relation-get -r invalid-id:forty-two - "
+            "2> {stderr})\n"
+            "echo -n $data > {stdout}\n",
+            files=["stderr", "stdout"])
+        result = yield exe(hook)
+        self.assertEqual(result, 0)
+        yield exe.ended
+        self.assert_file(args["stderr"],
+                         "Not a valid relation id: 'invalid-id:forty-two'\n")
+        self.assert_file(args["stdout"], "")
+        self.assertEqual(
+            hook_log.getvalue(),
+            "Cached relation hook contexts on 'db:0': ['db:1']\n"
+            "hook {} exited, exit code 0.\n".format(os.path.basename(hook)))
+
+    @defer.inlineCallbacks
+    def test_relation_get_unset_remote_unit_in_env(self):
+        """Verify error is reported if JUJU_REMOTE_UNIT is not defined."""
+        hook_log = self.capture_logging("hook", level=logging.DEBUG)
+        yield self.add_a_blog("wordpress2")
+        exe = yield self.ua.get_invoker(
+            "db:0", "add", "mysql/0", self.relation, client_id="client_id")
+        self.assertNotIn("JUJU_REMOTE_UNIT", exe.environment)
+        hook, args = self.create_capturing_hook(
+            "#!/bin/bash\n"
+            "set -eu\n"
+            "data=$({bin-path}/relation-get -r db:0 - 2> {stderr})\n"
+            "echo -n $data > {stdout}\n",
+            files=["stderr", "stdout"])
+        result = yield exe(hook)
+        self.assertEqual(result, 0)
+        yield exe.ended
+        self.assert_file(args["stdout"], "")
+        self.assert_file(args["stderr"], "Unit name is not defined\n")
+        self.assertEqual(
+            hook_log.getvalue(),
+            "Cached relation hook contexts on 'db:0': ['db:1']\n"
+            "hook {} exited, exit code 0.\n".format(os.path.basename(hook)))
+
+    @defer.inlineCallbacks
     def test_relation_list_with_relation_id_option(self):
         """Verify `relation-list` works with -r option."""
         yield self.add_a_blog("wordpress2")
@@ -1283,6 +1472,55 @@
             json.loads(hook_log.getvalue()),
             ["wordpress2/0"])
 
+    @defer.inlineCallbacks
+    def test_relation_list_with_nonexistent_relation_id(self):
+        """Verify a nonexistent relation id writes message to stderr."""
+        hook_log = self.capture_logging("hook", level=logging.DEBUG)
+        yield self.add_a_blog("wordpress2")
+        exe = yield self.ua.get_invoker(
+            "db:0", "add", "mysql/0", self.relation, client_id="client_id")
+        hook, args = self.create_capturing_hook(
+            "#!/bin/bash\n"
+            "set -eu\n"
+            "data=$({bin-path}/relation-list -r db:12345 2> {stderr})\n"
+            "echo -n $data > {stdout}\n",
+            files=["stdout", "stderr"])
+        result = yield exe(hook)
+        self.assertEqual(result, 0)
+        yield exe.ended
+        self.assert_file(args["stdout"], "")
+        self.assert_file(args["stderr"], "Relation not found\n")
+        self.assertEqual(
+            hook_log.getvalue(),
+            "Cached relation hook contexts on 'db:0': ['db:1']\n"
+            "hook {} exited, exit code 0.\n".format(os.path.basename(hook)))
+
+    @defer.inlineCallbacks
+    def test_relation_list_with_invalid_relation_id(self):
+        """Verify message is written to stderr for invalid formatted id."""
+        hook_log = self.capture_logging("hook", level=logging.DEBUG)
+        yield self.add_a_blog("wordpress2")
+        exe = yield self.ua.get_invoker(
+            "db:0", "add", "mysql/0", self.relation, client_id="client_id")
+        exe.environment["JUJU_REMOTE_UNIT"] = "wordpress/0"
+        hook, args = self.create_capturing_hook(
+            "#!/bin/bash\n"
+            "set -eu\n"
+            "data=$({bin-path}/relation-list -r invalid-id:forty-two "
+            "2> {stderr})\n"
+            "echo -n $data > {stdout}\n",
+            files=["stderr", "stdout"])
+        result = yield exe(hook)
+        self.assertEqual(result, 0)
+        yield exe.ended
+        self.assert_file(args["stdout"], "")
+        self.assert_file(args["stderr"],
+                         "Not a valid relation id: 'invalid-id:forty-two'\n")
+        self.assertEqual(
+            hook_log.getvalue(),
+            "Cached relation hook contexts on 'db:0': ['db:1']\n"
+            "hook {} exited, exit code 0.\n".format(os.path.basename(hook)))
+
 
 class PortCommandsTest(RelationInvokerTestBase):
 
@@ -1564,21 +1802,6 @@
                 "closed 80/tcp"])
 
 
-def capture_separate_log(name, level):
-    """Support the separate capture of logging at different log levels.
-
-    TestCase.capture_logging only allows one level to be captured at
-    any given time. Given that the hook log captures both data AND
-    traditional logging, it's useful to separate.
-    """
-    logger = logging.getLogger(name)
-    output = StringIO()
-    handler = logging.StreamHandler(output)
-    handler.setLevel(level)
-    logger.addHandler(handler)
-    return output
-
-
 class TestCharmFormatV1(RelationInvokerTestBase):
 
     @defer.inlineCallbacks
@@ -1641,6 +1864,7 @@
         yield exe(self.create_hook(
                 "relation-set", "d=@%s" % data_file))
         result = yield exe(self.create_hook("relation-get", "d mysql/0"))
+        self.assertEqual(result, 1)
         self.assertIn(
             "Error: \'utf8\' codec can\'t decode byte 0xca in position 30: "
             "invalid continuation byte",