Merge lp:~rackspace-titan/nova/rebuild-server-lp730233 into lp:~hudson-openstack/nova/trunk

[Brian Waldon]

Adding support for server rebuild to v1.0 and v1.1 of the Openstack API

[Dan Prince (dan-prince)]

I also re-factored the metadata properties quota branch to be a separate method in lp:~rackspace-titan/nova/server_metadata_quotas.

I named the method _check_metadata_properties_quota and it now takes a dict instead of an array.

Not a big deal we can merge this either way just wanted to make note of it.

[Cory Wright (corywright)]

FYI, I'm getting a conflict when I merge the latest trunk with this branch. May want to update that.

[Brian Lamar (blamar)]

Should be good now. Thanks for the heads up

[Cory Wright (corywright)]

Nice work. lgtm.

review: Approve

[Brian Lamar (blamar)]

Setting back to "review" because this was semi-dependent on a server metadata branch. So several merges and small tweaks later it should be good to go.

[Matt Dietz (cerberus)]

32 + msg = _("Instance %d is currently being rebuilt.") % instance_id

This message might be misleading, since the instance could also be building for the first time. Also, since you're already stating that in the compute API, why not catch and reraise, maintaining the original message?

52 + def _check_metadata(self, metadata):
53 + """Ensure that the metadata given is of the correct type."""
54 + try:
55 + metadata.iteritems()

This is confusing. Better written as "if isinstance(metadata, dict):" and remove the check_metadata method altogether

61 + def _check_personalities(self, personalities):

A more suitable method name might be in order. are_personalities_base64_encoded perhaps?

65 + path = personality["path"]
66 + contents = personality["contents"]

You can check for keys with the "in" keyword: 'path' in personality

03 + except exception.Error as ex:
104 + msg = _("Error encountered attempting to rebuild instance "
105 + "%(instance_id): %(ex)") % locals()

The inferred strings are lacking format parameters. I suspect you haven't had this exception raised in testing? You should still use the normal string formatting parameters you would in any other string inference. "%d(some_var) -> %s(the_other_one)" % locals(). Also, perhaps some unit tests to trigger these exception
s are raised?

155 + def _check_injected_file_format(self, injected_files):

You seem to be doing a lot of base64 decodability checking. Perhaps you should move such a thing somewhere common so you're not repeating yourself? Also, could you rename the method to be more explicit? check_injected_file_base64_encoded? Or if you move the method, "is_base64_encoded"

211 + def _update_state(self, context, instance_id, state=None):

Need some information on this one. Why would we call it to update a state to None?

621 - db.instance_set_state(context.get_admin_context(),
622 - instance['id'],
623 - power_state.NOSTATE,
624 - 'launching')

I would be very cautious about adding new power states. I've seen plenty of cases in the past where assumptions are made about what state something is *not*, which can cause all sorts of havoc with a new state it's not familiar with. I don't see anything particularly glaring about what you've done, but be aware of the potential for disaster, especially with the EC2 API possibly also relying on a "NOSTATE"

Nits:
97 + args = [context, instance_id, image_id, metadata, personalities]
98 + self.compute_api.rebuild(*args)

I suspect this is to avoid extending past the line-length limit. However, instantiating a new list for it seems a little pointless to me.
My personal preference is to do this instead:

self.compute_api.rebuild(context, instance_id, image_id, metadata,
                         personalities)

review: Needs Fixing

[Mark Washenberger (markwash)]

> 155 + def _check_injected_file_format(self, injected_files):
>
> You seem to be doing a lot of base64 decodability checking. Perhaps you should
> move such a thing somewhere common so you're not repeating yourself? Also,
> could you rename the method to be more explicit?
> check_injected_file_base64_encoded? Or if you move the method,
> "is_base64_encoded"
>

IMO you don't have to even do the check; just go ahead and decode from base64 in the openstack api, (catch any errors,) and pass the info as a plain python string to the compute layer. There is some prior art around this in the various create methods if I am not mistaken. Take a look at _get_injected_files in api.openstack.servers.

Cheers.

[Brian Lamar (blamar)]

Thanks for the through review Matt!

