From 1bf7ac804808eb0d4a68847e466f5fa7d1dcd55e Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Tue, 31 Jan 2017 18:39:15 +0000 Subject: [PATCH] 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 --- nova/compute/manager.py | 27 +++++++++++--- nova/tests/unit/compute/test_compute_mgr.py | 4 +- nova/tests/unit/virt/libvirt/test_driver.py | 41 +++++---------------- nova/virt/libvirt/driver.py | 15 ++++---- 4 files changed, 40 insertions(+), 47 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 624e31395194..b7734d643a8e 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -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', diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index d0f54ff5c3d0..ad953adab25b 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -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, diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index fdcb872a83cd..3a5e0b65f669 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -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, diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 0aad4105a436..a08b4f19146d 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -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)