Merge lp:~danilo/landscape-charm/maintenance-hook into lp:~landscape/landscape-charm/trunk

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

Make a generic MaintenanceHook which errors out if not in maintenance mode

Also moves UpgradeAction, MigrateSchemaAction and ResumeAction to the new hook.

Make a generic MaintenanceHook which errors out if not in maintenance mode

Also moves UpgradeAction, MigrateSchemaAction and ResumeAction to the new hook.

For testing, use landscape.yaml from https://pastebin.canonical.com/132332/ and deploy with

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

When playing around, note that if any of the 'upgrade', 'migrate-schema' or 'resume' are run when not in maintenance (iow, after 'pause' action), they should error out and no-op.

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 300
Branch: lp:~danilo/landscape-charm/maintenance-hook
Jenkins: https://ci.lscape.net/job/latch-test/1147/

review: Approve (test results)

[lp:~danilo/landscape-charm/maintenance-hook]

301. By Данило Шеган on 2015-06-01

Make error message more generic.

[Chris Glass (tribaal)]

Looks good! +1

review: Approve

[🤖 Landscape Builder (landscape-builder)]

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

review: Approve (test results)

[lp:~danilo/landscape-charm/maintenance-hook]

302. By Данило Шеган on 2015-06-01

Merged upgrade-only-in-maintenance into maintenance-hook.

[🤖 Landscape Builder (landscape-builder)]

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

review: Approve (test results)

[Free Ekanayaka (free.ekanayaka)]

I'd rather implement some state-tracking/state-machine code in the charm (in a similar way we currently do for leader info, e.g storing a file with the current state), without relying on the maintenance flag file, which is kind of an implementation detail of the server. Later down the road (with juju 1.24) we'll use unit status for this, but for now tracking the state at the charm level will get us closer to that.

Also, rather than exiting the hook with non-zero, we should code "action-fail":

https://jujucharms.com/docs/devel/authors-charm-actions#action-fail

(might need a change in charmhelpers to support the new actions executables).

[Benji York (benji)]

This branch looks good to me.

review: Approve

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

@Free, Bjorn: I'll move to action_fail (it's already there in core.hookenv.action_fail).

As for not using maintenance file, the entire goal of this branch is to move all dependencies on maint-file into a single place, which we can easily change (I was thinking of future move to juju unit status, tbh: I would only implement internal representation of status if we really need it, and I am not sure yet if we do, for eg. waiting for batch scripts to finish).

As for internal implementation details of the server, we already rely heavily on those (we expect "lsctl stop" to stop cron scripts—if that's not an implementation detail, I do not know what is :)). But I don't see this as a major issue with the charm, since it's heavily tied with the server code, and what is the public API for the server code is what we decide is the API.

Thanks for reviews, all. I'm testing with action_fail now and will land if that goes fine. As I said, the goal of this branch is to allow easier change to whatever new machinism of tracking state we switch to, and thus believe it's valuable regardless of the fact that the approach is not perfect today.

[lp:~danilo/landscape-charm/maintenance-hook]

303. By Данило Шеган on 2015-06-02

Switch to action_fail for MaintenanceHook.

[Björn Tillenius (bjornt)]

On Tue, Jun 02, 2015 at 07:02:10AM -0000, Данило Шеган wrote:
> @Free, Bjorn: I'll move to action_fail (it's already there in core.hookenv.action_fail).

Thanks.

> As for not using maintenance file, the entire goal of this branch is
> to move all dependencies on maint-file into a single place, which we can
> easily change (I was thinking of future move to juju unit status, tbh: I
> would only implement internal representation of status if we really need
> it, and I am not sure yet if we do, for eg. waiting for batch scripts to
> finish).

I think using the maintenance file is fine. Using it ensures that we
don't do anything bad if some other admin decided to start the servers
and removing the maintenance file. We should detect that situation
rather than ignore it.

When we have Juju unit status, we should check that as well, but I don't
see a reason not to check the maintenance file.

=== modified file 'lib/hook.py'
--- lib/hook.py	2015-01-28 11:53:18 +0000
+++ lib/hook.py	2015-06-02 07:06:39 +0000
@@ -1,6 +1,10 @@
+import os.path
+
 from charmhelpers.core import hookenv
 from charmhelpers.core.hookenv import ERROR
 
