launchpad-reviewers team mailing list archive

[Martin Packman <martin.packman@xxxxxxxxxxxxx>]

Martin Packman has proposed merging lp:~gz/maas/detailed_poweractionfail_1155175 into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1155175 in MAAS: "power management failures don't report useful error messages"
  https://bugs.launchpad.net/maas/+bug/1155175

For more details, see:
https://code.launchpad.net/~gz/maas/detailed_poweractionfail_1155175/+merge/153425

Give more detailed error message on failures from power management scripts.

This changes how the scripts are called a little to take advantage of nicer wrappers included in Python 2.7, and customises the PowerActionFail exception type to stringify with more details.

How exactly we want the output shown is an open question, currently it's just on (at least one) new line in the error string, perhaps indenting or otherwise wrapping it would be preferable.

Docstring update was needed, part of it (taking **kwargs) is just wrong, and the return is entirely ignored in real code so should probably not be promised.

Some testing specifics were fiddled with, just to make a couple of changes slightly neater.
--
-- 
https://code.launchpad.net/~gz/maas/detailed_poweractionfail_1155175/+merge/153425
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gz/maas/detailed_poweractionfail_1155175 into lp:maas.

=== modified file 'src/provisioningserver/power/poweraction.py'
--- src/provisioningserver/power/poweraction.py	2012-10-05 16:33:37 +0000
+++ src/provisioningserver/power/poweraction.py	2013-03-14 17:59:22 +0000
@@ -31,6 +31,16 @@
 class PowerActionFail(Exception):
     """Raised when there's a problem executing a power script."""
 
+    def __init__(self, power_action, err):
+        self.power_action = power_action
+        self.err = err
+
+    def __str__(self):
+        message = "%s failed: %s" % (self.power_action.power_type, self.err)
+        if isinstance(self.err, subprocess.CalledProcessError) and self.err.output:
+            message += ":\n" + self.err.output.strip()
+        return message
+
 
 def get_power_templates_dir():
     """Get the power-templates directory from the config."""
@@ -99,32 +109,23 @@
             kwargs.update(self.get_extra_context())
             return template.substitute(kwargs)
         except NameError as error:
-            raise PowerActionFail(*error.args)
+            raise PowerActionFail(self, error)
 
     def run_shell(self, commands):
         """Execute raw shell script (as rendered from a template).
 
         :param commands: String containing shell script.
-        :param **kwargs: Keyword arguments are passed on to the template as
-            substitution values.
-        :return: Tuple of strings: stdout, stderr.
+        :raises: :class:`PowerActionFail`
         """
         # This might need retrying but it could be better to leave that
         # to the individual scripts.
         try:
-            proc = subprocess.Popen(
-                commands, shell=True, stdout=subprocess.PIPE,
-                stderr=subprocess.PIPE, close_fds=True)
-        except OSError as e:
-            raise PowerActionFail(e)
-
-        stdout, stderr = proc.communicate()
-        # TODO: log output on errors
-        code = proc.returncode
-        if code != 0:
-            raise PowerActionFail("%s failed with return code %s" % (
-                self.power_type, code))
-        return stdout, stderr
+            output = subprocess.check_output(
+                commands, shell=True, stderr=subprocess.STDOUT, close_fds=True)
+        except Exception as e:
+            raise PowerActionFail(self, e)
+        # This output is only examined in tests, execute just ignores it
+        return output
 
     def execute(self, **kwargs):
         """Execute the template.

=== modified file 'src/provisioningserver/power/tests/test_poweraction.py'
--- src/provisioningserver/power/tests/test_poweraction.py	2013-01-14 22:06:12 +0000
+++ src/provisioningserver/power/tests/test_poweraction.py	2013-03-14 17:59:22 +0000
@@ -29,7 +29,8 @@
 from provisioningserver.utils import ShellTemplate
 from testtools.matchers import (
     FileContains,
-    MatchesRegex,
+    MatchesException,
+    Raises,
     )
 
 
@@ -107,12 +108,10 @@
         pa = PowerAction(POWER_TYPE.WAKE_ON_LAN)
         template_name = factory.getRandomString()
         template = ShellTemplate("template: {{mac}}", name=template_name)
-        exception = self.assertRaises(
-            PowerActionFail, pa.render_template, template)
-        self.assertThat(
-            exception.message, MatchesRegex(
-                "name 'mac' is not defined at line \d+ column \d+ "
-                "in file %s" % re.escape(template_name)))
+        self.assertThat(lambda: pa.render_template(template),
+            Raises(MatchesException(PowerActionFail,
+                ".*name 'mac' is not defined at line \d+ column \d+ "
+                "in file %s" % re.escape(template_name))))
 
     def _create_template_file(self, template):
         """Create a temporary template file with the given contents."""
@@ -135,9 +134,14 @@
 
     def test_execute_raises_PowerActionFail_when_script_fails(self):
         path = self._create_template_file("this_is_not_valid_shell")
-        exception = self.assertRaises(PowerActionFail, self.run_action, path)
-        self.assertEqual(
-            "ether_wake failed with return code 127", exception.message)
+        self.assertThat(lambda: self.run_action(path),
+            Raises(MatchesException(PowerActionFail,
+                "ether_wake failed.* return.* 127")))
+
+    def test_execute_raises_PowerActionFail_with_output(self):
+        path = self._create_template_file("echo reason for failure; exit 1")
+        self.assertThat(lambda: self.run_action(path),
+            Raises(MatchesException(PowerActionFail, ".*:\nreason for failure")))
 
     def test_wake_on_lan_cannot_shut_down_node(self):
         pa = PowerAction(POWER_TYPE.WAKE_ON_LAN)
@@ -156,8 +160,8 @@
             action.get_template(), power_change='on',
             power_address='qemu://example.com/', system_id='mysystem',
             power_id='mysystem', username='me', virsh='echo')
-        stdout, stderr = action.run_shell(script)
-        self.assertIn("Got unknown power state from virsh", stderr)
+        output = action.run_shell(script)
+        self.assertIn("Got unknown power state from virsh", output)
 
     def test_fence_cdu_checks_state(self):
         # We can't test the fence_cdu template in detail (and it may be
@@ -170,8 +174,8 @@
             action.get_template(), power_change='on',
             power_address='mysystem', power_id='system',
             power_user='me', power_pass='me', fence_cdu='echo')
-        stdout, stderr = action.run_shell(script)
-        self.assertIn("Got unknown power state from fence_cdu", stderr)
+        output = action.run_shell(script)
+        self.assertIn("Got unknown power state from fence_cdu", output)
 
     def test_ipmi_checks_state(self):
         action = PowerAction(POWER_TYPE.IPMI)
@@ -180,8 +184,8 @@
             power_address='mystystem', power_user='me', power_pass='me',
             ipmipower='echo', ipmi_chassis_config='echo', config_dir='dir',
             ipmi_config='file.conf', power_driver='LAN')
-        stdout, stderr = action.run_shell(script)
-        self.assertIn("Got unknown power state from ipmipower", stderr)
+        output = action.run_shell(script)
+        self.assertIn("Got unknown power state from ipmipower", output)
 
     def configure_power_config_dir(self, path=None):
         """Configure POWER_CONFIG_DIR to `path`."""