Merge lp:~danilo/landscape-charm/actions into lp:landscape-charm

[Данило Шеган]

Introduce Action and MaintenanceAction classes to replace Hook-usage in actions

At this time, we need this separation to allow action-fail to properly set error messages (otherwise, hook error codes override it with "exited with code X".

This branch now drops MaintenanceHook which is replaced with MaintenanceAction now.

Introduce Action and MaintenanceAction classes to replace Hook-usage in actions

All our existing actions are using Hook classes for their implementations. While this is not generally a problem other than naming problem, we do want to move away to having different behavior for actions.

Namely, they should not return error codes, because juju interprets their error code as the error message (see bug). Nor would that allow us to set the action return data with action-set.

This branch now drops MaintenanceHook which is replaced with MaintenanceAction now.

To test, use landscape.yaml from https://pastebin.canonical.com/132491/ and deploy with

  juju-deployer -vdW -w180 -c landscape.yaml landscape

and then play around with 'pause', 'resume', 'upgrade' and 'migrate-schema' actions. Check their status with 'juju action fetch $ID' to ensure error messages are properly set.

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 301
Branch: lp:~danilo/landscape-charm/actions
Jenkins: https://ci.lscape.net/job/latch-test/1166/

review: Approve (test results)

[Alberto Donato (ack)]

LGTM, +1

minor comments inline.

review: Approve

[Free Ekanayaka (free.ekanayaka)]

So technically the code triggered by actions can be considered "hook" code, in the sense that any charm script that gets invoked by juju is a "hook", but the reason it gets triggered may vary (a relation changed, and action was issued, the unit was stopped) and hence a the API available to the hook might change slightly (e.g. relation-set defaults to the relation ID being changed, or additional executables like action-set and action-fail are available).

With that in mind, and also considering the error handling issue in the comment, what about:

https://pastebin.canonical.com/132495/

For the maintenance behavior I'd also add a class-level attribute (a bit for sake of composition-over-inheritance, in principle there could be several behavior that you want to mix and match, and that's easier done with class attributes rather than with multiple inheritance).

review: Needs Information

[Free Ekanayaka (free.ekanayaka)]

> So technically the code triggered by actions can be considered "hook" code, in
> the sense that any charm script that gets invoked by juju is a "hook", but the
> reason it gets triggered may vary (a relation changed, and action was issued,
> the unit was stopped) and hence a the API available to the hook might change
> slightly (e.g. relation-set defaults to the relation ID being changed, or
> additional executables like action-set and action-fail are available).
>
> With that in mind, and also considering the error handling issue in the
> comment, what about:
>
> https://pastebin.canonical.com/132495/
>
> For the maintenance behavior I'd also add a class-level attribute (a bit for
> sake of composition-over-inheritance, in principle there could be several
> behavior that you want to mix and match, and that's easier done with class
> attributes rather than with multiple inheritance).

FWIW this type of implementation (a single Hook abstraction with little behavioral variations when run in the context of an action) is the same as the one Juju in: juju/worker/uniter/runner/context.go

[Free Ekanayaka (free.ekanayaka)]

> So technically the code triggered by actions can be considered "hook" code, in
> the sense that any charm script that gets invoked by juju is a "hook", but the
> reason it gets triggered may vary (a relation changed, and action was issued,
> the unit was stopped) and hence a the API available to the hook might change
> slightly (e.g. relation-set defaults to the relation ID being changed, or
> additional executables like action-set and action-fail are available).
>
> With that in mind, and also considering the error handling issue in the
> comment, what about:
>
> https://pastebin.canonical.com/132495/
>
> For the maintenance behavior I'd also add a class-level attribute (a bit for
> sake of composition-over-inheritance, in principle there could be several
> behavior that you want to mix and match, and that's easier done with class
> attributes rather than with multiple inheritance).