+from lib.paths import default_paths
+
 
 class HookError(Exception):
     """Raised by hooks when they want to fail.
@@ -35,3 +39,27 @@
     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 1
+        super(MaintenanceHook, self).__call__()

=== modified file 'lib/migrate_schema.py'
--- lib/migrate_schema.py	2015-05-21 07:31:32 +0000
+++ lib/migrate_schema.py	2015-06-02 07:06:39 +0000
@@ -2,15 +2,16 @@
 
 from charmhelpers.core import hookenv
 
-from lib.hook import Hook
-from lib.paths import SCHEMA_SCRIPT
-
-
-class MigrateSchemaAction(Hook):
+from lib.hook import MaintenanceHook
+from lib.paths import default_paths, SCHEMA_SCRIPT
+
+
+class MigrateSchemaAction(MaintenanceHook):
     """Execute schema upgrade action logic."""
 
-    def __init__(self, hookenv=hookenv, subprocess=subprocess):
-        super(MigrateSchemaAction, self).__init__(hookenv=hookenv)
+    def __init__(self, hookenv=hookenv, paths=default_paths,
+                 subprocess=subprocess):
+        super(MigrateSchemaAction, self).__init__(hookenv=hookenv, paths=paths)
         self._subprocess = subprocess
 
     def _run(self):

=== modified file 'lib/resume.py'
--- lib/resume.py	2015-05-29 09:25:24 +0000
+++ lib/resume.py	2015-06-02 07:06:39 +0000
@@ -2,15 +2,16 @@
 
 from charmhelpers.core import hookenv
 
-from lib.hook import Hook
-from lib.paths import LSCTL
-
-
-class ResumeAction(Hook):
+from lib.hook import MaintenanceHook
+from lib.paths import default_paths, LSCTL
+
+
+class ResumeAction(MaintenanceHook):
     """Resume all Landscape services on the unit."""
 
-    def __init__(self, hookenv=hookenv, subprocess=subprocess):
-        super(ResumeAction, self).__init__(hookenv=hookenv)
+    def __init__(self, hookenv=hookenv, paths=default_paths,
+                 subprocess=subprocess):
+        super(ResumeAction, self).__init__(hookenv=hookenv, paths=paths)
         self._subprocess = subprocess
 
     def _run(self):

=== modified file 'lib/tests/stubs.py'
--- lib/tests/stubs.py	2015-05-26 12:05:02 +0000
+++ lib/tests/stubs.py	2015-06-02 07:06:39 +0000
@@ -16,6 +16,7 @@
         self.relations = {}
         self._config = Config()
         self._charm_dir = charm_dir
+        self._action_fails = []
 
     def config(self):
         return self._config
@@ -64,6 +65,9 @@
     def charm_dir(self):
         return self._charm_dir
 
+    def action_fail(self, message):
+        self._action_fails.append(message)
+
 
 class FetchStub(object):
     """Provide a testable stub for C{charmhelpers.fetch}."""

=== added file 'lib/tests/test_hook.py'
--- lib/tests/test_hook.py	1970-01-01 00:00:00 +0000
+++ lib/tests/test_hook.py	2015-06-02 07:06:39 +0000
@@ -0,0 +1,45 @@
+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/tests/test_migrate_schema.py'
--- lib/tests/test_migrate_schema.py	2015-05-26 12:05:02 +0000
+++ lib/tests/test_migrate_schema.py	2015-06-02 07:06:39 +0000
@@ -1,7 +1,10 @@
+import os
+
+from lib.migrate_schema import MigrateSchemaAction
+from lib.paths import SCHEMA_SCRIPT
 from lib.tests.helpers import HookenvTest
-from lib.migrate_schema import MigrateSchemaAction
+from lib.tests.rootdir import RootDir
 from lib.tests.stubs import SubprocessStub
-from lib.paths import SCHEMA_SCRIPT
 
 
 class MigrateSchemaActionTest(HookenvTest):
@@ -10,12 +13,26 @@
         super(MigrateSchemaActionTest, self).setUp()
         self.subprocess = SubprocessStub()
         self.subprocess.add_fake_executable(SCHEMA_SCRIPT)
-        self.action = MigrateSchemaAction(
-            hookenv=self.hookenv, subprocess=self.subprocess)
+        self.root_dir = self.useFixture(RootDir())
+        self.paths = self.root_dir.paths
 
     def test_run(self):
         """
         The MigrateSchemaAction calls the schema script.
         """
-        self.action()
+        open(self.paths.maintenance_flag(), "w")
+        self.addCleanup(os.remove, self.paths.maintenance_flag())
+
+        action = MigrateSchemaAction(
+            hookenv=self.hookenv, paths=self.paths, subprocess=self.subprocess)
+        action()
         self.assertEqual([([SCHEMA_SCRIPT], {})], self.subprocess.calls)
+
+    def test_run_without_maintenance_flag(self):
+        """
+        The MigrateSchemaAction calls the schema script.
+        """
+        action = MigrateSchemaAction(
+            hookenv=self.hookenv, paths=self.paths, subprocess=self.subprocess)
+        action()
+        self.assertEqual([], self.subprocess.calls)

=== modified file 'lib/tests/test_resume.py'
--- lib/tests/test_resume.py	2015-06-01 08:57:04 +0000
+++ lib/tests/test_resume.py	2015-06-02 07:06:39 +0000
@@ -1,4 +1,7 @@
+import os
+
 from lib.tests.helpers import HookenvTest
+from lib.tests.rootdir import RootDir
 from lib.tests.stubs import SubprocessStub
 from lib.resume import ResumeAction
 from lib.paths import LSCTL
@@ -11,8 +14,8 @@
         self.subprocess = SubprocessStub()
         self.subprocess.add_fake_executable(LSCTL)
         self.subprocess.add_fake_executable("service")
-        self.action = ResumeAction(
-            hookenv=self.hookenv, subprocess=self.subprocess)
+        self.root_dir = self.useFixture(RootDir())
+        self.paths = self.root_dir.paths
 
     def test_run(self):
         """