> 32 + msg = _("Instance %d is currently being rebuilt.") %
> instance_id
>
> This message might be misleading, since the instance could also be building
> for the first time. Also, since you're already stating that in the compute
> API, why not catch and reraise, maintaining the original message?
>

I see the need that the real API (read: User-facing) will need a more specific and user-friendly error message than that of the Compute API. Reusing the exception.BuildInProgress exception with different messages serves this purpose. The exception message is for programmers and the message generated at the OpenStack API layer is for users. Maybe removing the message from the exception in the compute API would be a good compromise?

> 52 + def _check_metadata(self, metadata):
> 53 + """Ensure that the metadata given is of the correct type."""
> 54 + try:
> 55 + metadata.iteritems()
>
> This is confusing. Better written as "if isinstance(metadata, dict):" and
> remove the check_metadata method altogether

I tend to shy away from isinstance because really all I want out of that check is to know that the next section of code will run when I call `iteritems` on the object, if that makes sense. While I wouldn't call `isinstance` harmful, I do agree a bit with this: http://www.canonical.org/~kragen/isinstance/

>
> 61 + def _check_personalities(self, personalities):
>
> A more suitable method name might be in order.
> are_personalities_base64_encoded perhaps?
>
> 65 + path = personality["path"]
> 66 + contents = personality["contents"]
>
> You can check for keys with the "in" keyword: 'path' in personality

