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)