Merge lp:~ed-leafe/nova/powerstate into lp:~hudson-openstack/nova/trunk

[Ed Leafe]

This branch adds additional capability to the hosts API extension. The new options allow an admin to reboot or shutdown a host. I also added code to hide this extension if the --allow-admin-api is False, as regular users should have no access to host API calls.

[Vish Ishaya (vishvananda)]

79 -# vim: tabstop=4 shiftwidth=4 softtabstop=4
80 +#: tabstop=4 shiftwidth=4 softtabstop=4

That looks accidental.

I find the terminology a little confusing. What is the difference between set_power_state and set_host_powerstate?
Also, if i understand the code correctly, implementing it for libvirt it should be as simple as utils.execute("sudo", "shutdown", "-r", "now")

review: Needs Information

[Ed Leafe (ed-leafe)]

> 79 -# vim: tabstop=4 shiftwidth=4 softtabstop=4
> 80 +#: tabstop=4 shiftwidth=4 softtabstop=4
>
> That looks accidental.

It sure wasn't intentional. Fixed.

> I find the terminology a little confusing. What is the difference between
> set_power_state and set_host_powerstate?

Ugh, that's from an earlier version of the branch that I merged. There was already some stuff for VMs that used the term 'power_state', so I changed them in the newer version to be explicit that they were concerned with the host's power state. I'll have to clean that up and re-propose for merging.

> Also, if i understand the code correctly, implementing it for libvirt it
> should be as simple as utils.execute("sudo", "shutdown", "-r", "now")

For reboot, yeah, probably. Since I'm not using libvert, though, I left that for others to code and test. :)

[Soren Hansen (soren)]

It seems odd to me to think of "reboot" as a state.

Also, I think we should aim to add a get_* method for every set_* method, but reading back a "shutdown" or "reboot" state sounds really strange. :)

Maybe a more RPC-like interface would be in order?

[Ed Leafe (ed-leafe)]

> It seems odd to me to think of "reboot" as a state.

Would it help to think of it as a state change? :)

> Also, I think we should aim to add a get_* method for every set_* method, but
> reading back a "shutdown" or "reboot" state sounds really strange. :)

That's probably why dogmatic coding is a bad thing. :)

> Maybe a more RPC-like interface would be in order?

The request was for a RESTful API for managing the hosts for admins. Presumably then tools will be developed around that API. The tools themselves will likely be developed by each company that uses OpenStack for their own needs and deployment details. If this were for an action that would be carried out as part of normal Nova activity, then yes, an RPC interface might be more appropriate, but as this deals with the physical hosts and not the VMs, we went the RESTful API route.

[Trey Morris (tr3buchet)]

I'm with soren, reboot isn't really a state, I see it as an action.
Let's say you take action reboot, the host goes in to state "shutdown" until it comes back up and then it's "online" or i guess maybe "enabled" from the looks of it. It doesn't look like we track host state as we do vm states. Unless I'm mistaken hosts are either enabled or not.

I like the idea of specific actions for reboot, shutdown, and start (similar to _set_enabled_status) instead of a single set_powerstate action. It just seems more accurate and more simple.

These are somewhat drastic suggestions though..

I watched you demonstrate this and the code looks alright otherwise.

[Ed Leafe (ed-leafe)]

OK, I've changed from the PUT approach to an action-based API. I've also changed all references to 'host_power_state' to 'host_power_action'.

[Trey Morris (tr3buchet)]

I definitely like this better. The admin_only decorator is way better than checking FLAGS.allow_admin_api. I wonder if there is a way to decorate each exposed function with it instead of get_resources().

Out of curiosity, why keep power_action at all? Why combine only reboot and shutdown into power_action? If all of the actions were going to use power_action, it makes sense, but all of your docstrings say "reboot or shutdown". Maybe it would read better if the docstrings were more accurate and startup used power_action as well.

I also think that raising when calling startup should be moved in the virt/connection area. API shouldn't claim to know underlying technology and block the request, the underlying technology should raise if it can't complete the call.

