From a85753778f710f03616a682d294f8f638dea6baf Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Fri, 19 Jun 2020 11:18:04 +0200 Subject: [PATCH] Guard against missing image cache directory The original fix of bug 1878024 missed an edge case where on a fresh hypervisor the image cache directory hasn't been created yet. That directory is only created when the first image is downloaded. This patch makes sure that if the cache dir hasn't been created yet then 0 disk is reserved for the cache usage instead of raising and logging an exception. Change-Id: Id1bbc955a9099de1abc11b9063fe177896646d03 Related-Bug: #1878024 Closes-Bug: #1884214 --- .../unit/virt/libvirt/test_imagecache.py | 65 ++++++++++++------- nova/virt/libvirt/imagecache.py | 40 +++++++----- 2 files changed, 66 insertions(+), 39 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/test_imagecache.py b/nova/tests/unit/virt/libvirt/test_imagecache.py index 4b7ff59f2a17..8c6a31748375 100644 --- a/nova/tests/unit/virt/libvirt/test_imagecache.py +++ b/nova/tests/unit/virt/libvirt/test_imagecache.py @@ -674,12 +674,12 @@ class ImageCacheManagerTestCase(test.NoDBTestCase): lock_path=lock_path) @mock.patch('os.stat') - def test_cache_dir_is_on_same_dev_as_instances_dir(self, mock_stat): + def test_get_disk_usage_cache_is_on_different_disk(self, mock_stat): mock_stat.side_effect = [mock.Mock(st_dev=0), mock.Mock(st_dev=1)] - manager = imagecache.ImageCacheManager() - self.assertFalse(manager.cache_dir_is_on_same_dev_as_instances_dir) + self.assertEqual(0, manager.get_disk_usage()) + mock_stat.assert_has_calls([ mock.call(CONF.instances_path), mock.call( @@ -687,37 +687,34 @@ class ImageCacheManagerTestCase(test.NoDBTestCase): CONF.instances_path, CONF.image_cache.subdirectory_name))]) - mock_stat.reset_mock() - mock_stat.side_effect = [mock.Mock(st_dev=0), mock.Mock(st_dev=0)] - self.assertTrue(manager.cache_dir_is_on_same_dev_as_instances_dir) - - @mock.patch('nova.virt.libvirt.imagecache.ImageCacheManager.' - 'cache_dir_is_on_same_dev_as_instances_dir', - new=mock.PropertyMock(return_value=False)) - def test_get_disk_usage_cache_is_on_different_disk(self): - manager = imagecache.ImageCacheManager() - - self.assertEqual(0, manager.get_disk_usage()) - @mock.patch('os.listdir') - @mock.patch('nova.virt.libvirt.imagecache.ImageCacheManager.' - 'cache_dir_is_on_same_dev_as_instances_dir', - new=mock.PropertyMock(return_value=True)) - def test_get_disk_usage_empty(self, mock_listdir): + @mock.patch('os.stat') + def test_get_disk_usage_empty(self, mock_stat, mock_listdir): + mock_stat.side_effect = [mock.Mock(st_dev=0), mock.Mock(st_dev=0)] mock_listdir.return_value = [] manager = imagecache.ImageCacheManager() self.assertEqual(0, manager.get_disk_usage()) + @mock.patch('os.stat') + @mock.patch('os.listdir') + def test_get_disk_usage_cache_dir_missing(self, mock_listdir, mock_stat): + mock_stat.side_effect = [mock.Mock(st_dev=0), FileNotFoundError] + + manager = imagecache.ImageCacheManager() + + self.assertEqual(0, manager.get_disk_usage()) + mock_listdir.assert_not_called() + @mock.patch('os.path.isfile') @mock.patch('os.listdir') @mock.patch('os.stat') - @mock.patch('nova.virt.libvirt.imagecache.ImageCacheManager.' - 'cache_dir_is_on_same_dev_as_instances_dir', - new=mock.PropertyMock(return_value=True)) def test_get_disk_usage(self, mock_stat, mock_listdir, mock_isfile): mock_stat.side_effect = [ + # cache and instances dirs are on the same dev + mock.Mock(st_dev=0), + mock.Mock(st_dev=0), # stat calls on each file in the cache directory to get the size mock.Mock(st_blocks=10), # foo mock.Mock(st_blocks=20), # bar @@ -733,3 +730,27 @@ class ImageCacheManagerTestCase(test.NoDBTestCase): # size is calculated from as st_blocks * 512 self.assertEqual(10 * 512 + 20 * 512, manager.get_disk_usage()) + + @mock.patch('os.path.isfile') + @mock.patch('os.listdir') + @mock.patch('os.stat') + def test_get_disk_usage_io_error_while_reading( + self, mock_stat, mock_listdir, mock_isfile): + mock_stat.side_effect = [ + # cache and instances dirs are on the same dev + mock.Mock(st_dev=0), + mock.Mock(st_dev=0), + # stat calls on each file in the cache directory to get the size + mock.Mock(st_blocks=10), # foo + IOError, # error while checking bar + ] + mock_listdir.return_value = ['foo', 'bar', 'some-dir'] + mock_isfile.side_effect = [ + True, # foo + True, # bar + False, # some-dir + ] + + manager = imagecache.ImageCacheManager() + + self.assertEqual(0, manager.get_disk_usage()) diff --git a/nova/virt/libvirt/imagecache.py b/nova/virt/libvirt/imagecache.py index 780afa91a39a..179c5d70756f 100644 --- a/nova/virt/libvirt/imagecache.py +++ b/nova/virt/libvirt/imagecache.py @@ -356,26 +356,32 @@ class ImageCacheManager(imagecache.ImageCacheManager): self._age_and_verify_swap_images(context, base_dir) def get_disk_usage(self): - if not self.cache_dir_is_on_same_dev_as_instances_dir: - return 0 + try: + # If the cache is on a different device than the instance dir then + # it does not consume the same disk resource as instances. + # NOTE(gibi): this does not work on Windows properly as st_dev is + # always 0 + if (os.stat(CONF.instances_path).st_dev != + os.stat(self.cache_dir).st_dev): + return 0 - # NOTE(gibi): we need to use the disk size occupied from the file - # system as images in the cache will not grow to their virtual size. - # NOTE(gibi): st.blocks is always measured in 512 byte blocks see man - # fstat - return sum( - os.stat(os.path.join(self.cache_dir, f)).st_blocks * 512 - for f in os.listdir(self.cache_dir) - if os.path.isfile(os.path.join(self.cache_dir, f))) + # NOTE(gibi): we need to use the disk size occupied from the file + # system as images in the cache will not grow to their virtual + # size. + # NOTE(gibi): st.blocks is always measured in 512 byte blocks see + # man fstat + return sum( + os.stat(os.path.join(self.cache_dir, f)).st_blocks * 512 + for f in os.listdir(self.cache_dir) + if os.path.isfile(os.path.join(self.cache_dir, f))) + except OSError: + # NOTE(gibi): An error here can mean many things. E.g. the cache + # dir does not exists yet, the cache dir is deleted between the + # listdir() and the stat() calls, or a file is deleted between + # the listdir() and the stat() calls. + return 0 @property def cache_dir(self): return os.path.join( CONF.instances_path, CONF.image_cache.subdirectory_name) - - @property - def cache_dir_is_on_same_dev_as_instances_dir(self): - # NOTE(gibi): this does not work on Windows properly as st_dev is - # always 0 - return (os.stat(CONF.instances_path).st_dev == - os.stat(self.cache_dir).st_dev)