Merge lp:~bjornt/landscape-charm/workload-status into lp:~landscape/landscape-charm/trunk

[Björn Tillenius]

Initial go at adding workload status to the charm.

It currently sets the unit to active when the Landscape services are
running and maintenance when the are not running. It doesn't yet handle
when something is in an error state or when the unit is blocked.

The resume action is also changed to look at the workload status instead
of the maintenance flag, which solves the problem where non-leader units
couldn't be resumed.

Initial go at adding workload status to the charm.

It currently sets the unit to active when the Landscape services are
running and maintenance when the are not running. It doesn't yet handle
when something is in an error state or when the unit is blocked.

The resume action is also changed to look at the workload status instead
of the maintenance flag, which solves the problem where non-leader units
couldn't be resumed.

I also had to sync charmhelpers to be able to set workload status, which
required some test changes to cope with it.

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Fail
Revno: 351
Branch: lp:~bjornt/landscape-charm/workload-status
Jenkins: https://ci.lscape.net/job/latch-test/3044/

review: Needs Fixing (test results)

[Free Ekanayaka (free.ekanayaka)]

Could we back out the charmhelpers upgrade from this branch? As far as I'm concerned it can even be a trivial as long as unit/integration tests pass (I'm assuming there's not too much breakage in our own code, not sure it's the case).

review: Needs Information

[Björn Tillenius (bjornt)]

Sure, I've committed the charmhelpers change in trunk. I didn't do that before, since I couldn't run the integration tests. But I finally managed to work around the problem and managed to get a successful run.

[lp:~bjornt/landscape-charm/workload-status]

352. By Björn Tillenius on 2015-12-11

Merge trunk.

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 352
Branch: lp:~bjornt/landscape-charm/workload-status
Jenkins: https://ci.lscape.net/job/latch-test/3055/

review: Approve (test results)

[lp:~bjornt/landscape-charm/workload-status]

353. By Björn Tillenius on 2015-12-14

Merge trunk.

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Fail
Revno: 353
Branch: lp:~bjornt/landscape-charm/workload-status
Jenkins: https://ci.lscape.net/job/latch-test/3057/

review: Needs Fixing (test results)

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Fail
Revno: 353
Branch: lp:~bjornt/landscape-charm/workload-status
Jenkins: https://ci.lscape.net/job/latch-test/3060/

review: Needs Fixing (test results)

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 353
Branch: lp:~bjornt/landscape-charm/workload-status
Jenkins: https://ci.lscape.net/job/latch-test/3061/

review: Approve (test results)

[Free Ekanayaka (free.ekanayaka)]

+1

Just a question about whether the final status set by the LSCtl callback can be anything else than "active" (see the comment below).

review: Approve

[lp:~bjornt/landscape-charm/workload-status]

354. By Björn Tillenius on 2015-12-15

Rename final_status to current_status.

355. By Björn Tillenius on 2015-12-15

Rename 'valid_status' to 'required_status'.

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Fail
Revno: 355
Branch: lp:~bjornt/landscape-charm/workload-status
Jenkins: https://ci.lscape.net/job/latch-test/3070/

review: Needs Fixing (test results)

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Fail
Revno: 355
Branch: lp:~bjornt/landscape-charm/workload-status
Jenkins: https://ci.lscape.net/job/latch-test/3071/

review: Needs Fixing (test results)

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 355
Branch: lp:~bjornt/landscape-charm/workload-status
Jenkins: https://ci.lscape.net/job/latch-test/3072/

review: Approve (test results)

[Chris Glass (tribaal)]

Looks good! +1

review: Approve

=== modified file 'lib/action.py'
--- lib/action.py	2015-06-05 12:00:05 +0000
+++ lib/action.py	2015-12-15 12:32:20 +0000
@@ -1,5 +1,3 @@
-import os.path
-
 from charmhelpers.core import hookenv
 
 from lib.error import CharmError
@@ -8,7 +6,13 @@
 
 
 class Action(Hook):
-    """Juju action abstraction, providing dependency injection for testing."""
+    """Juju action abstraction, providing dependency injection for testing.
+
+    @ivar required_status: The status the unit has to be in when the action
+        is executed.
+    """
+
+    required_status = None
 
     def __call__(self):
         """
@@ -17,6 +21,13 @@
         If _run() returns a value, set it using action_set().
         If _run() throws a CharmError, fail using action.fail().
         """