[Ed Leafe (ed-leafe)]

> I definitely like this better. The admin_only decorator is way better than
> checking FLAGS.allow_admin_api. I wonder if there is a way to decorate each
> exposed function with it instead of get_resources().

Probably not, but that's a good thing - we shouldn't be mixing admin-only stuff with general user functionality - too much of a chance for someone down the line to confuse which is which. The idea is that if this is not running in an admin situation, the module will not get loaded at all.

> Out of curiosity, why keep power_action at all? Why combine only reboot and
> shutdown into power_action? If all of the actions were going to use
> power_action, it makes sense, but all of your docstrings say "reboot or
> shutdown". Maybe it would read better if the docstrings were more accurate and
> startup used power_action as well.

OK, I changed the docstrings to allow for all the actions.

> I also think that raising when calling startup should be moved in the
> virt/connection area. API shouldn't claim to know underlying technology and
> block the request, the underlying technology should raise if it can't complete
> the call.

Good catch - done.

[Trey Morris (tr3buchet)]

 > Probably not, but that's a good thing - we shouldn't be mixing admin-only stuff with general user
 > functionality - too much of a chance for someone down the line to confuse which is which. The idea
 > is that if this is not running in an admin situation, the module will not get loaded at all.

we're saying the same thing here. Would be nice if we could decorate the functions as @admin_api so that right there with the function definition you specify it's going to be a part of the admin api. Then get_resources() could always return all of the functions, excepting those with @admin_only decorators when the admin api isn't specified, which in this case would turn out to be [].

 > Good catch - done.

why not make the non-xen hypervisors raise NotImplemented instead of passing in virt/hypervisor/connection.py? This way it will be logged when you try to perform action that isn't underneath the hood, otherwise how would you know?

[Trey Morris (tr3buchet)]

Just want to add, I don't think you should come up with a function decorator for @admin_only as I described, I was just thinking out loud i guess.

[Ed Leafe (ed-leafe)]

> > Probably not, but that's a good thing - we shouldn't be mixing admin-only
> stuff with general user
> > functionality - too much of a chance for someone down the line to confuse
> which is which. The idea
> > is that if this is not running in an admin situation, the module will not
> get loaded at all.
>
> we're saying the same thing here. Would be nice if we could decorate the
> functions as @admin_api so that right there with the function definition you
> specify it's going to be a part of the admin api. Then get_resources() could
> always return all of the functions, excepting those with @admin_only
> decorators when the admin api isn't specified, which in this case would turn
> out to be [].

Again, I think that mixing the two in a single extension is a poor design. It would be much safer and cleaner to create two separate extensions if you needed both public and admin functionality.

> > Good catch - done.
>
> why not make the non-xen hypervisors raise NotImplemented instead of passing
> in virt/hypervisor/connection.py? This way it will be logged when you try to
> perform action that isn't underneath the hood, otherwise how would you know?

Just following the pattern of all the other non-implemented methods. If you think that these should raise exceptions instead of simply passing, you should file a separate bug, and have them all changed together.

[Trey Morris (tr3buchet)]

alrighty, good to go on my end!

review: Approve

[Josh Kearney (jk0)]

Everything looks good except for one little thing:

275 + except TypeError as e:

There are two spaces before as.

review: Needs Fixing

[Rick Harris (rconradharris)]

Nice job, Ed. Some small nits:

> + """Starts the host. NOTE: Currently not feasible, since the host

Per Hacking, this should probably be NOTE(dabo):

> + except TypeError as e:

Two spaces between 'TypeError' and 'as'. (Surprised Pep8 didn't catch this)

> 124 + def host_power_action(self, context, instance_id=None, host=None,
> 125 + action=None):

Should this take an instance_id parameter?

Since compute api requires that 'host' and 'action' are specified, could we have manager.py mirror that?

    def host_power_action(self, context, host, action):
        do stuff

In that case,

110 + return self._call_compute_message("host_power_action", context,
111 + instance_id=None, host=host, params={"action": action})

would become:

    return self._call_compute_message("host_power_action", context,
        host=host, action=action)

> 32 + return []

Out of curiosity, should this raise a 4XX (maybe a 401 error) to let the user know that they are doing something wrong if they are trying to access an admin_only api call?

review: Needs Fixing

[Ed Leafe (ed-leafe)]

> > + """Starts the host. NOTE: Currently not feasible, since the host
>
> Per Hacking, this should probably be NOTE(dabo):

