Merge lp:~salvatore-orlando/nova/bug723301 into lp:~hudson-openstack/nova/trunk

[Salvatore Orlando]

implemented clean-up logic when VM fails to spawn for xenapi back-end

The spawn method in VMOps has been modified in order to handle exceptions occuring before a VM record is created; the exception handling procedure will destroy any VDI created so far, as well as remove kernel/ramdisk files from /boot/guest in dom0 file system.

Exceptions generated by methods dealing with resources on the hypervisor called by spawn are caught and then raised again adding the identifier of any already created VDI.

[Brian Lamar (blamar)]

I'm not familiar with a lot of the terminology here but hopefully I can be of some assistance.

33: self.assertRaises(Exception,
34: + self._test_spawn, 1, 2, 3)

Can this look for a particular exception instead of `Exception`?

139 + task = session.call_xenapi('Async.VDI.destroy', vdi_ref)
140 + session.wait_for_task(task)

Do both line 139 and 140 raise the cls.XenAPI.Failure exception? If not, only the calls that can raise the exception in question should be encapsulated in the try/except.

I have the same question for lines 181 through 209. Which exact calls are you looking to catch an exception from? I don't like "except BaseException" which will catch all sort of things (SystemExit, etc.).

Same for line 216, is it possible to be more explicit than "except:"?

Same for 262 through 320. Error handling is best handed at a more granular level.
Same for 407 through 452. That is a lot of logic to handle for the error case.

Overall I'd like to see more concise error handling using more specific errors if/where possible.

review: Needs Fixing

[Salvatore Orlando (salvatore-orlando)]

Hi Brian,

thanks for your prompt review.
Please see my comments inline.

> I'm not familiar with a lot of the terminology here but hopefully I can be of
> some assistance.
>
> 33: self.assertRaises(Exception,
> 34: + self._test_spawn, 1, 2, 3)
>
> Can this look for a particular exception instead of `Exception`?

The reason for which I'm looking for a generic exception is that the spawn process might fail in several ways. However, since I'm forcing to fail in a particular way by stubbing out a method in the test case, I guess I can easily use a specific type of exception in assertRaises.

>
> 139 + task = session.call_xenapi('Async.VDI.destroy', vdi_ref)
> 140 + session.wait_for_task(task)
>
> Do both line 139 and 140 raise the cls.XenAPI.Failure exception? If not, only
> the calls that can raise the exception in question should be encapsulated in
> the try/except.

Yes, they can both raise a XenAPI Failure. Line 139 might fail, for instance, if we lose connection to the server, whereas line 140 raises a Failure if the async operation started at line 139 fails for some reason.

>
> I have the same question for lines 181 through 209. Which exact calls are you
> looking to catch an exception from? I don't like "except BaseException" which
> will catch all sort of things (SystemExit, etc.).
>

I used BaseException in order to be able to handle both system and user exception. However, as concerns line 181 to 209, I'm not interested in anything else than XenAPI.Failure. I will change the code to catch only this kind of failure

> Same for line 216, is it possible to be more explicit than "except:"?
>

Definitely.

> Same for 262 through 320. Error handling is best handed at a more granular
> level.

In this code fragment we can have several kinds of failures (Glance failure, OS failures during partition preparation, XenAPI failures). I used a single try/except block with BaseException for having a single point for handling all those exception. I understand however this code will not deal properly with unexpected exceptions that I'm not prepared to handle in the catch block.

I can either replace BaseException with the exact types of exception I'm ready to handle or have smaller try/except blocks only for bits that I'm interesting in.
Which approach would you suggest to be better (or should I probably have smaller try/except blocks catching only specific expections) ?
> Same for 407 through 452. That is a lot of logic to handle for the error case.
>
The except block in this code fragment deals with exception raised in the spawn method and exception throw by other methods. I agree that is quite a lot code. I'll try and find a way to reduce the error handling logic.
>
> Overall I'd like to see more concise error handling using more specific errors
> if/where possible.
Agreed.

[Brian Lamar (blamar)]

> I can either replace BaseException with the exact types of exception I'm ready to handle or
> have smaller try/except blocks only for bits that I'm interesting in.
> Which approach would you suggest to be better (or should I probably have smaller try/except
> blocks catching only specific expections) ?

I'd love to see several smaller try/except blocks encapsulating the relevant calls. If/when this isn't possible you can catch multiple types of exceptions. If code can be shared between the except blocks then break those out into functions etc.

Thanks for the code!

[Salvatore Orlando (salvatore-orlando)]

