Reduce scope of the lock for image volume cache
Refactor the code that creates a volume from a downloaded glance image to minimize the scope of the lock that prevents multiple entries in the volume image cache. Now the lock serializes only the portion of the code that causes the cache entry to be created. Locking is minimized when the volume is already cached, or when the volume won't be cached. Closes-Bug: #1758414 Change-Id: I547fb4bcdd4783225b8ca96d157c61ca3bcf4ef4
This commit is contained in:
Alan Bishop
parent
63aa5a9a7f
commit
a351cd01c0
|
|
@ -892,6 +892,9 @@ class CreateVolumeFlowManagerTestCase(test.TestCase):
|
|||
@mock.patch('cinder.volume.flows.manager.create_volume.'
|
||||
'CreateVolumeFromSpecTask.'
|
||||
'_cleanup_cg_in_volume')
|
||||
@mock.patch('cinder.volume.flows.manager.create_volume.'
|
||||
'CreateVolumeFromSpecTask.'
|
||||
'_prepare_image_cache_entry')
|
||||
@mock.patch('cinder.volume.flows.manager.create_volume.'
|
||||
'CreateVolumeFromSpecTask.'
|
||||
'_handle_bootable_volume_glance_meta')
|
||||
|
|
@ -903,12 +906,14 @@ class CreateVolumeFlowManagerTestCase(test.TestCase):
|
|||
mock_qemu_img,
|
||||
mock_fetch_img,
|
||||
mock_handle_bootable,
|
||||
mock_prepare_image_cache,
|
||||
mock_cleanup_cg):
|
||||
fake_db = mock.MagicMock()
|
||||
fake_driver = mock.MagicMock()
|
||||
fake_volume_manager = mock.MagicMock()
|
||||
fake_cache = mock.MagicMock()
|
||||
fake_manager = create_volume_manager.CreateVolumeFromSpecTask(
|
||||
fake_volume_manager, fake_db, fake_driver)
|
||||
fake_volume_manager, fake_db, fake_driver, fake_cache)
|
||||
volume = fake_volume.fake_volume_obj(
|
||||
self.ctxt,
|
||||
encryption_key_id=fakes.ENCRYPTION_KEY_ID,
|
||||
|
|
@ -930,6 +935,7 @@ class CreateVolumeFlowManagerTestCase(test.TestCase):
|
|||
fake_driver.create_volume.assert_called_once_with(volume)
|
||||
fake_driver.copy_image_to_encrypted_volume.assert_called_once_with(
|
||||
self.ctxt, volume, fake_image_service, image_id)
|
||||
mock_prepare_image_cache.assert_not_called()
|
||||
mock_handle_bootable.assert_called_once_with(self.ctxt, volume,
|
||||
image_id=image_id,
|
||||
image_meta=image_meta)
|
||||
|
|
@ -1872,3 +1878,56 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase):
|
|||
# Make sure we didn't try and create a cache entry
|
||||
self.assertFalse(self.mock_cache.ensure_space.called)
|
||||
self.assertFalse(self.mock_cache.create_cache_entry.called)
|
||||
|
||||
@ddt.data(None, {'volume_id': fakes.VOLUME_ID})
|
||||
@mock.patch('cinder.volume.flows.manager.create_volume.'
|
||||
'CreateVolumeFromSpecTask.'
|
||||
'_create_from_image_cache_or_download')
|
||||
def test_prepare_image_cache_entry(
|
||||
self,
|
||||
mock_cache_entry,
|
||||
mock_create_from_image_cache_or_download,
|
||||
mock_get_internal_context,
|
||||
mock_create_from_img_dl, mock_create_from_src,
|
||||
mock_handle_bootable, mock_fetch_img):
|
||||
self.mock_cache.get_entry.return_value = mock_cache_entry
|
||||
volume = fake_volume.fake_volume_obj(self.ctxt,
|
||||
id=fakes.VOLUME_ID,
|
||||
host='host@backend#pool')
|
||||
image_location = 'someImageLocationStr'
|
||||
image_id = fakes.IMAGE_ID
|
||||
image_meta = {'virtual_size': '1073741824', 'size': 1073741824}
|
||||
|
||||
manager = create_volume_manager.CreateVolumeFromSpecTask(
|
||||
self.mock_volume_manager,
|
||||
self.mock_db,
|
||||
self.mock_driver,
|
||||
image_volume_cache=self.mock_cache
|
||||
)
|
||||
model_update, cloned = manager._prepare_image_cache_entry(
|
||||
self.ctxt,
|
||||
volume,
|
||||
image_location,
|
||||
image_id,
|
||||
image_meta,
|
||||
self.mock_image_service)
|
||||
|
||||
if mock_cache_entry:
|
||||
# Entry is in cache, so basically don't do anything.
|
||||
self.assertFalse(cloned)
|
||||
self.assertIsNone(model_update)
|
||||
mock_create_from_image_cache_or_download.assert_not_called()
|
||||
else:
|
||||
# Entry is not in cache, so do the work that will add it.
|
||||
self.assertTrue(cloned)
|
||||
self.assertEqual(
|
||||
mock_create_from_image_cache_or_download.return_value,
|
||||
model_update)
|
||||
mock_create_from_image_cache_or_download.assert_called_once_with(
|
||||
self.ctxt,
|
||||
volume,
|
||||
image_location,
|
||||
image_id,
|
||||
image_meta,
|
||||
self.mock_image_service,
|
||||
update_cache=True)
|
||||
|
|
|
|||
|
|
@ -718,9 +718,45 @@ class CreateVolumeFromSpecTask(flow_utils.CinderTask):
|
|||
return None, False
|
||||
|
||||
@coordination.synchronized('{image_id}')
|
||||
def _prepare_image_cache_entry(self, context, volume,
|
||||
image_location, image_id,
|
||||
image_meta, image_service):
|
||||
internal_context = cinder_context.get_internal_tenant_context()
|
||||
if not internal_context:
|
||||
return None, False
|
||||
|
||||
cache_entry = self.image_volume_cache.get_entry(internal_context,
|
||||
volume,
|
||||
image_id,
|
||||
image_meta)
|
||||
|
||||
# If the entry is in the cache then return ASAP in order to minimize
|
||||
# the scope of the lock. If it isn't in the cache then do the work
|
||||
# that adds it. The work is done inside the locked region to ensure
|
||||
# only one cache entry is created.
|
||||
if cache_entry:
|
||||
LOG.debug('Found cache entry for image = '
|
||||
'%(image_id)s on host %(host)s.',
|
||||
{'image_id': image_id, 'host': volume.host})
|
||||
return None, False
|
||||
else:
|
||||
LOG.debug('Preparing cache entry for image = '
|
||||
'%(image_id)s on host %(host)s.',
|
||||
{'image_id': image_id, 'host': volume.host})
|
||||
model_update = self._create_from_image_cache_or_download(
|
||||
context,
|
||||
volume,
|
||||
image_location,
|
||||
image_id,
|
||||
image_meta,
|
||||
image_service,
|
||||
update_cache=True)
|
||||
return model_update, True
|
||||
|
||||
def _create_from_image_cache_or_download(self, context, volume,
|
||||
image_location, image_id,
|
||||
image_meta, image_service):
|
||||
image_meta, image_service,
|
||||
update_cache=False):
|
||||
# NOTE(e0ne): check for free space in image_conversion_dir before
|
||||
# image downloading.
|
||||
# NOTE(mnaser): This check *only* happens if the backend is not able
|
||||
|
|
@ -759,8 +795,8 @@ class CreateVolumeFromSpecTask(flow_utils.CinderTask):
|
|||
image_id,
|
||||
image_meta
|
||||
)
|
||||
# Don't cache encrypted volume.
|
||||
if not cloned and not volume.encryption_key_id:
|
||||
# Don't cache unless directed.
|
||||
if not cloned and update_cache:
|
||||
should_create_cache_entry = True
|
||||
# cleanup consistencygroup field in the volume,
|
||||
# because when creating cache entry, it will need
|
||||
|
|
@ -870,6 +906,21 @@ class CreateVolumeFromSpecTask(flow_utils.CinderTask):
|
|||
image_location,
|
||||
image_meta)
|
||||
|
||||
# If we're going to try using the image cache then prepare the cache
|
||||
# entry. Note: encrypted volume images are not cached.
|
||||
if not cloned and self.image_volume_cache and not volume_is_encrypted:
|
||||
# If _prepare_image_cache_entry() has to create the cache entry
|
||||
# then it will also create the volume. But if the volume image
|
||||
# is already in the cache then it returns (None, False), and
|
||||
# _create_from_image_cache_or_download() will use the cache.
|
||||
model_update, cloned = self._prepare_image_cache_entry(
|
||||
context,
|
||||
volume,
|
||||
image_location,
|
||||
image_id,
|
||||
image_meta,
|
||||
image_service)
|
||||
|
||||
# Try and use the image cache, and download if not cached.
|
||||
if not cloned:
|
||||
model_update = self._create_from_image_cache_or_download(
|
||||
|
|
|
|||
Loading…
Reference in New Issue