Added context manager for instance lock
Moved lock and unlock instance code to context manager. Updated _refresh volume attachment method to use instance lock context manager Now there will be a single request ID for the lock, refresh, and unlock actions. Earlier, the volume_attachment refresh operation used to have a unique req-id for each action. Related-Bug: #2012365 Change-Id: I6588836c3484a26d67a5995710761f0f6b6a4c18
This commit is contained in:
Amit Uniyal
parent
260dbd9761
commit
211737d0ce
|
|
@ -22,6 +22,7 @@
|
|||
"""
|
||||
|
||||
import collections
|
||||
from contextlib import contextmanager
|
||||
import functools
|
||||
import os
|
||||
import re
|
||||
|
|
@ -147,6 +148,37 @@ def format_dict(dct, dict_property="Property", dict_value='Value',
|
|||
return encodeutils.safe_encode(pt.get_string()).decode()
|
||||
|
||||
|
||||
@contextmanager
|
||||
def locked_instance(cell_mapping, instance, reason):
|
||||
"""Context manager to lock and unlock instance,
|
||||
lock state will be restored regardless of the success or failure
|
||||
of target functionality.
|
||||
|
||||
:param cell_mapping: instance-cell-mapping
|
||||
:param instance: instance to be lock and unlock
|
||||
:param reason: reason, why lock is required
|
||||
"""
|
||||
|
||||
compute_api = api.API()
|
||||
|
||||
initial_state = 'locked' if instance.locked else 'unlocked'
|
||||
if not instance.locked:
|
||||
with context.target_cell(
|
||||
context.get_admin_context(),
|
||||
cell_mapping
|
||||
) as cctxt:
|
||||
compute_api.lock(cctxt, instance, reason=reason)
|
||||
try:
|
||||
yield
|
||||
finally:
|
||||
if initial_state == 'unlocked':
|
||||
with context.target_cell(
|
||||
context.get_admin_context(),
|
||||
cell_mapping
|
||||
) as cctxt:
|
||||
compute_api.unlock(cctxt, instance)
|
||||
|
||||
|
||||
class DbCommands(object):
|
||||
"""Class for managing the main database."""
|
||||
|
||||
|
|
@ -3018,10 +3050,8 @@ class VolumeAttachmentCommands(object):
|
|||
:param instance_uuid: UUID of instance
|
||||
:param volume_id: ID of volume attached to the instance
|
||||
:param connector: Connector with which to create the new attachment
|
||||
:return status_code: volume-refresh status_code 0 on success
|
||||
"""
|
||||
volume_api = cinder.API()
|
||||
compute_rpcapi = rpcapi.ComputeAPI()
|
||||
compute_api = api.API()
|
||||
|
||||
ctxt = context.get_admin_context()
|
||||
im = objects.InstanceMapping.get_by_instance_uuid(ctxt, instance_uuid)
|
||||
|
|
@ -3037,111 +3067,97 @@ class VolumeAttachmentCommands(object):
|
|||
state=instance.vm_state,
|
||||
method='refresh connection_info (must be stopped)')
|
||||
|
||||
if instance.locked:
|
||||
raise exception.InstanceInvalidState(
|
||||
instance_uuid=instance_uuid, attr='locked', state='True',
|
||||
method='refresh connection_info (must be unlocked)')
|
||||
locking_reason = (
|
||||
f'Refreshing connection_info for BDM {bdm.uuid} '
|
||||
f'associated with instance {instance_uuid} and volume '
|
||||
f'{volume_id}.')
|
||||
|
||||
compute_api.lock(
|
||||
cctxt, instance,
|
||||
reason=(
|
||||
f'Refreshing connection_info for BDM {bdm.uuid} '
|
||||
f'associated with instance {instance_uuid} and volume '
|
||||
f'{volume_id}.'))
|
||||
with locked_instance(im.cell_mapping, instance, locking_reason):
|
||||
return self._do_refresh(
|
||||
cctxt, instance, volume_id, bdm, connector)
|
||||
|
||||
# NOTE(lyarwood): Yes this is weird but we need to recreate the admin
|
||||
# context here to ensure the lock above uses a unique request-id
|
||||
# versus the following refresh and eventual unlock.
|
||||
ctxt = context.get_admin_context()
|
||||
with context.target_cell(ctxt, im.cell_mapping) as cctxt:
|
||||
instance_action = None
|
||||
new_attachment_id = None
|
||||
def _do_refresh(self, cctxt, instance,
|
||||
volume_id, bdm, connector):
|
||||
volume_api = cinder.API()
|
||||
compute_rpcapi = rpcapi.ComputeAPI()
|
||||
|
||||
new_attachment_id = None
|
||||
try:
|
||||
# Log this as an instance action so operators and users are
|
||||
# aware that this has happened.
|
||||
instance_action = objects.InstanceAction.action_start(
|
||||
cctxt, instance.uuid,
|
||||
instance_actions.NOVA_MANAGE_REFRESH_VOLUME_ATTACHMENT)
|
||||
|
||||
# Create a blank attachment to keep the volume reserved
|
||||
new_attachment_id = volume_api.attachment_create(
|
||||
cctxt, volume_id, instance.uuid)['id']
|
||||
|
||||
# RPC call to the compute to cleanup the connections, which
|
||||
# will in turn unmap the volume from the compute host
|
||||
# TODO(lyarwood): Add delete_attachment as a kwarg to
|
||||
# remove_volume_connection as is available in the private
|
||||
# method within the manager.
|
||||
compute_rpcapi.remove_volume_connection(
|
||||
cctxt, instance, volume_id, instance.host)
|
||||
|
||||
# Delete the existing volume attachment if present in the bdm.
|
||||
# This isn't present when the original attachment was made
|
||||
# using the legacy cinderv2 APIs before the cinderv3 attachment
|
||||
# based APIs were present.
|
||||
if bdm.attachment_id:
|
||||
volume_api.attachment_delete(cctxt, bdm.attachment_id)
|
||||
|
||||
# Update the attachment with host connector, this regenerates
|
||||
# the connection_info that we can now stash in the bdm.
|
||||
new_connection_info = volume_api.attachment_update(
|
||||
cctxt, new_attachment_id, connector,
|
||||
bdm.device_name)['connection_info']
|
||||
|
||||
# Before we save it to the BDM ensure the serial is stashed as
|
||||
# is done in various other codepaths when attaching volumes.
|
||||
if 'serial' not in new_connection_info:
|
||||
new_connection_info['serial'] = bdm.volume_id
|
||||
|
||||
# Save the new attachment id and connection_info to the DB
|
||||
bdm.attachment_id = new_attachment_id
|
||||
bdm.connection_info = jsonutils.dumps(new_connection_info)
|
||||
bdm.save()
|
||||
|
||||
# Finally mark the attachment as complete, moving the volume
|
||||
# status from attaching to in-use ahead of the instance
|
||||
# restarting
|
||||
volume_api.attachment_complete(cctxt, new_attachment_id)
|
||||
return 0
|
||||
|
||||
finally:
|
||||
# If the bdm.attachment_id wasn't updated make sure we clean
|
||||
# up any attachments created during the run.
|
||||
bdm = objects.BlockDeviceMapping.get_by_volume_and_instance(
|
||||
cctxt, volume_id, instance.uuid)
|
||||
if (
|
||||
new_attachment_id and
|
||||
bdm.attachment_id != new_attachment_id
|
||||
):
|
||||
volume_api.attachment_delete(cctxt, new_attachment_id)
|
||||
|
||||
# If we failed during attachment_update the bdm.attachment_id
|
||||
# has already been deleted so recreate it now to ensure the
|
||||
# volume is still associated with the instance and clear the
|
||||
# now stale connection_info.
|
||||
try:
|
||||
# Log this as an instance action so operators and users are
|
||||
# aware that this has happened.
|
||||
instance_action = objects.InstanceAction.action_start(
|
||||
cctxt, instance_uuid,
|
||||
instance_actions.NOVA_MANAGE_REFRESH_VOLUME_ATTACHMENT)
|
||||
|
||||
# Create a blank attachment to keep the volume reserved
|
||||
new_attachment_id = volume_api.attachment_create(
|
||||
cctxt, volume_id, instance_uuid)['id']
|
||||
|
||||
# RPC call to the compute to cleanup the connections, which
|
||||
# will in turn unmap the volume from the compute host
|
||||
# TODO(lyarwood): Add delete_attachment as a kwarg to
|
||||
# remove_volume_connection as is available in the private
|
||||
# method within the manager.
|
||||
compute_rpcapi.remove_volume_connection(
|
||||
cctxt, instance, volume_id, instance.host)
|
||||
|
||||
# Delete the existing volume attachment if present in the bdm.
|
||||
# This isn't present when the original attachment was made
|
||||
# using the legacy cinderv2 APIs before the cinderv3 attachment
|
||||
# based APIs were present.
|
||||
if bdm.attachment_id:
|
||||
volume_api.attachment_delete(cctxt, bdm.attachment_id)
|
||||
|
||||
# Update the attachment with host connector, this regenerates
|
||||
# the connection_info that we can now stash in the bdm.
|
||||
new_connection_info = volume_api.attachment_update(
|
||||
cctxt, new_attachment_id, connector,
|
||||
bdm.device_name)['connection_info']
|
||||
|
||||
# Before we save it to the BDM ensure the serial is stashed as
|
||||
# is done in various other codepaths when attaching volumes.
|
||||
if 'serial' not in new_connection_info:
|
||||
new_connection_info['serial'] = bdm.volume_id
|
||||
|
||||
# Save the new attachment id and connection_info to the DB
|
||||
bdm.attachment_id = new_attachment_id
|
||||
bdm.connection_info = jsonutils.dumps(new_connection_info)
|
||||
volume_api.attachment_get(cctxt, bdm.attachment_id)
|
||||
except exception.VolumeAttachmentNotFound:
|
||||
bdm.attachment_id = volume_api.attachment_create(
|
||||
cctxt, volume_id, instance.uuid)['id']
|
||||
bdm.connection_info = None
|
||||
bdm.save()
|
||||
|
||||
# Finally mark the attachment as complete, moving the volume
|
||||
# status from attaching to in-use ahead of the instance
|
||||
# restarting
|
||||
volume_api.attachment_complete(cctxt, new_attachment_id)
|
||||
return 0
|
||||
|
||||
finally:
|
||||
# If the bdm.attachment_id wasn't updated make sure we clean
|
||||
# up any attachments created during the run.
|
||||
bdm = objects.BlockDeviceMapping.get_by_volume_and_instance(
|
||||
cctxt, volume_id, instance_uuid)
|
||||
if (
|
||||
new_attachment_id and
|
||||
bdm.attachment_id != new_attachment_id
|
||||
):
|
||||
volume_api.attachment_delete(cctxt, new_attachment_id)
|
||||
|
||||
# If we failed during attachment_update the bdm.attachment_id
|
||||
# has already been deleted so recreate it now to ensure the
|
||||
# volume is still associated with the instance and clear the
|
||||
# now stale connection_info.
|
||||
try:
|
||||
volume_api.attachment_get(cctxt, bdm.attachment_id)
|
||||
except exception.VolumeAttachmentNotFound:
|
||||
bdm.attachment_id = volume_api.attachment_create(
|
||||
cctxt, volume_id, instance_uuid)['id']
|
||||
bdm.connection_info = None
|
||||
bdm.save()
|
||||
|
||||
# Finish the instance action if it was created and started
|
||||
# TODO(lyarwood): While not really required we should store
|
||||
# the exec and traceback in here on failure.
|
||||
if instance_action:
|
||||
instance_action.finish()
|
||||
|
||||
# NOTE(lyarwood): As above we need to unlock the instance with
|
||||
# a fresh context and request-id to keep it unique. It's safe
|
||||
# to assume that the instance is locked as this point as the
|
||||
# earlier call to lock isn't part of this block.
|
||||
with context.target_cell(
|
||||
context.get_admin_context(),
|
||||
im.cell_mapping
|
||||
) as u_cctxt:
|
||||
compute_api.unlock(u_cctxt, instance)
|
||||
# Finish the instance action if it was created and started
|
||||
# TODO(lyarwood): While not really required we should store
|
||||
# the exec and traceback in here on failure.
|
||||
if instance_action:
|
||||
instance_action.finish()
|
||||
|
||||
@action_description(
|
||||
_("Refresh the connection info for a given volume attachment"))
|
||||
|
|
|
|||
|
|
@ -3492,6 +3492,7 @@ class VolumeAttachmentCommandsTestCase(test.NoDBTestCase):
|
|||
def _test_refresh(self, mock_exists):
|
||||
ctxt = context.get_admin_context()
|
||||
cell_ctxt = context.get_admin_context()
|
||||
cell_ctxt.cell_uuid = '39fd7a1f-db62-45bc-a7b6-8137cef87c9d'
|
||||
fake_connector = self._get_fake_connector_info()
|
||||
|
||||
mock_exists.return_value = True
|
||||
|
|
@ -3548,11 +3549,14 @@ class VolumeAttachmentCommandsTestCase(test.NoDBTestCase):
|
|||
output = self.output.getvalue().strip()
|
||||
self.assertIn('must be stopped', output)
|
||||
|
||||
@mock.patch.object(objects.InstanceAction, 'action_start')
|
||||
@mock.patch.object(manage.VolumeAttachmentCommands, '_do_refresh')
|
||||
@mock.patch.object(
|
||||
objects.BlockDeviceMapping, 'get_by_volume_and_instance')
|
||||
@mock.patch.object(objects.Instance, 'get_by_uuid')
|
||||
def test_refresh_instance_already_locked_failure(
|
||||
self, mock_get_instance, mock_get_bdm
|
||||
def test_refresh_instance_already_locked(
|
||||
self, mock_get_instance, mock_get_bdm,
|
||||
mock__do_refresh, mock_action_start
|
||||
):
|
||||
"""Test refresh with instance when instance is already locked."""
|
||||
mock_get_instance.return_value = objects.Instance(
|
||||
|
|
@ -3562,11 +3566,11 @@ class VolumeAttachmentCommandsTestCase(test.NoDBTestCase):
|
|||
mock_get_bdm.return_value = objects.BlockDeviceMapping(
|
||||
uuid=uuidsentinel.bdm, volume_id=uuidsentinel.volume,
|
||||
attachment_id=uuidsentinel.instance)
|
||||
mock_action = mock.Mock(spec=objects.InstanceAction)
|
||||
mock_action_start.return_value = mock_action
|
||||
|
||||
ret = self._test_refresh()
|
||||
self.assertEqual(5, ret)
|
||||
output = self.output.getvalue().strip()
|
||||
self.assertIn('must be unlocked', output)
|
||||
self._test_refresh()
|
||||
mock__do_refresh.assert_called_once()
|
||||
|
||||
@mock.patch.object(
|
||||
objects.BlockDeviceMapping, 'get_by_volume_and_instance')
|
||||
|
|
@ -3593,9 +3597,9 @@ class VolumeAttachmentCommandsTestCase(test.NoDBTestCase):
|
|||
@mock.patch.object(objects.Instance, 'get_by_uuid')
|
||||
@mock.patch.object(objects.InstanceAction, 'action_start')
|
||||
def test_refresh_attachment_unknown_failure(
|
||||
self, mock_action_start, mock_get_instance, mock_get_bdm, mock_lock,
|
||||
mock_unlock, mock_attachment_create, mock_attachment_delete,
|
||||
mock_attachment_get
|
||||
self, mock_action_start, mock_get_instance,
|
||||
mock_get_bdm, mock_lock, mock_unlock, mock_attachment_create,
|
||||
mock_attachment_delete, mock_attachment_get
|
||||
):
|
||||
"""Test refresh with instance when any other error happens.
|
||||
"""
|
||||
|
|
@ -3635,8 +3639,9 @@ class VolumeAttachmentCommandsTestCase(test.NoDBTestCase):
|
|||
@mock.patch.object(objects.Instance, 'get_by_uuid')
|
||||
@mock.patch.object(objects.InstanceAction, 'action_start')
|
||||
def test_refresh(
|
||||
self, mock_action_start, mock_get_instance, mock_get_bdm,
|
||||
mock_save_bdm, mock_compute_api, mock_volume_api, mock_compute_rpcapi
|
||||
self, mock_action_start, mock_get_instance,
|
||||
mock_get_bdm, mock_save_bdm, mock_compute_api, mock_volume_api,
|
||||
mock_compute_rpcapi
|
||||
):
|
||||
"""Test refresh with a successful code path."""
|
||||
fake_compute_api = mock_compute_api.return_value
|
||||
|
|
@ -3715,6 +3720,44 @@ class TestNovaManageMain(test.NoDBTestCase):
|
|||
self.assertEqual(255, manage.main())
|
||||
self.assertTrue(mock_pm.called)
|
||||
|
||||
def _lock_instance(self, ctxt, instance, reason):
|
||||
instance.locked = True
|
||||
|
||||
def _unlock_instance(self, ctxt, instance):
|
||||
instance.locked = False
|
||||
|
||||
def test_locked_instance(self):
|
||||
cm = objects.CellMapping(name='foo', uuid=uuidsentinel.cell)
|
||||
proj_uuid = uuidutils.generate_uuid()
|
||||
instance = objects.Instance(
|
||||
project_id=proj_uuid, uuid=uuidsentinel.instance)
|
||||
instance.locked = True
|
||||
|
||||
with mock.patch('nova.compute.api.API') as mock_api:
|
||||
mock_api.return_value.lock.side_effect = self._lock_instance
|
||||
mock_api.return_value.unlock.side_effect = self._unlock_instance
|
||||
|
||||
with manage.locked_instance(cm, instance, 'some'):
|
||||
self.assertTrue(instance.locked)
|
||||
|
||||
self.assertTrue(instance.locked)
|
||||
|
||||
def test_unlocked_instance(self):
|
||||
cm = objects.CellMapping(name='foo', uuid=uuidsentinel.cell)
|
||||
proj_uuid = uuidutils.generate_uuid()
|
||||
instance = objects.Instance(
|
||||
project_id=proj_uuid, uuid=uuidsentinel.instance)
|
||||
instance.locked = False
|
||||
|
||||
with mock.patch('nova.compute.api.API') as mock_api:
|
||||
mock_api.return_value.lock.side_effect = self._lock_instance
|
||||
mock_api.return_value.unlock.side_effect = self._unlock_instance
|
||||
|
||||
with manage.locked_instance(cm, instance, 'some'):
|
||||
self.assertTrue(instance.locked)
|
||||
|
||||
self.assertFalse(instance.locked)
|
||||
|
||||
|
||||
class LibvirtCommandsTestCase(test.NoDBTestCase):
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue