Merge lp:~jk0/nova/xs-rescue into lp:~hudson-openstack/nova/trunk

[Josh Kearney]

Provide the ability to rescue and unrescue a XenServer instance.

[Rick Harris (rconradharris)]

Good work jk0, this is a great start.

Some small nits:

> 80 + self.driver.rescue(instance_ref,
> 81 + lambda result: self._update_state_callback(self,
> 82 + context,
> 83 + instance_id,
> 84 + result))

Breaking this into two lines would probably aid readability.

> 145 + def rescue(self, instance, callback=None):

The callback function isn't firing for rescue or unrescue. It looks like the
other methods are using

        self._wait_with_callback(instance.id, task, callback)

    To fire it off.

> 342 + def _bootlock(self, vm, unlock=False):
> 343 + """Prevent an instance from booting"""
> 344 + if unlock:
> 345 + self._session.call_xenapi(
> 346 + "VM.remove_from_blocked_operations",
> 347 + vm,
> 348 + "start")
> 349 + else:
> 350 + self._session.call_xenapi(
> 351 + "VM.set_blocked_operations",
> 352 + vm,
> 353 + {"start": ""})
> 354 +

I think this make more sense as two methods:
_acquire_bootlock and _release_bootlock.

Another thing you could do, is assert that we're in the correct state. For
example, that we're not trying to unlock a VM that's already unlocked (that'd
indicate a bug in the code).

> 394 + try:
> 395 + task = self._session.call_xenapi("Async.VM.clean_shutdown", vm)
> 396 + self._session.wait_for_task(task, instance.id)
> 397 + except self.XenAPI.Failure:
> 398 + task = self._session.call_xenapi("Async.VM.hard_shutdown", vm)
> 399 + self._session.wait_for_task(task, instance.id)

By default, I think _shutdown should perform either a clean or hard shutdown
depending on whats requested (Dietz added a hard=true kwarg in his
xs-migrations branch).

If at all possible, I'd like to avoid silent-fall-backs (like from clean to
hard), because they're usually indicative of a larger problem

That said, a _force_shutdown that attempts a clean and then falls-back to
hard would be okay since it's very explicity when that behavior is being
requested (And will be easier to remove later on if we want to).

> 484 + if vbd["userdevice"] == str(1):

str(1) can just be "1".

> 488 + task1 = self._session.call_xenapi("Async.VM.hard_shutdown", rescue_vm)
> 489 + self._session.wait_for_task(task1, instance.id)

This could be something like _shutdown(instance, rescue_vm, hard=True).

> 491 + vdis = VMHelper.lookup_vm_vdis(self._session, rescue_vm)
> 492 + for vdi in vdis:
> 493 + try:
> 494 + task = self._session.call_xenapi('Async.VDI.destroy', vdi)
> 495 + self._session.wait_for_task(task, instance.id)
> 496 + except self.XenAPI.Failure:
> 497 + continue

Should be able to use `_destroy_vdis` here.

> 499 + task2 = self._session.call_xenapi('Async.VM.destroy', rescue_vm)
> 500 + self._session.wait_for_task(task2, instance.i...

review: Needs Fixing

[Josh Kearney (jk0)]

> Good work jk0, this is a great start.
>
>
> Some small nits:
>
> > 80 + self.driver.rescue(instance_ref,
> > 81 + lambda result: self._update_state_callback(self,
> > 82 + context,
> > 83 + instance_id,
> > 84 + result))
>
> Breaking this into two lines would probably aid readability.

Copied this directly from another call, wanted to maintain consistency. I'll go thru them all and clean up.

>
> > 145 + def rescue(self, instance, callback=None):
>
> The callback function isn't firing for rescue or unrescue. It looks like the
> other methods are using
>
> self._wait_with_callback(instance.id, task, callback)
>
> To fire it off.

I had to keep this in place since libvirt uses the callback (passed down to the driver).

>
> > 342 + def _bootlock(self, vm, unlock=False):
> > 343 + """Prevent an instance from booting"""
> > 344 + if unlock:
> > 345 + self._session.call_xenapi(
> > 346 + "VM.remove_from_blocked_operations",
> > 347 + vm,
> > 348 + "start")
> > 349 + else:
> > 350 + self._session.call_xenapi(
> > 351 + "VM.set_blocked_operations",
> > 352 + vm,
> > 353 + {"start": ""})
> > 354 +
>
> I think this make more sense as two methods:
> _acquire_bootlock and _release_bootlock.
>
> Another thing you could do, is assert that we're in the correct state. For
> example, that we're not trying to unlock a VM that's already unlocked (that'd
> indicate a bug in the code).

Good points, I'll break this down into two methods.

>
> > 394 + try:
> > 395 + task =
> self._session.call_xenapi("Async.VM.clean_shutdown", vm)
> > 396 + self._session.wait_for_task(task, instance.id)
> > 397 + except self.XenAPI.Failure:
> > 398 + task =
> self._session.call_xenapi("Async.VM.hard_shutdown", vm)
> > 399 + self._session.wait_for_task(task, instance.id)
>
> By default, I think _shutdown should perform either a clean or hard shutdown
> depending on whats requested (Dietz added a hard=true kwarg in his
> xs-migrations branch).
>
> If at all possible, I'd like to avoid silent-fall-backs (like from clean to
> hard), because they're usually indicative of a larger problem
>
> That said, a _force_shutdown that attempts a clean and then falls-back to
> hard would be okay since it's very explicity when that behavior is being
> requested (And will be easier to remove later on if we want to).
>

Sounds like I could get by using Dietz' method. I'll do this to avoid conflicts down the road.

> > 484 + if vbd["userdevice"] == str(1):
>
> str(1) can just be "1".

Nice catch. Not sure why I used str() here.

>
> > 488 + task1 = self._session.call_xenapi("Async.VM.hard_shutdown",
> rescue_vm)
> > 489 + self._session.wait_for_task(task1, instance.id)
>
> This could be som...

[lp:~jk0/nova/xs-rescue]

602. By Josh Kearney on 2011-03-01

Review feedback

[Matt Dietz (cerberus)]

Pinning rescue status on the name of the VM seems pretty messy to me. It seems to me that, since rescue is such a critical state to be aware of, that we should maintain that either through a flag on the instance model or through the instance status attribute. Is there a reason you didn't go this route?

review: Needs Information

[Josh Kearney (jk0)]

> Pinning rescue status on the name of the VM seems pretty messy to me. It seems
> to me that, since rescue is such a critical state to be aware of, that we
> should maintain that either through a flag on the instance model or through
> the instance status attribute. Is there a reason you didn't go this route?

We had to go this route to avoid duplicate name-label exceptions. By doing this, we can spin up a rescue image using the same instance object and easily refer back to the original image when the time comes to unrescue. The name-labels are guaranteed to be unique.

[Matt Dietz (cerberus)]

Sure, I get that. My concern comes in cases where we may want to auto-unrescue instances. If we were to search for anything with '-rescue' on the end, we might try to unrescue stuff that's just unfortunately named.

[Matt Dietz (cerberus)]

Nevermind, feedback received off list. Forgot that name-labels were not instance-descriptions (we should probably rename one or both of those...)

review: Approve

[Rick Harris (rconradharris)]

Thanks for the fixes jk0. Have a couple of more potential cleanups, nothing big:

> 136 @property
> 137 def name(self):
> 138 - return FLAGS.instance_name_template % self.id
> 139 + base_name = FLAGS.instance_name_template % self.id
> 140 + if getattr(self, '_rescue', False):
> 141 + base_name += "-rescue"
> 142 + return base_name

I know I suggested this, but I'm having second thoughts about putting this
logic here. At least right now, this is XenAPI hackiness, I'm not sure this
belongs in the generic instance model.

Instead, I think the XenAPI code should have a method, `get_name_label` which
it uses instead of instance.name to generate the VM name-labels. That code
would look like:

def get_name_label(instance):
    """Generate name label for the XenServer VM record

    We have two scenarios:

        1. We're in normal mode and the INSTANCE VM record uses the instance name as
           its name label

        2. We're in rescue mode and the RESCUE VM Record uses instance name +
           '-rescue' as its name label
    """
    base_name = instance.name
    if getattr(self, '_rescue', False):
        base_name += "-rescue"
    return base_name

The code is really pretty readable, but, it still might be a good idea to
include a big doc-string on the XenAPI rescue method that explains, at a
high-level, what's involved in a rescue, like:

