From de5bb0c3fe9e86166e81aa2465a8e7791f64ba39 Mon Sep 17 00:00:00 2001
From: Matthew Booth <mbooth@redhat.com>
Date: Wed, 7 Dec 2016 14:45:40 +0000
Subject: [PATCH] libvirt: Fix initialising of LVM ephemeral disks

The LVM backend expects to write directly to the target disk rather
than to the image cache when initialising an ephemeral disk. This is
confounded by Image.cache(), which doesn't call the given callback
(_create_ephemeral in this case), if the target already exists.

Closes-Bug: #1648109
Co-authored-by: Feodor Tersin <ftersin@hotmail.com>
Change-Id: Ic9178d6c013670611e63d54a7669f35dc9770e91
---
 .../unit/virt/libvirt/test_imagebackend.py    | 47 +++++++++++++++++++
 nova/virt/libvirt/imagebackend.py             | 23 ++++++---
 2 files changed, 63 insertions(+), 7 deletions(-)

diff --git a/nova/tests/unit/virt/libvirt/test_imagebackend.py b/nova/tests/unit/virt/libvirt/test_imagebackend.py
index 55b4ff4c90ad..56619c440e4d 100644
--- a/nova/tests/unit/virt/libvirt/test_imagebackend.py
+++ b/nova/tests/unit/virt/libvirt/test_imagebackend.py
@@ -732,6 +732,53 @@ class LvmTestCase(_ImageTestCase, test.NoDBTestCase):
 
         self.mox.VerifyAll()
 
+    @mock.patch('os.path.exists', autospec=True)
+    @mock.patch('nova.utils.synchronized', autospec=True)
+    @mock.patch.object(imagebackend, 'lvm', autospec=True)
+    def test_cache_ephemeral(self, mock_lvm, mock_synchronized, mock_exists):
+        # Ignores its arguments and returns the wrapped function unmodified
+        def fake_synchronized(*args, **kwargs):
+            def outer(fn):
+                def wrapper(*wargs, **wkwargs):
+                    fn(*wargs, **wkwargs)
+                return wrapper
+
+            return outer
+
+        mock_synchronized.side_effect = fake_synchronized
+
+        # Fake exists returns true for paths which have been added to the
+        # exists set
+        exists = set()
+
+        def fake_exists(path):
+            return path in exists
+
+        mock_exists.side_effect = fake_exists
+
+        # Fake create_volume causes exists to return true for the volume
+        def fake_create_volume(vg, lv, size, sparse=False):
+            exists.add(os.path.join('/dev', vg, lv))
+
+        mock_lvm.create_volume.side_effect = fake_create_volume
+
+        # Assert that when we call cache() for an ephemeral disk with the
+        # Lvm backend, we call fetch_func with a target of the Lvm disk
+        size_gb = 1
+        size = size_gb * units.Gi
+
+        fetch_func = mock.MagicMock()
+        image = self.image_class(self.INSTANCE, self.NAME)
+        image.cache(fetch_func, self.TEMPLATE,
+                    ephemeral_size=size_gb, size=size)
+
+        mock_lvm.create_volume.assert_called_once_with(self.VG, self.LV, size,
+                                                       sparse=False)
+        fetch_func.assert_called_once_with(target=self.PATH,
+                                           ephemeral_size=size_gb)
+
+        mock_synchronized.assert_called()
+
     def test_create_image(self):
         self._create_image(False)
 
diff --git a/nova/virt/libvirt/imagebackend.py b/nova/virt/libvirt/imagebackend.py
index a3ad4394c4ab..144a000d8d25 100644
--- a/nova/virt/libvirt/imagebackend.py
+++ b/nova/virt/libvirt/imagebackend.py
@@ -200,19 +200,28 @@ class Image(object):
         :filename: Name of the file in the image directory
         :size: Size of created image in bytes (optional)
         """
-        @utils.synchronized(filename, external=True, lock_path=self.lock_path)
-        def fetch_func_sync(target, *args, **kwargs):
-            # The image may have been fetched while a subsequent
-            # call was waiting to obtain the lock.
-            if not os.path.exists(target):
-                fetch_func(target=target, *args, **kwargs)
-
         base_dir = os.path.join(CONF.instances_path,
                                 CONF.image_cache_subdirectory_name)
         if not os.path.exists(base_dir):
             fileutils.ensure_tree(base_dir)
         base = os.path.join(base_dir, filename)
 
+        @utils.synchronized(filename, external=True, lock_path=self.lock_path)
+        def fetch_func_sync(target, *args, **kwargs):
+            # NOTE(mdbooth): This method is called as a callback by the
+            # create_image() method of a specific backend. It assumes that
+            # target will be in the image cache, which is why it holds a
+            # lock, and does not overwrite an existing file. However,
+            # this is not true for all backends. Specifically Lvm writes
+            # directly to the target rather than to the image cache,
+            # and additionally it creates the target in advance.
+            # This guard is only relevant in the context of the lock if the
+            # target is in the image cache. If it isn't, we should
+            # call fetch_func. The lock we're holding is also unnecessary in
+            # that case, but it will not result in incorrect behaviour.
+            if target != base or not os.path.exists(target):
+                fetch_func(target=target, *args, **kwargs)
+
         if not self.exists() or not os.path.exists(base):
             self.create_image(fetch_func_sync, base, size,
                               *args, **kwargs)