Merge ~andreserl/maas:lp1714273 into maas:master

[Andres Rodriguez]

LP: #1714273 - Fix power management inside the Snap

Do not set defaults for power driver binaries, as each power driver should know what binary they use. This fixes power management inside the snap, allowing it to access the binary without hardcoding a path.

[Newell Jensen (newell-jensen)]

Needs unit tests (at least some modifications). One inline comment below.

What you have looks good otherwise.

review: Needs Fixing

3f06f2c... by Andres Rodriguez on 2017-09-01

Add comment that was removed by mistake

f2ea6e0... by Andres Rodriguez on 2017-09-01

Fix tests

[Newell Jensen (newell-jensen)]

LGTM. Thanks for the fixes.

review: Approve

diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index 1ec195c..f1c3e7e 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -2406,15 +2406,6 @@ class Node(CleanSave, TimestampedModel):
         power_params = self.power_parameters.copy()
 
         power_params.setdefault('system_id', self.system_id)
-        # TODO: We should not be sending these paths to the templates;
-        # the templates ought to know which tool to use themselves.
-        power_params.setdefault('fence_cdu', '/usr/sbin/fence_cdu')
-        power_params.setdefault('ipmipower', '/usr/sbin/ipmipower')
-        power_params.setdefault('ipmitool', '/usr/bin/ipmitool')
-        power_params.setdefault(
-            'ipmi_chassis_config', '/usr/sbin/ipmi-chassis-config')
-        power_params.setdefault('ipmi_config', 'ipmi.conf')
-        # TODO: /end of paths that templates should know.
         # TODO: This default ought to be in the virsh template.
         if self.bmc is not None and self.bmc.power_type == "virsh":
             power_params.setdefault(
diff --git a/src/provisioningserver/drivers/power/fence_cdu.py b/src/provisioningserver/drivers/power/fence_cdu.py
index 8699e31..513cefa 100644
--- a/src/provisioningserver/drivers/power/fence_cdu.py
+++ b/src/provisioningserver/drivers/power/fence_cdu.py
@@ -47,12 +47,12 @@ class FenceCDUPowerDriver(PowerDriver):
         return []
 
     def _issue_fence_cdu_command(
-            self, command, fence_cdu=None, power_address=None, power_id=None,
+            self, command, power_address=None, power_id=None,
             power_user=None, power_pass=None, **extra):
         """Issue fence_cdu command for the given power change."""
         try:
             stdout = call_and_check([
-                fence_cdu, '-a', power_address, '-n', power_id, '-l',
+                'fence_cdu', '-a', power_address, '-n', power_id, '-l',
                 power_user, '-p', power_pass, '-o', command],
                 env=select_c_utf8_locale())
         except ExternalProcessError as e:
diff --git a/src/provisioningserver/drivers/power/ipmi.py b/src/provisioningserver/drivers/power/ipmi.py
index 7b9d945..9ef5ba4 100644
--- a/src/provisioningserver/drivers/power/ipmi.py
+++ b/src/provisioningserver/drivers/power/ipmi.py
@@ -234,8 +234,7 @@ class IPMIPowerDriver(PowerDriver):
     def _issue_ipmi_command(
             self, power_change, power_address=None, power_user=None,
             power_pass=None, power_driver=None, power_off_mode=None,
-            ipmipower=None, ipmi_chassis_config=None, mac_address=None,
-            **extra):
+            mac_address=None, **extra):
         """Issue command to ipmipower, for the given system."""
         # This script deliberately does not check the current power state
         # before issuing the requested power command. See bug 1171418 for an
@@ -249,9 +248,9 @@ class IPMIPowerDriver(PowerDriver):
         # should have no impact on BMCs that don't require it.
         # See https://bugs.launchpad.net/maas/+bug/1287964
         ipmi_chassis_config_command = [
-            ipmi_chassis_config, '-W', 'opensesspriv']
+            'ipmi-chassis-config', '-W', 'opensesspriv']
         ipmipower_command = [
-            ipmipower, '-W', 'opensesspriv']
+            'ipmipower', '-W', 'opensesspriv']
 
         # Arguments in common between chassis config and power control. See
         # https://launchpad.net/bugs/1053391 for details of modifying the
diff --git a/src/provisioningserver/drivers/power/moonshot.py b/src/provisioningserver/drivers/power/moonshot.py
index 2db3e9e..1f65424 100644
--- a/src/provisioningserver/drivers/power/moonshot.py
+++ b/src/provisioningserver/drivers/power/moonshot.py
@@ -46,11 +46,11 @@ class MoonshotIPMIPowerDriver(PowerDriver):
         return []
 
     def _issue_ipmitool_command(
-            self, power_change, ipmitool=None, power_address=None,
-            power_user=None, power_pass=None, power_hwaddress=None, **extra):
+            self, power_change, power_address=None, power_user=None,
+            power_pass=None, power_hwaddress=None, **extra):
         """Issue ipmitool command for HP Moonshot cartridge."""
         command = (
-            ipmitool, '-I', 'lanplus', '-H', power_address,
+            'ipmitool', '-I', 'lanplus', '-H', power_address,
             '-U', power_user, '-P', power_pass
         ) + tuple(power_hwaddress.split())
         if power_change == 'pxe':