"""
Rescue consists of a number of steps

1. We shutdown the INSTANCE VM

2. We apply a 'bootlock' to prevent the instance VM from being started while
   in rescue

2. We spawn a RESCUE VM (the vm name-label in this case will be
   instance-0002-rescue)

3. We attach the INSTANCE VM's VBDs to the RESCUE VM

Unrescue consists of:

1. We unplug the INSTANCE VM's from the RESCUE VM

2. We teardown the RESCUE VM

3. Release the bootlock to allow the INSTANCE VM to be started

4. Start the INSTANCE VM
"""

[lp:~jk0/nova/xs-rescue]

603. By Josh Kearney on 2011-03-02

Updated docstrings

[Rick Harris (rconradharris)]

Looks good.

review: Approve

=== modified file '.mailmap'
--- .mailmap	2011-02-18 18:27:30 +0000
+++ .mailmap	2011-03-02 23:12:16 +0000
@@ -19,6 +19,7 @@
 <[e-mail]> <[e-mail]>
 <[e-mail]> <[e-mail]>
 <[e-mail]> <[e-mail]>
+<[e-mail]> <[e-mail]>
 <[e-mail]> <justinsb@justinsb-desktop>
 <[e-mail]> <[e-mail]>
 <[e-mail]> Masumoto<[e-mail]>
 
=== modified file 'Authors'
--- Authors	2011-03-01 00:46:39 +0000
+++ Authors	2011-03-02 23:12:16 +0000
@@ -31,7 +31,7 @@
 Jonathan Bryce <[e-mail]>
 Jordan Rinke <[e-mail]>
 Josh Durgin <[e-mail]>
-Josh Kearney <[e-mail]>
+Josh Kearney <[e-mail]>
 Joshua McKenty <[e-mail]>
 Justin Santa Barbara <[e-mail]>
 Kei Masumoto <[e-mail]>
 
=== modified file 'nova/api/openstack/__init__.py'
--- nova/api/openstack/__init__.py	2011-02-24 23:35:21 +0000
+++ nova/api/openstack/__init__.py	2011-03-02 23:12:16 +0000
@@ -80,6 +80,8 @@
             server_members["actions"] = "GET"
             server_members['suspend'] = 'POST'
             server_members['resume'] = 'POST'
+            server_members['rescue'] = 'POST'
+            server_members['unrescue'] = 'POST'
             server_members['reset_network'] = 'POST'
             server_members['inject_network_info'] = 'POST'
 
 
=== modified file 'nova/api/openstack/servers.py'
--- nova/api/openstack/servers.py	2011-03-01 00:56:46 +0000
+++ nova/api/openstack/servers.py	2011-03-02 23:12:16 +0000
@@ -335,6 +335,28 @@
             return faults.Fault(exc.HTTPUnprocessableEntity())
         return exc.HTTPAccepted()
 
+    def rescue(self, req, id):
+        """Permit users to rescue the server."""
+        context = req.environ["nova.context"]
+        try:
+            self.compute_api.rescue(context, id)
+        except:
+            readable = traceback.format_exc()
+            LOG.exception(_("compute.api::rescue %s"), readable)
+            return faults.Fault(exc.HTTPUnprocessableEntity())
+        return exc.HTTPAccepted()
+
+    def unrescue(self, req, id):
+        """Permit users to unrescue the server."""
+        context = req.environ["nova.context"]
+        try:
+            self.compute_api.unrescue(context, id)
+        except:
+            readable = traceback.format_exc()
+            LOG.exception(_("compute.api::unrescue %s"), readable)
+            return faults.Fault(exc.HTTPUnprocessableEntity())
+        return exc.HTTPAccepted()
+
     def get_ajax_console(self, req, id):
         """ Returns a url to an instance's ajaxterm console. """
         try:
 
=== modified file 'nova/compute/manager.py'
--- nova/compute/manager.py	2011-02-24 23:35:21 +0000
+++ nova/compute/manager.py	2011-03-02 23:12:16 +0000
@@ -370,12 +370,19 @@
         context = context.elevated()
         instance_ref = self.db.instance_get(context, instance_id)
         LOG.audit(_('instance %s: rescuing'), instance_id, context=context)
-        self.db.instance_set_state(context,
-                                   instance_id,
-                                   power_state.NOSTATE,
-                                   'rescuing')
+        self.db.instance_set_state(
+            context,
+            instance_id,
+            power_state.NOSTATE,
+            'rescuing')
         self.network_manager.setup_compute_network(context, instance_id)