Some comments to help reviewers' task:
During the spawn process, we create several resources before the VM record is created (VDIs and files in dom0's filesystem). The aim of this bug fix is to make sure all these resources are removed if the spawn process fails *before* the VM record is created; if the spawn process fails after that, all the resources will be removed upon instance termination, as the VM record has a reference to all of them.

Resources to be removed might include:
- disk VDI
- kernel/ramdisk files on dom0's filesystem (in /boot/guest)
- temporary VDIs for kernel and ramdisk

The resources to be removed will be stored in a dict. Keys of the dict are Image Types (KERNEL, RAMDISK, VHD, DISK), whereas values are a 2-element tuple in which the 1st element is the VDI uuid and the 2nd element the filename on dom0.

We look for the following exceptions:
- XenAPI.Failure which can occurs in several places
- OSError, which can occur when operations like parted or pygrub fail
- IOError, which can occur when _stream_disk fails (e.g.: backend failures, glance failures)

When an exception occurs, exception handling blocks in interested function add information about resources created so far to the dict, and then raise again the error. Resources are then finally removed in _handle_spawn_error.

As an example:
{
  ImageType.KERNEL: (None, /boot/guest/kernel-file),
  ImageType.RAMDISK: (ramdisk-vdi-uuid,),
  ImageType.DISK: (disk-vdi-uuid,)
}

Represents the dict that would have been built if an exception occurred while copying the temporary ramdisk VDI to dom0. The 'main' exception handling function, VMOps._handle_spawn_error will therefore remove ramdisk and disk VDI using XenAPI's VDI.destroy, and invoke a XenAPI plugin call for removing /boot/guest/kernel-file

[Matt Dietz (cerberus)]

Thanks for this Salvatore! We've need this for a while now.

Could only find a few things to pick on ;-)

PEP8 / Style nit-picks:

96 import os
97 +import sys
98 import pickle

pep8: sys should come after re in this list

351 import subprocess
352 import tempfile
353 import uuid
354 +import sys

Same thing

review: Needs Fixing

[Sandy Walsh (sandy-walsh)]

Thanks for the clarification of what's going on here. That makes my life much easier. Perhaps consider putting that in a comment somewhere? :)

Great branch by the way! Much needed.

Minor style point: I think there're supposed to be a space after the hash in:
 # Comment ...

Easy changes:

433 + if len(vm_create_error.args) > 0:
434 + last_arg = vm_create_error.args[len(vm_create_error.args) - 1]
if vm_create_error.args:
   last_arg = vm_create_error.args[-1]

same thing in 455-456, 475, 479

465 + if vdi_to_remove != None:
if vdi_to_remove:

Set WIP so I can approve afterwards ...

review: Needs Fixing

[Sandy Walsh (sandy-walsh)]

easy to tell I'm fried today ... s/there're/there is/