+        if self.required_status is not None:
+            status, _ = self._hookenv.status_get()
+            if status != self.required_status:
+                self._hookenv.action_fail(
+                    "This action can only be called on a unit in status "
+                    "'{}'.".format(self.required_status))
+                return
         self._hookenv.log("Running action %s" % type(self).__name__)
         try:
             return_values = self._run()
@@ -29,6 +40,8 @@
 class MaintenanceAction(Action):
     """Action that only runs when in maintenance mode."""
 
+    required_status = "maintenance"
+
     def __init__(self, hookenv=hookenv, paths=default_paths):
         """
         @param hookenv: The charm-helpers C{hookenv} module, will be replaced
@@ -37,14 +50,3 @@
         """
         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/callbacks/scripts.py'
--- lib/callbacks/scripts.py	2015-09-29 10:26:00 +0000
+++ lib/callbacks/scripts.py	2015-12-15 12:32:20 +0000
@@ -69,6 +69,8 @@
         self._hookenv = hookenv
 
     def __call__(self, manager, service_name, event_name):
+        current_status, current_status_message = self._hookenv.status_get()
+        action_status_message = ""
         action = event_name
         if event_name == "start":
             # XXX the 'start' event in the services framework is called after
@@ -89,8 +91,18 @@
             elif hook_name == "db-relation-changed":
                 if not self._need_restart_db_relation_changed(db_new, db_old):
                     return
+        if action == "restart":
+            action_status_message = "Restarting services."
+            if current_status == "unknown":
+                # If the status is unknown, it means that the services
+                # have not been started yet.
+                action_status_message = "Starting services."
+            if current_status == "maintenance":
+                return
 
+        self._hookenv.status_set("maintenance", action_status_message)
         self._run(LSCTL, (action,))
+        self._hookenv.status_set("active", "")
 
     def _need_restart_config_changed(self):
         """Check whether we need to restart after a config change.
 
=== modified file 'lib/callbacks/tests/test_scripts.py'
--- lib/callbacks/tests/test_scripts.py	2015-09-29 10:26:00 +0000
+++ lib/callbacks/tests/test_scripts.py	2015-12-15 12:32:20 +0000
@@ -93,6 +93,53 @@
         self.assertEqual(
             ["/usr/bin/lsctl", "restart"], self.subprocess.calls[0][0])
 
+    def test_start_unknown_status(self):
+        """
+        If the 'lsctl' script is invoked with the 'restart' action when
+        the workload status is 'unknown', the status while restarting
+        the services will way 'starting services' and the final status
+        will be 'active'.
+        """
+        self.callback(self.manager, "landscape", "start")
+        self.assertEqual(("active", ""), self.hookenv.status_get())
+        self.assertEqual(
+            [{"status": "unknown", "message": ""},
+             {"status": "maintenance", "message": "Starting services."},
+             {"status": "active", "message": ""}],
+            self.hookenv.statuses)
+
+    def test_start_active_status(self):
+        """
+        If the 'lsctl' script is invoked with the 'restart' action when
+        the workload status is 'active', the status while restarting
+        the services will way 'restarting services' and the final status
+        will be 'active' with no status message.
+        """
+        self.hookenv.statuses = [{"status": "active", "message": "Something."}]
+        self.callback(self.manager, "landscape", "start")
+        self.assertEqual(("active", ""), self.hookenv.status_get())
+        self.assertEqual(
+            [{"status": "active", "message": "Something."},
+             {"status": "maintenance", "message": "Restarting services."},
+             {"status": "active", "message": ""}],
+            self.hookenv.statuses)
+
+    def test_start_maintenance_status(self):
+        """
+        If the 'lsctl' script is invoked with the 'restart' action when
+        the workload status is 'maintenance', the services won't be
+        restarted and the workload status won't be changed.
+        """
+        self.hookenv.statuses = [
+            {"status": "maintenance", "message": "Doing maintenance."}]
+        self.callback(self.manager, "landscape", "start")
+        self.assertEqual(
+            ("maintenance", "Doing maintenance."), self.hookenv.status_get())
+        self.assertEqual(
+            [{"status": "maintenance", "message": "Doing maintenance."}],
+            self.hookenv.statuses)
+        self.assertEqual([], self.subprocess.calls)
+
     def test_stop(self):
         """
         The 'lsctl' script is invoked with the 'stop' action if the event name
 
=== modified file 'lib/pause.py'
--- lib/pause.py	2015-06-03 08:55:54 +0000
+++ lib/pause.py	2015-12-15 12:32:20 +0000
@@ -14,4 +14,6 @@
         self._subprocess = subprocess
 
     def _run(self):