-        self.driver.rescue(instance_ref)
+        self.driver.rescue(
+            instance_ref,
+            lambda result: self._update_state_callback(
+                self,
+                context,
+                instance_id,
+                result))
         self._update_state(context, instance_id)
 
     @exception.wrap_exception
@@ -385,11 +392,18 @@
         context = context.elevated()
         instance_ref = self.db.instance_get(context, instance_id)
         LOG.audit(_('instance %s: unrescuing'), instance_id, context=context)
-        self.db.instance_set_state(context,
-                                   instance_id,
-                                   power_state.NOSTATE,
-                                   'unrescuing')
-        self.driver.unrescue(instance_ref)
+        self.db.instance_set_state(
+            context,
+            instance_id,
+            power_state.NOSTATE,
+            'unrescuing')
+        self.driver.unrescue(
+            instance_ref,
+            lambda result: self._update_state_callback(
+                self,
+                context,
+                instance_id,
+                result))
         self._update_state(context, instance_id)
 
     @staticmethod
 
=== modified file 'nova/db/sqlalchemy/models.py'
--- nova/db/sqlalchemy/models.py	2011-02-24 05:47:29 +0000
+++ nova/db/sqlalchemy/models.py	2011-03-02 23:12:16 +0000
@@ -126,11 +126,16 @@
 class Instance(BASE, NovaBase):
     """Represents a guest vm."""
     __tablename__ = 'instances'
+    onset_files = []
+
     id = Column(Integer, primary_key=True, autoincrement=True)
 
     @property
     def name(self):
-        return FLAGS.instance_name_template % self.id
+        base_name = FLAGS.instance_name_template % self.id
+        if getattr(self, '_rescue', False):
+            base_name += "-rescue"
+        return base_name
 
     admin_pass = Column(String(255))
     user_id = Column(String(255))
 
=== modified file 'nova/tests/xenapi/stubs.py'
--- nova/tests/xenapi/stubs.py	2011-02-25 16:47:08 +0000
+++ nova/tests/xenapi/stubs.py	2011-03-02 23:12:16 +0000
@@ -27,7 +27,7 @@
     def fake_fetch_image(cls, session, instance_id, image, user, project,
                          type):
         # Stubout wait_for_task
-        def fake_wait_for_task(self, id, task):
+        def fake_wait_for_task(self, task, id):
             class FakeEvent:
 
                 def send(self, value):
 
=== modified file 'nova/virt/libvirt_conn.py'
--- nova/virt/libvirt_conn.py	2011-01-27 23:58:22 +0000
+++ nova/virt/libvirt_conn.py	2011-03-02 23:12:16 +0000
@@ -362,7 +362,7 @@
         raise exception.APIError("resume not supported for libvirt")
 
     @exception.wrap_exception
-    def rescue(self, instance):
+    def rescue(self, instance, callback=None):
         self.destroy(instance, False)
 
         xml = self.to_xml(instance, rescue=True)
@@ -392,7 +392,7 @@
         return timer.start(interval=0.5, now=True)
 
     @exception.wrap_exception
-    def unrescue(self, instance):
+    def unrescue(self, instance, callback=None):
         # NOTE(vish): Because reboot destroys and recreates an instance using
         #             the normal xml file, we can just call reboot here
         self.reboot(instance)
 
=== modified file 'nova/virt/xenapi/fake.py'
--- nova/virt/xenapi/fake.py	2011-02-09 10:08:15 +0000
+++ nova/virt/xenapi/fake.py	2011-03-02 23:12:16 +0000
@@ -401,7 +401,7 @@
                 field in _db_content[cls][ref]):
                 return _db_content[cls][ref][field]
 
-        LOG.debuug(_('Raising NotImplemented'))
+        LOG.debug(_('Raising NotImplemented'))
         raise NotImplementedError(
             _('xenapi.fake does not have an implementation for %s or it has '
             'been called with the wrong number of arguments') % name)
 
=== modified file 'nova/virt/xenapi/vm_utils.py'
--- nova/virt/xenapi/vm_utils.py	2011-02-25 16:47:08 +0000
+++ nova/virt/xenapi/vm_utils.py	2011-03-02 23:12:16 +0000
@@ -205,19 +205,17 @@
         """Destroy VBD from host database"""
         try:
             task = session.call_xenapi('Async.VBD.destroy', vbd_ref)