That was left over when copied from another part of the code. I just removed the NOTE:, since it really isn't needed.

> > + except TypeError as e:
>
> Two spaces between 'TypeError' and 'as'. (Surprised Pep8 didn't catch this)

You and me both. Fixed.

> > 124 + def host_power_action(self, context, instance_id=None, host=None,
> > 125 + action=None):
>
> Should this take an instance_id parameter?

I think that this was the result of copying the signatures of similar methods. I've removed the instance_id references from both manager and api.

> > 32 + return []
>
> Out of curiosity, should this raise a 4XX (maybe a 401 error) to let the user
> know that they are doing something wrong if they are trying to access an
> admin_only api call?

This isn't a runtime thing; it only comes into play when the extensions are loaded. It is analogous to the code for the non-extension admin API methods: if the flag is False, they simply aren't loaded. At runtime, if the user tries to call them, they get a 404, since they simply aren't defined.

[Josh Kearney (jk0)]

LGTM

review: Approve

[Rick Harris (rconradharris)]

> This isn't a runtime thing; it only comes into play when the extensions are loaded.

Gotcha.

> 124 + def host_power_action(self, context, instance_id=None, host=None,
> 125 + action=None):

The other part that slightly stood out was having host and action default to None. I would think those should be required arguments (?)

Otherwise, looks great, thanks for the fixes, Ed.

review: Approve

=== added file 'nova/api/openstack/contrib/admin_only.py'
--- nova/api/openstack/contrib/admin_only.py	1970-01-01 00:00:00 +0000
+++ nova/api/openstack/contrib/admin_only.py	2011-08-08 17:37:25 +0000
@@ -0,0 +1,30 @@
+# Copyright (c) 2011 Openstack, LLC.
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+"""Decorator for limiting extensions that should be admin-only."""
+
+from functools import wraps
+from nova import flags
+FLAGS = flags.FLAGS
+
+
+def admin_only(fnc):
+    @wraps(fnc)
+    def _wrapped(self, *args, **kwargs):
+        if FLAGS.allow_admin_api:
+            return fnc(self, *args, **kwargs)
+        return []
+    _wrapped.func_name = fnc.func_name
+    return _wrapped

=== modified file 'nova/api/openstack/contrib/hosts.py'
--- nova/api/openstack/contrib/hosts.py	2011-07-06 16:53:08 +0000
+++ nova/api/openstack/contrib/hosts.py	2011-08-08 17:37:25 +0000
@@ -24,6 +24,7 @@
 from nova.api.openstack import common
 from nova.api.openstack import extensions
 from nova.api.openstack import faults
+from nova.api.openstack.contrib import admin_only
 from nova.scheduler import api as scheduler_api
 
 
@@ -70,7 +71,7 @@
             key = raw_key.lower().strip()
             val = raw_val.lower().strip()
             # NOTE: (dabo) Right now only 'status' can be set, but other
-            # actions may follow.
+            # settings may follow.
             if key == "status":
                 if val[:6] in ("enable", "disabl"):
                     return self._set_enabled_status(req, id,
@@ -89,8 +90,30 @@
         LOG.audit(_("Setting host %(host)s to %(state)s.") % locals())
         result = self.compute_api.set_host_enabled(context, host=host,
                 enabled=enabled)
+        if result not in ("enabled", "disabled"):
+            # An error message was returned
+            raise webob.exc.HTTPBadRequest(explanation=result)
         return {"host": host, "status": result}
 
+    def _host_power_action(self, req, host, action):
+        """Reboots, shuts down or powers up the host."""
+        context = req.environ['nova.context']
+        try:
+            result = self.compute_api.host_power_action(context, host=host,
+                    action=action)
+        except NotImplementedError as e:
+            raise webob.exc.HTTPBadRequest(explanation=e.msg)
+        return {"host": host, "power_action": result}
+
+    def startup(self, req, id):
+        return self._host_power_action(req, host=id, action="startup")
+
+    def shutdown(self, req, id):
+        return self._host_power_action(req, host=id, action="shutdown")
+
+    def reboot(self, req, id):
+        return self._host_power_action(req, host=id, action="reboot")
+
 
 class Hosts(extensions.ExtensionDescriptor):
     def get_name(self):
@@ -108,7 +131,10 @@
     def get_updated(self):
         return "2011-06-29T00:00:00+00:00"
 
+    @admin_only.admin_only
     def get_resources(self):
-        resources = [extensions.ResourceExtension('os-hosts', HostController(),
-                collection_actions={'update': 'PUT'}, member_actions={})]
+        resources = [extensions.ResourceExtension('os-hosts',
+                HostController(), collection_actions={'update': 'PUT'},
+                member_actions={"startup": "GET", "shutdown": "GET",
+                        "reboot": "GET"})]
         return resources

=== modified file 'nova/compute/api.py'
--- nova/compute/api.py	2011-08-05 23:41:03 +0000
+++ nova/compute/api.py	2011-08-08 17:37:25 +0000
@@ -997,7 +997,12 @@
     def set_host_enabled(self, context, host, enabled):
         """Sets the specified host's ability to accept new instances."""
         return self._call_compute_message("set_host_enabled", context,
-                instance_id=None, host=host, params={"enabled": enabled})
+                host=host, params={"enabled": enabled})
+
+    def host_power_action(self, context, host, action):
+        """Reboots, shuts down or powers up the host."""
+        return self._call_compute_message("host_power_action", context,
+                host=host, params={"action": action})
 
     @scheduler_api.reroute_compute("diagnostics")
     def get_diagnostics(self, context, instance_id):

=== modified file 'nova/compute/manager.py'
--- nova/compute/manager.py	2011-08-05 14:07:48 +0000
+++ nova/compute/manager.py	2011-08-08 17:37:25 +0000
@@ -958,8 +958,12 @@
                                                        result))
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
-    def set_host_enabled(self, context, instance_id=None, host=None,
-            enabled=None):
+    def host_power_action(self, context, host=None, action=None):
+        """Reboots, shuts down or powers up the host."""
+        return self.driver.host_power_action(host, action)
+
+    @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
+    def set_host_enabled(self, context, host=None, enabled=None):
         """Sets the specified host's ability to accept new instances."""
         return self.driver.set_host_enabled(host, enabled)
 

