launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/virsh-power into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/virsh-power/+merge/108110

This adds a template for the “virsh” power method.  The shell script is nontrivial, so I did a bit of refactoring to make it at least very slightly testable.

I don't see any sensible way (without providing a full-blown fake virsh command) to get the script succeeding in tests, at least without actually managing a virtual machine.  But I can get it to fail in a predictable way, with a sensible error, after exercising practically all of its code.  And so that's what I did, after a quick chat with Julian about it.  He points out that proper unit-testing is not appropriate because the power scripts are meant to be customizable anyway.

Given the problems in running successful virsh commands, I removed a test that used to do that with the old no-op stub script.  The test was no longer needed at any rate, since the wake-on-LAN power type no longer has the special case that warranted separate tests for wake-on-LAN and “other.”  We do test each of the power methods individually to some extent.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/virsh-power/+merge/108110
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/virsh-power into lp:maas.

=== modified file 'src/maasserver/models/__init__.py'
--- src/maasserver/models/__init__.py	2012-05-30 12:24:57 +0000
+++ src/maasserver/models/__init__.py	2012-05-31 07:31:22 +0000
@@ -602,6 +602,8 @@
             power_params = {}
 
         power_params.setdefault('system_id', self.system_id)
+        power_params.setdefault('virsh', '/usr/bin/virsh')
+
         # The "mac" parameter defaults to the node's primary MAC
         # address, but only if no power parameters were set at all.
         if not self.power_parameters:

=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py	2012-05-30 12:24:57 +0000
+++ src/maasserver/tests/test_models.py	2012-05-31 07:31:22 +0000
@@ -72,6 +72,7 @@
     Token,
     )
 from provisioningserver.enum import POWER_TYPE
+from provisioningserver.power.poweraction import PowerAction
 from testtools.matchers import FileContains
 
 
@@ -316,6 +317,32 @@
         node.add_mac_address(mac)
         self.assertNotIn('mac', node.get_effective_power_parameters())
 
+    def test_get_effective_power_parameters_provides_usable_defaults(self):
+        # For some power types at least, the defaults provided by
+        # get_effective_power_parameters is enough to get a basic setup
+        # working.
+        configless_power_types = [
+            POWER_TYPE.WAKE_ON_LAN,
+            POWER_TYPE.VIRSH,
+            ]
+        # We don't actually want to fire off power events, but we'll go
+        # through the motions right up to the point where we'd normally
+        # run shell commands.
+        self.patch(PowerAction, 'run_shell', lambda *args, **kwargs: ('', ''))
+        user = factory.make_admin()
+        nodes = [
+            factory.make_node(power_type=power_type)
+            for power_type in configless_power_types]
+        for node in nodes:
+            node.add_mac_address(factory.getRandomMACAddress())
+        node_power_types = {
+            node: node.get_effective_power_type()
+            for node in nodes}
+        started_nodes = Node.objects.start_nodes(
+            list(node_power_types.keys()), user)
+        successful_types = [node_power_types[node] for node in started_nodes]
+        self.assertItemsEqual(configless_power_types, successful_types)
+
     def test_acquire(self):
         node = factory.make_node(status=NODE_STATUS.READY)
         user = factory.make_user()
@@ -811,31 +838,6 @@
         self.assertItemsEqual([], output)
         self.assertEqual([], fixture.tasks)
 
-    def test_start_nodes_other_power_type(self):
-        # wakeonlan tests, above, are a special case. Test another type
-        # here that exclusively uses power_parameters from the node.
-        fixture = self.useFixture(CeleryFixture())
-        user = factory.make_user()
-        params = dict(
-            driver="qemu",
-            address="localhost",
-            user="nobody")
-        node, mac = self.make_node_with_mac(
-            user, power_type=POWER_TYPE.VIRSH, power_parameters=params)
-        output = Node.objects.start_nodes([node.system_id], user)
-
-        self.assertItemsEqual([node], output)
-        self.assertEqual(
-            (1, 'provisioningserver.tasks.power_on',
-                "qemu", "localhost", "nobody"),
-            (
-                len(fixture.tasks),
-                fixture.tasks[0]['task'].name,
-                fixture.tasks[0]['kwargs']['driver'],
-                fixture.tasks[0]['kwargs']['address'],
-                fixture.tasks[0]['kwargs']['user'],
-            ))
-
     def test_start_nodes_ignores_nodes_without_mac(self):
         user = factory.make_user()
         node = self.make_node(user)

