openstack
/
nova
Code Issues Proposed changes

libvirt: Remove redundant bdm serial mangling and saving during swap_volume 

During an initial swap_volume call the serial of the original volume was
previously stashed in the connection_info of the new volume by the
compute layer. This was used by I19d5182d11 to allow the virt driver to
lookup and update the existing BDM with the new volume's connection_info
after it had been used by connect_volume.

Future calls to swap_volume in the compute layer would not
update the serial found in the old volume connection_info. This would
result in an invalid serial being copied into the new volume
connection_info and used to preform a lookup of a BDM that didn't exist.

To correct this we now explicitly set the serial of the new volume to
that of the new volume id. While the correct serial id should already be
present in the connection_info provided by most backend Cinder volume
drivers the act of updating this dict is required by our own functional
tests to invoke a failure case :

https://git.io/vDmRE

The serial is updated once more to match the volume id returned
by os-migrate-volume-completion prior to the BDM being updated in the
compute layer.

The BDM lookup and save from the virt layer is also removed as the
compute layer retains a reference to new_cinfo and will update the BDM
with this, including any modifications, at the end of swap_volume.

Finally, the associated Tempest admin test is also extended by the
following change to now attempt a second volume swap to verify these
changes :

I2a2c80a164b9f75d0e7e0503a24194bedfc0e66b

Closes-bug: #1661016
Change-Id: If74dd9d8e7191047b6f1cd7e35b6fc667f004f91
(cherry picked from commit 1bf7ac8048)
This commit is contained in:
Lee Yarwood 2017-01-31 18:39:15 +00:00
parent 35fa120cdc
commit 4bf211c617
4 changed files with 40 additions and 47 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
nova/compute/manager.py
View File

