Code review comment for lp:~blake-rouse/maas/powernv-support

Revision history for this message
Jason Hobbs (jason-hobbs) wrote :

Here's the rest of my review. Overall it looks really good! I approve as long as the duplication in 1 & 2 get cleaned up. I think some more near duplication could be cleaned up with some creativity but I wouldn't block on it.

[9]

- Architecture(name="ppc64el/generic", description="ppc64el"),
+ Architecture(
+ name="ppc64el/generic", description="ppc64el",
+ kernel_options=['rootdelay=60']),

A comment on why the rootdelay is needed and why it's 60 would be nice.

[10]

=== modified file 'src/provisioningserver/import_images/download_resources.py'
--- src/provisioningserver/import_images/download_resources.py 2014-05-23 09:48:49 +0000
+++ src/provisioningserver/import_images/download_resources.py 2014-05-28 21:38:08 +0000
@@ -19,6 +19,8 @@
 from datetime import datetime
 from gzip import GzipFile
 import os.path
+import tempfile
+from urlparse import urlsplit

 from provisioningserver.import_images.helpers import (
     get_signing_policy,

Why are these new imports added when there are no other changes to this file?

=== modified file 'src/provisioningserver/utils/__init__.py'
--- src/provisioningserver/utils/__init__.py 2014-05-05 23:55:46 +0000
+++ src/provisioningserver/utils/__init__.py 2014-05-28 21:38:08 +0000
@@ -829,3 +829,24 @@
         if len(columns) == 5 and columns[2] == mac:
             return columns[0]
     return None

[11]

+def find_mac_via_ip(ip):
+ """Find the MAC address for `ip` by reading the output of arp -n.
+
+ Returns `None` if the IP is not found.
+
+ We do this because we aren't necessarily the only DHCP server on the
+ network, so we can't check our own leases file and be guaranteed to find an
+ IP that matches.
+
+ :param ip: The ip address, e.g. '192.168.1.1'.
+ """
+
+ output = call_capture_and_check(['arp', '-n']).split('\n')
+
+ for line in sorted(output):
+ columns = line.split()
+ if len(columns) == 5 and columns[0] == ip:
+ return columns[2]
+ return None

I don't see a unit test for this method.

review: Approve

« Back to merge proposal