Understood, however checking with "in" could result in either a True/False response or a TypeError exception (if the object isn't iterable) in which I'll need a try/except anyway. I attempted to cut to the chase here and just check for Key/Type errors in one go. Maybe I'm just thinking about it too much though, which happens.

>
> 03 + except exception.Error as ex:
> 104 + msg = _("Error encountered attempting to rebuild instance "
> 105 + "%(instance_id): %(ex)") % locals()
>
> The inferred strings are lacking format parameters. I suspect you haven't had
> this exception raised in testing? You should still use the normal string
> formatting parameters you would in any other string inference. "%d(some_var)
> -> %s(the_other_one)" % locals(). Also, perhaps some unit tests to trigger
> these exception
> s are raised?

Great catch. I really don't think those exception handling blocks are useful at all and as such I've removed them. There was an identical one in the V11 controller.

>
> 155 + def _check_injected_file_format(self, injected_files):
>
> You seem to be doing a lot of base64 decodability checking. Perhaps you should
> move such a thing somewhere common so you're not repeating yourself? Also,
> could you rename the method to be more explicit?
> check_injected_file_base64_encoded? Or if you move the method,
> "is_base64_encoded"
>

I've removed all base64 references in the co...

[Matt Dietz (cerberus)]

Great feedback Brian. Sorry for the delay in getting back to you. I think on several of my points I must have been doing something else, because I go back now and don't really see the issue I was taking with them then. Good stuff, overall

review: Approve

[OpenStack Infra (hudson-openstack)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[Brian Waldon (bcwaldon)]

Resolved remaining issues. I ended up resolving an issue with nova.exception.ApiError that caused nova.tests.test_exception.ApiErrorTestCase to fail for me.

[Jay Pipes (jaypipes)]

Hey guys,

Very nice work on this patch, and excellent review back and forth.

One thing I noticed:

82 + msg = _("Could not parse imageRef from request.")
83 + return faults.Fault(exc.HTTPBadRequest(explanation=msg))

Please put a LOG.debug(msg) in between those lines, please, to make the v10 and v11 _rebuild_server() methods match.

Excellent work on the test cases and removing the db calls from the driver layer.

-jay

review: Needs Fixing

[Brian Waldon (bcwaldon)]

Thanks, Jay! Most of this was Lamar, but I'll take the credit :) I added that debug line.

[Jay Pipes (jaypipes)]

coolio. lgtm.

review: Approve

=== modified file 'nova/api/openstack/servers.py'
--- nova/api/openstack/servers.py	2011-05-02 07:55:54 +0000
+++ nova/api/openstack/servers.py	2011-05-03 16:43:57 +0000
@@ -317,10 +317,6 @@
             return faults.Fault(exc.HTTPBadRequest())
         return exc.HTTPAccepted()
 
-    def _action_rebuild(self, input_dict, req, id):
-        LOG.debug(_("Rebuild server action is not implemented"))
-        return faults.Fault(exc.HTTPNotImplemented())
-
     def _action_resize(self, input_dict, req, id):
         """ Resizes a given instance to the flavor size requested """
         try:
@@ -603,6 +599,28 @@
             except exception.TimeoutException:
                 return exc.HTTPRequestTimeout()
 
+    def _action_rebuild(self, info, request, instance_id):
+        context = request.environ['nova.context']
+        instance_id = int(instance_id)
+
+        try:
+            image_id = info["rebuild"]["imageId"]
+        except (KeyError, TypeError):
+            msg = _("Could not parse imageId from request.")
+            LOG.debug(msg)
+            return faults.Fault(exc.HTTPBadRequest(explanation=msg))
+
+        try:
+            self.compute_api.rebuild(context, instance_id, image_id)
+        except exception.BuildInProgress:
+            msg = _("Instance %d is currently being rebuilt.") % instance_id
+            LOG.debug(msg)
+            return faults.Fault(exc.HTTPConflict(explanation=msg))
+
+        response = exc.HTTPAccepted()
+        response.empty_body = True
+        return response
+
 
 class ControllerV11(Controller):
     def _image_id_from_req_data(self, data):
@@ -639,6 +657,63 @@
     def _limit_items(self, items, req):
         return common.limited_by_marker(items, req)
 
+    def _validate_metadata(self, metadata):
+        """Ensure that we can work with the metadata given."""
+        try:
+            metadata.iteritems()
+        except AttributeError as ex:
+            msg = _("Unable to parse metadata key/value pairs.")
+            LOG.debug(msg)
+            raise faults.Fault(exc.HTTPBadRequest(explanation=msg))
+
+    def _decode_personalities(self, personalities):
+        """Decode the Base64-encoded personalities."""
+        for personality in personalities:
+            try:
+                path = personality["path"]
+                contents = personality["contents"]
+            except (KeyError, TypeError):
+                msg = _("Unable to parse personality path/contents.")
+                LOG.info(msg)
+                raise faults.Fault(exc.HTTPBadRequest(explanation=msg))
+
+            try:
+                personality["contents"] = base64.b64decode(contents)
+            except TypeError:
+                msg = _("Personality content could not be Base64 decoded.")
+                LOG.info(msg)
+                raise faults.Fault(exc.HTTPBadRequest(explanation=msg))
+
+    def _action_rebuild(self, info, request, instance_id):
+        context = request.environ['nova.context']
+        instance_id = int(instance_id)
+
+        try:
+            image_ref = info["rebuild"]["imageRef"]
+        except (KeyError, TypeError):
+            msg = _("Could not parse imageRef from request.")
+            LOG.debug(msg)
+            return faults.Fault(exc.HTTPBadRequest(explanation=msg))
+
+        image_id = common.get_id_from_href(image_ref)
+        personalities = info["rebuild"].get("personality", [])
+        metadata = info["rebuild"].get("metadata", {})
+
+        self._validate_metadata(metadata)
+        self._decode_personalities(personalities)
+
+        try:
+            self.compute_api.rebuild(context, instance_id, image_id, metadata,
+                                     personalities)
+        except exception.BuildInProgress:
+            msg = _("Instance %d is currently being rebuilt.") % instance_id
+            LOG.debug(msg)
+            return faults.Fault(exc.HTTPConflict(explanation=msg))
+
+        response = exc.HTTPAccepted()
+        response.empty_body = True
+        return response
+
     def _get_server_admin_password(self, server):
         """ Determine the admin password for a server on creation """
         password = server.get('adminPass')

=== modified file 'nova/api/openstack/views/servers.py'
--- nova/api/openstack/views/servers.py	2011-04-26 13:45:53 +0000
+++ nova/api/openstack/views/servers.py	2011-05-03 16:43:57 +0000
@@ -66,7 +66,9 @@
             power_state.SHUTDOWN: 'SHUTDOWN',
             power_state.SHUTOFF: 'SHUTOFF',
             power_state.CRASHED: 'ERROR',
-            power_state.FAILED: 'ERROR'}
+            power_state.FAILED: 'ERROR',
+            power_state.BUILDING: 'BUILD',
+        }
 
         inst_dict = {
             'id': int(inst['id']),

=== modified file 'nova/compute/api.py'
--- nova/compute/api.py	2011-04-22 19:44:24 +0000
+++ nova/compute/api.py	2011-05-03 16:43:57 +0000
@@ -32,6 +32,7 @@
 from nova import utils
 from nova import volume
 from nova.compute import instance_types
+from nova.compute import power_state
 from nova.scheduler import api as scheduler_api
 from nova.db import base
 
@@ -501,6 +502,33 @@
         """Reboot the given instance."""
         self._cast_compute_message('reboot_instance', context, instance_id)
 
+    def rebuild(self, context, instance_id, image_id, metadata=None,
+                files_to_inject=None):
+        """Rebuild the given instance with the provided metadata."""
+        instance = db.api.instance_get(context, instance_id)
+
+        if instance["state"] == power_state.BUILDING:
+            msg = _("Instance already building")
+            raise exception.BuildInProgress(msg)
+
+        metadata = metadata or {}
+        self._check_metadata_properties_quota(context, metadata)
+
+        files_to_inject = files_to_inject or []
+        self._check_injected_file_quota(context, files_to_inject)
+
+        self.db.instance_update(context, instance_id, {"metadata": metadata})
+
+        rebuild_params = {
+            "image_id": image_id,
+            "injected_files": files_to_inject,
+        }
+
+        self._cast_compute_message('rebuild_instance',
+                                   context,
+                                   instance_id,
+                                   params=rebuild_params)
+
     def revert_resize(self, context, instance_id):
         """Reverts a resize, deleting the 'new' instance in the process."""
         context = context.elevated()

=== modified file 'nova/compute/manager.py'
--- nova/compute/manager.py	2011-04-22 19:44:24 +0000
+++ nova/compute/manager.py	2011-05-03 16:43:57 +0000
@@ -137,17 +137,33 @@
         """Initialization for a standalone compute service."""
         self.driver.init_host(host=self.host)
 
-    def _update_state(self, context, instance_id):
+    def _update_state(self, context, instance_id, state=None):
         """Update the state of an instance from the driver info."""
-        # FIXME(ja): include other fields from state?
         instance_ref = self.db.instance_get(context, instance_id)
-        try:
-            info = self.driver.get_info(instance_ref['name'])
-            state = info['state']
-        except exception.NotFound:
-            state = power_state.FAILED
+
+        if state is None:
+            try:
+                info = self.driver.get_info(instance_ref['name'])
+            except exception.NotFound:
+                info = None
+
+            if info is not None:
+                state = info['state']
+            else:
+                state = power_state.FAILED
+
         self.db.instance_set_state(context, instance_id, state)
 
+    def _update_launched_at(self, context, instance_id, launched_at=None):
+        """Update the launched_at parameter of the given instance."""
+        data = {'launched_at': launched_at or datetime.datetime.utcnow()}
+        self.db.instance_update(context, instance_id, data)
+
+    def _update_image_id(self, context, instance_id, image_id):
+        """Update the image_id for the given instance."""
+        data = {'image_id': image_id}
+        self.db.instance_update(context, instance_id, data)
+
     def get_console_topic(self, context, **kwargs):
         """Retrieves the console host for a project on this host.
 
@@ -231,24 +247,15 @@
                                                        instance_id)
 
         # TODO(vish) check to make sure the availability zone matches
-        self.db.instance_set_state(context,
-                                   instance_id,
-                                   power_state.NOSTATE,
-                                   'spawning')
+        self._update_state(context, instance_id, power_state.BUILDING)
 
         try:
             self.driver.spawn(instance_ref)
-            now = datetime.datetime.utcnow()
-            self.db.instance_update(context,
-                                    instance_id,
-                                    {'launched_at': now})
-        except Exception:  # pylint: disable=W0702
-            LOG.exception(_("Instance '%s' failed to spawn. Is virtualization"
-                            " enabled in the BIOS?"), instance_id,
-                                                     context=context)
-            self.db.instance_set_state(context,
-                                       instance_id,
-                                       power_state.SHUTDOWN)
+        except Exception as ex:  # pylint: disable=W0702
+            msg = _("Instance '%(instance_id)s' failed to spawn. Is "
+                    "virtualization enabled in the BIOS? Details: "
+                    "%(ex)s") % locals()
+            LOG.exception(msg)
 
         if not FLAGS.stub_network and FLAGS.auto_assign_floating_ip:
             public_ip = self.network_api.allocate_floating_ip(context)
@@ -262,6 +269,8 @@
                                                    floating_ip,
                                                    fixed_ip,
                                                    affect_auto_assigned=True)
+
+        self._update_launched_at(context, instance_id)
         self._update_state(context, instance_id)
 
     @exception.wrap_exception
@@ -318,6 +327,33 @@
 
     @exception.wrap_exception
     @checks_instance_lock
+    def rebuild_instance(self, context, instance_id, image_id):
+        """Destroy and re-make this instance.
+
+        A 'rebuild' effectively purges all existing data from the system and
+        remakes the VM with given 'metadata' and 'personalities'.
+
+        :param context: `nova.RequestContext` object
+        :param instance_id: Instance identifier (integer)
+        :param image_id: Image identifier (integer)
+        """
+        context = context.elevated()
+
+        instance_ref = self.db.instance_get(context, instance_id)
+        LOG.audit(_("Rebuilding instance %s"), instance_id, context=context)
+
+        self._update_state(context, instance_id, power_state.BUILDING)
+
+        self.driver.destroy(instance_ref)
+        instance_ref.image_id = image_id
+        self.driver.spawn(instance_ref)
+
+        self._update_image_id(context, instance_id, image_id)
+        self._update_launched_at(context, instance_id)
+        self._update_state(context, instance_id)
+
+    @exception.wrap_exception
+    @checks_instance_lock
     def reboot_instance(self, context, instance_id):
         """Reboot an instance on this host."""
         context = context.elevated()
@@ -1072,8 +1108,7 @@
             if vm_instance is None:
                 # NOTE(justinsb): We have to be very careful here, because a
                 # concurrent operation could be in progress (e.g. a spawn)
-                if db_state == power_state.NOSTATE:
-                    # Assume that NOSTATE => spawning
+                if db_state == power_state.BUILDING:
                     # TODO(justinsb): This does mean that if we crash during a
                     # spawn, the machine will never leave the spawning state,
                     # but this is just the way nova is; this function isn't
@@ -1104,9 +1139,7 @@
             if vm_state != db_state:
                 LOG.info(_("DB/VM state mismatch. Changing state from "
                            "'%(db_state)s' to '%(vm_state)s'") % locals())
-                self.db.instance_set_state(context,
-                                           db_instance['id'],
-                                           vm_state)
+                self._update_state(context, db_instance['id'], vm_state)
 
             # NOTE(justinsb): We no longer auto-remove SHUTOFF instances
             # It's quite hard to get them back when we do.

=== modified file 'nova/compute/power_state.py'
--- nova/compute/power_state.py	2011-03-24 01:50:30 +0000
+++ nova/compute/power_state.py	2011-05-03 16:43:57 +0000
@@ -30,20 +30,23 @@
 CRASHED = 0x06
 SUSPENDED = 0x07
 FAILED = 0x08
+BUILDING = 0x09
 
 # TODO(justinsb): Power state really needs to be a proper class,
 # so that we're not locked into the libvirt status codes and can put mapping
 # logic here rather than spread throughout the code
 _STATE_MAP = {
-        NOSTATE: 'pending',
-        RUNNING: 'running',
-        BLOCKED: 'blocked',
-        PAUSED: 'paused',
-        SHUTDOWN: 'shutdown',
-        SHUTOFF: 'shutdown',
-        CRASHED: 'crashed',
-        SUSPENDED: 'suspended',
-        FAILED: 'failed to spawn'}
+    NOSTATE: 'pending',
+    RUNNING: 'running',
+    BLOCKED: 'blocked',
+    PAUSED: 'paused',
+    SHUTDOWN: 'shutdown',
+    SHUTOFF: 'shutdown',
+    CRASHED: 'crashed',
+    SUSPENDED: 'suspended',
+    FAILED: 'failed to spawn',
+    BUILDING: 'building',
+}
 
 
 def name(code):

=== modified file 'nova/exception.py'
--- nova/exception.py	2011-05-02 18:04:29 +0000
+++ nova/exception.py	2011-05-03 16:43:57 +0000
@@ -50,7 +50,7 @@
 
 class ApiError(Error):
     def __init__(self, message='Unknown', code=None):
-        self.message = message
+        self.msg = message
         self.code = code
         if code:
             outstr = '%s: %s' % (code, message)
@@ -59,6 +59,10 @@
         super(ApiError, self).__init__(outstr)
 
 
+class BuildInProgress(Error):
+    pass
+
+
 class DBError(Error):
     """Wraps an implementation specific exception."""
     def __init__(self, inner_exception):

=== modified file 'nova/tests/api/openstack/test_servers.py'
--- nova/tests/api/openstack/test_servers.py	2011-05-02 21:37:59 +0000
+++ nova/tests/api/openstack/test_servers.py	2011-05-03 16:43:57 +0000
@@ -1058,15 +1058,175 @@
         req.body = json.dumps(body)
         res = req.get_response(fakes.wsgi_app())
 
-    def test_server_rebuild(self):
-        body = dict(server=dict(
-            name='server_test', imageId=2, flavorId=2, metadata={},
-            personality={}))
-        req = webob.Request.blank('/v1.0/servers/1/action')
-        req.method = 'POST'
-        req.content_type = 'application/json'
-        req.body = json.dumps(body)
-        res = req.get_response(fakes.wsgi_app())
+    def test_server_rebuild_accepted(self):
+        body = {
+            "rebuild": {
+                "imageId": 2,
+            },
+        }
+
+        req = webob.Request.blank('/v1.0/servers/1/action')
+        req.method = 'POST'
+        req.content_type = 'application/json'
+        req.body = json.dumps(body)
+
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 202)
+        self.assertEqual(res.body, "")
+
+    def test_server_rebuild_rejected_when_building(self):
+        body = {
+            "rebuild": {
+                "imageId": 2,
+            },
+        }
+
+        state = power_state.BUILDING
+        new_return_server = return_server_with_power_state(state)
+        self.stubs.Set(nova.db.api, 'instance_get', new_return_server)
+
+        req = webob.Request.blank('/v1.0/servers/1/action')
+        req.method = 'POST'
+        req.content_type = 'application/json'
+        req.body = json.dumps(body)
+
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 409)
+
+    def test_server_rebuild_bad_entity(self):
+        body = {
+            "rebuild": {
+            },
+        }
+
+        req = webob.Request.blank('/v1.0/servers/1/action')
+        req.method = 'POST'
+        req.content_type = 'application/json'
+        req.body = json.dumps(body)
+
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 400)
+
+    def test_server_rebuild_accepted_minimum_v11(self):
+        body = {
+            "rebuild": {
+                "imageRef": "http://localhost/images/2",
+            },
+        }
+
+        req = webob.Request.blank('/v1.1/servers/1/action')
+        req.method = 'POST'
+        req.content_type = 'application/json'
+        req.body = json.dumps(body)
+
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 202)
+
+    def test_server_rebuild_rejected_when_building_v11(self):
+        body = {
+            "rebuild": {
+                "imageRef": "http://localhost/images/2",
+            },
+        }
+
+        state = power_state.BUILDING
+        new_return_server = return_server_with_power_state(state)
+        self.stubs.Set(nova.db.api, 'instance_get', new_return_server)
+
+        req = webob.Request.blank('/v1.1/servers/1/action')
+        req.method = 'POST'
+        req.content_type = 'application/json'
+        req.body = json.dumps(body)
+
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 409)
+
+    def test_server_rebuild_accepted_with_metadata_v11(self):
+        body = {
+            "rebuild": {
+                "imageRef": "http://localhost/images/2",
+                "metadata": {
+                    "new": "metadata",
+                },
+            },
+        }
+
+        req = webob.Request.blank('/v1.1/servers/1/action')
+        req.method = 'POST'
+        req.content_type = 'application/json'
+        req.body = json.dumps(body)
+
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 202)
+
+    def test_server_rebuild_accepted_with_bad_metadata_v11(self):
+        body = {
+            "rebuild": {
+                "imageRef": "http://localhost/images/2",
+                "metadata": "stack",
+            },
+        }
+
+        req = webob.Request.blank('/v1.1/servers/1/action')
+        req.method = 'POST'
+        req.content_type = 'application/json'
+        req.body = json.dumps(body)
+
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 400)
+
+    def test_server_rebuild_bad_entity_v11(self):
+        body = {
+            "rebuild": {
+                "imageId": 2,
+            },
+        }
+
+        req = webob.Request.blank('/v1.1/servers/1/action')
+        req.method = 'POST'
+        req.content_type = 'application/json'
+        req.body = json.dumps(body)
+
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 400)
+
+    def test_server_rebuild_bad_personality_v11(self):
+        body = {
+            "rebuild": {
+                "imageRef": "http://localhost/images/2",
+                "personality": [{
+                    "path": "/path/to/file",
+                    "contents": "INVALID b64",
+                }]
+            },
+        }
+
+        req = webob.Request.blank('/v1.1/servers/1/action')
+        req.method = 'POST'
+        req.content_type = 'application/json'
+        req.body = json.dumps(body)
+
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 400)
+
+    def test_server_rebuild_personality_v11(self):
+        body = {
+            "rebuild": {
+                "imageRef": "http://localhost/images/2",
+                "personality": [{
+                    "path": "/path/to/file",
+                    "contents": base64.b64encode("Test String"),
+                }]
+            },
+        }
+
+        req = webob.Request.blank('/v1.1/servers/1/action')
+        req.method = 'POST'
+        req.content_type = 'application/json'
+        req.body = json.dumps(body)
+
+        res = req.get_response(fakes.wsgi_app())
+        self.assertEqual(res.status_int, 202)
 
     def test_delete_server_instance(self):
         req = webob.Request.blank('/v1.0/servers/1')

=== modified file 'nova/tests/test_exception.py'
--- nova/tests/test_exception.py	2011-04-25 16:55:59 +0000
+++ nova/tests/test_exception.py	2011-05-03 16:43:57 +0000
@@ -26,9 +26,9 @@
         err = exception.ApiError('fake error')
         self.assertEqual(err.__str__(), 'fake error')
         self.assertEqual(err.code, None)
-        self.assertEqual(err.message, 'fake error')
+        self.assertEqual(err.msg, 'fake error')
         # with 'code' arg
         err = exception.ApiError('fake error', 'blah code')
         self.assertEqual(err.__str__(), 'blah code: fake error')
         self.assertEqual(err.code, 'blah code')
-        self.assertEqual(err.message, 'fake error')
+        self.assertEqual(err.msg, 'fake error')

=== modified file 'nova/virt/xenapi/vmops.py'
--- nova/virt/xenapi/vmops.py	2011-04-26 22:48:28 +0000
+++ nova/virt/xenapi/vmops.py	2011-05-03 16:43:57 +0000
@@ -210,8 +210,6 @@
         def _wait_for_boot():
             try:
                 state = self.get_info(instance_name)['state']
-                db.instance_set_state(context.get_admin_context(),
-                                      instance['id'], state)
                 if state == power_state.RUNNING:
                     LOG.debug(_('Instance %s: booted'), instance_name)
                     timer.stop()
@@ -219,11 +217,7 @@
                     return True
             except Exception, exc:
                 LOG.warn(exc)
-                LOG.exception(_('instance %s: failed to boot'),
-                              instance_name)
-                db.instance_set_state(context.get_admin_context(),
-                                      instance['id'],
-                                      power_state.SHUTDOWN)
+                LOG.exception(_('Instance %s: failed to boot'), instance_name)
                 timer.stop()
                 return False