ensure atomic manipulation of libvirt disk images

This pattern could probably be used elsewhere,
but only libvirt disk images are considered for now.
This change ensures there are no stale files left
anywhere in the path from glance, through the libvirt image cache.
These could cause subsequent operational errors either
directly or indirectly through disk wastage.

* nova/utils.py: Add a new remove_path_on_error() context manager
that is used to remove the passed PATH on a raised exception.
* nova/virt/images.py: Ensure temporary downloaded and
converted images are protected.
* nova/virt/libvirt/connection.py: Ensure all the images in
the image cache and instance dirs are protected.

Change-Id: I81a5407665a6998128c0dee41387ef00ebddeb4d

3 files changed, 57 insertions, 49 deletions

diff --git a/nova/utils.py b/nova/utils.py
index 29df28cea..8119861c6 100644
--- a/nova/utils.py
+++ b/nova/utils.py
@@ -21,6 +21,7 @@
 
 import contextlib
 import datetime
+import errno
 import functools
 import hashlib
 import inspect
@@ -1014,8 +1015,8 @@ def cleanup_file_locks():
             continue
         try:
             stat_info = os.stat(os.path.join(FLAGS.lock_path, filename))
-        except OSError as (errno, strerror):
-            if errno == 2:  # doesn't exist
+        except OSError as e:
+            if e.errno == errno.ENOENT:
                 continue
             else:
                 raise
@@ -1034,8 +1035,8 @@ def delete_if_exists(pathname):
 
     try:
         os.unlink(pathname)
-    except OSError as (errno, strerror):
-        if errno == 2:  # doesn't exist
+    except OSError as e:
+        if e.errno == errno.ENOENT:
             return
         else:
             raise
@@ -1345,6 +1346,18 @@ def logging_error(message):
             LOG.exception(message)
 
 