-            #FIXME(armando): find a solution to missing instance_id
-            #with Josh Kearney
-            session.wait_for_task(0, task)
+            session.wait_for_task(task)
         except cls.XenAPI.Failure, exc:
             LOG.exception(exc)
             raise StorageError(_('Unable to destroy VBD %s') % vbd_ref)
 
     @classmethod
-    def create_vif(cls, session, vm_ref, network_ref, mac_address):
+    def create_vif(cls, session, vm_ref, network_ref, mac_address, dev="0"):
         """Create a VIF record.  Returns a Deferred that gives the new
         VIF reference."""
         vif_rec = {}
-        vif_rec['device'] = '0'
+        vif_rec['device'] = dev
         vif_rec['network'] = network_ref
         vif_rec['VM'] = vm_ref
         vif_rec['MAC'] = mac_address
@@ -269,7 +267,7 @@
         original_parent_uuid = get_vhd_parent_uuid(session, vm_vdi_ref)
 
         task = session.call_xenapi('Async.VM.snapshot', vm_ref, label)
-        template_vm_ref = session.wait_for_task(instance_id, task)
+        template_vm_ref = session.wait_for_task(task, instance_id)
         template_vdi_rec = get_vdi_for_vm_safely(session, template_vm_ref)[1]
         template_vdi_uuid = template_vdi_rec["uuid"]
 
@@ -302,7 +300,7 @@
 
         kwargs = {'params': pickle.dumps(params)}
         task = session.async_call_plugin('glance', 'upload_vhd', kwargs)
-        session.wait_for_task(instance_id, task)
+        session.wait_for_task(task, instance_id)
 
     @classmethod
     def fetch_image(cls, session, instance_id, image, user, project,
@@ -345,7 +343,7 @@
 
         kwargs = {'params': pickle.dumps(params)}
         task = session.async_call_plugin('glance', 'download_vhd', kwargs)
-        vdi_uuid = session.wait_for_task(instance_id, task)
+        vdi_uuid = session.wait_for_task(task, instance_id)
 
         scan_sr(session, instance_id, sr_ref)
 
@@ -401,7 +399,7 @@
             #let the plugin copy the correct number of bytes
             args['image-size'] = str(vdi_size)
             task = session.async_call_plugin('glance', fn, args)
-            filename = session.wait_for_task(instance_id, task)
+            filename = session.wait_for_task(task, instance_id)
             #remove the VDI as it is not needed anymore
             session.get_xenapi().VDI.destroy(vdi)
             LOG.debug(_("Kernel/Ramdisk VDI %s destroyed"), vdi)
@@ -493,7 +491,7 @@
             if image_type == ImageType.DISK_RAW:
                 args['raw'] = 'true'
         task = session.async_call_plugin('objectstore', fn, args)
-        uuid = session.wait_for_task(instance_id, task)
+        uuid = session.wait_for_task(task, instance_id)
         return uuid
 
     @classmethod
@@ -513,7 +511,7 @@
         args = {}
         args['vdi-ref'] = vdi_ref
         task = session.async_call_plugin('objectstore', fn, args)
-        pv_str = session.wait_for_task(instance_id, task)
+        pv_str = session.wait_for_task(task, instance_id)
         pv = None
         if pv_str.lower() == 'true':
             pv = True
@@ -654,7 +652,7 @@
 def scan_sr(session, instance_id, sr_ref):
     LOG.debug(_("Re-scanning SR %s"), sr_ref)
     task = session.call_xenapi('Async.SR.scan', sr_ref)
-    session.wait_for_task(instance_id, task)
+    session.wait_for_task(task, instance_id)
 
 
 def wait_for_vhd_coalesce(session, instance_id, sr_ref, vdi_ref,
 
=== modified file 'nova/virt/xenapi/vmops.py'
--- nova/virt/xenapi/vmops.py	2011-02-25 16:47:08 +0000
+++ nova/virt/xenapi/vmops.py	2011-03-02 23:12:16 +0000
@@ -49,6 +49,7 @@
     def __init__(self, session):
         self.XenAPI = session.get_imported_xenapi()
         self._session = session
+
         VMHelper.XenAPI = self.XenAPI
 
     def list_instances(self):
@@ -62,20 +63,20 @@
 
     def spawn(self, instance):
         """Create VM instance"""
-        vm = VMHelper.lookup(self._session, instance.name)
+        instance_name = instance.name
+        vm = VMHelper.lookup(self._session, instance_name)
         if vm is not None:
             raise exception.Duplicate(_('Attempted to create'
-            ' non-unique name %s') % instance.name)
+            ' non-unique name %s') % instance_name)
 
         #ensure enough free memory is available
         if not VMHelper.ensure_free_mem(self._session, instance):
