From f26823c7805c96d155de6837a08ec34271117515 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. Conflicts: nova/virt/libvirt/imagecache.py due to I3c49825ac0d70152b6c8ee4c8ca01546265f4b80 not in stable/train FileNotFoundError -> OSError changes in the test file is due to py2.7 does not have FileNotFoundError. Change-Id: Id1bbc955a9099de1abc11b9063fe177896646d03 Related-Bug: #1878024 Closes-Bug: #1884214 (cherry picked from commit a85753778f710f03616a682d294f8f638dea6baf) (cherry picked from commit a6a48e876c9c4f9a218de882270ef098d46bf767) --- .../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 58682613d9cc..89fbccf1f41d 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), OSError] + + 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 + OSError, # 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 c05e3ff00063..38f46aac1bb5 100644 --- a/nova/virt/libvirt/imagecache.py +++ b/nova/virt/libvirt/imagecache.py @@ -355,26 +355,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)