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

Proposed by Gavin Panella
Status: Merged
Approved by: Gavin Panella
Approved revision: 4925
Merged at revision: 4926
Proposed branch: lp:~allenap/maas/xml-as-bytes--bug-1570609
Merge into: lp:maas
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
=== 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 @@
25 PowerDriver,25 PowerDriver,
26 PowerFatalError,26 PowerFatalError,
27)27)
28from provisioningserver.utils import shell28from provisioningserver.utils import (
29 shell,
30 typed,
31)
2932
3033
31REQUIRED_PACKAGES = [["amttool", "amtterm"], ["wsman", "wsmancli"]]34REQUIRED_PACKAGES = [["amttool", "amtterm"], ["wsman", "wsmancli"]]
@@ -44,7 +47,8 @@
44 missing_packages.append(package)47 missing_packages.append(package)
45 return missing_packages48 return missing_packages
4649
47 def _render_wsman_state_xml(self, power_change):50 @typed
51 def _render_wsman_state_xml(self, power_change) -> bytes:
48 """Render wsman state XML."""52 """Render wsman state XML."""
49 wsman_state_filename = join(dirname(__file__), "amt.wsman-state.xml")53 wsman_state_filename = join(dirname(__file__), "amt.wsman-state.xml")
50 wsman_state_ns = {54 wsman_state_ns = {
@@ -59,7 +63,8 @@
59 ps.text = power_states[power_change]63 ps.text = power_states[power_change]
60 return etree.tostring(tree)64 return etree.tostring(tree)
6165
62 def _parse_multiple_xml_docs(self, xml):66 @typed
67 def _parse_multiple_xml_docs(self, xml: bytes):
63 """Parse multiple XML documents.68 """Parse multiple XML documents.
6469
65 Each document must commence with an XML document declaration, i.e.70 Each document must commence with an XML document declaration, i.e.
@@ -75,7 +80,8 @@
75 frags = (xml[start:end] for start, end in zip(starts, ends))80 frags = (xml[start:end] for start, end in zip(starts, ends))
76 return (etree.fromstring(frag) for frag in frags)81 return (etree.fromstring(frag) for frag in frags)
7782
78 def get_power_state(self, xml):83 @typed
84 def get_power_state(self, xml: bytes) -> str:
79 """Get PowerState text from XML."""85 """Get PowerState text from XML."""
80 namespaces = {86 namespaces = {
81 "h": (87 "h": (
@@ -95,29 +101,35 @@
95 env['AMT_PASSWORD'] = power_pass101 env['AMT_PASSWORD'] = power_pass
96 return env102 return env
97103
98 def _run(self, command, power_pass, stdin=None):104 @typed
105 def _run(
106 self, command: tuple, power_pass: str,
107 stdin: bytes=None) -> bytes:
99 """Run a subprocess with stdin."""108 """Run a subprocess with stdin."""
100 env = self._get_amt_environment(power_pass)109 env = self._get_amt_environment(power_pass)
101 process = Popen(110 process = Popen(
102 command, stdin=PIPE, stdout=PIPE, stderr=PIPE, env=env)111 command, stdin=PIPE, stdout=PIPE, stderr=PIPE, env=env)
103 stdout, stderr = process.communicate(stdin)112 stdout, stderr = process.communicate(stdin)
104 stdout = stdout.decode("utf-8")
105 stderr = stderr.decode("utf-8")
106 if process.returncode != 0:113 if process.returncode != 0:
107 raise PowerActionError(114 raise PowerActionError(
108 "Failed to run command: %s with error: %s" % (command, stderr))115 "Failed to run command: %s with error: %s" % (
116 command, stderr.decode("utf-8", "replace")))
109 return stdout117 return stdout
110118
119 @typed
111 def _issue_amttool_command(120 def _issue_amttool_command(
112 self, cmd, ip_address, power_pass,121 self, cmd: str, ip_address: str, power_pass: str,
113 amttool_boot_mode=None, stdin=None):122 amttool_boot_mode=None, stdin=None) -> bytes:
114 """Perform a command using amttool."""123 """Perform a command using amttool."""
115 command = ('amttool', ip_address, cmd)124 command = ('amttool', ip_address, cmd)
116 if cmd in ('power-cycle', 'powerup'):125 if cmd in ('power-cycle', 'powerup'):
117 command += (amttool_boot_mode,)126 command += (amttool_boot_mode,)
118 return self._run(command, power_pass, stdin=stdin)127 return self._run(command, power_pass, stdin=stdin)
119128
120 def _issue_wsman_command(self, power_change, ip_address, power_pass):129 @typed
130 def _issue_wsman_command(
131 self, power_change: str, ip_address: str,
132 power_pass: str) -> bytes:
121 """Perform a command using wsman."""133 """Perform a command using wsman."""
122 wsman_power_schema_uri = (134 wsman_power_schema_uri = (
123 'http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/'135 'http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/'
@@ -156,7 +168,7 @@
156 for _ in range(10):168 for _ in range(10):
157 output = self._issue_amttool_command(169 output = self._issue_amttool_command(
158 'info', ip_address, power_pass)170 'info', ip_address, power_pass)
159 if output is not None and output != '':171 if output is not None and len(output) > 0:
160 break172 break
161 # Wait 1 second between retries. AMT controllers are generally173 # Wait 1 second between retries. AMT controllers are generally
162 # very light and may not be comfortable with more frequent174 # very light and may not be comfortable with more frequent
@@ -165,8 +177,10 @@
165177
166 if output is None:178 if output is None:
167 raise PowerFatalError("amttool power querying FAILED.")179 raise PowerFatalError("amttool power querying FAILED.")
180
168 # Ensure that from this point forward that output is a str.181 # Ensure that from this point forward that output is a str.
169 assert isinstance(output, str)182 output = output.decode("utf-8")
183
170 # Wide awake (S0), or asleep (S1-S4), but not a clean slate that184 # Wide awake (S0), or asleep (S1-S4), but not a clean slate that
171 # will lead to a fresh boot.185 # will lead to a fresh boot.
172 if 'S5' in output:186 if 'S5' in output:
@@ -183,7 +197,7 @@
183 output = None197 output = None
184 for _ in range(10):198 for _ in range(10):
185 output = self._issue_wsman_command('query', ip_address, power_pass)199 output = self._issue_wsman_command('query', ip_address, power_pass)
186 if output is not None and output != '':200 if output is not None and len(output) > 0:
187 break201 break
188 # Wait 1 second between retries. AMT controllers are generally202 # Wait 1 second between retries. AMT controllers are generally
189 # very light and may not be comfortable with more frequent203 # very light and may not be comfortable with more frequent
190204
=== 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 @@
149 def test__run_runs_command(self):149 def test__run_runs_command(self):
150 amt_power_driver = AMTPowerDriver()150 amt_power_driver = AMTPowerDriver()
151 amt_power_driver.env = None151 amt_power_driver.env = None
152 command = factory.make_name('command')152 command = factory.make_name('command'),
153 power_pass = factory.make_name('power_pass')153 power_pass = factory.make_name('power_pass')
154 stdin = factory.make_name('stdin').encode('utf-8')154 stdin = factory.make_name('stdin').encode('utf-8')
155 popen_mock = self.patch_popen(return_value=(b'stdout', b''))155 popen_mock = self.patch_popen(return_value=(b'stdout', b''))
@@ -157,9 +157,8 @@
157 result = amt_power_driver._run(157 result = amt_power_driver._run(
158 command, power_pass, stdin)158 command, power_pass, stdin)
159159
160 self.expectThat(popen_mock.communicate, MockCalledOnceWith(160 self.expectThat(popen_mock.communicate, MockCalledOnceWith(stdin))
161 stdin))161 self.expectThat(result, Equals(b'stdout'))
162 self.expectThat(result, Equals('stdout'))
163162
164 def test__run_raises_power_action_error(self):163 def test__run_raises_power_action_error(self):
165 amt_power_driver = AMTPowerDriver()164 amt_power_driver = AMTPowerDriver()
@@ -167,7 +166,8 @@
167 return_value=(b'', b''), returncode=1)166 return_value=(b'', b''), returncode=1)
168167
169 self.assertRaises(168 self.assertRaises(
170 PowerActionError, amt_power_driver._run, None, None, None)169 PowerActionError, amt_power_driver._run, (),
170 factory.make_name("power-pass"), None)
171171
172 def test__issue_amttool_command_calls__run(self):172 def test__issue_amttool_command_calls__run(self):
173 amt_power_driver = AMTPowerDriver()173 amt_power_driver = AMTPowerDriver()
@@ -176,11 +176,9 @@
176 amttool_boot_mode = factory.make_name('amttool_boot_mode')176 amttool_boot_mode = factory.make_name('amttool_boot_mode')
177 stdin = factory.make_name('stdin').encode('utf-8')177 stdin = factory.make_name('stdin').encode('utf-8')
178 cmd = choice(['power-cycle', 'powerup'])178 cmd = choice(['power-cycle', 'powerup'])
179 command = (179 command = 'amttool', ip_address, cmd, amttool_boot_mode
180 'amttool', ip_address,
181 cmd, amttool_boot_mode)
182 _run_mock = self.patch(amt_power_driver, '_run')180 _run_mock = self.patch(amt_power_driver, '_run')
183 _run_mock.return_value = 'output'181 _run_mock.return_value = b'output'
184182
185 result = amt_power_driver._issue_amttool_command(183 result = amt_power_driver._issue_amttool_command(
186 cmd, ip_address, power_pass,184 cmd, ip_address, power_pass,
@@ -188,7 +186,7 @@
188186
189 self.expectThat(187 self.expectThat(
190 _run_mock, MockCalledOnceWith(command, power_pass, stdin=stdin))188 _run_mock, MockCalledOnceWith(command, power_pass, stdin=stdin))
191 self.expectThat(result, Equals('output'))189 self.expectThat(result, Equals(b'output'))
192190
193 def test__issue_wsman_command_calls__run_for_power(self):191 def test__issue_wsman_command_calls__run_for_power(self):
194 amt_power_driver = AMTPowerDriver()192 amt_power_driver = AMTPowerDriver()
@@ -241,6 +239,7 @@
241 'wsman', 'enumerate', wsman_query_schema_uri239 'wsman', 'enumerate', wsman_query_schema_uri
242 ) + wsman_query_opts240 ) + wsman_query_opts
243 _run_mock = self.patch(amt_power_driver, '_run')241 _run_mock = self.patch(amt_power_driver, '_run')
242 _run_mock.return_value = b'ignored'
244243
245 amt_power_driver._issue_wsman_command('query', ip_address, power_pass)244 amt_power_driver._issue_wsman_command('query', ip_address, power_pass)
246245
@@ -254,7 +253,7 @@
254 _issue_amttool_command_mock = self.patch(253 _issue_amttool_command_mock = self.patch(
255 amt_power_driver, '_issue_amttool_command')254 amt_power_driver, '_issue_amttool_command')
256 _issue_amttool_command_mock.return_value = (255 _issue_amttool_command_mock.return_value = (
257 AMTTOOL_OUTPUT % (b'', b'S0')).decode("utf-8")256 AMTTOOL_OUTPUT % (b'', b'S0'))
258257
259 result = amt_power_driver.amttool_query_state(ip_address, power_pass)258 result = amt_power_driver.amttool_query_state(ip_address, power_pass)
260259
@@ -270,7 +269,7 @@
270 _issue_amttool_command_mock = self.patch(269 _issue_amttool_command_mock = self.patch(
271 amt_power_driver, '_issue_amttool_command')270 amt_power_driver, '_issue_amttool_command')
272 _issue_amttool_command_mock.return_value = (271 _issue_amttool_command_mock.return_value = (
273 AMTTOOL_OUTPUT % (b'', b'S5 (soft-off)')).decode("utf-8")272 AMTTOOL_OUTPUT % (b'', b'S5 (soft-off)'))
274273
275 result = amt_power_driver.amttool_query_state(ip_address, power_pass)274 result = amt_power_driver.amttool_query_state(ip_address, power_pass)
276275
@@ -287,7 +286,7 @@
287 _issue_amttool_command_mock = self.patch(286 _issue_amttool_command_mock = self.patch(
288 amt_power_driver, '_issue_amttool_command')287 amt_power_driver, '_issue_amttool_command')
289 _issue_amttool_command_mock.return_value = (288 _issue_amttool_command_mock.return_value = (
290 AMTTOOL_OUTPUT % (b'', b'error')).decode("utf-8")289 AMTTOOL_OUTPUT % (b'', b'error'))
291290
292 self.assertRaises(291 self.assertRaises(
293 PowerActionError, amt_power_driver.amttool_query_state,292 PowerActionError, amt_power_driver.amttool_query_state,