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

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)
 

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)