(there're ... wtf?)

[Salvatore Orlando (salvatore-orlando)]

Style comments have been addressed...
all but one:

if len(items_to_remove) > 1:
476 + # there is also a file to remove
477 + files_to_remove[vdi_type] = items_to_remove[1]

as we are actually checking that there are at least two elements in the list.

Thanks for spending time reviewing this branch, I hope you can find some time to give another round of review!

[termie (termie)]

- line 251: please put one arg per line, or all on the same line on the next line, double indented
- please format all docstrings according to guide in HACKING
- line 258: perhaps assign the lambda on the line before and pass it in to this function instead of defining it inline here as it gets a bit messy
- line 526: dictionary contents on the same line as the opening parens, you can remove the comment and you doing foo[:255] is the normal way of saying foo[0:255]
- line 551: you actually need that extra line

review: Needs Fixing

[Salvatore Orlando (salvatore-orlando)]

Guys, I have addressed the last review's comments, ball in your court now :)

Cheers!

[Brian Lamar (blamar)]

review: Approve

[termie (termie)]

almost there, just looks like the couple docstrings in nova/tests/test_xenapi.py need to be formatted correctly.

review: Needs Fixing

[Soren Hansen (soren)]

> === modified file 'nova/tests/test_xenapi.py'
> --- nova/tests/test_xenapi.py 2011-03-28 21:00:17 +0000
> +++ nova/tests/test_xenapi.py 2011-03-31 09:25:51 +0000
> @@ -327,6 +327,19 @@
> self.assertEquals(self.vm['HVM_boot_params'], {})
> self.assertEquals(self.vm['HVM_boot_policy'], '')
>
> + def _check_no_unbound_vdi(self):
> + url = FLAGS.xenapi_connection_url
> + username = FLAGS.xenapi_connection_username
> + password = FLAGS.xenapi_connection_password
> + session = xenapi_conn.XenAPISession(url, username, password)
> + vdi_refs = session.call_xenapi('VDI.get_all')
> + for vdi_ref in vdi_refs:
> + vdi_rec = session.call_xenapi('VDI.get_record', vdi_ref)
> + if 'VBDs' in vdi_rec:
> + self.assertEquals(vdi_rec['VBDs'], {})
> + else:
> + self.fail('Found unexpected unbound VDI:%s' % vdi_rec['uuid'])

I'm having trouble followin the data model.
If vdi_rec has a 'VBDs' key, it has to be empty. If there's no 'VBDs' key, that
means unexpected unbound VDI's are found? Is that accurate?

review: Needs Information

[Salvatore Orlando (salvatore-orlando)]

@termie:

docstrings are now formatted according to PEP-257

@Soren:

I agree the code is at least cryptic. Basically the test fails if a VDI without a VBD was found (hence 'unbound'). The VBD is the 'Virtual Block Device' that binds a VDI to a VM.
The other check:

if 'VBDs' in vdi_rec:
             self.assertEquals(vdi_rec['VBDs'], {})

was in place because we are dealing with the fake xenapi driver, but was not strictly required.

However, I have improved the unit tests trying to make them more clear: we now extract the whole list of VDIs before and after the spawn process, and verify that no new VDI has been added.

[Rick Harris (rconradharris)]

Really nice work here, Salvatore. Marking as approved with a few optional cleanup suggestions:

> 401 + if not image_type in (ImageType.KERNEL, ImageType.RAMDISK):

`X not in Y` seems a bit more readable:

    if image_type not in (ImageType.KERNEL, ImageType.RAMDISK):

> 205 + def destroy_vdi(cls, session, vdi_ref):

Now that we have a destroy_vdi helper, can we modify vmops:_destroy_vdis and
vmops:_destroy_rescue_vdis to use this. Should DRY up the code somewhat.

May have to add a `raise_on_error` kwarg since _destroy_rescue_vdis (for some
reason) just `continue`s.

> + except (self.XenAPI.Failure, OSError, IOError) as vm_create_error:

For me, the code which propogates the 'rollback-context` to the handler is a
little hard to follow. I think it may be due to trying to re-use the original
exception and attaching the rollback-context as a positional-arg.

As a point of discussion, another route might be to create a wrapper exception called
`ExceptionWithRollback` where any information needed to rollback after the
original exception is triggered is stored within a `rollback_context` dict.

Then, after handling the `ExceptionWithRollback` exception, we can just
re-raise the inner exception, something like:

  raise rollback_exc.orig_exc # re-raise the wrapped exception

The advantage of this approach, is that it clearly delinates the original
exception from its rollback-context, we don't have to fool around with last
position-arg code, and we can potentially re-use this concept/pattern in other
portions of Xenapi virt layer.

review: Approve

[Matt Dietz (cerberus)]

lgtm. We'll see what Termie has to say

review: Approve

[termie (termie)]

These two docstrings should have a one line summary, followed by a newline and then the additional information.

37 + """Simulates an error while downloading an image.
38 + Verifies that VDIs created are properly cleaned up.
39 +
40 + """

For example, would become:

"""Simulates an error while downloading an image.

Verifies that VDIs created are properly cleaned up.

"""

50 + def test_spawn_fail_cleanup_2(self):
51 + """Simulates an error while creating VM record. It
52 + verifies that VDIs created are properly cleaned up.
53 +
54 + """

review: Needs Fixing

[Dan Prince (dan-prince)]

Hi Salvatore,

I get conflicts when trying to merge your latest code w/ lp:nova. Can you have a look?

Dan

[Salvatore Orlando (salvatore-orlando)]

Sorry for the delay in fixing this code, I was on vacation.

[Salvatore Orlando (salvatore-orlando)]

Hi,

I am reproposing this branch for merging.
Code has been adapted to latest changes in routines for creating VDIs and fetching images from glance and objectstore.

However, I think the code can be improved as we are now using two distinct sets of values for describing image types, the ImageType enumeration, and the vdi_type key returned by methods which fetch images.

I was thinking of using just one enumeration, thus simplifying future code maintenance.
I submitted a question on this topic: https://answers.launchpad.net/nova/+question/161496

Thanks,
Salvatore

[Sandy Walsh (sandy-walsh)]

+252/259 commented out?
+295/303 ... remove?
+436 necessary?

Needs a trunk merge ... conflicts.

Nit picky: Comments should be proper sentences. Capitalized and punctuated. Space after #

review: Needs Fixing

[Salvatore Orlando (salvatore-orlando)]

Hi Sandy,

I've addressed your comments and resolved the conflicts.

Thanks,
Salvatore

[Sandy Walsh (sandy-walsh)]

Thanks ... works for me!

review: Approve

[Vish Ishaya (vishvananda)]

looks good to go as soon as termie's docstring concerns are addressed.

[Brian Waldon (bcwaldon)]

Setting to WIP until test_xenapi.py docstrings are formatted correctly.

[Salvatore Orlando (salvatore-orlando)]

docstrings fixed

[Salvatore Orlando (salvatore-orlando)]

No setting back to needs reviews though, as I get the following exception while running tests:

======================================================================
ERROR: <nose.suite.ContextSuite context=nova.tests>
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/pymodules/python2.6/nose/suite.py", line 183, in run
    self.setUp()
  File "/usr/lib/pymodules/python2.6/nose/suite.py", line 264, in setUp
    self.setupContext(ancestor)
  File "/usr/lib/pymodules/python2.6/nose/suite.py", line 287, in setupContext
    try_run(context, names)
  File "/usr/lib/pymodules/python2.6/nose/util.py", line 487, in try_run
    return func()
  File "/home/salvatore/workspace/Openstack-bug723301/nova/tests/__init__.py", line 73, in setup
    cleandb = os.path.join(FLAGS.state_path, FLAGS.sqlite_clean_db)
  File "/home/salvatore/workspace/Openstack-bug723301/nova/flags.py", line 144, in __getattr__
    val = gflags.FlagValues.__getattr__(self, name)
  File "/usr/lib/pymodules/python2.6/gflags.py", line 810, in __getattr__
    raise AttributeError(name)
AttributeError: sqlite_clean_db

It is the first time I get it, I don't think it depends on my code, but I prefer to fix it before proposing again for merge.

[Salvatore Orlando (salvatore-orlando)]

Previous issue with tests has now been solved.
Setting back to needs review.

[Brian Lamar (blamar)]

review: Approve

[Brian Waldon (bcwaldon)]

I think termie's concerns have been addressed. Waiting to hear back from Soren

[Soren Hansen (soren)]

My question has been answered, and this MP has more than enough +1's. Just ignore me :)

review: Abstain

[Brian Waldon (bcwaldon)]

Looks good to me

review: Approve

=== modified file 'nova/tests/test_xenapi.py'
--- nova/tests/test_xenapi.py	2011-06-27 21:48:03 +0000
+++ nova/tests/test_xenapi.py	2011-07-04 16:24:47 +0000
@@ -381,6 +381,18 @@
         self.assertEquals(self.vm['HVM_boot_params'], {})
         self.assertEquals(self.vm['HVM_boot_policy'], '')
 
+    def _list_vdis(self):
+        url = FLAGS.xenapi_connection_url
+        username = FLAGS.xenapi_connection_username
+        password = FLAGS.xenapi_connection_password
+        session = xenapi_conn.XenAPISession(url, username, password)
+        return session.call_xenapi('VDI.get_all')
+
+    def _check_vdis(self, start_list, end_list):
+        for vdi_ref in end_list:
+            if not vdi_ref in start_list:
+                self.fail('Found unexpected VDI:%s' % vdi_ref)
+
     def _test_spawn(self, image_ref, kernel_id, ramdisk_id,
                     instance_type_id="3", os_type="linux",
                     architecture="x86-64", instance_id=1,
@@ -422,6 +434,36 @@
                           self._test_spawn,
                           1, 2, 3, "4")  # m1.xlarge
 
+    def test_spawn_fail_cleanup_1(self):
+        """Simulates an error while downloading an image.
+
+        Verifies that VDIs created are properly cleaned up.
+
+        """
+        vdi_recs_start = self._list_vdis()
+        FLAGS.xenapi_image_service = 'glance'
+        stubs.stubout_fetch_image_glance_disk(self.stubs)
+        self.assertRaises(xenapi_fake.Failure,
+                          self._test_spawn, 1, 2, 3)
+        # No additional VDI should be found.
+        vdi_recs_end = self._list_vdis()
+        self._check_vdis(vdi_recs_start, vdi_recs_end)
+
+    def test_spawn_fail_cleanup_2(self):
+        """Simulates an error while creating VM record.
+
+        It verifies that VDIs created are properly cleaned up.
+
+        """
+        vdi_recs_start = self._list_vdis()
+        FLAGS.xenapi_image_service = 'glance'
+        stubs.stubout_create_vm(self.stubs)
+        self.assertRaises(xenapi_fake.Failure,
+                          self._test_spawn, 1, 2, 3)
+        # No additional VDI should be found.
+        vdi_recs_end = self._list_vdis()
+        self._check_vdis(vdi_recs_start, vdi_recs_end)
+
     def test_spawn_raw_objectstore(self):
         FLAGS.xenapi_image_service = 'objectstore'
         self._test_spawn(1, None, None)

=== modified file 'nova/tests/xenapi/stubs.py'
--- nova/tests/xenapi/stubs.py	2011-06-06 15:00:51 +0000
+++ nova/tests/xenapi/stubs.py	2011-07-04 16:24:47 +0000
@@ -98,6 +98,42 @@
     stubs.Set(vm_utils, '_is_vdi_pv', f)
 
 
+def stubout_determine_is_pv_objectstore(stubs):
+    """Assumes VMs never have PV kernels"""
+
+    @classmethod
+    def f(cls, *args):
+        return False
+    stubs.Set(vm_utils.VMHelper, '_determine_is_pv_objectstore', f)
+
+
+def stubout_lookup_image(stubs):
+    """Simulates a failure in lookup image."""
+    def f(_1, _2, _3, _4):
+        raise Exception("Test Exception raised by fake lookup_image")
+    stubs.Set(vm_utils, 'lookup_image', f)
+
+
+def stubout_fetch_image_glance_disk(stubs):
+    """Simulates a failure in fetch image_glance_disk."""
+
+    @classmethod
+    def f(cls, *args):
+        raise fake.Failure("Test Exception raised by " +
+                           "fake fetch_image_glance_disk")
+    stubs.Set(vm_utils.VMHelper, '_fetch_image_glance_disk', f)
+
+
+def stubout_create_vm(stubs):
+    """Simulates a failure in create_vm."""
+
+    @classmethod
+    def f(cls, *args):
+        raise fake.Failure("Test Exception raised by " +
+                           "fake create_vm")
+    stubs.Set(vm_utils.VMHelper, 'create_vm', f)
+
+
 def stubout_loopingcall_start(stubs):
     def fake_start(self, interval, now=True):
         self.f(*self.args, **self.kw)
@@ -120,6 +156,9 @@
         super(FakeSessionForVMTests, self).__init__(uri)
 
     def host_call_plugin(self, _1, _2, plugin, method, _5):
+        # If the call is for 'copy_kernel_vdi' return None.
+        if method == 'copy_kernel_vdi':
+            return
         sr_ref = fake.get_all('SR')[0]
         vdi_ref = fake.create_vdi('', False, sr_ref, False)
         vdi_rec = fake.get_record('VDI', vdi_ref)

=== modified file 'nova/virt/xenapi/vm_utils.py'
--- nova/virt/xenapi/vm_utils.py	2011-06-24 12:01:51 +0000
+++ nova/virt/xenapi/vm_utils.py	2011-07-04 16:24:47 +0000
@@ -23,6 +23,7 @@
 import os
 import pickle
 import re
+import sys
 import tempfile
 import time
 import urllib
@@ -71,17 +72,51 @@
 class ImageType:
     """
     Enumeration class for distinguishing different image types
-        0 - kernel/ramdisk image (goes on dom0's filesystem)
-        1 - disk image (local SR, partitioned by objectstore plugin)
-        2 - raw disk image (local SR, NOT partitioned by plugin)
-        3 - vhd disk image (local SR, NOT inspected by XS, PV assumed for
+        0 - kernel image (goes on dom0's filesystem)
+        1 - ramdisk image (goes on dom0's filesystem)
+        2 - disk image (local SR, partitioned by objectstore plugin)
+        3 - raw disk image (local SR, NOT partitioned by plugin)
+        4 - vhd disk image (local SR, NOT inspected by XS, PV assumed for
             linux, HVM assumed for Windows)
     """
 
-    KERNEL_RAMDISK = 0
-    DISK = 1
-    DISK_RAW = 2
-    DISK_VHD = 3
+    KERNEL = 0
+    RAMDISK = 1
+    DISK = 2
+    DISK_RAW = 3
+    DISK_VHD = 4
+
+    KERNEL_STR = "kernel"
+    RAMDISK_STR = "ramdisk"
+    DISK_STR = "os"
+    DISK_RAW_STR = "os_raw"
+    DISK_VHD_STR = "vhd"
+
+    @classmethod
+    def to_string(cls, image_type):
+        if image_type == ImageType.KERNEL:
+            return ImageType.KERNEL_STR
+        elif image_type == ImageType.RAMDISK:
+            return ImageType.RAMDISK_STR
+        elif image_type == ImageType.DISK:
+            return ImageType.DISK_STR
+        elif image_type == ImageType.DISK_RAW:
+            return ImageType.DISK_RAW_STR
+        elif image_type == ImageType.DISK_VHD:
+            return ImageType.VHD_STR
+
+    @classmethod
+    def from_string(cls, image_type_str):
+        if image_type_str == ImageType.KERNEL_STR:
+            return ImageType.KERNEL
+        elif image_type == ImageType.RAMDISK_STR:
+            return ImageType.RAMDISK
+        elif image_type == ImageType.DISK_STR:
+            return ImageType.DISK
+        elif image_type == ImageType.DISK_RAW_STR:
+            return ImageType.DISK_RAW
+        elif image_type == ImageType.DISK_VHD_STR:
+            return ImageType.VHD
 
 
 class VMHelper(HelperBase):
@@ -145,7 +180,6 @@
             'VCPUs_max': vcpus,
             'VCPUs_params': {},
             'xenstore_data': {}}