+        self._hookenv.status_set("maintenance", "Stopping services.")
         self._subprocess.check_call((LSCTL, "stop"))
+        self._hookenv.status_set("maintenance", "Services stopped.")
 
=== modified file 'lib/resume.py'
--- lib/resume.py	2015-06-16 07:51:03 +0000
+++ lib/resume.py	2015-12-15 12:32:20 +0000
@@ -25,9 +25,15 @@
         self._subprocess = subprocess
 
     def _run(self):
+        self._hookenv.status_set("maintenance", "Starting services.")
         start_output = self._subprocess.check_output((LSCTL, "start"))
         try:
             self._subprocess.check_output((LSCTL, "status"))
         except subprocess.CalledProcessError as status_error:
+            self._hookenv.status_set("maintenance", "Stopping services.")
             self._subprocess.call((LSCTL, "stop"))
+            self._hookenv.status_set(
+                "maintenance", "Services failed to start.")
             raise ProcessesNotStartedError(start_output, status_error.output)
+        else:
+            self._hookenv.status_set("active", "")
 
=== modified file 'lib/tests/stubs.py'
--- lib/tests/stubs.py	2015-11-03 15:20:36 +0000
+++ lib/tests/stubs.py	2015-12-15 12:32:20 +0000
@@ -17,6 +17,7 @@
         self.relations = {}
         self.action_fails = []
         self.action_sets = []
+        self.statuses = [{"status": "unknown", "message": ""}]
 
         # We should disable implicit saving since it runs at charm exit using
         # globals :(
@@ -95,6 +96,13 @@
         self.action_gets.append(key)
         return "%s-value" % key
 
+    def status_get(self):
+        current_status = self.statuses[-1]
+        return current_status["status"], current_status["message"]
+
+    def status_set(self, workload_state, message):
+        self.statuses.append({"status": workload_state, "message": message})
+
 
 class FetchStub(object):
     """Provide a testable stub for C{charmhelpers.fetch}."""
 
=== modified file 'lib/tests/test_action.py'
--- lib/tests/test_action.py	2015-06-05 12:02:34 +0000
+++ lib/tests/test_action.py	2015-12-15 12:32:20 +0000
@@ -1,5 +1,3 @@
-import os
-
 from lib.action import Action, MaintenanceAction
 from lib.error import CharmError
 
@@ -52,6 +50,31 @@
         action()
         self.assertEqual(["no go"], self.hookenv.action_fails)
 
+    def test_run_required_status(self):
+        """
+        If required_status is set, the action will be executed if the
+        current status is the same.
+        """
+        action = DummyAction(hookenv=self.hookenv)
+        action.required_status = "active"
+        self.hookenv.status_set("active", "")
+        action()
+        self.assertTrue(action.executed)
+
+    def test_run_invalid_status(self):
+        """
+        If required_status is set, the action won't be executed if the
+        current status is different.
+        """
+        action = DummyAction(hookenv=self.hookenv)
+        action.required_status = "active"
+        self.hookenv.status_set("maintenance", "")
+        action()
+        self.assertFalse(action.executed)
+        self.assertEqual(
+            ["This action can only be called on a unit in status 'active'."],
+            self.hookenv.action_fails)
+
 
 class DummyMaintenanceAction(MaintenanceAction):
     executed = False
@@ -69,25 +92,25 @@
 
     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())
-
+        self.hookenv.status_set("maintenance", "")
         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.
-        """
+    def test_run_without_maintenance_status(self):
+        """
+        If the workload status isn't 'maintenance', the maintenance
+        action won't be executed.
+        """
+        self.hookenv.status_set("active", "")
         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."],
+            ["This action can only be called on a unit in status "
+             "'maintenance'."],
             self.hookenv.action_fails)
 
=== modified file 'lib/tests/test_migrate_schema.py'
--- lib/tests/test_migrate_schema.py	2015-06-01 10:39:56 +0000
+++ lib/tests/test_migrate_schema.py	2015-12-15 12:32:20 +0000
@@ -1,5 +1,3 @@
-import os
-
 from lib.migrate_schema import MigrateSchemaAction
 from lib.paths import SCHEMA_SCRIPT
 from lib.tests.helpers import HookenvTest
@@ -20,8 +18,7 @@
         """
         The MigrateSchemaAction calls the schema script.
         """
-        open(self.paths.maintenance_flag(), "w")
-        self.addCleanup(os.remove, self.paths.maintenance_flag())
+        self.hookenv.status_set("maintenance", "")
 
         action = MigrateSchemaAction(
             hookenv=self.hookenv, paths=self.paths, subprocess=self.subprocess)
@@ -30,8 +27,11 @@
 
     def test_run_without_maintenance_flag(self):
         """