Re automatically detecting if we're in an action hook context, reading the Juju source code it seems we can use the JUJU_ACTION_NAME, JUJU_ACTION_UUID and JUJU_ACTION_TAG environment vars.

[Данило Шеган (danilo)]

Free, we basically disagree about what's "easier" in this context. I'd resolve the problem you mention by making Hook class suitable for reuse as a parent for Action class, and then Action class could still have all the action-specific functionality, but would still catch HookError.

Another option, that I like even more, is catching all the exceptions in run() and constructing a nice failure/log message out of those. I generally dislike having a particular exception as the API to signal failure, and would rather have failures automatically propagated and logged.

[Free Ekanayaka (free.ekanayaka)]

> Free, we basically disagree about what's "easier" in this context. I'd
> resolve the problem you mention by making Hook class suitable for reuse as a
> parent for Action class, and then Action class could still have all the
> action-specific functionality, but would still catch HookError.

This works for me. As said, I'd rather have just a Hook class and trigger the action-specific behavior automatically (we can know that by look at the env variables mentioned above)

> Another option, that I like even more, is catching all the exceptions in run()
> and constructing a nice failure/log message out of those.

Do you mean a catch-all try/except? E.g.

try:
    self._run()
except Exception, error:
    # do logging

> I generally dislike
> having a particular exception as the API to signal failure, and would rather
> have failures automatically propagated and logged.

