Merge lp:~allenap/maas/remove-ip-from-amt into lp:~maas-committers/maas/trunk

[Gavin Panella]

Don't record IP address in AMT power parameters.

Instead use its MAC address to find it on the network because we cannot guarantee that the recorded IP address will not change.

This is based on Tycho's work in lp:~tycho-s/maas/remove-ip-from-amt.

[Julian Edwards (julian-edwards)]

Mostly fine but a question and a nit! Setting to approved though as I don't want to block you.

[1]
49 + if 'mac_address' in kwargs:
50 + kwargs['ip_address'] = find_ip_via_arp(kwargs['mac_address'])

If ip_address is not set, then the template will blow up. Why would mac_address not be set in kwargs here?

[2]
You're using a bit of a testing antipattern by initialising sentinel.ip_address in the setup and then depending on it in the test assertions. I think it would be better if each test patched out this separately, despite the lack of factoring.

BTW I think we should use more sentinel, it's much easier on the eye that the factory.makeRandomXXX stuff!

review: Approve

[Gavin Panella (allenap)]

> Mostly fine but a question and a nit!  Setting to approved though as I don't
> want to block you.
>
> [1]
> 49      +    if 'mac_address' in kwargs:
> 50      +        kwargs['ip_address'] = find_ip_via_arp(kwargs['mac_address'])
>
> If ip_address is not set, then the template will blow up.  Why would
> mac_address not be set in kwargs here?

It's only in the power parameters for Etherwake and AMT.

>
> [2]
> You're using a bit of a testing antipattern by initialising
> sentinel.ip_address in the setup and then depending on it in the test
> assertions.  I think it would be better if each test patched out this
> separately, despite the lack of factoring.

Okay, I'll fix that up.

>
> BTW I think we should use more sentinel, it's much easier on the eye that the
> factory.makeRandomXXX stuff!

Indeed! It doesn't always work - sometimes we really do need quacking
ducks - but when it does it's concise and clear.

[Julian Edwards (julian-edwards)]

On Thursday 13 Feb 2014 10:11:09 you wrote:
> > Mostly fine but a question and a nit! Setting to approved though as I
> > don't want to block you.
> >
> > [1]
> > 49 + if 'mac_address' in kwargs:
> > 50 + kwargs['ip_address'] =
> > find_ip_via_arp(kwargs['mac_address'])
> >
> > If ip_address is not set, then the template will blow up. Why would
> > mac_address not be set in kwargs here?
>
> It's only in the power parameters for Etherwake and AMT.

Oh OK - the diff segment confused me more than it should have.

cheers.

[MAAS Lander (maas-lander)]