-                name = instance['name']
-                LOG.exception(_('instance %(name)s: not enough free memory')
-                              % locals())
-                db.instance_set_state(context.get_admin_context(),
-                                      instance['id'],
-                                      power_state.SHUTDOWN)
-                return
+            LOG.exception(_('instance %(instance_name)s: not enough free '
+                          'memory') % locals())
+            db.instance_set_state(context.get_admin_context(),
+                                  instance['id'],
+                                  power_state.SHUTDOWN)
+            return
 
         user = AuthManager().get_user(instance.user_id)
         project = AuthManager().get_project(instance.project_id)
@@ -116,10 +117,9 @@
         self.create_vifs(instance, networks)
 
         LOG.debug(_('Starting VM %s...'), vm_ref)
-        self._session.call_xenapi('VM.start', vm_ref, False, False)
-        instance_name = instance.name
+        self._start(instance, vm_ref)
         LOG.info(_('Spawning VM %(instance_name)s created %(vm_ref)s.')
-                % locals())
+                 % locals())
 
         def _inject_onset_files():
             onset_files = instance.onset_files
@@ -143,18 +143,18 @@
 
         def _wait_for_boot():
             try:
-                state = self.get_info(instance['name'])['state']
+                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'])
+                    LOG.debug(_('Instance %s: booted'), instance_name)
                     timer.stop()
                     _inject_onset_files()
                     return True
             except Exception, exc:
                 LOG.warn(exc)
                 LOG.exception(_('instance %s: failed to boot'),
-                              instance['name'])
+                              instance_name)
                 db.instance_set_state(context.get_admin_context(),
                                       instance['id'],
                                       power_state.SHUTDOWN)
@@ -202,6 +202,20 @@
                             _('Instance not present %s') % instance_name)
         return vm
 
+    def _acquire_bootlock(self, vm):
+        """Prevent an instance from booting"""
+        self._session.call_xenapi(
+            "VM.set_blocked_operations",
+            vm,
+            {"start": ""})
+
+    def _release_bootlock(self, vm):
+        """Allow an instance to boot"""
+        self._session.call_xenapi(
+            "VM.remove_from_blocked_operations",
+            vm,
+            "start")
+
     def snapshot(self, instance, image_id):
         """ Create snapshot from a running VM instance
 
@@ -254,7 +268,7 @@
         """Reboot VM instance"""
         vm = self._get_vm_opaque_ref(instance)
         task = self._session.call_xenapi('Async.VM.clean_reboot', vm)
-        self._session.wait_for_task(instance.id, task)
+        self._session.wait_for_task(task, instance.id)
 
     def set_admin_password(self, instance, new_pass):
         """Set the root/admin password on the VM instance. This is done via
@@ -294,6 +308,11 @@
             raise RuntimeError(resp_dict['message'])
         return resp_dict['message']
 