I'm not convinced by this. The HookError is an internal way to propagate errors generated by *expected* conditions (and typically will not traceback and will provide to the user information that he can use to resolve the situation). Random errors should be probably considered just bugs and I think it's probably better to traceback (there's no way you can provide insight to the users about what to do).

So in short, HookError is for known failures mode, hard-crash for all the others.

[Данило Шеган (danilo)]

Thanks for commentary, Free. Basically, automatic behaviour would mean
that we'd have same classes doing different things based on their
environment: do you really foresee us having a hook code that should
behave differently if it's ran as an action?

Since I assume no, and believe that having a single class act in
multiple roles is more confusing, I am keeping Action* classes. I am
depending on Hook* classes, but couldn't come up with a better way to
extract common functionality without reworking everything, so left them
as they are.

I still have some reservations about using HookError directly: imho,
your example of apt module throwing HookError is a bad one: it should
never do that, but should instead throw errors specific to that module.
Callsite should catch that error, and if it's inside a hook/action, turn
it into a HookError/ActionError. I'll split that into a separate
branch, however.

You are right about not catching general exceptions and letting them
traceback, so I am not doing that.

[Данило Шеган (danilo)]

Thanks for commentary, Free. Basically, automatic behaviour would mean that we'd have same classes doing different things based on their environment: do you really foresee us having a hook code that should behave differently if it's ran as an action?

Since I assume no, and believe that having a single class act in multiple roles is more confusing, I am keeping Action* classes. I am depending on Hook* classes, but couldn't come up with a better way to extract common functionality without reworking everything, so left them as they are.

I still have some reservations about using HookError directly: imho, your example of apt module throwing HookError is a bad one: it should never do that, but should instead throw errors specific to that module. Callsite should catch that error, and if it's inside a hook/action, turn it into a HookError/ActionError. I'll split that into a separate branch, however.

You are right about not catching general exceptions and letting them traceback, so I am not doing that.

[Данило Шеган (danilo)]

Thanks for commentary, Free. Basically, automatic behaviour would mean that we'd have same classes doing different things based on their environment: do you really foresee us having a hook code that should behave differently if it's ran as an action?

Since I assume no, and believe that having a single class act in multiple roles is more confusing, I am keeping Action* classes. I am depending on Hook* classes, but couldn't come up with a better way to extract common functionality without reworking everything, so left them as they are.

I still have some reservations about using HookError directly: imho, your example of apt module throwing HookError is a bad one: it should never do that, but should instead throw errors specific to that module. Callsite should catch that error, and if it's inside a hook/action, turn it into a HookError/ActionError. I'll split that into a separate branch, however.

You are right about not catching general exceptions and letting them traceback, so I am not doing that.

[Данило Шеган (danilo)]

Thanks for commentary, Free. Basically, automatic behaviour would mean that we'd have same classes doing different things based on their environment: do you really foresee us having a hook code that should behave differently if it's ran as an action?

Since I assume no, and believe that having a single class act in multiple roles is more confusing, I am keeping Action* classes. I am depending on Hook* classes, but couldn't come up with a better way to extract common functionality without reworking everything, so left them as they are.

I still have some reservations about using HookError directly: imho, your example of apt module throwing HookError is a bad one: it should never do that, but should instead throw errors specific to that module. Callsite should catch that error, and if it's inside a hook/action, turn it into a HookError/ActionError. I'll split that into a separate branch, however.

You are right about not catching general exceptions and letting them traceback, so I am not doing that.

[Данило Шеган (danilo)]

Thanks for commentary, Free. Basically, automatic behaviour would mean that we'd have same classes doing different things based on their environment: do you really foresee us having a hook code that should behave differently if it's ran as an action?

Since I assume no, and believe that having a single class act in multiple roles is more confusing, I am keeping Action* classes. I am depending on Hook* classes, but couldn't come up with a better way to extract common functionality without reworking everything, so left them as they are.

I still have some reservations about using HookError directly: imho, your example of apt module throwing HookError is a bad one: it should never do that, but should instead throw errors specific to that module. Callsite should catch that error, and if it's inside a hook/action, turn it into a HookError/ActionError. I'll split that into a separate branch, however.

You are right about not catching general exceptions and letting them traceback, so I am not doing that.

[Данило Шеган (danilo)]

Thanks for commentary, Free. Basically, automatic behaviour would mean that we'd have same classes doing different things based on their environment: do you really foresee us having a hook code that should behave differently if it's ran as an action?

Since I assume no, and believe that having a single class act in multiple roles is more confusing, I am keeping Action* classes. I am depending on Hook* classes, but couldn't come up with a better way to extract common functionality without reworking everything, so left them as they are.

I still have some reservations about using HookError directly: imho, your example of apt module throwing HookError is a bad one: it should never do that, but should instead throw errors specific to that module. Callsite should catch that error, and if it's inside a hook/action, turn it into a HookError/ActionError. I'll split that into a separate branch, however.

You are right about not catching general exceptions and letting them traceback, so I am not doing that.

[Данило Шеган (danilo)]

Thanks for commentary, Free. Basically, automatic behaviour would mean that we'd have same classes doing different things based on their environment: do you really foresee us having a hook code that should behave differently if it's ran as an action?

Since I assume no, and believe that having a single class act in multiple roles is more confusing, I am keeping Action* classes. I am depending on Hook* classes, but couldn't come up with a better way to extract common functionality without reworking everything, so left them as they are.

I still have some reservations about using HookError directly: imho, your example of apt module throwing HookError is a bad one: it should never do that, but should instead throw errors specific to that module. Callsite should catch that error, and if it's inside a hook/action, turn it into a HookError/ActionError. I'll split that into a separate branch, however.

You are right about not catching general exceptions and letting them traceback, so I am not doing that.

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 302
Branch: lp:~danilo/landscape-charm/actions
Jenkins: https://ci.lscape.net/job/latch-test/1189/

review: Approve (test results)

[Данило Шеган (danilo)]

As discussed on IRC regarding structure of exceptions, we've agreed to meet mid-way: we'll introduce a base CharmError and make ActionError or HookError catch that, and each individual module can throw specific errors they want. If they need actions or hooks to gracefully fail, those exceptions need to inherit CharmError.

[Данило Шеган (danilo)]

With charm-errors landed, this is now ready for another round of reviews: it hasn't changed much, so I am not resubmitting (basically, just dropped ActionError and switched to CharmError in Action).

[Free Ekanayaka (free.ekanayaka)]

+1 with a couple of additional comments

review: Approve

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 308
Branch: lp:~danilo/landscape-charm/actions
Jenkins: https://ci.lscape.net/job/latch-test/1219/

review: Approve (test results)

[lp:~danilo/landscape-charm/actions]

309. By Данило Шеган on 2015-06-05

Drop Action set/fail methods and use hookenv.action_set, action_fail directly.

310. By Данило Шеган on 2015-06-05

Make HookenvStub action_sets and action_fails public for use in tests: addresses review comments by Free.

[Данило Шеган (danilo)]

I've addressed your review comments, thanks Free.

Will do another round of testing before landing.

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 310
Branch: lp:~danilo/landscape-charm/actions
Jenkins: https://ci.lscape.net/job/latch-test/1225/

review: Approve (test results)

=== added file 'lib/action.py'
--- lib/action.py	1970-01-01 00:00:00 +0000
+++ lib/action.py	2015-06-05 12:02:41 +0000
@@ -0,0 +1,50 @@
+import os.path
+
+from charmhelpers.core import hookenv
+
+from lib.error import CharmError
+from lib.hook import Hook
+from lib.paths import default_paths
+
+
+class Action(Hook):
+    """Juju action abstraction, providing dependency injection for testing."""
+
+    def __call__(self):
+        """
+        Invoke the action's run() method.
+
+        If _run() returns a value, set it using action_set().
+        If _run() throws a CharmError, fail using action.fail().
+        """
+        self._hookenv.log("Running action %s" % type(self).__name__)
+        try:
+            return_values = self._run()
+            if return_values is not None:
+                self._hookenv.action_set(return_values)
+        except CharmError, error:
+            self._hookenv.action_fail(str(error))
+
+
+class MaintenanceAction(Action):
+    """Action that only runs when in maintenance mode."""
+
+    def __init__(self, hookenv=hookenv, paths=default_paths):
+        """
+        @param hookenv: The charm-helpers C{hookenv} module, will be replaced
+            by tests.
+        @param paths: The landscape-server default paths class.
+        """
+        self._hookenv = hookenv
+        self._paths = paths
+
+    def __call__(self):
+        """Invoke the action.
+
+        @return: An integer with the exit code for the hook.
+        """
+        if not os.path.exists(self._paths.maintenance_flag()):
+            self._hookenv.action_fail(
+                "This action can only be called on a unit in paused state.")
+            return
+        super(MaintenanceAction, self).__call__()

=== modified file 'lib/hook.py'
--- lib/hook.py	2015-06-05 09:02:17 +0000
+++ lib/hook.py	2015-06-05 12:02:41 +0000
@@ -1,10 +1,7 @@
-import os.path
-
 from charmhelpers.core import hookenv
 from charmhelpers.core.hookenv import ERROR
 
 from lib.error import CharmError
-from lib.paths import default_paths
 
 
 class Hook(object):
@@ -33,27 +30,3 @@
     def _run(self):
         """Do the job."""
         raise NotImplementedError("Must be implemented by sub-classes")
-
-
-class MaintenanceHook(Hook):
-    """Hook that only runs when in maintenance mode."""
-
-    def __init__(self, hookenv=hookenv, paths=default_paths):
-        """
-        @param hookenv: The charm-helpers C{hookenv} module, will be replaced
-            by tests.
-        @param paths: The landscape-server default paths class.
-        """
-        self._hookenv = hookenv
-        self._paths = paths
-
-    def __call__(self):
-        """Invoke the hook.
-
-        @return: An integer with the exit code for the hook.
-        """
-        if not os.path.exists(self._paths.maintenance_flag()):
-            self._hookenv.action_fail(
-                "This action can only be called on a unit in paused state.")
-            return
-        super(MaintenanceHook, self).__call__()

=== modified file 'lib/migrate_schema.py'
--- lib/migrate_schema.py	2015-06-01 10:46:39 +0000
+++ lib/migrate_schema.py	2015-06-05 12:02:41 +0000
@@ -2,11 +2,11 @@
 
 from charmhelpers.core import hookenv
 
-from lib.hook import MaintenanceHook
+from lib.action import MaintenanceAction
 from lib.paths import default_paths, SCHEMA_SCRIPT
 
 
-class MigrateSchemaAction(MaintenanceHook):
+class MigrateSchemaAction(MaintenanceAction):
     """Execute schema upgrade action logic."""
 
     def __init__(self, hookenv=hookenv, paths=default_paths,

=== modified file 'lib/pause.py'
--- lib/pause.py	2015-05-29 09:25:24 +0000
+++ lib/pause.py	2015-06-05 12:02:41 +0000
@@ -2,11 +2,11 @@
 
 from charmhelpers.core import hookenv
 
-from lib.hook import Hook
+from lib.action import Action
 from lib.paths import LSCTL
 
 
-class PauseAction(Hook):
+class PauseAction(Action):
     """Execute pause action logic."""
 
     def __init__(self, hookenv=hookenv, subprocess=subprocess):

=== modified file 'lib/resume.py'
--- lib/resume.py	2015-06-01 10:46:39 +0000
+++ lib/resume.py	2015-06-05 12:02:41 +0000
@@ -2,11 +2,11 @@
 
 from charmhelpers.core import hookenv
 
-from lib.hook import MaintenanceHook
+from lib.action import MaintenanceAction
 from lib.paths import default_paths, LSCTL
 
 
-class ResumeAction(MaintenanceHook):
+class ResumeAction(MaintenanceAction):
     """Resume all Landscape services on the unit."""
 
     def __init__(self, hookenv=hookenv, paths=default_paths,

=== modified file 'lib/tests/stubs.py'
--- lib/tests/stubs.py	2015-06-03 12:30:33 +0000
+++ lib/tests/stubs.py	2015-06-05 12:02:41 +0000
@@ -17,7 +17,8 @@
         self.relations = {}
         self._config = Config()
         self._charm_dir = charm_dir
-        self._action_fails = []
+        self.action_fails = []
+        self.action_sets = []
 
     def config(self):
         return self._config
@@ -66,12 +67,15 @@
     def charm_dir(self):
         return self._charm_dir
 
-    def action_fail(self, message):
-        self._action_fails.append(message)
-
     def is_leader(self):
         return self.leader
 
+    def action_fail(self, message):
+        self.action_fails.append(message)
+
+    def action_set(self, values):
+        self.action_sets.append(values)
+
 
 class FetchStub(object):
     """Provide a testable stub for C{charmhelpers.fetch}."""

=== added file 'lib/tests/test_action.py'
--- lib/tests/test_action.py	1970-01-01 00:00:00 +0000
+++ lib/tests/test_action.py	2015-06-05 12:02:41 +0000
@@ -0,0 +1,93 @@
+import os
+
+from lib.action import Action, MaintenanceAction
+from lib.error import CharmError
+
+from lib.tests.helpers import HookenvTest
+from lib.tests.rootdir import RootDir
+
+
+class DummyAction(Action):
+    executed = False
+
+    def _run(self):
+        self.executed = True
+
+
+class DummyActionWithValues(Action):
+    def _run(self):
+        return {"key": "value"}
+
+
+class DummyErrorAction(Action):
+    def _run(self):
+        raise CharmError("no go")
+
+
+class ActionTest(HookenvTest):
+
+    def setUp(self):
+        super(ActionTest, self).setUp()
+        self.root_dir = self.useFixture(RootDir())
+        self.paths = self.root_dir.paths
+
+    def test_run(self):
+        """Calling an action executes the _run method."""
+        action = DummyAction(hookenv=self.hookenv)
+        action()
+        self.assertTrue(action.executed)
+
+    def test_run_with_return_values(self):
+        """If _run returns values, they are set with action_set()."""
+        action = DummyActionWithValues(hookenv=self.hookenv)
+        action()
+        self.assertEqual([{"key": "value"}], self.hookenv.action_sets)
+
+    def test_run_raises_error(self):
+        """
+        If action fails with a CharmError, it is set as failed with the
+        exception message as the error message.
+        """
+        action = DummyErrorAction(hookenv=self.hookenv)
+        action()
+        self.assertEqual(["no go"], self.hookenv.action_fails)
+
+
+class DummyMaintenanceAction(MaintenanceAction):
+    executed = False
+
+    def _run(self):
+        self.executed = True
+
+
+class MaintenanceActionTest(HookenvTest):
+
+    def setUp(self):
+        super(MaintenanceActionTest, self).setUp()
+        self.root_dir = self.useFixture(RootDir())
+        self.paths = self.root_dir.paths
+
+    def test_run(self):
+        """Calling a dummy hook runs only with maintenance flag set."""
+
+        open(self.paths.maintenance_flag(), "w")
+        self.addCleanup(os.remove, self.paths.maintenance_flag())
+
+        action = DummyMaintenanceAction(
+            hookenv=self.hookenv, paths=self.paths)
+
+        action()
+        self.assertTrue(action.executed)
+
+    def test_run_without_maintenance_flag(self):
+        """
+        When maintenance flag file is absent, maintenance hooks are no-ops.
+        """
+        action = DummyMaintenanceAction(
+            hookenv=self.hookenv, paths=self.paths)
+
+        action()
+        self.assertFalse(action.executed)
+        self.assertEqual(
+            ["This action can only be called on a unit in paused state."],
+            self.hookenv.action_fails)

=== removed file 'lib/tests/test_hook.py'
--- lib/tests/test_hook.py	2015-06-02 07:06:30 +0000
+++ lib/tests/test_hook.py	1970-01-01 00:00:00 +0000
@@ -1,45 +0,0 @@
-import os
-
-from lib.tests.helpers import HookenvTest
-from lib.tests.rootdir import RootDir
-from lib.hook import MaintenanceHook
-
-
-class DummyMaintenanceHook(MaintenanceHook):
-    executed = False
-
-    def _run(self):
-        self.executed = True
-
-
-class MaintenanceHookTest(HookenvTest):
-
-    def setUp(self):
-        super(MaintenanceHookTest, self).setUp()
-        self.root_dir = self.useFixture(RootDir())
-        self.paths = self.root_dir.paths
-
-    def test_run(self):
-        """Calling a dummy hook runs only with maintenance flag set."""
-
-        open(self.paths.maintenance_flag(), "w")
-        self.addCleanup(os.remove, self.paths.maintenance_flag())
-
-        action = DummyMaintenanceHook(
-            hookenv=self.hookenv, paths=self.paths)
-
-        action()
-        self.assertTrue(action.executed)
-
-    def test_run_without_maintenance_flag(self):
-        """
-        When maintenance flag file is absent, maintenance hooks are no-ops.
-        """
-        action = DummyMaintenanceHook(
-            hookenv=self.hookenv, paths=self.paths)
-
-        action()
-        self.assertFalse(action.executed)
-        self.assertEqual(
-            ["This action can only be called on a unit in paused state."],
-            self.hookenv._action_fails)

=== modified file 'lib/upgrade.py'
--- lib/upgrade.py	2015-06-01 10:39:56 +0000
+++ lib/upgrade.py	2015-06-05 12:02:41 +0000
@@ -4,11 +4,11 @@
 from charmhelpers import fetch
 
 from lib.apt import Apt
-from lib.hook import MaintenanceHook
+from lib.action import MaintenanceAction
 from lib.paths import default_paths
 
 
-class UpgradeAction(MaintenanceHook):
+class UpgradeAction(MaintenanceAction):
     """Execute package upgrade action logic."""
 
     def __init__(self, hookenv=hookenv, fetch=fetch, paths=default_paths,