-
         # Complete VM configuration record according to the image type
         # non-raw/raw with PV kernel/raw in HVM mode
         if use_pv_kernel:
@@ -240,6 +274,15 @@
             raise StorageError(_('Unable to destroy VBD %s') % vbd_ref)
 
     @classmethod
+    def destroy_vdi(cls, session, vdi_ref):
+        try:
+            task = session.call_xenapi('Async.VDI.destroy', vdi_ref)
+            session.wait_for_task(task)
+        except cls.XenAPI.Failure, exc:
+            LOG.exception(exc)
+            raise StorageError(_('Unable to destroy VDI %s') % vdi_ref)
+
+    @classmethod
     def create_vif(cls, session, vm_ref, network_ref, mac_address,
                    dev, rxtx_cap=0):
         """Create a VIF record.  Returns a Deferred that gives the new
@@ -394,12 +437,12 @@
         """
         LOG.debug(_("Asking xapi to fetch vhd image %(image)s")
                     % locals())
-
         sr_ref = safe_find_sr(session)
 
-        # NOTE(sirp): The Glance plugin runs under Python 2.4 which does not
-        # have the `uuid` module. To work around this, we generate the uuids
-        # here (under Python 2.6+) and pass them as arguments
+        # NOTE(sirp): The Glance plugin runs under Python 2.4
+        # which does not have the `uuid` module. To work around this,
+        # we generate the uuids here (under Python 2.6+) and
+        # pass them as arguments
         uuid_stack = [str(uuid.uuid4()) for i in xrange(2)]
 
         glance_host, glance_port = \