=== modified file 'nova/tests/test_hosts.py'
--- nova/tests/test_hosts.py	2011-07-06 17:14:46 +0000
+++ nova/tests/test_hosts.py	2011-08-08 17:37:25 +0000
@@ -48,6 +48,10 @@
     return status
 
 
+def stub_host_power_action(context, host, action):
+    return action
+
+
 class FakeRequest(object):
     environ = {"nova.context": context.get_admin_context()}
 
@@ -62,6 +66,8 @@
         self.stubs.Set(scheduler_api, 'get_host_list', stub_get_host_list)
         self.stubs.Set(self.controller.compute_api, 'set_host_enabled',
                 stub_set_host_enabled)
+        self.stubs.Set(self.controller.compute_api, 'host_power_action',
+                stub_host_power_action)
 
     def test_list_hosts(self):
         """Verify that the compute hosts are returned."""
@@ -87,6 +93,18 @@
         result_c2 = self.controller.update(self.req, "host_c2", body=en_body)
         self.assertEqual(result_c2["status"], "disabled")
 
+    def test_host_startup(self):
+        result = self.controller.startup(self.req, "host_c1")
+        self.assertEqual(result["power_action"], "startup")
+
+    def test_host_shutdown(self):
+        result = self.controller.shutdown(self.req, "host_c1")
+        self.assertEqual(result["power_action"], "shutdown")
+
+    def test_host_reboot(self):
+        result = self.controller.reboot(self.req, "host_c1")
+        self.assertEqual(result["power_action"], "reboot")
+
     def test_bad_status_value(self):
         bad_body = {"status": "bad"}
         self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update,

