Merge lp:~trapnine/maas/power-package-checks into lp:~maas-committers/maas/trunk

[Jeffrey C Jones]

MAAS will check that power driver packages are installed before performing operations that require them. Messages in the UI will list the required packages and on which cluster they should be installed. They can be found:
  - on the node list page when mass operations are performed
  - on the node detail page before allowing selected operations

[Gavin Panella (allenap)]

I have lots of comments and things do need changing but ultimately this is good stuff.

I have a deeper reservation about it though: why are we not installing the packages we need for the drivers we ship in the first place? It's seems daft that we tell people afterwards that they need to install some other package.

To take the AMT driver as an example, I can see that the packages it needs -- amtterm and wsmancli -- are in universe. MAAS, which is in main, cannot depend on those. That means that MAAS, whatever it might claim, does NOT actually ship with the AMT driver in main.

Instead of investing energy in this approach we ought to finally make drivers pluggable (which is NOT that hard) and ship the AMT driver as a package in universe too. OR we should get amtterm and wsmancli into main.

That doesn't reflect badly on your code or your efforts, but we have an opportunity to do this in a much better way.

review: Needs Fixing

[Jeffrey C Jones (trapnine)]

FYI This feature was requested in https://bugs.launchpad.net/maas/+bug/1381000.

[Jeffrey C Jones (trapnine)]

It would definitely be cool if the drivers were pluggable instead of 'hardcoded' into MAAS. These plugins could include the client drivers, and be optional, allowing the customer to pick what they want installed.

Until then, though, I wouldn't want every power package MAAS supports installed on all my clusterd's. I may never buy AMT boxes, use virsh VM's, etc. Also, some of the power drivers (like for the IBM Power7 stuff) require privately hosted software to work, so we'd still need to check for that client software.

It seems sane to check for the real commands we're literally about to run before we attempt to do what will change power. If these commands aren't there, we tell the user what packages to install. It's not a big departure code-design-wise, and it follows the existing driver paradigm.

My only reservation with it is that it's a hard gate - there's not way to override MAAS's objection and start/commission/release, except by changing the power type or installing the desired package to make MAAS happy - maybe there should be a 'manual' power type?) Then again, the commands really won't work if clusterd can't find these binaries.

[Gavin Panella (allenap)]

> It would definitely be cool if the drivers were pluggable instead of
> 'hardcoded' into MAAS. These plugins could include the client drivers,
> and be optional, allowing the customer to pick what they want
> installed.

They're only hard-coded into the cluster; the region always asks the
cluster for power driver information, so making the drivers pluggable
only needs to be solved on the cluster.

>
> Until then, though, I wouldn't want every power package MAAS supports
> installed on all my clusterd's. I may never buy AMT boxes, use virsh
> VM's, etc. Also, some of the power drivers (like for the IBM Power7
> stuff) require privately hosted software to work, so we'd still need
> to check for that client software.

If all drivers were pluggable then we could have packages like:

   maas-driver-ipmi (in main)
   maas-driver-amt (in universe)
   maas-driver-ibm-power-7 (in partner, or provided separately)

maas-driver-amt, for example, would depend on amtterm and wsmancli. The
IPMI driver might be installed by default, but the others would be
optional. Getting fancy, we could search for packages named
maas-driver-* and inform users of them via the UI.

>
> It seems sane to check for the real commands we're literally about to
> run before we attempt to do what will change power. If these commands
> aren't there, we tell the user what packages to install. It's not a
> big departure code-design-wise, and it follows the existing driver
> paradigm.

This can be done pre-flight. The drivers can decide to also check at
power-on, power-off, or power-query time, but they'll crash anyway when
they come to use those commands... at which point we can show the user
the pre-flight errors.

For AMT, for example, I need only amtterm, not wsmancli, so the absence
of the latter is a warning only.

>
> My only reservation with it is that it's a hard gate - there's not way
> to override MAAS's objection and start/commission/release, except by
> changing the power type or installing the desired package to make MAAS
> happy - maybe there should be a 'manual' power type?) Then again, the
> commands really won't work if clusterd can't find these binaries.

I agree, and I think this goes back to my point. If a driver appears in
MAAS's UI then it should be fully installed. It should not be possible
to have a half-installed driver, like we can have now. However, once
half-installed drivers are no longer possible we would need to inform
users that other drivers are available, and the steps needed to install
them.

[Blake Rouse (blake-rouse)]

I agree with all of Gavin's points. It would be a huge improvement and using python-stevedore will allow us this ability.

But for 1.9, we need to fix this bug and doing what is described here does not fit. Lets track this in a bug and see if we can get time in 2.0 to fix this.

[Gavin Panella (allenap)]

Here's my rough suggestion for what to do now:

Change PowerDriverCheck to something like:

  from provisioningserver.rpc.arguments import (
      CompressedAmpList,
      Choice,
  )

  class PowerDriverCheck(amp.Command):
      """Check power driver on cluster for missing packages.

      :since: 1.9
      """

      arguments = [
          (b"power_type", amp.Unicode()),
      ]
      response = [
          (b"messages": CompressedAmpList([
              (b"severity", Choice({
                   "INFO": b"INFO",
                   "WARNING": b"WARNING",
                   "ERROR": b"ERROR",
               })),
              (b"message", amp.Unicode()),
          ])),
      ]
      errors = {
          UnknownPowerType: (
              b"UnknownPowerType"),
          NotImplementedError: (
              b"NotImplementedError"),
      }

Use this to display warnings and errors in the UI. Informational
messages can be logged but perhaps not displayed.

In drivers, look for the commands (or other resources) needed rather
than packages, and report about each of them.

This is simple enough to cover most/any scenarios, not just missing
packages or commands, but there's enough structure that we COULD use
WARNING or ERROR messages when measuring and reporting on MAAS's health
(a feature planned for this cycle).

Next, raise PowerActionFail; dispense with PowerDriverMissingPackages.
This will remove several code-paths.

Then, don't gate use of power-on, power-off, or power-query on the
outcome of the above.

If calling PowerDriverCheck frequently one day causes problems we can do
it periodically and cache the results in the database, but that's not an
issue right now.

[Gavin Panella (allenap)]

One more comment.

[Blake Rouse (blake-rouse)]

The code does look at binaries, but reports the package that provides that binary. I don't think that needs changing here.

[Gavin Panella (allenap)]

> The code does look at binaries, but reports the package that provides that
> binary. I don't think that needs changing here.

Okay. That's compatible with the change I suggested to PowerDriverCheck. The difference is that the message is composed on the cluster.

[Jeffrey C Jones (trapnine)]

> > The code does look at binaries, but reports the package that provides that
> > binary. I don't think that needs changing here.
>
> Okay. That's compatible with the change I suggested to PowerDriverCheck. The
> difference is that the message is composed on the cluster.

Nice, I'll remove that new exception type.

However, I purposely sent symbolic info to the region so the smarts of rendering a meaningful message can be in the UI (to achieve various designs).

If we are ever going to localize, it belongs in the html/frontend so the clusterd need never be locale aware. Each user on a MAAS can have their browser in a different language without informing any backend. If we're NEVER going to localize, then shunting raw error messages across is 'future proof', but the web client won't know what it's showing and many formatting / design concepts will not be possible.

[Gavin Panella (allenap)]

[...]
> However, I purposely sent symbolic info to the region so the smarts of
> rendering a meaningful message can be in the UI (to achieve various
> designs).
>
> If we are ever going to localize, it belongs in the html/frontend so
> the clusterd need never be locale aware. Each user on a MAAS can have
> their browser in a different language without informing any backend.
> If we're NEVER going to localize, then shunting raw error messages
> across is 'future proof', but the web client won't know what it's
> showing and many formatting / design concepts will not be possible.

I agree that that would be the right thing to do, but the likelihood of
localising MAAS any day soon is near zero. However, dealing with non-deb
environments like Ubuntu Core is imminent, as is dealing with more
general ideas of health.

Your other point about formatting and design is a good one.

Once the RPC call spec has been put into a released product we can't
change it. It'll be outdated almost as soon as it's released, and we'll
need to revisit this problem again.

Mmm. It's six of one, half-a-dozen of the other. There's a danger I'm
over-thinking this. If you go with what you've got we can rev the call
once we know more. I've talked myself into agreement with you :)

[Gavin Panella (allenap)]

A few more comments but they're all minor. Nice stuff!

review: Approve

=== modified file 'src/maasserver/api/tests/test_enlistment.py'
--- src/maasserver/api/tests/test_enlistment.py	2015-10-09 20:37:59 +0000
+++ src/maasserver/api/tests/test_enlistment.py	2015-10-20 18:00:40 +0000
@@ -55,6 +55,7 @@
         super(EnlistmentAPITest, self).setUp()
         self.patch_autospec(node_module, 'power_on_node')
         self.patch_autospec(node_module, 'power_off_node')
+        self.patch_autospec(node_module, 'power_driver_check')
         self.patch_autospec(Node, 'start_transition_monitor')
 
     def test_POST_new_creates_node(self):
@@ -369,6 +370,7 @@
         super(NodeHostnameEnlistmentTest, self).setUp()
         self.patch_autospec(node_module, 'power_on_node')
         self.patch_autospec(node_module, 'power_off_node')
