From 8c93d7df8358b75b6cb948e1f1815f7604d694db Mon Sep 17 00:00:00 2001 From: David Hill Date: Fri, 7 Jul 2023 09:16:39 -0400 Subject: [PATCH] Cleanup if images.fetch fails Cleanup if images.fetch fails as in some cases, we might get a stale .part file that is incomplete and corrupted (ie: full disk due to image conversion) and this prevents future deployment from working. Co-Authored-By: Julia Kreger Change-Id: I53bfd0dcc6289e51316795fbe352c70d608e4f31 --- ironic/drivers/modules/image_cache.py | 3 +++ .../unit/drivers/modules/test_image_cache.py | 27 ++++++++++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/ironic/drivers/modules/image_cache.py b/ironic/drivers/modules/image_cache.py index 5ca053f361..08383898c0 100644 --- a/ironic/drivers/modules/image_cache.py +++ b/ironic/drivers/modules/image_cache.py @@ -336,6 +336,9 @@ def _free_disk_space_for(path): def _fetch(context, image_href, path, force_raw=False): """Fetch image and convert to raw format if needed.""" path_tmp = "%s.part" % path + if os.path.exists(path_tmp): + LOG.warning("%s exist, assuming it's stale", path_tmp) + os.remove(path_tmp) images.fetch(context, image_href, path_tmp, force_raw=False) # Notes(yjiang5): If glance can provide the virtual size information, # then we can firstly clean cache and then invoke images.fetch(). diff --git a/ironic/tests/unit/drivers/modules/test_image_cache.py b/ironic/tests/unit/drivers/modules/test_image_cache.py index ffe6bed09a..a05075bec2 100644 --- a/ironic/tests/unit/drivers/modules/test_image_cache.py +++ b/ironic/tests/unit/drivers/modules/test_image_cache.py @@ -754,6 +754,7 @@ class CleanupImageCacheTestCase(base.TestCase): class TestFetchCleanup(base.TestCase): + @mock.patch.object(os, 'remove', autospec=True) @mock.patch.object(images, 'converted_size', autospec=True) @mock.patch.object(images, 'fetch', autospec=True) @mock.patch.object(images, 'image_to_raw', autospec=True) @@ -762,7 +763,7 @@ class TestFetchCleanup(base.TestCase): @mock.patch.object(image_cache, '_clean_up_caches', autospec=True) def test__fetch( self, mock_clean, mock_will_convert, mock_raw, mock_fetch, - mock_size): + mock_size, mock_remove): mock_size.return_value = 100 image_cache._fetch('fake', 'fake-uuid', '/foo/bar', force_raw=True) mock_fetch.assert_called_once_with('fake', 'fake-uuid', @@ -771,6 +772,30 @@ class TestFetchCleanup(base.TestCase): mock_raw.assert_called_once_with('fake-uuid', '/foo/bar', '/foo/bar.part') mock_will_convert.assert_called_once_with('fake-uuid', '/foo/bar.part') + mock_remove.assert_not_called() + + @mock.patch.object(os, 'remove', autospec=True) + @mock.patch.object(os.path, 'exists', autospec=True) + @mock.patch.object(images, 'converted_size', autospec=True) + @mock.patch.object(images, 'fetch', autospec=True) + @mock.patch.object(images, 'image_to_raw', autospec=True) + @mock.patch.object(images, 'force_raw_will_convert', autospec=True, + return_value=True) + @mock.patch.object(image_cache, '_clean_up_caches', autospec=True) + def test__fetch_part_already_exists( + self, mock_clean, mock_will_convert, mock_raw, mock_fetch, + mock_size, mock_exists, mock_remove): + mock_exists.return_value = True + mock_size.return_value = 100 + image_cache._fetch('fake', 'fake-uuid', '/foo/bar', force_raw=True) + mock_fetch.assert_called_once_with('fake', 'fake-uuid', + '/foo/bar.part', force_raw=False) + mock_clean.assert_called_once_with('/foo', 100) + mock_raw.assert_called_once_with('fake-uuid', '/foo/bar', + '/foo/bar.part') + mock_will_convert.assert_called_once_with('fake-uuid', '/foo/bar.part') + self.assertEqual(1, mock_exists.call_count) + self.assertEqual(1, mock_remove.call_count) @mock.patch.object(images, 'converted_size', autospec=True) @mock.patch.object(images, 'fetch', autospec=True)