=== modified file 'nova/virt/driver.py'
--- nova/virt/driver.py	2011-07-29 21:58:27 +0000
+++ nova/virt/driver.py	2011-08-08 17:37:25 +0000
@@ -282,6 +282,10 @@
         # TODO(Vek): Need to pass context in for access to auth_token
         raise NotImplementedError()
 
+    def host_power_action(self, host, action):
+        """Reboots, shuts down or powers up the host."""
+        raise NotImplementedError()
+
     def set_host_enabled(self, host, enabled):
         """Sets the specified host's ability to accept new instances."""
         # TODO(Vek): Need to pass context in for access to auth_token

=== modified file 'nova/virt/fake.py'
--- nova/virt/fake.py	2011-08-02 21:45:57 +0000
+++ nova/virt/fake.py	2011-08-08 17:37:25 +0000
@@ -512,6 +512,10 @@
         """Return fake Host Status of ram, disk, network."""
         return self.host_status
 
+    def host_power_action(self, host, action):
+        """Reboots, shuts down or powers up the host."""
+        pass
+
     def set_host_enabled(self, host, enabled):
         """Sets the specified host's ability to accept new instances."""
         pass

=== modified file 'nova/virt/hyperv.py'
--- nova/virt/hyperv.py	2011-07-29 21:58:27 +0000
+++ nova/virt/hyperv.py	2011-08-08 17:37:25 +0000
@@ -499,6 +499,10 @@
         """See xenapi_conn.py implementation."""
         pass
 
+    def host_power_action(self, host, action):
+        """Reboots, shuts down or powers up the host."""
+        pass
+
     def set_host_enabled(self, host, enabled):
         """Sets the specified host's ability to accept new instances."""
         pass

=== modified file 'nova/virt/libvirt/connection.py'
--- nova/virt/libvirt/connection.py	2011-08-05 18:14:26 +0000
+++ nova/virt/libvirt/connection.py	2011-08-08 17:37:25 +0000
@@ -1562,6 +1562,10 @@
         """See xenapi_conn.py implementation."""
         pass
 
+    def host_power_action(self, host, action):
+        """Reboots, shuts down or powers up the host."""
+        pass
+
     def set_host_enabled(self, host, enabled):
         """Sets the specified host's ability to accept new instances."""
         pass

=== modified file 'nova/virt/vmwareapi_conn.py'
--- nova/virt/vmwareapi_conn.py	2011-07-29 21:58:27 +0000
+++ nova/virt/vmwareapi_conn.py	2011-08-08 17:37:25 +0000
@@ -191,6 +191,10 @@
         """This method is supported only by libvirt."""
         return
 
+    def host_power_action(self, host, action):
+        """Reboots, shuts down or powers up the host."""
+        pass
+
     def set_host_enabled(self, host, enabled):
         """Sets the specified host's ability to accept new instances."""
         pass

=== modified file 'nova/virt/xenapi/vmops.py'
--- nova/virt/xenapi/vmops.py	2011-08-04 21:04:21 +0000
+++ nova/virt/xenapi/vmops.py	2011-08-08 17:37:25 +0000
@@ -1031,11 +1031,23 @@
         # TODO: implement this!
         return 'http://fakeajaxconsole/fake_url'
 
+    def host_power_action(self, host, action):
+        """Reboots or shuts down the host."""
+        args = {"action": json.dumps(action)}
+        methods = {"reboot": "host_reboot", "shutdown": "host_shutdown"}
+        json_resp = self._call_xenhost(methods[action], args)
+        resp = json.loads(json_resp)
+        return resp["power_action"]
+
     def set_host_enabled(self, host, enabled):
         """Sets the specified host's ability to accept new instances."""
         args = {"enabled": json.dumps(enabled)}
