From 6a3eabcd01981c6ccead47e2b610bd82b5d6be80 Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?P=C3=A1draig=20Brady?= <pbrady@redhat.com>

Date: Fri, 16 Mar 2012 03:43:49 +0000

Subject: [PATCH] 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

---

 nova/utils.py                   |   21 +++++++++--

 nova/virt/images.py             |   69 +++++++++++++++++----------------------

 nova/virt/libvirt/connection.py |   16 ++++++---

 3 files changed, 57 insertions(+), 49 deletions(-)

diff --git a/nova/utils.py b/nova/utils.py

index 819929a..7188d98 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

@@ -1013,8 +1014,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

@@ -1033,8 +1034,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

@@ -1344,6 +1345,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 1e0ae0a..626f3ff 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 31e6511..dc16d05 100644

--- a/nova/virt/libvirt/connection.py

+++ b/nova/virt/libvirt/connection.py

@@ -1105,7 +1105,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)

@@ -1121,8 +1122,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)

@@ -1132,7 +1134,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',

@@ -1305,8 +1308,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 FLAGS.libvirt_inject_key and instance['key_data']:

             key = str(instance['key_data'])