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

Proposed by Gavin Panella
Status: Merged
Approved by: Gavin Panella
Approved revision: no longer in the source branch.
Merged at revision: 4926
Proposed branch: lp:~allenap/maas/xml-as-bytes--bug-1570609
Merge into: lp:~maas-committers/maas/trunk
Diff against target: 211 lines (+40/-27)
2 files modified
src/provisioningserver/drivers/power/amt.py (+28/-14)
src/provisioningserver/drivers/power/tests/test_amt.py (+12/-13)
To merge this branch: bzr merge lp:~allenap/maas/xml-as-bytes--bug-1570609
Reviewer Review Type Date Requested Status
Mike Pontillo (community) Approve
Andres Rodriguez (community) Approve
Blake Rouse Pending
Review via email: mp+292012@code.launchpad.net

Commit message

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.

To post a comment you must log in.
Revision history for this message
Andres Rodriguez (andreserl) wrote :

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
Revision history for this message
Mike Pontillo (mpontillo) wrote :

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.

Revision history for this message
Mike Pontillo (mpontillo) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/provisioningserver/drivers/power/amt.py'
2--- src/provisioningserver/drivers/power/amt.py 2016-04-14 15:25:36 +0000
3+++ src/provisioningserver/drivers/power/amt.py 2016-04-15 14:58:22 +0000
4@@ -25,7 +25,10 @@
5 PowerDriver,
6 PowerFatalError,
7 )
8-from provisioningserver.utils import shell
9+from provisioningserver.utils import (
10+ shell,
11+ typed,
12+)
13
14
15 REQUIRED_PACKAGES = [["amttool", "amtterm"], ["wsman", "wsmancli"]]
16@@ -44,7 +47,8 @@
17 missing_packages.append(package)
18 return missing_packages
19
20- def _render_wsman_state_xml(self, power_change):
21+ @typed
22+ def _render_wsman_state_xml(self, power_change) -> bytes:
23 """Render wsman state XML."""
24 wsman_state_filename = join(dirname(__file__), "amt.wsman-state.xml")
25 wsman_state_ns = {
26@@ -59,7 +63,8 @@
27 ps.text = power_states[power_change]
28 return etree.tostring(tree)
29
30- def _parse_multiple_xml_docs(self, xml):
31+ @typed
32+ def _parse_multiple_xml_docs(self, xml: bytes):
33 """Parse multiple XML documents.
34
35 Each document must commence with an XML document declaration, i.e.
36@@ -75,7 +80,8 @@
37 frags = (xml[start:end] for start, end in zip(starts, ends))
38 return (etree.fromstring(frag) for frag in frags)
39
40- def get_power_state(self, xml):
41+ @typed
42+ def get_power_state(self, xml: bytes) -> str:
43 """Get PowerState text from XML."""
44 namespaces = {
45 "h": (
46@@ -95,29 +101,35 @@
47 env['AMT_PASSWORD'] = power_pass
48 return env
49
50- def _run(self, command, power_pass, stdin=None):
51+ @typed
52+ def _run(
53+ self, command: tuple, power_pass: str,
54+ stdin: bytes=None) -> bytes:
55 """Run a subprocess with stdin."""
56 env = self._get_amt_environment(power_pass)
57 process = Popen(
58 command, stdin=PIPE, stdout=PIPE, stderr=PIPE, env=env)
59 stdout, stderr = process.communicate(stdin)
60- stdout = stdout.decode("utf-8")
61- stderr = stderr.decode("utf-8")
62 if process.returncode != 0:
63 raise PowerActionError(
64- "Failed to run command: %s with error: %s" % (command, stderr))
65+ "Failed to run command: %s with error: %s" % (
66+ command, stderr.decode("utf-8", "replace")))
67 return stdout
68
69+ @typed
70 def _issue_amttool_command(
71- self, cmd, ip_address, power_pass,
72- amttool_boot_mode=None, stdin=None):
73+ self, cmd: str, ip_address: str, power_pass: str,
74+ amttool_boot_mode=None, stdin=None) -> bytes:
75 """Perform a command using amttool."""
76 command = ('amttool', ip_address, cmd)
77 if cmd in ('power-cycle', 'powerup'):
78 command += (amttool_boot_mode,)
79 return self._run(command, power_pass, stdin=stdin)
80
81- def _issue_wsman_command(self, power_change, ip_address, power_pass):
82+ @typed
83+ def _issue_wsman_command(
84+ self, power_change: str, ip_address: str,
85+ power_pass: str) -> bytes:
86 """Perform a command using wsman."""
87 wsman_power_schema_uri = (
88 'http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/'
89@@ -156,7 +168,7 @@
90 for _ in range(10):
91 output = self._issue_amttool_command(
92 'info', ip_address, power_pass)
93- if output is not None and output != '':
94+ if output is not None and len(output) > 0:
95 break
96 # Wait 1 second between retries. AMT controllers are generally
97 # very light and may not be comfortable with more frequent
98@@ -165,8 +177,10 @@
99
100 if output is None:
101 raise PowerFatalError("amttool power querying FAILED.")
102+
103 # Ensure that from this point forward that output is a str.
104- assert isinstance(output, str)
105+ output = output.decode("utf-8")
106+
107 # Wide awake (S0), or asleep (S1-S4), but not a clean slate that
108 # will lead to a fresh boot.
109 if 'S5' in output:
110@@ -183,7 +197,7 @@
111 output = None
112 for _ in range(10):
113 output = self._issue_wsman_command('query', ip_address, power_pass)
114- if output is not None and output != '':
115+ if output is not None and len(output) > 0:
116 break
117 # Wait 1 second between retries. AMT controllers are generally
118 # very light and may not be comfortable with more frequent
119
120=== modified file 'src/provisioningserver/drivers/power/tests/test_amt.py'
121--- src/provisioningserver/drivers/power/tests/test_amt.py 2016-04-14 15:25:36 +0000
122+++ src/provisioningserver/drivers/power/tests/test_amt.py 2016-04-15 14:58:22 +0000
123@@ -149,7 +149,7 @@
124 def test__run_runs_command(self):
125 amt_power_driver = AMTPowerDriver()
126 amt_power_driver.env = None
127- command = factory.make_name('command')
128+ command = factory.make_name('command'),
129 power_pass = factory.make_name('power_pass')
130 stdin = factory.make_name('stdin').encode('utf-8')
131 popen_mock = self.patch_popen(return_value=(b'stdout', b''))
132@@ -157,9 +157,8 @@
133 result = amt_power_driver._run(
134 command, power_pass, stdin)
135
136- self.expectThat(popen_mock.communicate, MockCalledOnceWith(
137- stdin))
138- self.expectThat(result, Equals('stdout'))
139+ self.expectThat(popen_mock.communicate, MockCalledOnceWith(stdin))
140+ self.expectThat(result, Equals(b'stdout'))
141
142 def test__run_raises_power_action_error(self):
143 amt_power_driver = AMTPowerDriver()
144@@ -167,7 +166,8 @@
145 return_value=(b'', b''), returncode=1)
146
147 self.assertRaises(
148- PowerActionError, amt_power_driver._run, None, None, None)
149+ PowerActionError, amt_power_driver._run, (),
150+ factory.make_name("power-pass"), None)
151
152 def test__issue_amttool_command_calls__run(self):
153 amt_power_driver = AMTPowerDriver()
154@@ -176,11 +176,9 @@
155 amttool_boot_mode = factory.make_name('amttool_boot_mode')
156 stdin = factory.make_name('stdin').encode('utf-8')
157 cmd = choice(['power-cycle', 'powerup'])
158- command = (
159- 'amttool', ip_address,
160- cmd, amttool_boot_mode)
161+ command = 'amttool', ip_address, cmd, amttool_boot_mode
162 _run_mock = self.patch(amt_power_driver, '_run')
163- _run_mock.return_value = 'output'
164+ _run_mock.return_value = b'output'
165
166 result = amt_power_driver._issue_amttool_command(
167 cmd, ip_address, power_pass,
168@@ -188,7 +186,7 @@
169
170 self.expectThat(
171 _run_mock, MockCalledOnceWith(command, power_pass, stdin=stdin))
172- self.expectThat(result, Equals('output'))
173+ self.expectThat(result, Equals(b'output'))
174
175 def test__issue_wsman_command_calls__run_for_power(self):
176 amt_power_driver = AMTPowerDriver()
177@@ -241,6 +239,7 @@
178 'wsman', 'enumerate', wsman_query_schema_uri
179 ) + wsman_query_opts
180 _run_mock = self.patch(amt_power_driver, '_run')
181+ _run_mock.return_value = b'ignored'
182
183 amt_power_driver._issue_wsman_command('query', ip_address, power_pass)
184
185@@ -254,7 +253,7 @@
186 _issue_amttool_command_mock = self.patch(
187 amt_power_driver, '_issue_amttool_command')
188 _issue_amttool_command_mock.return_value = (
189- AMTTOOL_OUTPUT % (b'', b'S0')).decode("utf-8")
190+ AMTTOOL_OUTPUT % (b'', b'S0'))
191
192 result = amt_power_driver.amttool_query_state(ip_address, power_pass)
193
194@@ -270,7 +269,7 @@
195 _issue_amttool_command_mock = self.patch(
196 amt_power_driver, '_issue_amttool_command')
197 _issue_amttool_command_mock.return_value = (
198- AMTTOOL_OUTPUT % (b'', b'S5 (soft-off)')).decode("utf-8")
199+ AMTTOOL_OUTPUT % (b'', b'S5 (soft-off)'))
200
201 result = amt_power_driver.amttool_query_state(ip_address, power_pass)
202
203@@ -287,7 +286,7 @@
204 _issue_amttool_command_mock = self.patch(
205 amt_power_driver, '_issue_amttool_command')
206 _issue_amttool_command_mock.return_value = (
207- AMTTOOL_OUTPUT % (b'', b'error')).decode("utf-8")
208+ AMTTOOL_OUTPUT % (b'', b'error'))
209
210 self.assertRaises(
211 PowerActionError, amt_power_driver.amttool_query_state,