+        self.patch_autospec(node_module, 'power_driver_check')
         self.patch_autospec(Node, 'start_transition_monitor')
 
     scenarios = [
@@ -480,6 +482,7 @@
         super(NonAdminEnlistmentAPITest, self).setUp()
         self.patch_autospec(node_module, 'power_on_node')
         self.patch_autospec(node_module, 'power_off_node')
+        self.patch_autospec(node_module, 'power_driver_check')
 
     def test_POST_non_admin_creates_node_in_declared_state(self):
         # Upon non-admin enlistment, a node goes into the New
@@ -508,6 +511,7 @@
         super(AnonymousEnlistmentAPITest, self).setUp()
         self.patch_autospec(node_module, 'power_on_node')
         self.patch_autospec(node_module, 'power_off_node')
+        self.patch_autospec(node_module, 'power_driver_check')
 
     def test_POST_accept_not_allowed(self):
         # An anonymous user is not allowed to accept an anonymously
@@ -574,6 +578,7 @@
         super(SimpleUserLoggedInEnlistmentAPITest, self).setUp()
         self.patch_autospec(node_module, 'power_on_node')
         self.patch_autospec(node_module, 'power_off_node')
+        self.patch_autospec(node_module, 'power_driver_check')
 
     def test_POST_accept_not_allowed(self):
         # An non-admin user is not allowed to accept an anonymously
@@ -683,6 +688,7 @@
         super(AdminLoggedInEnlistmentAPITest, self).setUp()
         self.patch_autospec(node_module, 'power_on_node')
         self.patch_autospec(node_module, 'power_off_node')
+        self.patch_autospec(node_module, 'power_driver_check')
         self.patch_autospec(Node, 'start_transition_monitor')
 
     def test_POST_new_sets_power_type_if_admin(self):

=== modified file 'src/maasserver/api/tests/test_node.py'
--- src/maasserver/api/tests/test_node.py	2015-10-09 14:36:18 +0000
+++ src/maasserver/api/tests/test_node.py	2015-10-20 18:00:40 +0000
@@ -91,6 +91,7 @@
         super(NodeAnonAPITest, self).setUp()
         self.patch(node_module, 'power_on_node')
         self.patch(node_module, 'power_off_node')
+        self.patch(node_module, 'power_driver_check')
 
     def test_anon_nodes_GET(self):
         # Anonymous requests to the API without a specified operation
@@ -140,6 +141,7 @@
         super(TestNodeAPI, self).setUp()
         self.patch(node_module, 'power_on_node')
         self.patch(node_module, 'power_off_node')
+        self.patch(node_module, 'power_driver_check')
 
     def test_handler_path(self):
         self.assertEqual(
@@ -1619,6 +1621,7 @@
     '''
 
     def test_POST_start_returns_error_when_static_ips_exhausted(self):
+        self.patch(node_module, 'power_driver_check')
         node = factory.make_Node_with_Interface_on_Subnet(
             owner=self.logged_in_user, status=NODE_STATUS.ALLOCATED,
             architecture=make_usable_architecture(self))

=== modified file 'src/maasserver/clusterrpc/power.py'
--- src/maasserver/clusterrpc/power.py	2015-05-07 18:14:38 +0000
+++ src/maasserver/clusterrpc/power.py	2015-10-20 18:00:40 +0000
@@ -18,16 +18,20 @@
 ]
 
 from functools import partial
+import logging
 
 from maasserver.rpc import getClientFor
 from provisioningserver.logger import get_maas_logger
 from provisioningserver.rpc.cluster import (
+    PowerDriverCheck,
     PowerOff,
     PowerOn,
 )
 from provisioningserver.utils.twisted import asynchronous
-
-
+from twisted.protocols.amp import UnhandledCommand
+
+
+logger = logging.getLogger(__name__)
 maaslog = get_maas_logger("power")
 
 
@@ -52,7 +56,7 @@
     def call_power_command(client, **kwargs):
         return client(command, **kwargs)
 
-    maaslog.debug("%s: Asking cluster to power on node.", hostname)
+    maaslog.debug("%s: Asking cluster to power on/off node.", hostname)
     d = getClientFor(cluster_uuid).addCallback(
         call_power_command, system_id=system_id, hostname=hostname,
         power_type=power_info.power_type,
@@ -73,3 +77,41 @@
 
 power_off_node = partial(power_node, PowerOff)
 power_on_node = partial(power_node, PowerOn)
+
+
+@asynchronous(timeout=30)
+def power_driver_check(cluster_uuid, power_type):
+    """Call PowerDriverCheck on the given cluster and wait for response.
+
+    :param cluster-uuid: The UUID of the cluster to check.
+    :param power_type: The power type to check.
+    :return: A list of missing power drivers for the power_type, if any.
+
+    :raises NoConnectionsAvailable: When no connections to the node's
+        cluster are available for use.
+    :raises UnknownPowerType: When the requested power type is not known
+        to the cluster.
+    :raises NotImplementedError: When the power driver hasn't implemented
+        the missing packages check.
+    :raises TimeoutError: If a response has not been received within 30
+        seconds.
+    """
+    def get_missing_packages(client):
+        d = client(PowerDriverCheck, power_type=power_type)
+        d.addCallbacks(extract_missing_packages, ignore_unhandled_command)
+        return d
+
+    def extract_missing_packages(response):
+        return response["missing_packages"]
+
+    def ignore_unhandled_command(failure):
+        failure.trap(UnhandledCommand)
+        # The region hasn't been upgraded to support this method yet, so give
+        # up. Returning an empty list indicates that the power driver is OK, so
+        # the power attempt will continue and any errors will be caught later.
+        logger.warn(
+            "Unable to query cluster for power packages. Cluster does not"
+            "support the PowerDriverCheck RPC method. Returning OK.")
+        return []
+
+    return getClientFor(cluster_uuid).addCallback(get_missing_packages)

=== modified file 'src/maasserver/clusterrpc/power_parameters.py'
--- src/maasserver/clusterrpc/power_parameters.py	2015-10-01 22:48:50 +0000
+++ src/maasserver/clusterrpc/power_parameters.py	2015-10-20 18:00:40 +0000
@@ -99,6 +99,8 @@
     :param fields: The fields that make up the parameters for the power
         type. Will be validated against
         POWER_TYPE_PARAMETER_FIELD_SCHEMA.
+    :param missing_packages: System packages that must be installed on
+        the cluster before the power type can be used.
     :type fields: list of `make_json_field` results.
     :param parameters_set: An existing list of power type parameters to
         mutate.

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2015-10-16 17:27:09 +0000
+++ src/maasserver/models/node.py	2015-10-20 18:00:40 +0000
@@ -54,6 +54,7 @@
 from maasserver import DefaultMeta
 from maasserver.clusterrpc.dhcp import remove_host_maps
 from maasserver.clusterrpc.power import (
+    power_driver_check,
     power_off_node,
     power_on_node,
 )
@@ -132,7 +133,10 @@
 )
 from provisioningserver.logger import get_maas_logger
 from provisioningserver.power import QUERY_POWER_TYPES
-from provisioningserver.power.poweraction import UnknownPowerType
+from provisioningserver.power.poweraction import (
+    PowerActionFail,
+    UnknownPowerType,
+)
 from provisioningserver.utils.enum import map_enum_reverse
 from provisioningserver.utils.twisted import (
     asynchronous,
@@ -1456,6 +1460,23 @@
                 power_type, power_params,
             )
 
+    def confirm_power_driver_operable(self):
+        power_type = self.get_effective_power_type()
+        missing_packages = power_driver_check(self.nodegroup.uuid, power_type)
+        if len(missing_packages) > 0:
+            missing_packages = sorted(missing_packages)
+            if len(missing_packages) > 2:
+                missing_packages = [", ".join(
+                    missing_packages[:-1]), missing_packages[-1]]
+            package_list = " and ".join(missing_packages)
+            raise PowerActionFail(
+                "Power control software is missing from the cluster "
+                "controller. To proceed, "
+                "install the %s package%s on the %s cluster." % (
+                    package_list,
+                    "s" if len(missing_packages) > 1 else "",
+                    self.nodegroup.cluster_name))
+
     def acquire(
             self, user, token=None, agent_name='', comment=None):
         """Mark commissioned node as acquired by the given user and token."""
@@ -2276,7 +2297,7 @@
             # You can't start a node you don't own unless you're an admin.
             raise PermissionDenied()
         event = EVENT_TYPES.REQUEST_NODE_START
-        # if status is ALLOCATED, this start is actually for a deplyment
+        # if status is ALLOCATED, this start is actually for a deployment
         if self.status == NODE_STATUS.ALLOCATED:
             event = EVENT_TYPES.REQUEST_NODE_START_DEPLOYMENT
         self._register_request_event(
@@ -2336,6 +2357,7 @@
             # Everything we've done up to this point is still valid;
             # this is not an error state.
             return None
+        self.confirm_power_driver_operable()
 
         def pc_power_on_node(system_id, hostname, nodegroup_uuid, power_info):
             d = power_on_node(system_id, hostname, nodegroup_uuid, power_info)
@@ -2389,6 +2411,7 @@
             # node we don't know how to stop isn't an error state, but
             # it's a no-op.
             return None
+        self.confirm_power_driver_operable()
 
         # Smuggle in a hint about how to power-off the self.
         power_info.power_parameters['power_off_mode'] = stop_mode

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2015-10-16 17:27:09 +0000
+++ src/maasserver/models/tests/test_node.py	2015-10-20 18:00:40 +0000
@@ -142,6 +142,10 @@
 
 class TestNode(MAASServerTestCase):
 
+    def setUp(self):
+        super(TestNode, self).setUp()
+        self.patch_autospec(node_module, 'power_driver_check')
+
     def disable_node_query(self):
         self.addCleanup(node_query.enable)
         node_query.disable()
@@ -3378,6 +3382,7 @@
         self.useFixture(RegionEventLoopFixture("rpc"))
         self.useFixture(RunningEventLoopFixture())
         self.rpc_fixture = self.useFixture(MockLiveRegionToClusterRPCFixture())
+        self.patch_autospec(node_module, 'power_driver_check')
 
     def prepare_rpc_to_cluster(self, nodegroup):
         protocol = self.rpc_fixture.makeCluster(
@@ -3485,7 +3490,17 @@
             node._start_transition_monitor_async,
             MockCalledOnceWith(ANY, node.hostname))
 
-    def test_start_logs_user_request(self):
+    def test__node_start_checks_power_driver(self):
+        self.patch_autospec(node_module, "power_on_node")
+        user = factory.make_User()
+        node = self.make_acquired_node_with_interface(user)
+        driver_check = self.patch_autospec(node_module, "power_driver_check")
+        with post_commit_hooks:
+            node.start(user)
+        self.expectThat(driver_check, MockCalledOnceWith(
+            node.nodegroup.uuid, node.get_effective_power_type()))
+
+    def test__start_logs_user_request(self):
         admin = factory.make_admin()
         node = factory.make_Node(status=NODE_STATUS.NEW, owner=admin)
         self.patch(node, '_start')
@@ -3597,6 +3612,10 @@
 class TestNode_Stop(MAASServerTestCase):
     """Tests for Node.stop()."""
 
+    def setUp(self):
+        super(TestNode_Stop, self).setUp()
+        self.patch_autospec(node_module, 'power_driver_check')
+
     def make_node_with_interface(
             self, user, nodegroup=None, power_type="virsh"):
         node = factory.make_Node_with_Interface_on_Subnet(
@@ -3625,6 +3644,16 @@
                 node.system_id, node.hostname, node.nodegroup.uuid,
                 expected_power_info))
 
+    def test__node_stop_checks_power_driver(self):
+        self.patch_autospec(node_module, "power_off_node")
+        user = factory.make_User()
+        node = self.make_node_with_interface(user)
+        driver_check = self.patch_autospec(node_module, "power_driver_check")
+        with post_commit_hooks:
+            node.stop(user, factory.make_name('stop-mode'))
+        self.expectThat(driver_check, MockCalledOnceWith(
+            node.nodegroup.uuid, node.get_effective_power_type()))
+
     def test__does_not_stop_nodes_the_user_cannot_edit(self):
         power_off_node = self.patch_autospec(node_module, "power_off_node")
         owner = factory.make_User()

=== modified file 'src/maasserver/static/js/angular/controllers/node_details.js'
--- src/maasserver/static/js/angular/controllers/node_details.js	2015-10-01 22:48:50 +0000
+++ src/maasserver/static/js/angular/controllers/node_details.js	2015-10-20 18:00:40 +0000
@@ -903,6 +903,25 @@
             }
         };
 
+        // true if power error prevents the provided action
+        $scope.hasActionPowerError = function(actionName) {
+            if(!$scope.hasPowerError()) {
+                return false; // no error, no need to check state
+            }
+            // these states attempt to manipulate power
+            var powerChangingStates = [
+                'commission',
+                'deploy',
+                'on',
+                'off',
+                'release'
+            ];
+            if(actionName && powerChangingStates.indexOf(actionName) > -1) {
+                return true;
+            }
+            return false;
+        };
+
         // Check to see if the power type has any missing system packages.
         $scope.hasPowerError = function() {
             if(angular.isObject($scope.power.type)) {
@@ -912,13 +931,25 @@
             }
         };
 
-        // Returns an array of missing system packages.
-        $scope.getPowerError = function() {
+        // Returns a formatted string of missing system packages.
+        $scope.getPowerErrors = function() {
+            var i;
+            var result = "";
             if(angular.isObject($scope.power.type)) {
-                return $scope.power.type.missing_packages;
-            } else {
-                return false;
+                var packages = $scope.power.type.missing_packages;
+                packages.sort();
+                for(i = 0; i < packages.length; i++) {
+                    result += packages[i];
+                    if(i+2 < packages.length) {
+                        result += ", ";
+                    }
+                    else if(i+1 < packages.length) {
+                        result += " and ";
+                    }
+                }
+                result += packages.length > 1 ? " packages" : " package";
             }
+            return result;
         };
 
         // Load all the required managers.

=== modified file 'src/maasserver/static/partials/node-details.html'
--- src/maasserver/static/partials/node-details.html	2015-10-19 17:56:35 +0000
+++ src/maasserver/static/partials/node-details.html	2015-10-20 18:00:40 +0000
@@ -48,7 +48,7 @@
                 <!-- XXX blake_r 2015-02-19 - Need to add e2e test. -->
                 <div class="page-header__dropdown ng-hide" data-ng-show="actionOption">
                     <!-- XXX blake_r 2015-02-19 - Need to add e2e test. -->
-                    <div class="page-header__feedback ng-hide" data-ng-hide="isActionError() || isDeployError() || isSSHKeyError()">
+                    <div class="page-header__feedback ng-hide" data-ng-hide="isActionError() || isDeployError() || isSSHKeyError() || hasActionPowerError(actionOption.name)">
                         <form class="form-inline">
                             <div class="eight-col no-margin-bottom ng-hide" data-ng-show="actionOption.name === 'commission'">
                                 <div class="inline four-col">
@@ -104,6 +104,17 @@
                         </div>
                     </div>
 
+                    <div class="page-header__feedback ng-hide" data-ng-show="hasActionPowerError(actionOption.name)">
+                        <p class="page-header__feedback-message info">
+                            Node cannot be {$ actionOption.sentence $}, because power control software for the 
+                            node is missing from its cluster controller. To proceed, install the
+                            {$ getPowerErrors() $} on the {$ node.nodegroup.cluster_name $} cluster.
+                        </p>
+                        <div class="right">
+                            <a href="" class="margin-right" data-ng-click="actionCancel()">Cancel</a>
+                        </div>
+                    </div>
+
                     <div class="page-header__feedback ng-hide" data-ng-show="isSSHKeyError()">
                         <p class="page-header__feedback-message info">
                             Node cannot be {$ actionOption.sentence $}, because an SSH key has not been added to your account. To add an SSH key, visit <a href="account/prefs/">your account page</a>.
@@ -270,9 +281,9 @@
                 <div class="twelve-col ng-hide error" data-ng-show="hasPowerError()">
                     <ul class="flash-messages">
                         <li class="flash-messages__item error">
-                            To control power with this power type, install the
-                            '<span ng-repeat="error in getPowerError()">{$ error $}{$$last ? '' : ', '$}</span>'
-                            package(s) on cluster '{$ node.nodegroup.name $}'
+                            Power control software for this power type is missing from the
+                            cluster controller.  To proceed, install the {$ getPowerErrors() $}
+                            on the {$ node.nodegroup.cluster_name $} cluster.
                         </li>
                     </ul>
                 </div>

=== modified file 'src/provisioningserver/power/change.py'
--- src/provisioningserver/power/change.py	2015-09-25 14:39:44 +0000
+++ src/provisioningserver/power/change.py	2015-10-20 18:00:40 +0000
@@ -22,6 +22,7 @@
 from provisioningserver.drivers.power import (
     DEFAULT_WAITING_POLICY,
     get_error_message,
+    power_drivers_by_name,
     PowerDriverRegistry,
 )
 from provisioningserver.events import (
@@ -200,6 +201,16 @@
     assert power_change in ('on', 'off'), (
         "Unknown power change: %s" % power_change)
 
+    power_driver = power_drivers_by_name.get(power_type)
+    if power_driver is None:
+        raise poweraction.PowerActionFail(
+            "Unknown power_type '%s'" % power_type)
+    missing_packages = power_driver.detect_missing_packages()
+    if len(missing_packages):
+        raise poweraction.PowerActionFail(
+            "'%s' package(s) are not installed" % " ".join(
+                missing_packages))
+
     # There should be one and only one power change for each system ID.
     if system_id in power.power_action_registry:
         current_power_change, d = power.power_action_registry[system_id]

=== modified file 'src/provisioningserver/power/query.py'
--- src/provisioningserver/power/query.py	2015-10-02 11:18:07 +0000
+++ src/provisioningserver/power/query.py	2015-10-20 18:00:40 +0000
@@ -23,6 +23,7 @@
 from provisioningserver import power
 from provisioningserver.drivers.power import (
     DEFAULT_WAITING_POLICY,
+    power_drivers_by_name,
     PowerDriverRegistry,
 )
 from provisioningserver.events import (
@@ -107,6 +108,16 @@
     # Capture errors as we go along.
     exc_info = None, None, None
 
+    power_driver = power_drivers_by_name.get(power_type)
+    if power_driver is None:
+        raise poweraction.PowerActionFail(
+            "Unknown power_type '%s'" % power_type)
+    missing_packages = power_driver.detect_missing_packages()
+    if len(missing_packages):
+        raise poweraction.PowerActionFail(
+            "'%s' package(s) are not installed" % ", ".join(
+                missing_packages))
+
     if power.is_driver_available(power_type):
         # New-style power drivers handle retries for themselves, so we only
         # ever call them once.

=== modified file 'src/provisioningserver/power/tests/test_change.py'
--- src/provisioningserver/power/tests/test_change.py	2015-10-01 19:33:36 +0000
+++ src/provisioningserver/power/tests/test_change.py	2015-10-20 18:00:40 +0000
@@ -41,6 +41,7 @@
 from provisioningserver.drivers.power import (
     DEFAULT_WAITING_POLICY,
     get_error_message as get_driver_error_message,
+    power_drivers_by_name,
     PowerDriverRegistry,
     PowerError,
 )
@@ -553,6 +554,9 @@
     def setUp(self):
         super(TestMaybeChangePowerState, self).setUp()
         self.patch(power, 'power_action_registry', {})
+        for power_driver in power_drivers_by_name.values():
+            self.patch(
+                power_driver, "detect_missing_packages").return_value = []
         self.useFixture(EventTypesAllRegistered())
         do_not_pause(self)
 
@@ -566,8 +570,9 @@
 
     def test_always_returns_deferred(self):
         clock = Clock()
+        power_type = random.choice(power.QUERY_POWER_TYPES)
         d = power.change.maybe_change_power_state(
-            sentinel.system_id, sentinel.hostname, sentinel.power_type,
+            sentinel.system_id, sentinel.hostname, power_type,
             random.choice(("on", "off")), sentinel.context, clock=clock)
         self.assertThat(d, IsInstance(Deferred))
 
@@ -592,6 +597,46 @@
         self.assertEqual({}, power.power_action_registry)
 
     @inlineCallbacks
+    def test_checks_missing_packages(self):
+        self.patch_methods_using_rpc()
+
+        system_id = factory.make_name('system_id')
+        hostname = factory.make_name('hostname')
+        power_type = random.choice(power.QUERY_POWER_TYPES)
+        power_change = random.choice(['on', 'off'])
+        context = {
+            factory.make_name('context-key'): factory.make_name('context-val')
+        }
+        power_driver = power_drivers_by_name.get(power_type)
+        detect_packages = self.patch_autospec(
+            power_driver, "detect_missing_packages")
+        detect_packages.return_value = []
+        yield power.change.maybe_change_power_state(
+            system_id, hostname, power_type, power_change, context)
+        reactor.runUntilCurrent()  # Run all delayed calls.
+        self.assertThat(detect_packages, MockCalledOnceWith())
+
+    @inlineCallbacks
+    def test_errors_when_missing_packages(self):
+        self.patch_methods_using_rpc()
+
+        system_id = factory.make_name('system_id')
+        hostname = factory.make_name('hostname')
+        power_type = random.choice(power.QUERY_POWER_TYPES)
+        power_change = random.choice(['on', 'off'])
+        context = {
+            factory.make_name('context-key'): factory.make_name('context-val')
+        }
+        power_driver = power_drivers_by_name.get(power_type)
+        detect_packages = self.patch_autospec(
+            power_driver, "detect_missing_packages")
+        detect_packages.return_value = ['gone']
+        with ExpectedException(poweraction.PowerActionFail):
+            yield power.change.maybe_change_power_state(
+                system_id, hostname, power_type, power_change, context)
+        self.assertThat(detect_packages, MockCalledOnceWith())
+
+    @inlineCallbacks
     def test_errors_when_change_conflicts_with_in_progress_change(self):
         system_id = factory.make_name('system_id')
         hostname = factory.make_name('hostname')
@@ -673,7 +718,7 @@
 
         system_id = factory.make_name('system_id')
         hostname = factory.make_hostname()
-        power_type = sentinel.power_type
+        power_type = random.choice(power.QUERY_POWER_TYPES)
         power_change = random.choice(['on', 'off'])
         context = sentinel.context
 
@@ -702,7 +747,7 @@
 
         system_id = factory.make_name('system_id')
         hostname = factory.make_hostname()
-        power_type = sentinel.power_type
+        power_type = random.choice(power.QUERY_POWER_TYPES)
         power_change = random.choice(['on', 'off'])
         context = sentinel.context
 

=== modified file 'src/provisioningserver/power/tests/test_query.py'
--- src/provisioningserver/power/tests/test_query.py	2015-10-02 11:18:07 +0000
+++ src/provisioningserver/power/tests/test_query.py	2015-10-20 18:00:40 +0000
@@ -48,6 +48,7 @@
 from provisioningserver import power
 from provisioningserver.drivers.power import (
     DEFAULT_WAITING_POLICY,
+    power_drivers_by_name,
     PowerDriverRegistry,
 )
 from provisioningserver.events import EVENT_TYPES
@@ -127,7 +128,7 @@
         protocol, io = self.patch_rpc_methods()
         d = power.query.power_query_failure(
             system_id, hostname, Failure(Exception(message)))
-        # This blocks until the deferred is complete
+        # This blocks until the deferred is complete.
         io.flush()
         self.assertIsNone(extract_result(d))
         self.assertThat(
@@ -145,6 +146,9 @@
         super(TestPowerQuery, self).setUp()
         self.useFixture(EventTypesAllRegistered())
         self.patch(power.query, "deferToThread", maybeDeferred)
+        for power_driver in power_drivers_by_name.values():
+            self.patch(
+                power_driver, "detect_missing_packages").return_value = []
 
     def patch_rpc_methods(self, return_value={}, side_effect=None):
         fixture = self.useFixture(MockClusterToRegionRPCFixture())
@@ -164,6 +168,12 @@
             factory.make_name('context-key'): factory.make_name('context-val')
         }
         self.patch(power, 'is_driver_available').return_value = False
+
+        power_driver = power_drivers_by_name.get(power_type)
+        detect_packages = self.patch_autospec(
+            power_driver, "detect_missing_packages")
+        detect_packages.return_value = []
+
         # Patch the power action utility so that it says the node is
         # in on/off power state.
         power_action, execute = patch_PowerAction(
@@ -172,9 +182,10 @@
 
         d = power.query.get_power_state(
             system_id, hostname, power_type, context)
-        # This blocks until the deferred is complete
+        # This blocks until the deferred is complete.
         io.flush()
         self.assertEqual(power_state, extract_result(d))
+        self.assertThat(detect_packages, MockCalledOnceWith())
         self.assertThat(
             execute,
             MockCallsMatch(
@@ -183,6 +194,32 @@
             ),
         )
 
+    def test_get_power_state_fails_for_missing_packages(self):
+        system_id = factory.make_name('system_id')
+        hostname = factory.make_name('hostname')
+        power_type = random.choice(power.QUERY_POWER_TYPES)
+        power_state = random.choice(['on', 'off'])
+        context = {
+            factory.make_name('context-key'): factory.make_name('context-val')
+        }
+        self.patch(power, 'is_driver_available').return_value = False
+        # Patch the power action utility so that it says the node is
+        # in on/off power state.
+        power_action, execute = patch_PowerAction(
+            self, return_value=power_state)
+        _, markNodeBroken, io = self.patch_rpc_methods()
+
+        power_driver = power_drivers_by_name.get(power_type)
+        detect_packages = self.patch_autospec(
+            power_driver, "detect_missing_packages")
+        detect_packages.return_value = ['gone']
+
+        d = power.query.get_power_state(
+            system_id, hostname, power_type, context)
+
+        self.assertThat(detect_packages, MockCalledOnceWith())
+        return assert_fails_with(d, poweraction.PowerActionFail)
+
     def test_get_power_state_returns_unknown_for_certain_power_types(self):
         system_id = factory.make_name('system_id')
         hostname = factory.make_name('hostname')
@@ -217,7 +254,7 @@
 
         d = power.query.get_power_state(
             system_id, hostname, power_type, context)
-        # This blocks until the deferred is complete
+        # This blocks until the deferred is complete.
         io.flush()
         self.assertEqual(power_state, extract_result(d))
         self.assertThat(
@@ -320,7 +357,7 @@
             system_id, hostname, power_type, context, clock=clock)
         for newcalls, waiting_time in calls_and_pause:
             calls.extend(newcalls)
-            # This blocks until the deferred is complete
+            # This blocks until the deferred is complete.
             io.flush()
             self.assertThat(execute, MockCallsMatch(*calls))
             clock.advance(waiting_time)
@@ -332,6 +369,7 @@
     scenarios = tuple(
         (power_type, {
             "power_type": power_type,
+            "power_driver": power_drivers_by_name.get(power_type),
             "func": (  # Function to invoke driver.
                 "perform_power_driver_query"
                 if power_type in PowerDriverRegistry
@@ -367,6 +405,9 @@
         send_event_node = self.patch_autospec(power.query, "send_event_node")
         send_event_node.return_value = succeed(None)
 
+        self.patch(
+            self.power_driver, "detect_missing_packages").return_value = []
+
         system_id = factory.make_name('system_id')
         hostname = factory.make_name('hostname')
         context = sentinel.context

=== modified file 'src/provisioningserver/rpc/cluster.py'
--- src/provisioningserver/rpc/cluster.py	2015-09-10 19:19:24 +0000
+++ src/provisioningserver/rpc/cluster.py	2015-10-20 18:00:40 +0000
@@ -29,6 +29,7 @@
     "PowerOff",
     "PowerOn",
     "PowerQuery",
+    "PowerDriverCheck",
     "ValidateLicenseKey",
 ]
 
@@ -187,6 +188,26 @@
     }
 
 
+class PowerDriverCheck(amp.Command):
+    """Check power driver on cluster for missing packages
+
+    :since: 1.9
+    """
+
+    arguments = [
+        (b"power_type", amp.Unicode()),
+    ]
+    response = [
+        (b"missing_packages", amp.ListOf(amp.Unicode())),
+    ]
+    errors = {
+        UnknownPowerType: (
+            b"UnknownPowerType"),
+        NotImplementedError: (
+            b"NotImplementedError"),
+    }
+
+
 class GetPreseedData(amp.Command):
     """Get OS-specific preseed data.
 

=== modified file 'src/provisioningserver/rpc/clusterservice.py'
--- src/provisioningserver/rpc/clusterservice.py	2015-09-30 07:29:27 +0000
+++ src/provisioningserver/rpc/clusterservice.py	2015-10-20 18:00:40 +0000
@@ -41,9 +41,11 @@
 from provisioningserver.drivers.hardware.ucsm import probe_and_enlist_ucsm
 from provisioningserver.drivers.hardware.virsh import probe_virsh_and_enlist
 from provisioningserver.drivers.hardware.vmware import probe_vmware_and_enlist
+from provisioningserver.drivers.power import power_drivers_by_name
 from provisioningserver.logger.log import get_maas_logger
 from provisioningserver.network import discover_networks
 from provisioningserver.power.change import maybe_change_power_state
+from provisioningserver.power.poweraction import UnknownPowerType
 from provisioningserver.power.query import get_power_state
 from provisioningserver.rpc import (
     cluster,
@@ -272,6 +274,15 @@
         d.addCallback(lambda x: {'state': x})
         return d
 
+    @cluster.PowerDriverCheck.responder
+    def power_driver_check(self, power_type):
+        """Return a list of missing power driver packages, if any."""
+        driver = power_drivers_by_name.get(power_type)
+        if driver is None:
+            raise UnknownPowerType(
+                "No driver found for power type '%s'" % power_type)
+        return {"missing_packages": driver.detect_missing_packages()}
+
     @cluster.ConfigureDHCPv4.responder
     def configure_dhcpv4(self, omapi_key, subnet_configs):
         server = dhcp.DHCPv4Server(omapi_key)

=== modified file 'src/provisioningserver/rpc/tests/test_clusterservice.py'
--- src/provisioningserver/rpc/tests/test_clusterservice.py	2015-10-02 13:27:07 +0000
+++ src/provisioningserver/rpc/tests/test_clusterservice.py	2015-10-20 18:00:40 +0000
@@ -63,6 +63,7 @@
     OperatingSystem,
     OperatingSystemRegistry,
 )
+from provisioningserver.drivers.power import power_drivers_by_name
 from provisioningserver.network import discover_networks
 from provisioningserver.power import QUERY_POWER_TYPES
 from provisioningserver.power.poweraction import (
@@ -1587,12 +1588,20 @@
         report_power_state = self.patch(
             power_module.query, "report_power_state")
 
+        power_type = random.choice(QUERY_POWER_TYPES)
         arguments = {
             'system_id': factory.make_name('system'),
             'hostname': factory.make_name('hostname'),
-            'power_type': random.choice(QUERY_POWER_TYPES),
+            'power_type': power_type,
             'context': factory.make_name('context'),
         }
+
+        # Make sure power driver doesn't check for installed packages.
+        power_driver = power_drivers_by_name.get(power_type)
+        if power_driver:
+            self.patch_autospec(
+                power_driver, "detect_missing_packages").return_value = []
+
         observed = yield call_responder(
             Cluster(), cluster.PowerQuery, arguments)
         self.assertEqual({'state': state}, observed)