+@contextlib.contextmanager
+def remove_path_on_error(path):
+    """Protect code that wants to operate on PATH atomically.
+    Any exception will cause PATH to be removed.
+    """
+    try:
+        yield
+    except Exception:
+        with save_and_reraise_exception():
+            delete_if_exists(path)
+
+
 def make_dev_path(dev, partition=None, base='/dev'):
     """Return a path to a particular device.
 
diff --git a/nova/virt/images.py b/nova/virt/images.py
index 1e0ae0a22..626f3ff76 100644
--- a/nova/virt/images.py
+++ b/nova/virt/images.py
@@ -51,18 +51,10 @@ def fetch(context, image_href, path, _user_id, _project_id):
     #             checked before we got here.
     (image_service, image_id) = nova.image.get_image_service(context,
                                                              image_href)
-    try:
+    with utils.remove_path_on_error(path):
         with open(path, "wb") as image_file:
             metadata = image_service.get(context, image_id, image_file)
-    except Exception:
-        with utils.save_and_reraise_exception():
-            try:
-                os.unlink(path)
-            except OSError, e:
-                if e.errno != errno.ENOENT:
-                    LOG.warn("unable to remove stale image '%s': %s" %
-                             (path, e.strerror))
-    return metadata
+            return metadata
 
 
 def fetch_to_raw(context, image_href, path, user_id, project_id):
@@ -85,37 +77,36 @@ def fetch_to_raw(context, image_href, path, user_id, project_id):
 
         return(data)
 
-    data = _qemu_img_info(path_tmp)
-
-    fmt = data.get("file format")
-    if fmt is None:
-        os.unlink(path_tmp)
-        raise exception.ImageUnacceptable(
-            reason=_("'qemu-img info' parsing failed."), image_id=image_href)
-
-    if "backing file" in data:
-        backing_file = data['backing file']
-        os.unlink(path_tmp)
-        raise exception.ImageUnacceptable(image_id=image_href,
-            reason=_("fmt=%(fmt)s backed by: %(backing_file)s") % locals())
-
-    if fmt != "raw" and FLAGS.force_raw_images:
-        staged = "%s.converted" % path
-        LOG.debug("%s was %s, converting to raw" % (image_href, fmt))
-        out, err = utils.execute('qemu-img', 'convert', '-O', 'raw',
-                                 path_tmp, staged)
-        os.unlink(path_tmp)
-
-        data = _qemu_img_info(staged)
-        if data.get('file format', None) != "raw":
-            os.unlink(staged)
+    with utils.remove_path_on_error(path_tmp):
+        data = _qemu_img_info(path_tmp)
+
+        fmt = data.get("file format")
+        if fmt is None:
+            raise exception.ImageUnacceptable(
+                reason=_("'qemu-img info' parsing failed."),
+                image_id=image_href)
+
+        if "backing file" in data:
+            backing_file = data['backing file']
             raise exception.ImageUnacceptable(image_id=image_href,
-                reason=_("Converted to raw, but format is now %s") %
-                data.get('file format', None))
+                reason=_("fmt=%(fmt)s backed by: %(backing_file)s") % locals())
+
+        if fmt != "raw" and FLAGS.force_raw_images:
+            staged = "%s.converted" % path
+            LOG.debug("%s was %s, converting to raw" % (image_href, fmt))
+            with utils.remove_path_on_error(staged):
+                out, err = utils.execute('qemu-img', 'convert', '-O', 'raw',
+                                         path_tmp, staged)
+
+                data = _qemu_img_info(staged)
+                if data.get('file format', None) != "raw":
+                    raise exception.ImageUnacceptable(image_id=image_href,
+                        reason=_("Converted to raw, but format is now %s") %
+                        data.get('file format', None))
 
-        os.rename(staged, path)
+                os.rename(staged, path)
 
-    else:
-        os.rename(path_tmp, path)
+        else:
+            os.rename(path_tmp, path)
 
     return metadata
diff --git a/nova/virt/libvirt/connection.py b/nova/virt/libvirt/connection.py
index d41f4551c..97deae048 100644
--- a/nova/virt/libvirt/connection.py
+++ b/nova/virt/libvirt/connection.py
@@ -1047,7 +1047,8 @@ class LibvirtConnection(driver.ComputeDriver):
             @utils.synchronized(fname)
             def call_if_not_exists(base, fn, *args, **kwargs):
                 if not os.path.exists(base):
-                    fn(target=base, *args, **kwargs)
+                    with utils.remove_path_on_error(base):
+                        fn(target=base, *args, **kwargs)
 
             if cow or not generating:
                 call_if_not_exists(base, fn, *args, **kwargs)
@@ -1063,8 +1064,9 @@ class LibvirtConnection(driver.ComputeDriver):
                         size_gb = size / (1024 * 1024 * 1024)
                         cow_base += "_%d" % size_gb
                         if not os.path.exists(cow_base):
-                            libvirt_utils.copy_image(base, cow_base)
-                            disk.extend(cow_base, size)
+                            with utils.remove_path_on_error(cow_base):
+                                libvirt_utils.copy_image(base, cow_base)
+                                disk.extend(cow_base, size)
                     libvirt_utils.create_cow_image(cow_base, target)
                 elif not generating:
                     libvirt_utils.copy_image(base, target)
@@ -1074,7 +1076,8 @@ class LibvirtConnection(driver.ComputeDriver):
                     if size:
                         disk.extend(target, size)
 
-            copy_and_extend(cow, generating, base, target, size)
+            with utils.remove_path_on_error(target):
+                copy_and_extend(cow, generating, base, target, size)
 
     @staticmethod
     def _create_local(target, local_size, unit='G',
@@ -1234,8 +1237,9 @@ class LibvirtConnection(driver.ComputeDriver):
                               project_id=instance['project_id'],)
         elif config_drive:
             label = 'config'
-            self._create_local(basepath('disk.config'), 64, unit='M',
-                               fs_format='msdos', label=label)  # 64MB
+            with utils.remove_path_on_error(basepath('disk.config')):
+                self._create_local(basepath('disk.config'), 64, unit='M',
+                                   fs_format='msdos', label=label)  # 64MB
 
         if instance['key_data']:
             key = str(instance['key_data'])