-        The MigrateSchemaAction calls the schema script.
+        The MigrateSchemaAction doesn't call the schema script if the
+        unit is in an 'active' state.
         """
+        self.hookenv.status_set("active", "")
+
         action = MigrateSchemaAction(
             hookenv=self.hookenv, paths=self.paths, subprocess=self.subprocess)
         action()
 
=== modified file 'lib/tests/test_pause.py'
--- lib/tests/test_pause.py	2015-06-01 08:57:04 +0000
+++ lib/tests/test_pause.py	2015-12-15 12:32:20 +0000
@@ -21,3 +21,17 @@
         self.action()
         self.assertEqual(
             [(("/usr/bin/lsctl", "stop"), {})], self.subprocess.calls)
+
+    def test_run_status(self):
+        """
+        The workload status is changed to 'maintenance' while stopping
+        the services and after the services have been stopped.
+        """
+        self.action()
+        self.assertEqual(
+            ("maintenance", "Services stopped."), self.hookenv.status_get())
+        self.assertEqual(
+            [{"status": "unknown", "message": ""},
+             {"status": "maintenance", "message": "Stopping services."},
+             {"status": "maintenance", "message": "Services stopped."}],
+            self.hookenv.statuses)
 
=== modified file 'lib/tests/test_resume.py'
--- lib/tests/test_resume.py	2015-06-16 07:51:03 +0000
+++ lib/tests/test_resume.py	2015-12-15 12:32:20 +0000
@@ -1,5 +1,3 @@
-import os
-
 from lib.tests.helpers import HookenvTest
 from lib.tests.rootdir import RootDir
 from lib.tests.stubs import SubprocessStub
@@ -23,8 +21,7 @@
         enabling the cron service so that Landscape cron jobs can start
         running again.
         """
-        open(self.paths.maintenance_flag(), "w")
-        self.addCleanup(os.remove, self.paths.maintenance_flag())
+        self.hookenv.status_set("maintenance", "")
 
         action = ResumeAction(
             hookenv=self.hookenv, subprocess=self.subprocess, paths=self.paths)
@@ -34,10 +31,29 @@
              (("/usr/bin/lsctl", "status"), {})],
             self.subprocess.calls)
 
+    def test_run_status(self):
+        """
+        The status is set to 'maintenance' while starting the services
+        and is set to 'active' after the services have been started.
+        """
+        self.hookenv.status_set("maintenance", "")
+
+        action = ResumeAction(
+            hookenv=self.hookenv, subprocess=self.subprocess, paths=self.paths)
+        action()
+        self.assertEqual(("active", ""), self.hookenv.status_get())
+        self.assertEqual(
+            [{"status": "unknown", "message": ""},
+             {"status": "maintenance", "message": ""},
+             {"status": "maintenance", "message": "Starting services."},
+             {"status": "active", "message": ""}],
+            self.hookenv.statuses)
+
     def test_run_without_maintenance_flag(self):
         """
         When no maintenance flag file is present, resume action is a no-op.
         """
+        self.hookenv.status_set("active", "")
         action = ResumeAction(
             hookenv=self.hookenv, subprocess=self.subprocess, paths=self.paths)
         action()