-        json_resp = self._call_xenhost("set_host_enabled", args)
-        resp = json.loads(json_resp)
+        xenapi_resp = self._call_xenhost("set_host_enabled", args)
+        try:
+            resp = json.loads(xenapi_resp)
+        except TypeError as e:
+            # Already logged; return the message
+            return xenapi_resp.details[-1]
         return resp["status"]
 
     def _call_xenhost(self, method, arg_dict):
@@ -1051,7 +1063,7 @@
                     #args={"params": arg_dict})
             ret = self._session.wait_for_task(task, task_id)
         except self.XenAPI.Failure as e:
-            ret = None
+            ret = e
             LOG.error(_("The call to %(method)s returned an error: %(e)s.")
                     % locals())
         return ret

=== modified file 'nova/virt/xenapi_conn.py'
--- nova/virt/xenapi_conn.py	2011-08-05 14:07:48 +0000
+++ nova/virt/xenapi_conn.py	2011-08-08 17:37:25 +0000
@@ -332,6 +332,19 @@
            True, run the update first."""
         return self.HostState.get_host_stats(refresh=refresh)
 
+    def host_power_action(self, host, action):
+        """The only valid values for 'action' on XenServer are 'reboot' or
+        'shutdown', even though the API also accepts 'startup'. As this is
+        not technically possible on XenServer, since the host is the same
+        physical machine as the hypervisor, if this is requested, we need to
+        raise an exception.
+        """
+        if action in ("reboot", "shutdown"):
+            return self._vmops.host_power_action(host, action)
+        else:
+            msg = _("Host startup on XenServer is not supported.")
+            raise NotImplementedError(msg)
+
     def set_host_enabled(self, host, enabled):
         """Sets the specified host's ability to accept new instances."""
         return self._vmops.set_host_enabled(host, enabled)

=== modified file 'plugins/xenserver/xenapi/etc/xapi.d/plugins/xenhost'
--- plugins/xenserver/xenapi/etc/xapi.d/plugins/xenhost	2011-07-20 15:16:36 +0000
+++ plugins/xenserver/xenapi/etc/xapi.d/plugins/xenhost	2011-08-08 17:37:25 +0000
@@ -103,6 +103,45 @@
     return {"status": status}
 
 
+def _power_action(action):
+    host_uuid = _get_host_uuid()
+    # Host must be disabled first
+    result = _run_command("xe host-disable")
+    if result:
+        raise pluginlib.PluginError(result)
+    # All running VMs must be shutdown
+    result = _run_command("xe vm-shutdown --multiple power-state=running")
+    if result:
+        raise pluginlib.PluginError(result)
+    cmds = {"reboot": "xe host-reboot", "startup": "xe host-power-on",
+            "shutdown": "xe host-shutdown"}
+    result = _run_command(cmds[action])
+    # Should be empty string
+    if result:
+        raise pluginlib.PluginError(result)
+    return {"power_action": action}
+
+
+@jsonify
+def host_reboot(self, arg_dict):
+    """Reboots the host."""
+    return _power_action("reboot")
+
+
+@jsonify
+def host_shutdown(self, arg_dict):
+    """Reboots the host."""
+    return _power_action("shutdown")
+
+
+@jsonify
+def host_start(self, arg_dict):
+    """Starts the host. Currently not feasible, since the host
+    runs on the same machine as Xen.
+    """
+    return _power_action("startup")
+
+
 @jsonify
 def host_data(self, arg_dict):
     """Runs the commands on the xenstore host to return the current status
@@ -217,4 +256,7 @@
 if __name__ == "__main__":
     XenAPIPlugin.dispatch(
             {"host_data": host_data,
-            "set_host_enabled": set_host_enabled})
+            "set_host_enabled": set_host_enabled,
+            "host_shutdown": host_shutdown,
+            "host_reboot": host_reboot,
+            "host_start": host_start})