launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #10253
[Merge] lp:~julian-edwards/maas/omshell-cleanup into lp:maas
Julian Edwards has proposed merging lp:~julian-edwards/maas/omshell-cleanup into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~julian-edwards/maas/omshell-cleanup/+merge/116577
Fix a load of problems, some of which were mentioned in a post-facto review at https://code.launchpad.net/~julian-edwards/maas/omshell-wrapper/+merge/116414
1. Fix a leaky Popen
2. Make omshell retryable
3. Make the docstring useful
4. Use format() instead of dictionary-based string interpolation.
I hope this meet's Gavin's high standards :)
--
https://code.launchpad.net/~julian-edwards/maas/omshell-cleanup/+merge/116577
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/maas/omshell-cleanup into lp:maas.
=== modified file 'src/provisioningserver/omshell.py'
--- src/provisioningserver/omshell.py 2012-07-24 09:39:26 +0000
+++ src/provisioningserver/omshell.py 2012-07-25 03:00:25 +0000
@@ -25,59 +25,72 @@
class Omshell:
- """Wrap up the omshell utility in Python."""
+ """Wrap up the omshell utility in Python.
+
+ :param server_address: The address for the DHCP server (ip or hostname)
+ :param shared_key: An HMAC-MD5 key generated by dnssec-keygen like:
+ $ dnssec-keygen -r /dev/urandom -a HMAC-MD5 -b 512 -n HOST omapi_key
+ $ cat Komapi_key.+*.private |grep ^Key|cut -d ' ' -f2-
+ It must match the key set in the DHCP server's config which looks
+ like this:
+
+ omapi-port 7911;
+ key omapi_key {
+ algorithm HMAC-MD5;
+ secret "XXXXXXXXX"; #<-The output from the generated key above.
+ };
+ omapi-key omapi_key;
+ """
def __init__(self, server_address, shared_key):
self.server_address = server_address
self.shared_key = shared_key
- self.proc = Popen("omshell", stdin=PIPE, stdout=PIPE)
def _run(self, stdin):
- stdout, stderr = self.proc.communicate(stdin)
- return stdout
+ command = ["omshell"]
+ proc = Popen(command, stdin=PIPE, stdout=PIPE)
+ stdout, stderr = proc.communicate(stdin)
+ if proc.poll() != 0:
+ raise CalledProcessError(proc.returncode, command, stdout)
+ return proc.returncode, stdout
def create(self, ip_address, mac_address):
stdin = dedent("""\
- server %(server)s
- key omapi_key %(key)s
+ server {self.server_address}
+ key omapi_key {self.shared_key}
connect
new host
- set ip-address = %(ip_address)s
- set hardware-address = %(mac_address)s
- set name = %(ip_address)s
+ set ip-address = {ip_address}
+ set hardware-address = {mac_address}
+ set name = {ip_address}
create
""")
- stdin = stdin % dict(
- server=self.server_address,
- key=self.shared_key,
- ip_address=ip_address,
- mac_address=mac_address)
+ stdin = stdin.format(
+ self=self, ip_address=ip_address, mac_address=mac_address)
- output = self._run(stdin)
+ returncode, output = self._run(stdin)
# If the call to omshell doesn't result in output containing the
# magic string 'hardware-type' then we can be reasonably sure
# that the 'create' command failed. Unfortunately there's no
# other output like "successful" to check so this is the best we
# can do.
if "hardware-type" not in output:
- raise CalledProcessError(self.proc.returncode, "omshell", output)
+ raise CalledProcessError(returncode, "omshell", output)
def remove(self, ip_address):
stdin = dedent("""\
- server %(server)s
- key omapi_key %(key)s
+ server {self.server_address}
+ key omapi_key {self.shared_key}
connect
new host
- set name = %(ip_address)s
+ set name = {ip_address}
open
remove
""")
- stdin = stdin % dict(
- server=self.server_address,
- key=self.shared_key,
- ip_address=ip_address)
+ stdin = stdin.format(
+ self=self, ip_address=ip_address)
- output = self._run(stdin)
+ returncode, output = self._run(stdin)
# If the omshell worked, the last line should reference a null
# object.
@@ -87,4 +100,4 @@
except IndexError:
last_line = ""
if last_line != "obj: <null>":
- raise CalledProcessError(self.proc.returncode, "omshell", output)
+ raise CalledProcessError(returncode, "omshell", output)
=== modified file 'src/provisioningserver/tests/test_omshell.py'
--- src/provisioningserver/tests/test_omshell.py 2012-07-24 07:35:43 +0000
+++ src/provisioningserver/tests/test_omshell.py 2012-07-25 03:00:25 +0000
@@ -13,6 +13,7 @@
__all__ = []
from subprocess import CalledProcessError
+from textwrap import dedent
from maastesting.factory import factory
from maastesting.fakemethod import FakeMethod
@@ -40,21 +41,22 @@
shell = Omshell(server_address, shared_key)
# Instead of calling a real omshell, we'll just record the
- # parameters passed to 'check_call'.
- recorder = FakeMethod(result=("hardware-type", None))
- self.patch(shell.proc, 'communicate', recorder)
+ # parameters passed to Popen.
+ recorder = FakeMethod(result=(0, "hardware-type"))
+ self.patch(shell, '_run', recorder)
shell.create(ip_address, mac_address)
- expected_args = (
- "server %(server)s\n"
- "key omapi_key %(key)s\n"
- "connect\n"
- "new host\n"
- "set ip-address = %(ip)s\n"
- "set hardware-address = %(mac)s\n"
- "set name = %(ip)s\n"
- "create\n" % dict(
+ expected_args = (dedent("""\
+ server {server}
+ key omapi_key {key}
+ connect
+ new host
+ set ip-address = {ip}
+ set hardware-address = {mac}
+ set name = {ip}
+ create
+ """).format(
server=server_address,
key=shared_key,
ip=ip_address,
@@ -79,8 +81,8 @@
# Fake a call that results in a failure with random output.
random_output = factory.getRandomString()
- recorder = FakeMethod(result=(random_output, None))
- self.patch(shell.proc, 'communicate', recorder)
+ recorder = FakeMethod(result=(0, random_output))
+ self.patch(shell, '_run', recorder)
exc = self.assertRaises(
CalledProcessError, shell.create, ip_address, mac_address)
@@ -93,20 +95,21 @@
shell = Omshell(server_address, shared_key)
# Instead of calling a real omshell, we'll just record the
- # parameters passed to 'check_call'.
- recorder = FakeMethod(result=("thing1\nthing2\nobj: <null>", None))
- self.patch(shell.proc, 'communicate', recorder)
+ # parameters passed to Popen.
+ recorder = FakeMethod(result=(0, "thing1\nthing2\nobj: <null>"))
+ self.patch(shell, '_run', recorder)
shell.remove(ip_address)
- expected_args = (
- "server %(server)s\n"
- "key omapi_key %(key)s\n"
- "connect\n"
- "new host\n"
- "set name = %(ip)s\n"
- "open\n"
- "remove\n" % dict(
+ expected_args = (dedent("""\
+ server {server}
+ key omapi_key {key}
+ connect
+ new host
+ set name = {ip}
+ open
+ remove
+ """).format(
server=server_address,
key=shared_key,
ip=ip_address),)
@@ -128,8 +131,8 @@
# Fake a call that results in a failure with random output.
random_output = factory.getRandomString()
- recorder = FakeMethod(result=(random_output, None))
- self.patch(shell.proc, 'communicate', recorder)
+ recorder = FakeMethod(result=(0, random_output))
+ self.patch(shell, '_run', recorder)
exc = self.assertRaises(
CalledProcessError, shell.remove, ip_address)