@@ -53,17 +69,58 @@
         The unit is stopped again, to ensure that the 'resume' action
         can run again after the problems have been addressed.
         """
-        open(self.paths.maintenance_flag(), "w")
-        self.addCleanup(os.remove, self.paths.maintenance_flag())
-        self.subprocess.add_fake_executable(
-            LSCTL, args=["start"], stdout="start output")
-        self.subprocess.add_fake_executable(
-            LSCTL, args=["status"], stdout="status failure", return_code=3)
-        self.subprocess.add_fake_executable(LSCTL, args=["stop"])
-
-        action = ResumeAction(
-            hookenv=self.hookenv, subprocess=self.subprocess, paths=self.paths)
-        action()
+        self.hookenv.status_set("maintenance", "")
+
+        self.subprocess.add_fake_executable(
+            LSCTL, args=["start"], stdout="start output")
+        self.subprocess.add_fake_executable(
+            LSCTL, args=["status"], stdout="status failure", return_code=3)
+        self.subprocess.add_fake_executable(LSCTL, args=["stop"])
+
+        action = ResumeAction(
+            hookenv=self.hookenv, subprocess=self.subprocess, paths=self.paths)
+        action()
+        self.assertEqual(
+            ["Some services failed to start.\n\nstart output\n\n"
+             "status failure"],
+            self.hookenv.action_fails)
+        self.assertEqual(
+            [(("/usr/bin/lsctl", "start"), {}),
+             (("/usr/bin/lsctl", "status"), {}),
+             (("/usr/bin/lsctl", "stop"), {})],
+            self.subprocess.calls)
+
+    def test_run_fail_status(self):
+        """
+        If the action fails, the final status is put back to
+        maintenance, with a message saying that services failed to
+        start.
+
+        XXX: The final status probably should be 'error', but it's not
+             clear on how to recover from that.
+        """
+        self.hookenv.status_set("maintenance", "")
+
+        self.subprocess.add_fake_executable(
+            LSCTL, args=["start"], stdout="start output")
+        self.subprocess.add_fake_executable(
+            LSCTL, args=["status"], stdout="status failure", return_code=3)
+        self.subprocess.add_fake_executable(LSCTL, args=["stop"])
+
+        action = ResumeAction(
+            hookenv=self.hookenv, subprocess=self.subprocess, paths=self.paths)
+        action()
+        self.assertEqual(
+            ("maintenance", "Services failed to start."),
+            self.hookenv.status_get())
+        self.assertEqual(
+            [{"status": "unknown", "message": ""},
+             {"status": "maintenance", "message": ""},
+             {"status": "maintenance", "message": "Starting services."},
+             {"status": "maintenance", "message": "Stopping services."},
+             {"status": "maintenance",
+              "message": "Services failed to start."}],
+            self.hookenv.statuses)
         self.assertEqual(
             ["Some services failed to start.\n\nstart output\n\n"
              "status failure"],
 
=== modified file 'lib/tests/test_upgrade.py'
--- lib/tests/test_upgrade.py	2015-11-03 13:58:50 +0000
+++ lib/tests/test_upgrade.py	2015-12-15 12:32:20 +0000
@@ -1,5 +1,3 @@
-import os
-
 from lib.tests.helpers import HookenvTest
 from lib.tests.rootdir import RootDir
 from lib.tests.stubs import FetchStub
@@ -19,9 +17,7 @@
         The UpgradeAction refreshes package indexes and upgrades
         landscape-server package.
         """
-
-        open(self.paths.maintenance_flag(), "w")
-        self.addCleanup(os.remove, self.paths.maintenance_flag())
+        self.hookenv.status_set("maintenance", "")
 
         self.hookenv.config()["source"] = "ppa:my-ppa"
         action = UpgradeAction(
@@ -41,8 +37,10 @@
 
     def test_run_without_maintenance_flag(self):
         """
-        When maintenance flag file is absent, upgrade action is a no-op.
+        If the unit is not in the 'maintenance' state, the upgrade
+        action is a no-op.
         """
+        self.hookenv.status_set("active", "")
 
         action = UpgradeAction(
             hookenv=self.hookenv, fetch=self.fetch, paths=self.paths)
 
=== modified file 'tests/basic/test_actions.py'
--- tests/basic/test_actions.py	2015-07-09 11:17:22 +0000
+++ tests/basic/test_actions.py	2015-12-15 12:32:20 +0000
@@ -3,7 +3,7 @@
 """
 
 from helpers import IntegrationTest
-from layers import OneLandscapeUnitLayer
+from layers import OneLandscapeUnitLayer, TwoLandscapeUnitsLayer
 
 
 class ActionsTest(IntegrationTest):
@@ -60,3 +60,22 @@
 
         # Logging in should now work.
         self.environment.login("foo@bar", "bar")
+
+
+class ActionsMultipleUnitsTest(IntegrationTest):
+
+    layer = TwoLandscapeUnitsLayer
+
+    def setUp(self):
+        super(ActionsMultipleUnitsTest, self).setUp()
+        [self.non_leader] = self.layer.non_leaders
+
+    def test_non_leader_pause_resume(self):
+        """
+        The non-leader unit can be paused and later resumed.
+        """
+        result = self.environment.pause_landscape(unit=self.non_leader)
+        self.assertEqual("completed", result["status"])
+
+        result = self.environment.resume_landscape(unit=self.non_leader)
+        self.assertEqual("completed", result["status"])