@ -4958,7 +4958,11 @@ class ComputeManager(manager.Manager):
old_cinfo = jsonutils.loads(bdm['connection_info'])
if old_cinfo and 'serial' not in old_cinfo:
old_cinfo['serial'] = old_volume_id
new_cinfo['serial'] = old_cinfo['serial']
# NOTE(lyarwood): serial is not always present in the returned
# connection_info so set it if it is missing as we do in
# DriverVolumeBlockDevice.attach().
if 'serial' not in new_cinfo:
new_cinfo['serial'] = new_volume_id
return (old_cinfo, new_cinfo)
def _swap_volume(self, context, instance, bdm, connector,
@ -4973,6 +4977,10 @@ class ComputeManager(manager.Manager):
connector,
instance,
bdm)
# NOTE(lyarwood): The Libvirt driver, the only virt driver
# currently implementing swap_volume, will modify the contents of
# new_cinfo when connect_volume is called. This is then saved to
# the BDM in swap_volume for future use outside of this flow.
LOG.debug("swap_volume: Calling driver volume swap with "
"connection infos: new: %(new_cinfo)s; "
"old: %(old_cinfo)s",
@ -4980,6 +4988,9 @@ class ComputeManager(manager.Manager):
instance=instance)
self.driver.swap_volume(old_cinfo, new_cinfo, instance, mountpoint,
resize_to)
LOG.debug("swap_volume: Driver volume swap returned, new "
"connection_info is now : %(new_cinfo)s",
{'new_cinfo': new_cinfo})
except Exception as ex:
failed = True
with excutils.save_and_reraise_exception():
@ -5011,8 +5022,13 @@ class ComputeManager(manager.Manager):
self.volume_api.terminate_connection(context,
conn_volume,
connector)
# If Cinder initiated the swap, it will keep
# the original ID
# NOTE(lyarwood): The following call to
# os-migrate-volume-completion returns a dict containing
# save_volume_id, this volume id has two possible values :
# 1. old_volume_id if we are migrating (retyping) volumes
# 2. new_volume_id if we are swapping between two existing volumes
# This volume id is later used to update the volume_id and
# connection_info['serial'] of the BDM.
comp_ret = self.volume_api.migrate_volume_completion(
context,
old_volume_id,
@ -5057,9 +5073,10 @@ class ComputeManager(manager.Manager):
new_volume_id,
resize_to)
# NOTE(lyarwood): Update the BDM with the modified new_cinfo and
# correct volume_id returned by Cinder.
save_volume_id = comp_ret['save_volume_id']
# Update bdm
new_cinfo['serial'] = save_volume_id
values = {
'connection_info': jsonutils.dumps(new_cinfo),
'source_type': 'volume',

4
nova/tests/unit/compute/test_compute_mgr.py
View File

@ -1943,7 +1943,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
'delete_on_termination': True,
'connection_info': '{"foo": "bar"}'})
comp_ret = {'save_volume_id': old_volume_id}
new_info = {"foo": "bar"}
new_info = {"foo": "bar", "serial": old_volume_id}
swap_volume_mock.return_value = (comp_ret, new_info)
volume_connector_mock.return_value = {}
update_bdm_mock.return_value = fake_bdm
@ -1953,7 +1953,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase):
fake_instance.fake_instance_obj(self.context,
**{'uuid': uuids.instance}))
update_values = {'no_device': False,
'connection_info': u'{"foo": "bar"}',
'connection_info': jsonutils.dumps(new_info),
'volume_id': old_volume_id,
'source_type': u'volume',
'snapshot_id': None,

41
nova/tests/unit/virt/libvirt/test_driver.py
View File

@ -14735,18 +14735,12 @@ class LibvirtConnTestCase(test.NoDBTestCase):
@mock.patch('nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume')
@mock.patch('nova.virt.libvirt.driver.LibvirtDriver._swap_volume')
@mock.patch('nova.objects.block_device.BlockDeviceMapping.'
'get_by_volume_and_instance')
@mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_volume_config')
@mock.patch('nova.virt.libvirt.driver.LibvirtDriver._connect_volume')
@mock.patch('nova.virt.libvirt.host.Host.get_guest')
def _test_swap_volume_driver_bdm_save(self, get_guest,
connect_volume, get_volume_config,
get_by_volume_and_instance,
swap_volume,
disconnect_volume,
volume_save,
source_type):
def _test_swap_volume_driver(self, get_guest, connect_volume,
get_volume_config, swap_volume,
disconnect_volume, source_type):
conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI())
instance = objects.Instance(**self.test_instance)
old_connection_info = {'driver_volume_type': 'fake',
@ -14775,16 +14769,6 @@ class LibvirtConnTestCase(test.NoDBTestCase):
get_volume_config.return_value = mock.MagicMock(
source_path='/fake-new-volume')
bdm = objects.BlockDeviceMapping(self.context,
**fake_block_device.FakeDbBlockDeviceDict(
{'id': 2, 'instance_uuid': uuids.instance,
'device_name': '/dev/vdb',
'source_type': source_type,
'destination_type': 'volume',
'volume_id': 'fake-volume-id-2',
'boot_index': 0}))
get_by_volume_and_instance.return_value = bdm
conn.swap_volume(old_connection_info, new_connection_info, instance,
'/dev/vdb', 1)
@ -14794,22 +14778,15 @@ class LibvirtConnTestCase(test.NoDBTestCase):
swap_volume.assert_called_once_with(guest, 'vdb',
'/fake-new-volume', 1)
disconnect_volume.assert_called_once_with(old_connection_info, 'vdb')
volume_save.assert_called_once_with()
@mock.patch('nova.virt.block_device.DriverVolumeBlockDevice.save')
def test_swap_volume_driver_bdm_save_source_is_volume(self, volume_save):
self._test_swap_volume_driver_bdm_save(volume_save=volume_save,
source_type='volume')
def test_swap_volume_driver_source_is_volume(self):
self._test_swap_volume_driver(source_type='volume')
@mock.patch('nova.virt.block_device.DriverImageBlockDevice.save')
def test_swap_volume_driver_bdm_save_source_is_image(self, volume_save):
self._test_swap_volume_driver_bdm_save(volume_save=volume_save,
source_type='image')
def test_swap_volume_driver_source_is_image(self):
self._test_swap_volume_driver(source_type='image')
@mock.patch('nova.virt.block_device.DriverSnapshotBlockDevice.save')
def test_swap_volume_driver_bdm_save_source_is_snapshot(self, volume_save):
self._test_swap_volume_driver_bdm_save(volume_save=volume_save,
source_type='snapshot')
def test_swap_volume_driver_source_is_snapshot(self):
self._test_swap_volume_driver(source_type='snapshot')
@mock.patch('nova.virt.libvirt.guest.BlockDevice.is_job_complete')
def _test_live_snapshot(self, mock_is_job_complete,

15
nova/virt/libvirt/driver.py
View File

@ -1286,20 +1286,19 @@ class LibvirtDriver(driver.ComputeDriver):
CONF.libvirt.virt_type, disk_dev),
'type': 'disk',
}
# NOTE (lyarwood): new_connection_info will be modified by the
# following _connect_volume call down into the volume drivers. The
# majority of the volume drivers will add a device_path that is in turn
# used by _get_volume_config to set the source_path of the
# LibvirtConfigGuestDisk object it returns. We do not explicitly save
# this to the BDM here as the upper compute swap_volume method will
# eventually do this for us.
self._connect_volume(new_connection_info, disk_info)
conf = self._get_volume_config(new_connection_info, disk_info)
if not conf.source_path:
self._disconnect_volume(new_connection_info, disk_dev)
raise NotImplementedError(_("Swap only supports host devices"))
# Save updates made in connection_info when connect_volume was called
volume_id = new_connection_info.get('serial')
bdm = objects.BlockDeviceMapping.get_by_volume_and_instance(
nova_context.get_admin_context(), volume_id, instance.uuid)
driver_bdm = driver_block_device.convert_volume(bdm)
driver_bdm['connection_info'] = new_connection_info
driver_bdm.save()
self._swap_volume(guest, disk_dev, conf.source_path, resize_to)
self._disconnect_volume(old_connection_info, disk_dev)