=== modified file 'src/provisioningserver/power/poweraction.py'
--- src/provisioningserver/power/poweraction.py	2012-05-29 06:13:48 +0000
+++ src/provisioningserver/power/poweraction.py	2012-05-31 07:31:22 +0000
@@ -62,22 +62,19 @@
                 "Template is missing at least the %s parameter." % e.message)
         return rendered
 
-    def execute(self, **kwargs):
-        """Execute the template.
+    def run_shell(self, commands):
+        """Execute raw shell script (as rendered from a template).
 
-        Any supplied parameters will be passed to the template as substitution
-        values.
+        :param commands: String containing shell script.
+        :return: Tuple of strings: stdout, stderr.
         """
-        template = self.get_template()
-        rendered = self.render_template(template, **kwargs)
-
         # This might need retrying but it could be better to leave that
         # to the individual scripts.
         try:
             proc = subprocess.Popen(
-                rendered, shell=True, stdout=subprocess.PIPE,
+                commands, shell=True, stdout=subprocess.PIPE,
                 stderr=subprocess.PIPE, close_fds=True)
-        except OSError, e:
+        except OSError as e:
             raise PowerActionFail(e)
 
         stdout, stderr = proc.communicate()
@@ -86,3 +83,14 @@
         if code != 0:
             raise PowerActionFail("%s failed with return code %s" % (
                 self.power_type, code))
+        return stdout, stderr
+
+    def execute(self, **kwargs):
+        """Execute the template.
+
+        Any supplied parameters will be passed to the template as substitution
+        values.
+        """
+        template = self.get_template()
+        rendered = self.render_template(template, **kwargs)
+        self.run_shell(rendered)

=== modified file 'src/provisioningserver/power/templates/virsh.template'
--- src/provisioningserver/power/templates/virsh.template	2012-05-24 07:04:56 +0000
+++ src/provisioningserver/power/templates/virsh.template	2012-05-31 07:31:22 +0000
@@ -1,1 +1,49 @@
-# FIXME: Doing nothing yet.
+# Control virtual system's "power" through virsh.
+
+# Parameters.
+power_change="%(power_change)s"
+virsh_url="%(virsh_url)s"
+system_id="%(system_id)s"
+virsh="%(virsh)s"
+
+
+# Choose command for virsh to make the requested power change happen.
+formulate_power_command() {
+  if [ ${power_change} = 'on' ]
+  then
+      echo 'start'
+  else
+      echo 'destroy'
+  fi
+}
+
+
+# Express system's current state as expressed by virsh as "on" or "off".
+formulate_power_state() {
+    case $1 in
+    'running') echo 'on' ;;
+    'shut off') echo 'off' ;;
+    *)
+        echo "Got unknown power state from virsh: '$1'" >&2
+        exit 1
+    esac
+}
+
+
+# Issue command to virsh, for the given system.
+issue_virsh_command() {
+    ${virsh} --connect ${virsh_url}/system $1 ${system_id}
+}
+
+
+# Get the given system's power state: 'on' or 'off'.
+get_power_state() {
+    virsh_state=$(issue_virsh_command domstate)
+    formulate_power_state ${virsh_state}
+}
+
+
+if [ "$(get_power_state)" != "${power_change}" ]
+then
+    issue_virsh_command $(formulate_power_command)
+fi

=== modified file 'src/provisioningserver/power/tests/test_poweraction.py'
--- src/provisioningserver/power/tests/test_poweraction.py	2012-05-29 06:13:48 +0000
+++ src/provisioningserver/power/tests/test_poweraction.py	2012-05-31 07:31:22 +0000
@@ -110,3 +110,17 @@
         self.assertRaises(
             PowerActionFail,
             pa.execute, power_change='off', mac=factory.getRandomMACAddress())
+
+    def test_virsh_checks_vm_state(self):
+        # We can't test the virsh template in detail (and it may be
+        # customized), but by making it use "echo" instead of a real
+        # virsh we can make it get a bogus answer from its status check.
+        # The bogus answer is actually the rest of the virsh command
+        # line.  It will complain about this and fail.
+        action = PowerAction(POWER_TYPE.VIRSH)
+        script = action.render_template(
+            action.get_template(), power_change='on',
+            virsh_url='qemu://example.com/', system_id='mysystem',
+            virsh='echo')
+        stdout, stderr = action.run_shell(script)
+        self.assertIn("Got unknown power state from virsh", stderr)