+    def _start(self, instance, vm):
+        """Start an instance"""
+        task = self._session.call_xenapi("Async.VM.start", vm, False, False)
+        self._session.wait_for_task(task, instance.id)
+
     def inject_file(self, instance, b64_path, b64_contents):
         """Write a file to the VM instance. The path to which it is to be
         written and the contents of the file need to be supplied; both should
@@ -320,8 +339,8 @@
             raise RuntimeError(resp_dict['message'])
         return resp_dict['message']
 
-    def _shutdown(self, instance, vm):
-        """Shutdown an instance """
+    def _shutdown(self, instance, vm, hard=True):
+        """Shutdown an instance"""
         state = self.get_info(instance['name'])['state']
         if state == power_state.SHUTDOWN:
             LOG.warn(_("VM %(vm)s already halted, skipping shutdown...") %
@@ -332,8 +351,13 @@
         LOG.debug(_("Shutting down VM for Instance %(instance_id)s")
                   % locals())
         try:
-            task = self._session.call_xenapi('Async.VM.hard_shutdown', vm)
-            self._session.wait_for_task(instance.id, task)
+            task = None
+            if hard:
+                task = self._session.call_xenapi("Async.VM.hard_shutdown", vm)
+            else:
+                task = self._session.call_xenapi("Async.VM.clean_shutdown", vm)
+
+            self._session.wait_for_task(task, instance.id)
         except self.XenAPI.Failure, exc:
             LOG.exception(exc)
 
@@ -350,7 +374,7 @@
         for vdi in vdis:
             try:
                 task = self._session.call_xenapi('Async.VDI.destroy', vdi)
-                self._session.wait_for_task(instance.id, task)
+                self._session.wait_for_task(task, instance.id)
             except self.XenAPI.Failure, exc:
                 LOG.exception(exc)
 
@@ -389,7 +413,7 @@
         args = {'kernel-file': kernel, 'ramdisk-file': ramdisk}
         task = self._session.async_call_plugin(
             'glance', 'remove_kernel_ramdisk', args)
-        self._session.wait_for_task(instance.id, task)
+        self._session.wait_for_task(task, instance.id)
 
         LOG.debug(_("kernel/ramdisk files removed"))
 
@@ -398,7 +422,7 @@
         instance_id = instance.id
         try:
             task = self._session.call_xenapi('Async.VM.destroy', vm)
-            self._session.wait_for_task(instance_id, task)
+            self._session.wait_for_task(task, instance_id)
         except self.XenAPI.Failure, exc:
             LOG.exception(exc)
 
@@ -441,7 +465,7 @@
     def _wait_with_callback(self, instance_id, task, callback):
         ret = None
         try:
-            ret = self._session.wait_for_task(instance_id, task)
+            ret = self._session.wait_for_task(task, instance_id)
         except self.XenAPI.Failure, exc:
             LOG.exception(exc)
         callback(ret)
@@ -470,6 +494,78 @@
         task = self._session.call_xenapi('Async.VM.resume', vm, False, True)
         self._wait_with_callback(instance.id, task, callback)
 
+    def rescue(self, instance, callback):
+        """Rescue the specified instance
+            - shutdown the instance VM
+            - set 'bootlock' to prevent the instance from starting in rescue
+            - spawn a rescue VM (the vm name-label will be instance-N-rescue)
+
+        """
+        rescue_vm = VMHelper.lookup(self._session, instance.name + "-rescue")
+        if rescue_vm:
+            raise RuntimeError(_(
+                "Instance is already in Rescue Mode: %s" % instance.name))
+
+        vm = self._get_vm_opaque_ref(instance)
+        self._shutdown(instance, vm)
+        self._acquire_bootlock(vm)
+
+        instance._rescue = True
+        self.spawn(instance)
+        rescue_vm = self._get_vm_opaque_ref(instance)
+
+        vbd = self._session.get_xenapi().VM.get_VBDs(vm)[0]
+        vdi_ref = self._session.get_xenapi().VBD.get_record(vbd)["VDI"]
+        vbd_ref = VMHelper.create_vbd(
+            self._session,
+            rescue_vm,
+            vdi_ref,
+            1,
+            False)
+
+        self._session.call_xenapi("Async.VBD.plug", vbd_ref)
+
+    def unrescue(self, instance, callback):
+        """Unrescue the specified instance
+            - unplug the instance VM's disk from the rescue VM
+            - teardown the rescue VM
+            - release the bootlock to allow the instance VM to start
+
+        """
+        rescue_vm = VMHelper.lookup(self._session, instance.name + "-rescue")
+
+        if not rescue_vm:
+            raise exception.NotFound(_(
+                "Instance is not in Rescue Mode: %s" % instance.name))
+
+        original_vm = self._get_vm_opaque_ref(instance)
+        vbds = self._session.get_xenapi().VM.get_VBDs(rescue_vm)
+
+        instance._rescue = False
+
+        for vbd_ref in vbds:
+            vbd = self._session.get_xenapi().VBD.get_record(vbd_ref)
+            if vbd["userdevice"] == "1":
+                VMHelper.unplug_vbd(self._session, vbd_ref)
+                VMHelper.destroy_vbd(self._session, vbd_ref)
+
+        task1 = self._session.call_xenapi("Async.VM.hard_shutdown", rescue_vm)
+        self._session.wait_for_task(task1, instance.id)
+
+        vdis = VMHelper.lookup_vm_vdis(self._session, rescue_vm)
+        for vdi in vdis:
+            try:
+                task = self._session.call_xenapi('Async.VDI.destroy', vdi)
+                self._session.wait_for_task(task, instance.id)
+            except self.XenAPI.Failure:
+                continue
+
+        task2 = self._session.call_xenapi('Async.VM.destroy', rescue_vm)
+        self._session.wait_for_task(task2, instance.id)
+
+        self._release_bootlock(original_vm)
+        self._start(instance, original_vm)
+
     def get_info(self, instance):
         """Return data about VM instance"""
         vm = self._get_vm_opaque_ref(instance)
@@ -556,8 +652,17 @@
                 NetworkHelper.find_network_with_bridge(self._session, bridge)
 
             if network_ref:
-                VMHelper.create_vif(self._session, vm_opaque_ref,
-                                    network_ref, instance.mac_address)
+                try:
+                    device = "1" if instance._rescue else "0"
+                except AttributeError:
+                    device = "0"
+
+                VMHelper.create_vif(
+                    self._session,
+                    vm_opaque_ref,
+                    network_ref,
+                    instance.mac_address,
+                    device)
 
     def reset_network(self, instance):
         """
