openstack/ironic
Code Issues Proposed changes

ImageCache: respect Cache-Control: no-store

Browse Source 
Images can be generated on fly, so let us respect the decision of the
server to disable caching.

Also disable caching for file images: it's just waste of space.

Change-Id: I425b1915c73354af90329d5b3a1fb0a616adaa93
This commit is contained in:
Dmitry Tantsur 2021-12-20 15:44:00 +01:00
parent 2e2d6b8eeb
commit 7043a414a8
5 changed files with 147 additions and 121 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
ironic/common/image_service.py
View File

@ -183,10 +183,13 @@ class HttpImageService(BaseImageService):
except ValueError: except ValueError:
continue continue
no_cache = 'no-store' in response.headers.get('Cache-Control', '')
return { return {
'size': int(image_size), 'size': int(image_size),
'updated_at': date, 'updated_at': date,
'properties': {} 'properties': {},
'no_cache': no_cache,
} }
@ -259,7 +262,9 @@ class FileImageService(BaseImageService):
'size': os.path.getsize(source_image_path), 'size': os.path.getsize(source_image_path),
'updated_at': utils.unix_file_modification_datetime( 'updated_at': utils.unix_file_modification_datetime(
source_image_path), source_image_path),
'properties': {} 'properties': {},
# No point in caching local file images
'no_cache': True,
} }

33
ironic/drivers/modules/image_cache.py
View File