diff --git a/src/provisioningserver/drivers/power/tests/test_fence_cdu.py b/src/provisioningserver/drivers/power/tests/test_fence_cdu.py
index 292a078..cec9ca8 100644
--- a/src/provisioningserver/drivers/power/tests/test_fence_cdu.py
+++ b/src/provisioningserver/drivers/power/tests/test_fence_cdu.py
@@ -51,11 +51,11 @@ class TestFenceCDUPowerDriver(MAASTestCase):
         mock = self.patch(fence_cdu_module, 'call_and_check')
         mock.return_value = b'test'
         stdout = driver._issue_fence_cdu_command(
-            sentinel.command, sentinel.fence_cdu, sentinel.power_address,
+            sentinel.command, sentinel.power_address,
             sentinel.power_id, sentinel.power_user, sentinel.power_pass)
         self.expectThat(
             mock, MockCalledOnceWith([
-                sentinel.fence_cdu, '-a', sentinel.power_address, '-n',
+                'fence_cdu', '-a', sentinel.power_address, '-n',
                 sentinel.power_id, '-l', sentinel.power_user, '-p',
                 sentinel.power_pass, '-o', sentinel.command],
                 env=select_c_utf8_locale()))
@@ -66,7 +66,7 @@ class TestFenceCDUPowerDriver(MAASTestCase):
         mock = self.patch(fence_cdu_module, 'call_and_check')
         mock.side_effect = ExternalProcessError(2, "Fence CDU error")
         stdout = driver._issue_fence_cdu_command(
-            'status', sentinel.fence_cdu, sentinel.power_address,
+            'status', sentinel.power_address,
             sentinel.power_id, sentinel.power_user, sentinel.power_pass)
         self.assertThat(stdout, Equals("Status: OFF\n"))
 
@@ -76,12 +76,12 @@ class TestFenceCDUPowerDriver(MAASTestCase):
         mock.side_effect = ExternalProcessError(1, "Fence CDU error")
         self.assertRaises(
             PowerError, driver._issue_fence_cdu_command, sentinel.command,
-            sentinel.fence_cdu, sentinel.power_address, sentinel.power_id,
+            sentinel.power_address, sentinel.power_id,
             sentinel.power_user, sentinel.power_pass)
 
     def make_context(self):
         return {
-            'fence_cdu': sentinel.fence_cdu,
+            'fence_cdu': 'fence_cdu',
             'power_address': sentinel.power_address,
             'power_id': sentinel.power_id,
             'power_user': sentinel.power_user,
@@ -101,16 +101,16 @@ class TestFenceCDUPowerDriver(MAASTestCase):
 
         self.assertThat(
             mock, MockCallsMatch(
-                call([sentinel.fence_cdu, '-a', sentinel.power_address, '-n',
+                call(['fence_cdu', '-a', sentinel.power_address, '-n',
                       sentinel.power_id, '-l', sentinel.power_user, '-p',
                       sentinel.power_pass, '-o', 'status'], env=environ),
-                call([sentinel.fence_cdu, '-a', sentinel.power_address, '-n',
+                call(['fence_cdu', '-a', sentinel.power_address, '-n',
                       sentinel.power_id, '-l', sentinel.power_user, '-p',
                       sentinel.power_pass, '-o', 'off'], env=environ),
-                call([sentinel.fence_cdu, '-a', sentinel.power_address, '-n',
+                call(['fence_cdu', '-a', sentinel.power_address, '-n',
                       sentinel.power_id, '-l', sentinel.power_user, '-p',
                       sentinel.power_pass, '-o', 'status'], env=environ),
-                call([sentinel.fence_cdu, '-a', sentinel.power_address, '-n',
+                call(['fence_cdu', '-a', sentinel.power_address, '-n',
                       sentinel.power_id, '-l', sentinel.power_user, '-p',
                       sentinel.power_pass, '-o', 'on'], env=environ)))
 
diff --git a/src/provisioningserver/drivers/power/tests/test_ipmi.py b/src/provisioningserver/drivers/power/tests/test_ipmi.py
index cc3e6a8..bc5d3fb 100644
--- a/src/provisioningserver/drivers/power/tests/test_ipmi.py
+++ b/src/provisioningserver/drivers/power/tests/test_ipmi.py
@@ -47,8 +47,8 @@ def make_context():
         'power_pass': factory.make_name('power_pass'),
         'power_driver': factory.make_name('power_driver'),
         'power_off_mode': factory.make_name('power_off_mode'),
-        'ipmipower': factory.make_name('ipmipower'),
-        'ipmi_chassis_config': factory.make_name('ipmi_chassis_config'),
+        'ipmipower': 'ipmipower',
+        'ipmi_chassis_config': 'ipmi-chassis-config',
     }
 
 
diff --git a/src/provisioningserver/drivers/power/tests/test_moonshot.py b/src/provisioningserver/drivers/power/tests/test_moonshot.py
index 3e8ea13..388ad78 100644
--- a/src/provisioningserver/drivers/power/tests/test_moonshot.py
+++ b/src/provisioningserver/drivers/power/tests/test_moonshot.py
@@ -28,7 +28,7 @@ from testtools.matchers import Equals
 
 def make_context():
     return {
-        'ipmitool': factory.make_name('ipmitool'),
+        'ipmitool': 'ipmitool',
         'power_address': factory.make_name('power_address'),
         'power_user': factory.make_name('power_user'),
         'power_pass': factory.make_name('power_pass'),