@@ -627,7 +732,7 @@
         args.update(addl_args)
         try:
             task = self._session.async_call_plugin(plugin, method, args)
-            ret = self._session.wait_for_task(instance_id, task)
+            ret = self._session.wait_for_task(task, instance_id)
         except self.XenAPI.Failure, e:
             ret = None
             err_trace = e.details[-1]
 
=== modified file 'nova/virt/xenapi/volumeops.py'
--- nova/virt/xenapi/volumeops.py	2011-01-19 20:26:09 +0000
+++ nova/virt/xenapi/volumeops.py	2011-03-02 23:12:16 +0000
@@ -83,7 +83,7 @@
                 try:
                     task = self._session.call_xenapi('Async.VBD.plug',
                                                            vbd_ref)
-                    self._session.wait_for_task(vol_rec['deviceNumber'], task)
+                    self._session.wait_for_task(task, vol_rec['deviceNumber'])
                 except self.XenAPI.Failure, exc:
                     LOG.exception(exc)
                     VolumeHelper.destroy_iscsi_storage(self._session,
 
=== modified file 'nova/virt/xenapi_conn.py'
--- nova/virt/xenapi_conn.py	2011-02-25 16:47:08 +0000
+++ nova/virt/xenapi_conn.py	2011-03-02 23:12:16 +0000
@@ -196,6 +196,14 @@
         """resume the specified instance"""
         self._vmops.resume(instance, callback)
 
+    def rescue(self, instance, callback):
+        """Rescue the specified instance"""
+        self._vmops.rescue(instance, callback)
+
+    def unrescue(self, instance, callback):
+        """Unrescue the specified instance"""
+        self._vmops.unrescue(instance, callback)
+
     def reset_network(self, instance):
         """reset networking for specified instance"""
         self._vmops.reset_network(instance)
@@ -279,7 +287,7 @@
                              self._session.xenapi.Async.host.call_plugin,
                              self.get_xenapi_host(), plugin, fn, args)
 
-    def wait_for_task(self, id, task):
+    def wait_for_task(self, task, id=None):
         """Return the result of the given task. The task is polled
         until it completes. Not re-entrant."""
         done = event.Event()
@@ -306,10 +314,11 @@
         try:
             name = self._session.xenapi.task.get_name_label(task)
             status = self._session.xenapi.task.get_status(task)
-            action = dict(
-                instance_id=int(id),
-                action=name[0:255],  # Ensure action is never > 255
-                error=None)
+            if id:
+                action = dict(
+                    instance_id=int(id),
+                    action=name[0:255],  # Ensure action is never > 255
+                    error=None)
             if status == "pending":
                 return
             elif status == "success":
@@ -323,7 +332,9 @@
                 LOG.warn(_("Task [%(name)s] %(task)s status:"
                         " %(status)s    %(error_info)s") % locals())
                 done.send_exception(self.XenAPI.Failure(error_info))
-            db.instance_action_create(context.get_admin_context(), action)
+
+            if id:
+                db.instance_action_create(context.get_admin_context(), action)
         except self.XenAPI.Failure, exc:
             LOG.warn(exc)
             done.send_exception(*sys.exc_info())