@ -112,11 +112,13 @@ class ImageCache(object):
# TODO(dtantsur): lock expiration time # TODO(dtantsur): lock expiration time
with lockutils.lock(img_download_lock_name): with lockutils.lock(img_download_lock_name):
img_service = image_service.get_image_service(href, context=ctx)
img_info = img_service.show(href)
# NOTE(vdrok): After rebuild requested image can change, so we # NOTE(vdrok): After rebuild requested image can change, so we
# should ensure that dest_path and master_path (if exists) are # should ensure that dest_path and master_path (if exists) are
# pointing to the same file and their content is up to date # pointing to the same file and their content is up to date
cache_up_to_date = _delete_master_path_if_stale(master_path, href, cache_up_to_date = _delete_master_path_if_stale(master_path, href,
ctx) img_info)
dest_up_to_date = _delete_dest_path_if_stale(master_path, dest_up_to_date = _delete_dest_path_if_stale(master_path,
dest_path) dest_path)
@ -137,13 +139,14 @@ class ImageCache(object):
LOG.info("Master cache miss for image %(href)s, will download", LOG.info("Master cache miss for image %(href)s, will download",
{'href': href}) {'href': href})
self._download_image( self._download_image(
href, master_path, dest_path, ctx=ctx, force_raw=force_raw) href, master_path, dest_path, img_info,
ctx=ctx, force_raw=force_raw)
# NOTE(dtantsur): we increased cache size - time to clean up # NOTE(dtantsur): we increased cache size - time to clean up
self.clean_up() self.clean_up()
def _download_image(self, href, master_path, dest_path, ctx=None, def _download_image(self, href, master_path, dest_path, img_info,
force_raw=True): ctx=None, force_raw=True):
"""Download image by href and store at a given path. """Download image by href and store at a given path.
This method should be called with uuid-specific lock taken. This method should be called with uuid-specific lock taken.
@ -151,6 +154,7 @@ class ImageCache(object):
:param href: image UUID or href to fetch :param href: image UUID or href to fetch
:param master_path: destination master path :param master_path: destination master path
:param dest_path: destination file path :param dest_path: destination file path
:param img_info: image information from the image service
:param ctx: context :param ctx: context
:param force_raw: boolean value, whether to convert the image to raw :param force_raw: boolean value, whether to convert the image to raw
format format
@ -166,10 +170,16 @@ class ImageCache(object):
try: try:
with _concurrency_semaphore: with _concurrency_semaphore:
_fetch(ctx, href, tmp_path, force_raw) _fetch(ctx, href, tmp_path, force_raw)
# NOTE(dtantsur): no need for global lock here - master_path
# will have link count >1 at any moment, so won't be cleaned up if img_info.get('no_cache'):
os.link(tmp_path, master_path) LOG.debug("Caching is disabled for image %s", href)
os.link(master_path, dest_path) # Cache disabled, link directly to destination
os.link(tmp_path, dest_path)
else:
# NOTE(dtantsur): no need for global lock here - master_path
# will have link count >1 at any moment, so won't be cleaned up
os.link(tmp_path, master_path)
os.link(master_path, dest_path)
except OSError as exc: except OSError as exc:
msg = (_("Could not link image %(img_href)s from %(src_path)s " msg = (_("Could not link image %(img_href)s from %(src_path)s "
"to %(dst_path)s, error: %(exc)s") % "to %(dst_path)s, error: %(exc)s") %
@ -410,12 +420,12 @@ def cleanup(priority):
return _add_property_to_class_func return _add_property_to_class_func
def _delete_master_path_if_stale(master_path, href, ctx): def _delete_master_path_if_stale(master_path, href, img_info):
"""Delete image from cache if it is not up to date with href contents. """Delete image from cache if it is not up to date with href contents.
:param master_path: path to an image in master cache :param master_path: path to an image in master cache
:param href: image href :param href: image href
:param ctx: context to use :param img_info: image information from the service
:returns: True if master_path is up to date with href contents, :returns: True if master_path is up to date with href contents,
False if master_path was stale and was deleted or it didn't exist False if master_path was stale and was deleted or it didn't exist
""" """
@ -423,8 +433,7 @@ def _delete_master_path_if_stale(master_path, href, ctx):
# Glance image contents cannot be updated without changing image's UUID # Glance image contents cannot be updated without changing image's UUID
return os.path.exists(master_path) return os.path.exists(master_path)
if os.path.exists(master_path): if os.path.exists(master_path):
img_service = image_service.get_image_service(href, context=ctx) img_mtime = img_info.get('updated_at')
img_mtime = img_service.show(href).get('updated_at')
if not img_mtime: if not img_mtime:
# This means that href is not a glance image and doesn't have an # This means that href is not a glance image and doesn't have an
# updated_at attribute. To play on the safe side, redownload the # updated_at attribute. To play on the safe side, redownload the

32
ironic/tests/unit/common/test_image_service.py
View File

@ -205,7 +205,7 @@ class HttpImageServiceTestCase(base.TestCase):
head_mock.assert_called_once_with(self.href, verify=True, head_mock.assert_called_once_with(self.href, verify=True,
timeout=60) timeout=60)
self.assertEqual({'size': 100, 'updated_at': mtime_date, self.assertEqual({'size': 100, 'updated_at': mtime_date,
'properties': {}}, result) 'properties': {}, 'no_cache': False}, result)
def test_show_rfc_822(self): def test_show_rfc_822(self):
self._test_show(mtime='Tue, 15 Nov 2014 08:12:31 GMT', self._test_show(mtime='Tue, 15 Nov 2014 08:12:31 GMT',
@ -219,6 +219,33 @@ class HttpImageServiceTestCase(base.TestCase):
self._test_show(mtime='Tue Nov 15 08:12:31 2014', self._test_show(mtime='Tue Nov 15 08:12:31 2014',
mtime_date=datetime.datetime(2014, 11, 15, 8, 12, 31)) mtime_date=datetime.datetime(2014, 11, 15, 8, 12, 31))
@mock.patch.object(requests, 'head', autospec=True)
def _test_show_with_cache(self, head_mock, cache_control, no_cache):
head_mock.return_value.status_code = http_client.OK
head_mock.return_value.headers = {
'Content-Length': 100,
'Last-Modified': 'Tue, 15 Nov 2014 08:12:31 GMT',
'Cache-Control': cache_control,
}
result = self.service.show(self.href)
head_mock.assert_called_once_with(self.href, verify=True,
timeout=60)
self.assertEqual({
'size': 100,
'updated_at': datetime.datetime(2014, 11, 15, 8, 12, 31),
'properties': {},
'no_cache': no_cache}, result)
def test_show_cache_allowed(self):
self._test_show_with_cache(
# Just because we cannot have nice things, "no-cache" actually
# means "cache, but always re-validate".
cache_control='no-cache, private', no_cache=False)
def test_show_cache_disabled(self):
self._test_show_with_cache(
cache_control='no-store', no_cache=True)
@mock.patch.object(requests, 'head', autospec=True) @mock.patch.object(requests, 'head', autospec=True)
def test_show_no_content_length(self, head_mock): def test_show_no_content_length(self, head_mock):
head_mock.return_value.status_code = http_client.OK head_mock.return_value.status_code = http_client.OK
@ -425,7 +452,8 @@ class FileImageServiceTestCase(base.TestCase):
self.assertEqual({'size': 42, self.assertEqual({'size': 42,
'updated_at': datetime.datetime(2015, 5, 8, 'updated_at': datetime.datetime(2015, 5, 8,
12, 25, 9, 164191), 12, 25, 9, 164191),
'properties': {}}, result) 'properties': {},
'no_cache': True}, result)
@mock.patch.object(os, 'link', autospec=True) @mock.patch.object(os, 'link', autospec=True)
@mock.patch.object(os, 'remove', autospec=True) @mock.patch.object(os, 'remove', autospec=True)

186
ironic/tests/unit/drivers/modules/test_image_cache.py
View File

@ -37,10 +37,10 @@ def touch(filename):
open(filename, 'w').close() open(filename, 'w').close()
class TestImageCacheFetch(base.TestCase): class BaseTest(base.TestCase):
def setUp(self): def setUp(self):
super(TestImageCacheFetch, self).setUp() super().setUp()
self.master_dir = tempfile.mkdtemp() self.master_dir = tempfile.mkdtemp()
self.cache = image_cache.ImageCache(self.master_dir, None, None) self.cache = image_cache.ImageCache(self.master_dir, None, None)
self.dest_dir = tempfile.mkdtemp() self.dest_dir = tempfile.mkdtemp()
@ -48,28 +48,31 @@ class TestImageCacheFetch(base.TestCase):
self.uuid = uuidutils.generate_uuid() self.uuid = uuidutils.generate_uuid()
self.master_path = ''.join([os.path.join(self.master_dir, self.uuid), self.master_path = ''.join([os.path.join(self.master_dir, self.uuid),
'.converted']) '.converted'])
self.img_info = {}
@mock.patch.object(image_service, 'get_image_service', autospec=True)
@mock.patch.object(image_cache.ImageCache, 'clean_up', autospec=True)
@mock.patch.object(image_cache.ImageCache, '_download_image', autospec=True)
class TestImageCacheFetch(BaseTest):
@mock.patch.object(image_cache, '_fetch', autospec=True) @mock.patch.object(image_cache, '_fetch', autospec=True)
@mock.patch.object(image_cache.ImageCache, 'clean_up', autospec=True) def test_fetch_image_no_master_dir(self, mock_fetch, mock_download,
@mock.patch.object(image_cache.ImageCache, '_download_image', mock_clean_up, mock_image_service):
autospec=True)
def test_fetch_image_no_master_dir(self, mock_download, mock_clean_up,
mock_fetch):
self.cache.master_dir = None self.cache.master_dir = None
self.cache.fetch_image(self.uuid, self.dest_path) self.cache.fetch_image(self.uuid, self.dest_path)
self.assertFalse(mock_download.called) self.assertFalse(mock_download.called)
mock_fetch.assert_called_once_with( mock_fetch.assert_called_once_with(
None, self.uuid, self.dest_path, True) None, self.uuid, self.dest_path, True)
self.assertFalse(mock_clean_up.called) self.assertFalse(mock_clean_up.called)
mock_image_service.assert_not_called()
@mock.patch.object(image_cache, '_fetch', autospec=True) @mock.patch.object(image_cache, '_fetch', autospec=True)
@mock.patch.object(image_cache.ImageCache, 'clean_up', autospec=True)
@mock.patch.object(image_cache.ImageCache, '_download_image',
autospec=True)
def test_fetch_image_no_master_dir_memory_low(self, def test_fetch_image_no_master_dir_memory_low(self,
mock_fetch,
mock_download, mock_download,
mock_clean_up, mock_clean_up,
mock_fetch): mock_image_service):
mock_fetch.side_effect = exception.InsufficentMemory mock_fetch.side_effect = exception.InsufficentMemory
self.cache.master_dir = None self.cache.master_dir = None
self.assertRaises(exception.InsufficentMemory, self.assertRaises(exception.InsufficentMemory,
@ -79,10 +82,8 @@ class TestImageCacheFetch(base.TestCase):
mock_fetch.assert_called_once_with( mock_fetch.assert_called_once_with(
None, self.uuid, self.dest_path, True) None, self.uuid, self.dest_path, True)
self.assertFalse(mock_clean_up.called) self.assertFalse(mock_clean_up.called)
mock_image_service.assert_not_called()
@mock.patch.object(image_cache.ImageCache, 'clean_up', autospec=True)
@mock.patch.object(image_cache.ImageCache, '_download_image',
autospec=True)
@mock.patch.object(os, 'link', autospec=True) @mock.patch.object(os, 'link', autospec=True)
@mock.patch.object(image_cache, '_delete_dest_path_if_stale', @mock.patch.object(image_cache, '_delete_dest_path_if_stale',
return_value=True, autospec=True) return_value=True, autospec=True)
@ -90,18 +91,18 @@ class TestImageCacheFetch(base.TestCase):
return_value=True, autospec=True) return_value=True, autospec=True)
def test_fetch_image_dest_and_master_uptodate( def test_fetch_image_dest_and_master_uptodate(
self, mock_cache_upd, mock_dest_upd, mock_link, mock_download, self, mock_cache_upd, mock_dest_upd, mock_link, mock_download,
mock_clean_up): mock_clean_up, mock_image_service):
self.cache.fetch_image(self.uuid, self.dest_path) self.cache.fetch_image(self.uuid, self.dest_path)
mock_cache_upd.assert_called_once_with(self.master_path, self.uuid, mock_cache_upd.assert_called_once_with(
None) self.master_path, self.uuid,
mock_image_service.return_value.show.return_value)
mock_dest_upd.assert_called_once_with(self.master_path, self.dest_path) mock_dest_upd.assert_called_once_with(self.master_path, self.dest_path)
self.assertFalse(mock_link.called) self.assertFalse(mock_link.called)
self.assertFalse(mock_download.called) self.assertFalse(mock_download.called)
self.assertFalse(mock_clean_up.called) self.assertFalse(mock_clean_up.called)
mock_image_service.assert_called_once_with(self.uuid, context=None)
mock_image_service.return_value.show.assert_called_once_with(self.uuid)
@mock.patch.object(image_cache.ImageCache, 'clean_up', autospec=True)
@mock.patch.object(image_cache.ImageCache, '_download_image',
autospec=True)
@mock.patch.object(os, 'link', autospec=True) @mock.patch.object(os, 'link', autospec=True)
@mock.patch.object(image_cache, '_delete_dest_path_if_stale', @mock.patch.object(image_cache, '_delete_dest_path_if_stale',
return_value=True, autospec=True) return_value=True, autospec=True)
@ -109,19 +110,17 @@ class TestImageCacheFetch(base.TestCase):
return_value=True, autospec=True) return_value=True, autospec=True)
def test_fetch_image_dest_and_master_uptodate_no_force_raw( def test_fetch_image_dest_and_master_uptodate_no_force_raw(
self, mock_cache_upd, mock_dest_upd, mock_link, mock_download, self, mock_cache_upd, mock_dest_upd, mock_link, mock_download,
mock_clean_up): mock_clean_up, mock_image_service):
master_path = os.path.join(self.master_dir, self.uuid) master_path = os.path.join(self.master_dir, self.uuid)
self.cache.fetch_image(self.uuid, self.dest_path, force_raw=False) self.cache.fetch_image(self.uuid, self.dest_path, force_raw=False)
mock_cache_upd.assert_called_once_with(master_path, self.uuid, mock_cache_upd.assert_called_once_with(
None) master_path, self.uuid,
mock_image_service.return_value.show.return_value)
mock_dest_upd.assert_called_once_with(master_path, self.dest_path) mock_dest_upd.assert_called_once_with(master_path, self.dest_path)
self.assertFalse(mock_link.called) self.assertFalse(mock_link.called)
self.assertFalse(mock_download.called) self.assertFalse(mock_download.called)
self.assertFalse(mock_clean_up.called) self.assertFalse(mock_clean_up.called)
@mock.patch.object(image_cache.ImageCache, 'clean_up', autospec=True)
@mock.patch.object(image_cache.ImageCache, '_download_image',
autospec=True)
@mock.patch.object(os, 'link', autospec=True) @mock.patch.object(os, 'link', autospec=True)
@mock.patch.object(image_cache, '_delete_dest_path_if_stale', @mock.patch.object(image_cache, '_delete_dest_path_if_stale',
return_value=False, autospec=True) return_value=False, autospec=True)
@ -129,18 +128,16 @@ class TestImageCacheFetch(base.TestCase):
return_value=True, autospec=True) return_value=True, autospec=True)
def test_fetch_image_dest_out_of_date( def test_fetch_image_dest_out_of_date(
self, mock_cache_upd, mock_dest_upd, mock_link, mock_download, self, mock_cache_upd, mock_dest_upd, mock_link, mock_download,
mock_clean_up): mock_clean_up, mock_image_service):
self.cache.fetch_image(self.uuid, self.dest_path) self.cache.fetch_image(self.uuid, self.dest_path)
mock_cache_upd.assert_called_once_with(self.master_path, self.uuid, mock_cache_upd.assert_called_once_with(
None) self.master_path, self.uuid,
mock_image_service.return_value.show.return_value)
mock_dest_upd.assert_called_once_with(self.master_path, self.dest_path) mock_dest_upd.assert_called_once_with(self.master_path, self.dest_path)
mock_link.assert_called_once_with(self.master_path, self.dest_path) mock_link.assert_called_once_with(self.master_path, self.dest_path)
self.assertFalse(mock_download.called) self.assertFalse(mock_download.called)
self.assertFalse(mock_clean_up.called) self.assertFalse(mock_clean_up.called)
@mock.patch.object(image_cache.ImageCache, 'clean_up', autospec=True)
@mock.patch.object(image_cache.ImageCache, '_download_image',
autospec=True)
@mock.patch.object(os, 'link', autospec=True) @mock.patch.object(os, 'link', autospec=True)
@mock.patch.object(image_cache, '_delete_dest_path_if_stale', @mock.patch.object(image_cache, '_delete_dest_path_if_stale',
return_value=True, autospec=True) return_value=True, autospec=True)
@ -148,20 +145,21 @@ class TestImageCacheFetch(base.TestCase):
return_value=False, autospec=True) return_value=False, autospec=True)
def test_fetch_image_master_out_of_date( def test_fetch_image_master_out_of_date(
self, mock_cache_upd, mock_dest_upd, mock_link, mock_download, self, mock_cache_upd, mock_dest_upd, mock_link, mock_download,
mock_clean_up): mock_clean_up, mock_image_service):
self.cache.fetch_image(self.uuid, self.dest_path) self.cache.fetch_image(self.uuid, self.dest_path)
mock_cache_upd.assert_called_once_with(self.master_path, self.uuid, mock_cache_upd.assert_called_once_with(
None) self.master_path, self.uuid,
mock_image_service.return_value.show.return_value)
mock_dest_upd.assert_called_once_with(self.master_path, self.dest_path) mock_dest_upd.assert_called_once_with(self.master_path, self.dest_path)
self.assertFalse(mock_link.called) self.assertFalse(mock_link.called)
mock_download.assert_called_once_with( mock_download.assert_called_once_with(
self.cache, self.uuid, self.master_path, self.dest_path, self.cache, self.uuid, self.master_path, self.dest_path,
mock_image_service.return_value.show.return_value,
ctx=None, force_raw=True) ctx=None, force_raw=True)
mock_clean_up.assert_called_once_with(self.cache) mock_clean_up.assert_called_once_with(self.cache)
mock_image_service.assert_called_once_with(self.uuid, context=None)
mock_image_service.return_value.show.assert_called_once_with(self.uuid)
@mock.patch.object(image_cache.ImageCache, 'clean_up', autospec=True)
@mock.patch.object(image_cache.ImageCache, '_download_image',
autospec=True)
@mock.patch.object(os, 'link', autospec=True) @mock.patch.object(os, 'link', autospec=True)
@mock.patch.object(image_cache, '_delete_dest_path_if_stale', @mock.patch.object(image_cache, '_delete_dest_path_if_stale',
return_value=True, autospec=True) return_value=True, autospec=True)
@ -169,21 +167,21 @@ class TestImageCacheFetch(base.TestCase):
return_value=False, autospec=True) return_value=False, autospec=True)
def test_fetch_image_both_master_and_dest_out_of_date( def test_fetch_image_both_master_and_dest_out_of_date(
self, mock_cache_upd, mock_dest_upd, mock_link, mock_download, self, mock_cache_upd, mock_dest_upd, mock_link, mock_download,
mock_clean_up): mock_clean_up, mock_image_service):
self.cache.fetch_image(self.uuid, self.dest_path) self.cache.fetch_image(self.uuid, self.dest_path)
mock_cache_upd.assert_called_once_with(self.master_path, self.uuid, mock_cache_upd.assert_called_once_with(
None) self.master_path, self.uuid,
mock_image_service.return_value.show.return_value)
mock_dest_upd.assert_called_once_with(self.master_path, self.dest_path) mock_dest_upd.assert_called_once_with(self.master_path, self.dest_path)
self.assertFalse(mock_link.called) self.assertFalse(mock_link.called)
mock_download.assert_called_once_with( mock_download.assert_called_once_with(
self.cache, self.uuid, self.master_path, self.dest_path, self.cache, self.uuid, self.master_path, self.dest_path,
mock_image_service.return_value.show.return_value,
ctx=None, force_raw=True) ctx=None, force_raw=True)
mock_clean_up.assert_called_once_with(self.cache) mock_clean_up.assert_called_once_with(self.cache)
@mock.patch.object(image_cache.ImageCache, 'clean_up', autospec=True) def test_fetch_image_not_uuid(self, mock_download, mock_clean_up,
@mock.patch.object(image_cache.ImageCache, '_download_image', mock_image_service):
autospec=True)
def test_fetch_image_not_uuid(self, mock_download, mock_clean_up):
href = u'http://abc.com/ubuntu.qcow2' href = u'http://abc.com/ubuntu.qcow2'
href_converted = str(uuid.uuid5(uuid.NAMESPACE_URL, href)) href_converted = str(uuid.uuid5(uuid.NAMESPACE_URL, href))
master_path = ''.join([os.path.join(self.master_dir, href_converted), master_path = ''.join([os.path.join(self.master_dir, href_converted),
@ -191,24 +189,27 @@ class TestImageCacheFetch(base.TestCase):
self.cache.fetch_image(href, self.dest_path) self.cache.fetch_image(href, self.dest_path)
mock_download.assert_called_once_with( mock_download.assert_called_once_with(
self.cache, href, master_path, self.dest_path, self.cache, href, master_path, self.dest_path,
mock_image_service.return_value.show.return_value,
ctx=None, force_raw=True) ctx=None, force_raw=True)
self.assertTrue(mock_clean_up.called) self.assertTrue(mock_clean_up.called)
@mock.patch.object(image_cache.ImageCache, 'clean_up', autospec=True)
@mock.patch.object(image_cache.ImageCache, '_download_image',
autospec=True)
def test_fetch_image_not_uuid_no_force_raw(self, mock_download, def test_fetch_image_not_uuid_no_force_raw(self, mock_download,
mock_clean_up): mock_clean_up,
mock_image_service):
href = u'http://abc.com/ubuntu.qcow2' href = u'http://abc.com/ubuntu.qcow2'
href_converted = str(uuid.uuid5(uuid.NAMESPACE_URL, href)) href_converted = str(uuid.uuid5(uuid.NAMESPACE_URL, href))
master_path = os.path.join(self.master_dir, href_converted) master_path = os.path.join(self.master_dir, href_converted)
self.cache.fetch_image(href, self.dest_path, force_raw=False) self.cache.fetch_image(href, self.dest_path, force_raw=False)
mock_download.assert_called_once_with( mock_download.assert_called_once_with(
self.cache, href, master_path, self.dest_path, self.cache, href, master_path, self.dest_path,
mock_image_service.return_value.show.return_value,
ctx=None, force_raw=False) ctx=None, force_raw=False)
self.assertTrue(mock_clean_up.called) self.assertTrue(mock_clean_up.called)
@mock.patch.object(image_cache, '_fetch', autospec=True)
@mock.patch.object(image_cache, '_fetch', autospec=True)
class TestImageCacheDownload(BaseTest):
def test__download_image(self, mock_fetch): def test__download_image(self, mock_fetch):
def _fake_fetch(ctx, uuid, tmp_path, *args): def _fake_fetch(ctx, uuid, tmp_path, *args):
self.assertEqual(self.uuid, uuid) self.assertEqual(self.uuid, uuid)
@ -218,7 +219,8 @@ class TestImageCacheFetch(base.TestCase):
fp.write("TEST") fp.write("TEST")
mock_fetch.side_effect = _fake_fetch mock_fetch.side_effect = _fake_fetch
self.cache._download_image(self.uuid, self.master_path, self.dest_path) self.cache._download_image(self.uuid, self.master_path, self.dest_path,
self.img_info)
self.assertTrue(os.path.isfile(self.dest_path)) self.assertTrue(os.path.isfile(self.dest_path))
self.assertTrue(os.path.isfile(self.master_path)) self.assertTrue(os.path.isfile(self.master_path))
self.assertEqual(os.stat(self.dest_path).st_ino, self.assertEqual(os.stat(self.dest_path).st_ino,
@ -226,7 +228,6 @@ class TestImageCacheFetch(base.TestCase):
with open(self.dest_path) as fp: with open(self.dest_path) as fp:
self.assertEqual("TEST", fp.read()) self.assertEqual("TEST", fp.read())
@mock.patch.object(image_cache, '_fetch', autospec=True)
def test__download_image_large_url(self, mock_fetch): def test__download_image_large_url(self, mock_fetch):
# A long enough URL may exceed the file name limits of the file system. # A long enough URL may exceed the file name limits of the file system.
# Make sure we don't use any parts of the URL anywhere. # Make sure we don't use any parts of the URL anywhere.
@ -240,7 +241,8 @@ class TestImageCacheFetch(base.TestCase):
fp.write("TEST") fp.write("TEST")
mock_fetch.side_effect = _fake_fetch mock_fetch.side_effect = _fake_fetch
self.cache._download_image(url, self.master_path, self.dest_path) self.cache._download_image(url, self.master_path, self.dest_path,
self.img_info)
self.assertTrue(os.path.isfile(self.dest_path)) self.assertTrue(os.path.isfile(self.dest_path))
self.assertTrue(os.path.isfile(self.master_path)) self.assertTrue(os.path.isfile(self.master_path))
self.assertEqual(os.stat(self.dest_path).st_ino, self.assertEqual(os.stat(self.dest_path).st_ino,
@ -248,123 +250,97 @@ class TestImageCacheFetch(base.TestCase):
with open(self.dest_path) as fp: with open(self.dest_path) as fp:
self.assertEqual("TEST", fp.read()) self.assertEqual("TEST", fp.read())
@mock.patch.object(image_cache, '_fetch', autospec=True)
@mock.patch.object(image_cache, 'LOG', autospec=True) @mock.patch.object(image_cache, 'LOG', autospec=True)
@mock.patch.object(os, 'link', autospec=True) @mock.patch.object(os, 'link', autospec=True)
def test__download_image_linkfail(self, mock_link, mock_log, mock_fetch): def test__download_image_linkfail(self, mock_link, mock_log, mock_fetch):
mock_link.side_effect = [None, OSError] mock_link.side_effect = [None, OSError]
self.assertRaises(exception.ImageDownloadFailed, self.assertRaises(exception.ImageDownloadFailed,
self.cache._download_image, self.cache._download_image,
self.uuid, self.master_path, self.dest_path) self.uuid, self.master_path, self.dest_path,
self.img_info)
self.assertTrue(mock_fetch.called) self.assertTrue(mock_fetch.called)
self.assertEqual(2, mock_link.call_count) self.assertEqual(2, mock_link.call_count)
self.assertTrue(mock_log.error.called) self.assertTrue(mock_log.error.called)
@mock.patch.object(image_cache, '_fetch', autospec=True)
def test__download_image_raises_memory_guard(self, mock_fetch): def test__download_image_raises_memory_guard(self, mock_fetch):
mock_fetch.side_effect = exception.InsufficentMemory mock_fetch.side_effect = exception.InsufficentMemory
self.assertRaises(exception.InsufficentMemory, self.assertRaises(exception.InsufficentMemory,
self.cache._download_image, self.cache._download_image,
self.uuid, self.master_path, self.uuid, self.master_path,
self.dest_path) self.dest_path, self.img_info)
@mock.patch.object(os, 'unlink', autospec=True) @mock.patch.object(os, 'unlink', autospec=True)
class TestUpdateImages(base.TestCase): class TestUpdateImages(BaseTest):
def setUp(self):
super(TestUpdateImages, self).setUp()
self.master_dir = tempfile.mkdtemp()
self.dest_dir = tempfile.mkdtemp()
self.dest_path = os.path.join(self.dest_dir, 'dest')
self.uuid = uuidutils.generate_uuid()
self.master_path = os.path.join(self.master_dir, self.uuid)
@mock.patch.object(os.path, 'exists', return_value=False, autospec=True) @mock.patch.object(os.path, 'exists', return_value=False, autospec=True)
@mock.patch.object(image_service, 'get_image_service', autospec=True)
def test__delete_master_path_if_stale_glance_img_not_cached( def test__delete_master_path_if_stale_glance_img_not_cached(
self, mock_gis, mock_path_exists, mock_unlink): self, mock_path_exists, mock_unlink):
res = image_cache._delete_master_path_if_stale(self.master_path, res = image_cache._delete_master_path_if_stale(self.master_path,
self.uuid, None) self.uuid,
self.assertFalse(mock_gis.called) self.img_info)
self.assertFalse(mock_unlink.called) self.assertFalse(mock_unlink.called)
mock_path_exists.assert_called_once_with(self.master_path) mock_path_exists.assert_called_once_with(self.master_path)
self.assertFalse(res) self.assertFalse(res)
@mock.patch.object(os.path, 'exists', return_value=True, autospec=True) @mock.patch.object(os.path, 'exists', return_value=True, autospec=True)
@mock.patch.object(image_service, 'get_image_service', autospec=True)
def test__delete_master_path_if_stale_glance_img( def test__delete_master_path_if_stale_glance_img(
self, mock_gis, mock_path_exists, mock_unlink): self, mock_path_exists, mock_unlink):
res = image_cache._delete_master_path_if_stale(self.master_path, res = image_cache._delete_master_path_if_stale(self.master_path,
self.uuid, None) self.uuid,
self.assertFalse(mock_gis.called) self.img_info)
self.assertFalse(mock_unlink.called) self.assertFalse(mock_unlink.called)
mock_path_exists.assert_called_once_with(self.master_path) mock_path_exists.assert_called_once_with(self.master_path)
self.assertTrue(res) self.assertTrue(res)
@mock.patch.object(image_service, 'get_image_service', autospec=True) def test__delete_master_path_if_stale_no_master(self, mock_unlink):
def test__delete_master_path_if_stale_no_master(self, mock_gis,
mock_unlink):
res = image_cache._delete_master_path_if_stale(self.master_path, res = image_cache._delete_master_path_if_stale(self.master_path,
'http://11', None) 'http://11',
self.assertFalse(mock_gis.called) self.img_info)
self.assertFalse(mock_unlink.called) self.assertFalse(mock_unlink.called)
self.assertFalse(res) self.assertFalse(res)
@mock.patch.object(image_service, 'get_image_service', autospec=True) def test__delete_master_path_if_stale_no_updated_at(self, mock_unlink):
def test__delete_master_path_if_stale_no_updated_at(self, mock_gis,
mock_unlink):
touch(self.master_path) touch(self.master_path)
href = 'http://awesomefreeimages.al/img111' href = 'http://awesomefreeimages.al/img111'
mock_gis.return_value.show.return_value = {}
res = image_cache._delete_master_path_if_stale(self.master_path, href, res = image_cache._delete_master_path_if_stale(self.master_path, href,
None) self.img_info)
mock_gis.assert_called_once_with(href, context=None)
mock_unlink.assert_called_once_with(self.master_path) mock_unlink.assert_called_once_with(self.master_path)
self.assertFalse(res) self.assertFalse(res)
@mock.patch.object(image_service, 'get_image_service', autospec=True) def test__delete_master_path_if_stale_master_up_to_date(self, mock_unlink):
def test__delete_master_path_if_stale_master_up_to_date(self, mock_gis,
mock_unlink):
touch(self.master_path) touch(self.master_path)
href = 'http://awesomefreeimages.al/img999' href = 'http://awesomefreeimages.al/img999'
mock_gis.return_value.show.return_value = { self.img_info = {
'updated_at': datetime.datetime(1999, 11, 15, 8, 12, 31) 'updated_at': datetime.datetime(1999, 11, 15, 8, 12, 31)
} }
res = image_cache._delete_master_path_if_stale(self.master_path, href, res = image_cache._delete_master_path_if_stale(self.master_path, href,
None) self.img_info)
mock_gis.assert_called_once_with(href, context=None)
self.assertFalse(mock_unlink.called) self.assertFalse(mock_unlink.called)
self.assertTrue(res) self.assertTrue(res)
@mock.patch.object(image_service, 'get_image_service', autospec=True) def test__delete_master_path_if_stale_master_same_time(self, mock_unlink):
def test__delete_master_path_if_stale_master_same_time(self, mock_gis,
mock_unlink):
# When times identical should not delete cached file # When times identical should not delete cached file
touch(self.master_path) touch(self.master_path)
mtime = utils.unix_file_modification_datetime(self.master_path) mtime = utils.unix_file_modification_datetime(self.master_path)
href = 'http://awesomefreeimages.al/img999' href = 'http://awesomefreeimages.al/img999'
mock_gis.return_value.show.return_value = { self.img_info = {
'updated_at': mtime 'updated_at': mtime
} }
res = image_cache._delete_master_path_if_stale(self.master_path, href, res = image_cache._delete_master_path_if_stale(self.master_path, href,
None) self.img_info)
mock_gis.assert_called_once_with(href, context=None)
self.assertFalse(mock_unlink.called) self.assertFalse(mock_unlink.called)
self.assertTrue(res) self.assertTrue(res)
@mock.patch.object(image_service, 'get_image_service', autospec=True) def test__delete_master_path_if_stale_out_of_date(self, mock_unlink):
def test__delete_master_path_if_stale_out_of_date(self, mock_gis,
mock_unlink):
touch(self.master_path) touch(self.master_path)
href = 'http://awesomefreeimages.al/img999' href = 'http://awesomefreeimages.al/img999'
mock_gis.return_value.show.return_value = { self.img_info = {
'updated_at': datetime.datetime((datetime.datetime.utcnow().year 'updated_at': datetime.datetime((datetime.datetime.utcnow().year
+ 1), 11, 15, 8, 12, 31) + 1), 11, 15, 8, 12, 31)
} }
res = image_cache._delete_master_path_if_stale(self.master_path, href, res = image_cache._delete_master_path_if_stale(self.master_path, href,
None) self.img_info)
mock_gis.assert_called_once_with(href, context=None)
mock_unlink.assert_called_once_with(self.master_path) mock_unlink.assert_called_once_with(self.master_path)
self.assertFalse(res) self.assertFalse(res)
@ -552,7 +528,7 @@ class TestImageCacheCleanUp(base.TestCase):
mock_fetch.side_effect = _fake_fetch mock_fetch.side_effect = _fake_fetch
master_path = os.path.join(self.master_dir, 'uuid') master_path = os.path.join(self.master_dir, 'uuid')
dest_path = os.path.join(tempfile.mkdtemp(), 'dest') dest_path = os.path.join(tempfile.mkdtemp(), 'dest')
self.cache._download_image('uuid', master_path, dest_path) self.cache._download_image('uuid', master_path, dest_path, {})
self.assertTrue(mock_rmtree.called) self.assertTrue(mock_rmtree.called)
@mock.patch.object(utils, 'rmtree_without_raise', autospec=True) @mock.patch.object(utils, 'rmtree_without_raise', autospec=True)
@ -561,7 +537,7 @@ class TestImageCacheCleanUp(base.TestCase):
mock_fetch.side_effect = exception.IronicException mock_fetch.side_effect = exception.IronicException
self.assertRaises(exception.IronicException, self.assertRaises(exception.IronicException,
self.cache._download_image, self.cache._download_image,
'uuid', 'fake', 'fake') 'uuid', 'fake', 'fake', {})
self.assertTrue(mock_rmtree.called) self.assertTrue(mock_rmtree.called)
@mock.patch.object(image_cache.LOG, 'warning', autospec=True) @mock.patch.object(image_cache.LOG, 'warning', autospec=True)

8
releasenotes/notes/no-cache-df7caa45f3d8b6d7.yaml Normal file
View File

@ -0,0 +1,8 @@
---
fixes:
- |
The image cache now respects the ``Cache-Control: no-store`` header
for HTTP(s) images.
- |
File images are no longer cached in the image cache to avoid unnecessary
consumption of the disk space.