The attempt to merge lp:~allenap/maas/remove-ip-from-amt into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Hit http://security.ubuntu.com trusty-security Release.gpg
Hit http://security.ubuntu.com trusty-security Release
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Get:1 http://nova.clouds.archive.ubuntu.com trusty Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg
Get:2 http://nova.clouds.archive.ubuntu.com trusty Release [58.5 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release
Hit http://security.ubuntu.com trusty-security/main Sources
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Get:3 http://nova.clouds.archive.ubuntu.com trusty/main Sources [1,061 kB]
Ign http://security.ubuntu.com trusty-security/main Translation-en_US
Ign http://security.ubuntu.com trusty-security/universe Translation-en_US
Get:4 http://nova.clouds.archive.ubuntu.com trusty/universe Sources [6,414 kB]
Get:5 http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages [1,343 kB]
Get:6 http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages [5,881 kB]
Get:7 http://nova.clouds.archive.ubuntu.com trusty/main Translation-en [760 kB]
Get:8 http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en [4,055 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en_US
Fetched 19.6 MB in 7s (2,610 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 bind9 bind9utils build-essential curl daemontools distro-info dnsutils firefox freeipmi-tools ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make postgresql python-amqplib python-bzrlib python-celery python-convoy python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-formencode python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-netaddr python-netifaces python-oauth python-...

=== modified file 'etc/maas/templates/power/amt.template'
--- etc/maas/templates/power/amt.template	2014-01-27 16:56:02 +0000
+++ etc/maas/templates/power/amt.template	2014-02-13 11:16:49 +0000
@@ -2,19 +2,18 @@
 # -*- mode: shell-script -*-
 #
 # Control a system via amttool
-power_address='{{power_address}}'
 power_change='{{power_change}}'
 power_pass='{{power_pass}}'
 
 echo amt.template starting $*
 
-echo power_address $power_address
+echo ip_address $ip_address
 echo power_change $power_change
 echo power_pass $power_pass
 
 amt() {
-  echo running amttool "$power_address" $*
-  AMT_PASSWORD="$power_pass" amttool "$power_address" "$@"
+  echo running amttool "$ip_address" $*
+  AMT_PASSWORD="$power_pass" amttool "$ip_address" "$@"
 }
 
 state() {

=== modified file 'src/maasserver/power_parameters.py'
--- src/maasserver/power_parameters.py	2014-02-10 12:06:51 +0000
+++ src/maasserver/power_parameters.py	2014-02-13 11:16:49 +0000
@@ -199,7 +199,8 @@
     {
         'name': 'amt',
         'fields': [
-            make_json_field('power_address', "Power address"),
+            make_json_field(
+                'mac_address', "MAC Address", field_type='mac_address'),
             make_json_field('power_pass', "Power password"),
         ],
     },

=== modified file 'src/provisioningserver/tasks.py'
--- src/provisioningserver/tasks.py	2014-01-05 21:27:58 +0000
+++ src/provisioningserver/tasks.py	2014-02-13 11:16:49 +0000
@@ -134,6 +134,8 @@
     assert power_change in ('on', 'off'), (
         "Unknown power change keyword: %s" % power_change)
     kwargs['power_change'] = power_change
+    if 'mac_address' in kwargs:
+        kwargs['ip_address'] = find_ip_via_arp(kwargs['mac_address'])
     try:
         pa = PowerAction(power_type)
         pa.execute(**kwargs)

=== modified file 'src/provisioningserver/tests/test_tasks.py'
--- src/provisioningserver/tests/test_tasks.py	2014-01-27 20:16:45 +0000
+++ src/provisioningserver/tests/test_tasks.py	2014-02-13 11:16:49 +0000
@@ -39,6 +39,7 @@
 from mock import (
     ANY,
     Mock,
+    sentinel,
     )
 from netaddr import IPNetwork
 from provisioningserver import (
@@ -158,6 +159,43 @@
             "ether_wake", mac=arbitrary_mac)
 
 
+class TestPowerTasksResolveMACAddresses(PservTestCase):
+
+    def test_ip_address_is_looked_up_from_mac_address(self):
+        # Patch out PowerAction; we're just trying to demonstrate that
+        # it's invoked with an IP address even if one is not supplied.
+        PowerAction = self.patch(tasks, "PowerAction")
+        # find_ip_via_arp() is tested elsewhere; we just want to know
+        # that it has been used.
+        self.patch(tasks, "find_ip_via_arp").return_value = sentinel.ip_address
+        # PowerAction.execute() is passed an ip_address argument in
+        # addition to the mac_address argument when the latter is
+        # supplied.
+        tasks.issue_power_action(
+            sentinel.power_type, "on", mac_address=sentinel.mac_address)
+        PowerAction.assert_called_once_with(sentinel.power_type)
+        PowerAction.return_value.execute.assert_called_once_with(
+            power_change="on", mac_address=sentinel.mac_address,
+            ip_address=sentinel.ip_address)
+
+    def test_ip_address_is_looked_up_when_already_supplied(self):
+        # Patch out PowerAction; we're just trying to demonstrate that
+        # it's invoked with an IP address even if one is not supplied.
+        PowerAction = self.patch(tasks, "PowerAction")
+        # find_ip_via_arp() is tested elsewhere; we just want to know
+        # that it has been used.
+        self.patch(tasks, "find_ip_via_arp").return_value = sentinel.ip_address
+        # The ip_address argument passed to PowerAction.execute() is
+        # looked-up via find_ip_via_arp() even if an ip_address is
+        # passed into issue_power_action().
+        tasks.issue_power_action(
+            sentinel.power_type, "on", mac_address=sentinel.mac_address,
+            ip_address=sentinel.another_ip_address)
+        PowerAction.return_value.execute.assert_called_once_with(
+            power_change="on", mac_address=sentinel.mac_address,
+            ip_address=sentinel.ip_address)
+
+
 class TestDHCPTasks(PservTestCase):
 
     resources = (