@@ -449,18 +492,20 @@
         # FIXME(sirp): Since the Glance plugin seems to be required for the
         # VHD disk, it may be worth using the plugin for both VHD and RAW and
         # DISK restores
+        LOG.debug(_("Fetching image %(image)s") % locals())
+        LOG.debug(_("Image Type: %s"), ImageType.to_string(image_type))
         sr_ref = safe_find_sr(session)
 
         glance_client, image_id = nova.image.get_glance_client(image)
         meta, image_file = glance_client.get_image(image_id)
         virtual_size = int(meta['size'])
         vdi_size = virtual_size
-        LOG.debug(_("Size for image %(image)s:%(virtual_size)d") % locals())
-
+        LOG.debug(_("Size for image %(image)s:" +
+                    "%(virtual_size)d") % locals())
         if image_type == ImageType.DISK:
             # Make room for MBR.
             vdi_size += MBR_SIZE_BYTES
-        elif image_type == ImageType.KERNEL_RAMDISK and \
+        elif image_type in (ImageType.KERNEL, ImageType.RAMDISK) and \
              vdi_size > FLAGS.max_kernel_ramdisk_size:
             max_size = FLAGS.max_kernel_ramdisk_size
             raise exception.Error(
@@ -469,29 +514,45 @@
 
         name_label = get_name_label_for_image(image)
         vdi_ref = cls.create_vdi(session, sr_ref, name_label, vdi_size, False)
-
-        with_vdi_attached_here(session, vdi_ref, False,
-                               lambda dev:
-                               _stream_disk(dev, image_type,
-                                            virtual_size, image_file))
-        if image_type == ImageType.KERNEL_RAMDISK:
-            #we need to invoke a plugin for copying VDI's
-            #content into proper path
-            LOG.debug(_("Copying VDI %s to /boot/guest on dom0"), vdi_ref)
-            fn = "copy_kernel_vdi"
-            args = {}
-            args['vdi-ref'] = vdi_ref
-            #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(task, instance_id)
-            #remove the VDI as it is not needed anymore
-            session.get_xenapi().VDI.destroy(vdi_ref)
-            LOG.debug(_("Kernel/Ramdisk VDI %s destroyed"), vdi_ref)
-            return filename
-        else:
+        # From this point we have a VDI on Xen host;
+        # If anything goes wrong, we need to remember its uuid.
+        try:
+            filename = None
             vdi_uuid = session.get_xenapi().VDI.get_uuid(vdi_ref)
-            return [dict(vdi_type='os', vdi_uuid=vdi_uuid)]
+            with_vdi_attached_here(session, vdi_ref, False,
+                                   lambda dev:
+                                   _stream_disk(dev, image_type,
+                                                virtual_size, image_file))
+            if image_type in (ImageType.KERNEL, ImageType.RAMDISK):
+                # We need to invoke a plugin for copying the
+                # content of the VDI into the proper path.
+                LOG.debug(_("Copying VDI %s to /boot/guest on dom0"), vdi_ref)
+                fn = "copy_kernel_vdi"
+                args = {}
+                args['vdi-ref'] = vdi_ref
+                # 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(task, instance_id)
+                # Remove the VDI as it is not needed anymore.
+                session.get_xenapi().VDI.destroy(vdi_ref)
+                LOG.debug(_("Kernel/Ramdisk VDI %s destroyed"), vdi_ref)
+                return [dict(vdi_type=ImageType.to_string(image_type),
+                             vdi_uuid=None,
+                             file=filename)]
+            else:
+                return [dict(vdi_type=ImageType.to_string(image_type),
+                             vdi_uuid=vdi_uuid,
+                             file=None)]
+        except (cls.XenAPI.Failure, IOError, OSError) as e:
+            # We look for XenAPI and OS failures.
+            LOG.exception(_("instance %s: Failed to fetch glance image"),
+                          instance_id, exc_info=sys.exc_info())
+            e.args = e.args + ([dict(vdi_type=ImageType.
+                                              to_string(image_type),
+                                    vdi_uuid=vdi_uuid,
+                                    file=filename)],)
+            raise e
 
     @classmethod
     def determine_disk_image_type(cls, instance):
@@ -506,7 +567,8 @@
            whether a kernel_id is specified.
         """
         def log_disk_format(image_type):
-            pretty_format = {ImageType.KERNEL_RAMDISK: 'KERNEL_RAMDISK',
+            pretty_format = {ImageType.KERNEL: 'KERNEL',
+                             ImageType.RAMDISK: 'RAMDISK',
                              ImageType.DISK: 'DISK',
                              ImageType.DISK_RAW: 'DISK_RAW',
                              ImageType.DISK_VHD: 'DISK_VHD'}
@@ -519,8 +581,8 @@
         def determine_from_glance():
             glance_disk_format2nova_type = {
                 'ami': ImageType.DISK,
-                'aki': ImageType.KERNEL_RAMDISK,
-                'ari': ImageType.KERNEL_RAMDISK,
+                'aki': ImageType.KERNEL,
+                'ari': ImageType.RAMDISK,
                 'raw': ImageType.DISK_RAW,
                 'vhd': ImageType.DISK_VHD}
             image_ref = instance.image_ref
@@ -553,7 +615,7 @@
                             image_type):
         """Fetch image from glance based on image type.
 
-        Returns: A single filename if image_type is KERNEL_RAMDISK
+        Returns: A single filename if image_type is KERNEL or RAMDISK
                  A list of dictionaries that describe VDIs, otherwise
         """
         if image_type == ImageType.DISK_VHD:
@@ -568,13 +630,13 @@
                                  secret, image_type):
         """Fetch an image from objectstore.
 
-        Returns: A single filename if image_type is KERNEL_RAMDISK
+        Returns: A single filename if image_type is KERNEL or RAMDISK
                  A list of dictionaries that describe VDIs, otherwise
         """
         url = "http://%s:%s/_images/%s/image" % (FLAGS.s3_host, FLAGS.s3_port,
                                                  image)
         LOG.debug(_("Asking xapi to fetch %(url)s as %(access)s") % locals())
-        if image_type == ImageType.KERNEL_RAMDISK:
+        if image_type in (ImageType.KERNEL, ImageType.RAMDISK):
             fn = 'get_kernel'
         else:
             fn = 'get_vdi'
@@ -584,15 +646,20 @@
         args['password'] = secret
         args['add_partition'] = 'false'
         args['raw'] = 'false'
-        if image_type != ImageType.KERNEL_RAMDISK:
+        if not image_type in (ImageType.KERNEL, ImageType.RAMDISK):
             args['add_partition'] = 'true'
             if image_type == ImageType.DISK_RAW:
                 args['raw'] = 'true'
         task = session.async_call_plugin('objectstore', fn, args)
-        uuid_or_fn = session.wait_for_task(task, instance_id)
-        if image_type != ImageType.KERNEL_RAMDISK:
-            return [dict(vdi_type='os', vdi_uuid=uuid_or_fn)]
-        return uuid_or_fn
+        vdi_uuid = None
+        filename = None
+        if image_type in (ImageType.KERNEL, ImageType.RAMDISK):
+            filename = session.wait_for_task(task, instance_id)
+        else:
+            vdi_uuid = session.wait_for_task(task, instance_id)
+        return [dict(vdi_type=ImageType.to_string(image_type),
+                     vdi_uuid=vdi_uuid,
+                     file=filename)]
 
     @classmethod
     def determine_is_pv(cls, session, instance_id, vdi_ref, disk_image_type,

=== modified file 'nova/virt/xenapi/vmops.py'
--- nova/virt/xenapi/vmops.py	2011-06-30 19:20:59 +0000
+++ nova/virt/xenapi/vmops.py	2011-07-04 16:24:47 +0000
@@ -25,6 +25,7 @@
 import os
 import pickle
 import subprocess
+import sys
 import time
 import uuid
 
@@ -137,9 +138,18 @@
         return vdis
 
     def spawn(self, instance, network_info):
-        vdis = self._create_disks(instance)
-        vm_ref = self._create_vm(instance, vdis, network_info)
-        self._spawn(instance, vm_ref)
+        vdis = None
+        try:
+            vdis = self._create_disks(instance)
+            vm_ref = self._create_vm(instance, vdis, network_info)
+            self._spawn(instance, vm_ref)
+        except (self.XenAPI.Failure, OSError, IOError) as spawn_error:
+            LOG.exception(_("instance %s: Failed to spawn"),
+                          instance.id, exc_info=sys.exc_info())
+            LOG.debug(_('Instance %s failed to spawn - performing clean-up'),
+                      instance.id)
+            self._handle_spawn_error(vdis, spawn_error)
+            raise spawn_error
 
     def spawn_rescue(self, instance):
         """Spawn a rescue instance."""
@@ -165,42 +175,64 @@
         project = AuthManager().get_project(instance.project_id)
 
         disk_image_type = VMHelper.determine_disk_image_type(instance)
-
         kernel = None
-        if instance.kernel_id:
-            kernel = VMHelper.fetch_image(self._session, instance.id,
-                    instance.kernel_id, user, project,
-                    ImageType.KERNEL_RAMDISK)
-
         ramdisk = None
-        if instance.ramdisk_id:
-            ramdisk = VMHelper.fetch_image(self._session, instance.id,
-                    instance.ramdisk_id, user, project,
-                    ImageType.KERNEL_RAMDISK)
-
-        # Create the VM ref and attach the first disk
-        first_vdi_ref = self._session.call_xenapi('VDI.get_by_uuid',
-                vdis[0]['vdi_uuid'])
-
-        vm_mode = instance.vm_mode and instance.vm_mode.lower()
-        if vm_mode == 'pv':
-            use_pv_kernel = True
-        elif vm_mode in ('hv', 'hvm'):
-            use_pv_kernel = False
-            vm_mode = 'hvm'  # Normalize
-        else:
-            use_pv_kernel = VMHelper.determine_is_pv(self._session,
-                    instance.id, first_vdi_ref, disk_image_type,
-                    instance.os_type)
-            vm_mode = use_pv_kernel and 'pv' or 'hvm'
-
-        if instance.vm_mode != vm_mode:
-            # Update database with normalized (or determined) value
-            db.instance_update(context.get_admin_context(),
-                               instance['id'], {'vm_mode': vm_mode})
-
-        vm_ref = VMHelper.create_vm(self._session, instance,
-                kernel, ramdisk, use_pv_kernel)
+        try:
+            if instance.kernel_id:
+                kernel = VMHelper.fetch_image(self._session, instance.id,
+                        instance.kernel_id, user, project,
+                        ImageType.KERNEL)[0]
+            if instance.ramdisk_id:
+                ramdisk = VMHelper.fetch_image(self._session, instance.id,
+                        instance.ramdisk_id, user, project,
+                        ImageType.RAMDISK)[0]
+            # Create the VM ref and attach the first disk
+            first_vdi_ref = self._session.call_xenapi('VDI.get_by_uuid',
+                    vdis[0]['vdi_uuid'])
+
+            vm_mode = instance.vm_mode and instance.vm_mode.lower()
+            if vm_mode == 'pv':
+                use_pv_kernel = True
+            elif vm_mode in ('hv', 'hvm'):
+                use_pv_kernel = False
+                vm_mode = 'hvm'  # Normalize
+            else:
+                use_pv_kernel = VMHelper.determine_is_pv(self._session,
+                        instance.id, first_vdi_ref, disk_image_type,
+                        instance.os_type)
+                vm_mode = use_pv_kernel and 'pv' or 'hvm'
+
+            if instance.vm_mode != vm_mode:
+                # Update database with normalized (or determined) value
+                db.instance_update(context.get_admin_context(),
+                                   instance['id'], {'vm_mode': vm_mode})
+            vm_ref = VMHelper.create_vm(self._session, instance,
+                    kernel and kernel.get('file', None) or None,
+                    ramdisk and ramdisk.get('file', None) or None,
+                    use_pv_kernel)
+        except (self.XenAPI.Failure, OSError, IOError) as vm_create_error:
+            # Collect VDI/file resources to clean up;
+            # These resources will be removed by _handle_spawn_error.
+            LOG.exception(_("instance %s: Failed to spawn - " +
+                            "Unable to create VM"),
+                          instance.id, exc_info=sys.exc_info())
+            last_arg = None
+            resources = []
+
+            if vm_create_error.args:
+                last_arg = vm_create_error.args[-1]
+            if isinstance(last_arg, list):
+                resources = last_arg
+            else:
+                vm_create_error.args = vm_create_error.args + (resources,)
+
+            if kernel:
+                resources.append(kernel)
+            if ramdisk:
+                resources.append(ramdisk)
+
+            raise vm_create_error
+
         VMHelper.create_vbd(session=self._session, vm_ref=vm_ref,
                 vdi_ref=first_vdi_ref, userdevice=0, bootable=True)
 
@@ -321,6 +353,47 @@
 
         return timer.start(interval=0.5, now=True)
 
+    def _handle_spawn_error(self, vdis, spawn_error):
+        # Extract resource list from spawn_error.
+        resources = []
+        if spawn_error.args:
+            last_arg = spawn_error.args[-1]
+            resources = last_arg
+        if vdis:
+            for vdi in vdis:
+                resources.append(dict(vdi_type=vdi['vdi_type'],
+                                      vdi_uuid=vdi['vdi_uuid'],
+                                      file=None))
+
+        LOG.debug(_("Resources to remove:%s"), resources)
+        kernel_file = None
+        ramdisk_file = None
+
+        for item in resources:
+            vdi_type = item['vdi_type']
+            vdi_to_remove = item['vdi_uuid']
+            if vdi_to_remove:
+                try:
+                    vdi_ref = self._session.call_xenapi('VDI.get_by_uuid',
+                            vdi_to_remove)
+                    LOG.debug(_('Removing VDI %(vdi_ref)s' +
+                                '(uuid:%(vdi_to_remove)s)'), locals())
+                    VMHelper.destroy_vdi(self._session, vdi_ref)
+                except self.XenAPI.Failure:
+                    # Vdi has already been deleted
+                    LOG.debug(_("Skipping VDI destroy for %s"), vdi_to_remove)
+            if item['file']:
+                # There is also a file to remove.
+                if vdi_type == ImageType.KERNEL_STR:
+                    kernel_file = item['file']
+                elif vdi_type == ImageType.RAMDISK_STR:
+                    ramdisk_file = item['file']
+
+        if kernel_file or ramdisk_file:
+            LOG.debug(_("Removing kernel/ramdisk files from dom0"))
+            self._destroy_kernel_ramdisk_plugin_call(kernel_file,
+                                                     ramdisk_file)
+
     def _get_vm_opaque_ref(self, instance_or_vm):
         """
         Refactored out the common code of many methods that receive either
@@ -698,6 +771,16 @@
                 VMHelper.unplug_vbd(self._session, vbd_ref)
                 VMHelper.destroy_vbd(self._session, vbd_ref)
 
+    def _destroy_kernel_ramdisk_plugin_call(self, kernel, ramdisk):
+        args = {}
+        if kernel:
+            args['kernel-file'] = kernel
+        if ramdisk:
+            args['ramdisk-file'] = ramdisk
+        task = self._session.async_call_plugin(
+            'glance', 'remove_kernel_ramdisk', args)
+        self._session.wait_for_task(task)
+
     def _destroy_kernel_ramdisk(self, instance, vm_ref):
         """Three situations can occur:
 
@@ -727,13 +810,7 @@
         (kernel, ramdisk) = VMHelper.lookup_kernel_ramdisk(self._session,
                                                            vm_ref)
 
-        LOG.debug(_("Removing kernel/ramdisk files"))
-
-        args = {'kernel-file': kernel, 'ramdisk-file': ramdisk}
-        task = self._session.async_call_plugin(
-            'glance', 'remove_kernel_ramdisk', args)
-        self._session.wait_for_task(task, instance.id)
-
+        self._destroy_kernel_ramdisk_plugin_call(kernel, ramdisk)
         LOG.debug(_("kernel/ramdisk files removed"))
 
     def _destroy_vm(self, instance, vm_ref):

=== modified file 'plugins/xenserver/xenapi/etc/xapi.d/plugins/glance'
--- plugins/xenserver/xenapi/etc/xapi.d/plugins/glance	2011-06-03 18:14:28 +0000
+++ plugins/xenserver/xenapi/etc/xapi.d/plugins/glance	2011-07-04 16:24:47 +0000
@@ -412,8 +412,8 @@
 
 def remove_kernel_ramdisk(session, args):
     """Removes kernel and/or ramdisk from dom0's file system"""
-    kernel_file = exists(args, 'kernel-file')
-    ramdisk_file = exists(args, 'ramdisk-file')
+    kernel_file = optional(args, 'kernel-file')
+    ramdisk_file = optional(args, 'ramdisk-file')
     if kernel_file:
         os.remove(kernel_file)
     if ramdisk_file: