Merge lp:~allenap/maas/xml-as-bytes--bug-1570609 into lp:~maas-committers/maas/trunk

[Gavin Panella]

Use @typed to untangle bytes/str problems in the AMT driver.

This ensures that XML output from wsman is parsed as bytes directly from source without any intermediate conversions.

[Andres Rodriguez (andreserl)]

seems to be working for me. I just want to make sure Blake tests in his older NUC's that use amttool instead. So let's have blake test this branch too please!

review: Approve

[Mike Pontillo (mpontillo)]

Ah, I totally missed that you already did this branch. Nice job. I can test it quickly as I think my NUCs still use amttool.

[Mike Pontillo (mpontillo)]

I tested with AMT version 8.1.57 as follows:

In [1]: import provisioningserver.drivers.power.amt as amt
In [2]: driver = amt.AMTPowerDriver()
In [3]: driver.power_off(None, {'ip_address': '172.16.100.251', 'power_pass': 'xxx'})
In [4]: driver.power_on(None, {'ip_address': '172.16.100.251', 'power_pass': 'xxx', 'boot_mode': 'local'})
In [9]: driver.power_query(None, {'ip_address': '172.16.100.251', 'power_pass': 'xxx', 'boot_mode': 'local'})
Out[9]: 'on'

I visually verified that it worked.

review: Approve

=== modified file 'src/provisioningserver/drivers/power/amt.py'
--- src/provisioningserver/drivers/power/amt.py	2016-04-14 15:25:36 +0000
+++ src/provisioningserver/drivers/power/amt.py	2016-04-15 14:58:22 +0000
@@ -25,7 +25,10 @@
     PowerDriver,
     PowerFatalError,
 )
-from provisioningserver.utils import shell
+from provisioningserver.utils import (
+    shell,
+    typed,
+)
 
 
 REQUIRED_PACKAGES = [["amttool", "amtterm"], ["wsman", "wsmancli"]]
@@ -44,7 +47,8 @@
                 missing_packages.append(package)
         return missing_packages
 
-    def _render_wsman_state_xml(self, power_change):
+    @typed
+    def _render_wsman_state_xml(self, power_change) -> bytes:
         """Render wsman state XML."""
         wsman_state_filename = join(dirname(__file__), "amt.wsman-state.xml")
         wsman_state_ns = {
@@ -59,7 +63,8 @@
         ps.text = power_states[power_change]
         return etree.tostring(tree)
 
-    def _parse_multiple_xml_docs(self, xml):
+    @typed
+    def _parse_multiple_xml_docs(self, xml: bytes):
         """Parse multiple XML documents.
 
         Each document must commence with an XML document declaration, i.e.
@@ -75,7 +80,8 @@
         frags = (xml[start:end] for start, end in zip(starts, ends))
         return (etree.fromstring(frag) for frag in frags)
 
-    def get_power_state(self, xml):
+    @typed
+    def get_power_state(self, xml: bytes) -> str:
         """Get PowerState text from XML."""
         namespaces = {
             "h": (
@@ -95,29 +101,35 @@
         env['AMT_PASSWORD'] = power_pass
         return env
 
-    def _run(self, command, power_pass, stdin=None):
+    @typed
+    def _run(
+            self, command: tuple, power_pass: str,
+            stdin: bytes=None) -> bytes:
         """Run a subprocess with stdin."""
         env = self._get_amt_environment(power_pass)
         process = Popen(
             command, stdin=PIPE, stdout=PIPE, stderr=PIPE, env=env)
         stdout, stderr = process.communicate(stdin)
-        stdout = stdout.decode("utf-8")
-        stderr = stderr.decode("utf-8")
         if process.returncode != 0:
             raise PowerActionError(
-                "Failed to run command: %s with error: %s" % (command, stderr))
+                "Failed to run command: %s with error: %s" % (
+                    command, stderr.decode("utf-8", "replace")))
         return stdout
 
+    @typed
     def _issue_amttool_command(
-            self, cmd, ip_address, power_pass,
-            amttool_boot_mode=None, stdin=None):
+            self, cmd: str, ip_address: str, power_pass: str,
+            amttool_boot_mode=None, stdin=None) -> bytes:
         """Perform a command using amttool."""
         command = ('amttool', ip_address, cmd)
         if cmd in ('power-cycle', 'powerup'):
             command += (amttool_boot_mode,)
         return self._run(command, power_pass, stdin=stdin)
 
-    def _issue_wsman_command(self, power_change, ip_address, power_pass):
+    @typed
+    def _issue_wsman_command(
+            self, power_change: str, ip_address: str,
+            power_pass: str) -> bytes:
         """Perform a command using wsman."""
         wsman_power_schema_uri = (
             'http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/'
@@ -156,7 +168,7 @@
         for _ in range(10):
             output = self._issue_amttool_command(
                 'info', ip_address, power_pass)
-            if output is not None and output != '':
+            if output is not None and len(output) > 0:
                 break
             # Wait 1 second between retries.  AMT controllers are generally
             # very light and may not be comfortable with more frequent
@@ -165,8 +177,10 @@
 
         if output is None:
             raise PowerFatalError("amttool power querying FAILED.")
+
         # Ensure that from this point forward that output is a str.
-        assert isinstance(output, str)
+        output = output.decode("utf-8")
+
         # Wide awake (S0), or asleep (S1-S4), but not a clean slate that
         # will lead to a fresh boot.
         if 'S5' in output:
@@ -183,7 +197,7 @@
         output = None
         for _ in range(10):
             output = self._issue_wsman_command('query', ip_address, power_pass)
-            if output is not None and output != '':
+            if output is not None and len(output) > 0:
                 break
             # Wait 1 second between retries.  AMT controllers are generally
             # very light and may not be comfortable with more frequent
 
=== modified file 'src/provisioningserver/drivers/power/tests/test_amt.py'
--- src/provisioningserver/drivers/power/tests/test_amt.py	2016-04-14 15:25:36 +0000
+++ src/provisioningserver/drivers/power/tests/test_amt.py	2016-04-15 14:58:22 +0000
@@ -149,7 +149,7 @@
     def test__run_runs_command(self):
         amt_power_driver = AMTPowerDriver()
         amt_power_driver.env = None
-        command = factory.make_name('command')
+        command = factory.make_name('command'),
         power_pass = factory.make_name('power_pass')
         stdin = factory.make_name('stdin').encode('utf-8')
         popen_mock = self.patch_popen(return_value=(b'stdout', b''))
@@ -157,9 +157,8 @@
         result = amt_power_driver._run(
             command, power_pass, stdin)
 
-        self.expectThat(popen_mock.communicate, MockCalledOnceWith(
-            stdin))
-        self.expectThat(result, Equals('stdout'))
+        self.expectThat(popen_mock.communicate, MockCalledOnceWith(stdin))
+        self.expectThat(result, Equals(b'stdout'))
 
     def test__run_raises_power_action_error(self):
         amt_power_driver = AMTPowerDriver()
@@ -167,7 +166,8 @@
             return_value=(b'', b''), returncode=1)
 
         self.assertRaises(
-            PowerActionError, amt_power_driver._run, None, None, None)
+            PowerActionError, amt_power_driver._run, (),
+            factory.make_name("power-pass"), None)
 
     def test__issue_amttool_command_calls__run(self):
         amt_power_driver = AMTPowerDriver()
@@ -176,11 +176,9 @@
         amttool_boot_mode = factory.make_name('amttool_boot_mode')
         stdin = factory.make_name('stdin').encode('utf-8')
         cmd = choice(['power-cycle', 'powerup'])
-        command = (
-            'amttool', ip_address,
-            cmd, amttool_boot_mode)
+        command = 'amttool', ip_address, cmd, amttool_boot_mode
         _run_mock = self.patch(amt_power_driver, '_run')
-        _run_mock.return_value = 'output'
+        _run_mock.return_value = b'output'
 
         result = amt_power_driver._issue_amttool_command(
             cmd, ip_address, power_pass,
@@ -188,7 +186,7 @@
 
         self.expectThat(
             _run_mock, MockCalledOnceWith(command, power_pass, stdin=stdin))
-        self.expectThat(result, Equals('output'))
+        self.expectThat(result, Equals(b'output'))
 
     def test__issue_wsman_command_calls__run_for_power(self):
         amt_power_driver = AMTPowerDriver()
@@ -241,6 +239,7 @@
             'wsman', 'enumerate', wsman_query_schema_uri
         ) + wsman_query_opts
         _run_mock = self.patch(amt_power_driver, '_run')
+        _run_mock.return_value = b'ignored'
 
         amt_power_driver._issue_wsman_command('query', ip_address, power_pass)
 
@@ -254,7 +253,7 @@
         _issue_amttool_command_mock = self.patch(
             amt_power_driver, '_issue_amttool_command')
         _issue_amttool_command_mock.return_value = (
-            AMTTOOL_OUTPUT % (b'', b'S0')).decode("utf-8")
+            AMTTOOL_OUTPUT % (b'', b'S0'))
 
         result = amt_power_driver.amttool_query_state(ip_address, power_pass)
 
@@ -270,7 +269,7 @@
         _issue_amttool_command_mock = self.patch(
             amt_power_driver, '_issue_amttool_command')
         _issue_amttool_command_mock.return_value = (
-            AMTTOOL_OUTPUT % (b'', b'S5 (soft-off)')).decode("utf-8")
+            AMTTOOL_OUTPUT % (b'', b'S5 (soft-off)'))
 
         result = amt_power_driver.amttool_query_state(ip_address, power_pass)
 
@@ -287,7 +286,7 @@
         _issue_amttool_command_mock = self.patch(
             amt_power_driver, '_issue_amttool_command')
         _issue_amttool_command_mock.return_value = (
-            AMTTOOL_OUTPUT % (b'', b'error')).decode("utf-8")
+            AMTTOOL_OUTPUT % (b'', b'error'))
 
         self.assertRaises(
             PowerActionError, amt_power_driver.amttool_query_state,