@@ -20,6 +23,20 @@
         enabling the cron service so that Landscape cron jobs can start
         running again.
         """
-        self.action()
+        open(self.paths.maintenance_flag(), "w")
+        self.addCleanup(os.remove, self.paths.maintenance_flag())
+
+        action = ResumeAction(
+            hookenv=self.hookenv, subprocess=self.subprocess, paths=self.paths)
+        action()
         self.assertEqual(
             [(("/usr/bin/lsctl", "start"), {})], self.subprocess.calls)
+
+    def test_run_without_maintenance_flag(self):
+        """
+        When no maintenance flag file is present, resume action is a no-op.
+        """
+        action = ResumeAction(
+            hookenv=self.hookenv, subprocess=self.subprocess, paths=self.paths)
+        action()
+        self.assertEqual([], self.subprocess.calls)

=== modified file 'lib/upgrade.py'
--- lib/upgrade.py	2015-06-01 10:37:32 +0000
+++ lib/upgrade.py	2015-06-02 07:06:39 +0000
@@ -1,28 +1,23 @@
-import os.path
 import subprocess
 
 from charmhelpers.core import hookenv
 from charmhelpers import fetch
 
 from lib.apt import Apt
-from lib.hook import Hook, HookError
+from lib.hook import MaintenanceHook
 from lib.paths import default_paths
 
 
-class UpgradeAction(Hook):
+class UpgradeAction(MaintenanceHook):
     """Execute package upgrade action logic."""
 
     def __init__(self, hookenv=hookenv, fetch=fetch, paths=default_paths,
                  subprocess=subprocess):
-        super(UpgradeAction, self).__init__(hookenv=hookenv)
+        super(UpgradeAction, self).__init__(hookenv=hookenv, paths=paths)
         self._fetch = fetch
-        self._paths = paths
         self._subprocess = subprocess
 
     def _run(self):
-        if not os.path.exists(self._paths.maintenance_flag()):
-            raise HookError(
-                "Upgrade action can only be called on a unit in paused state.")
         apt_install_options = [
             # Ensure we keep the existing service.conf and
             # /etc/defaults/landscape-server configuration files