From ee2c0a00db9d6006fb7c3a07ee252d4ca4d73eff Mon Sep 17 00:00:00 2001 From: Sylvain Bauza Date: Tue, 20 Sep 2016 16:50:16 +0200 Subject: [PATCH] Revert "Set 'serial' to new volume ID in swap volumes" The below commit introduced a regression by updating the wrong value to the attachment field when calling the volume-update swap operation where the value is now the previous volume ID instead of the current volume ID. It litterally makes it now imposible to detech the volume from the instance once it has been swapped (even after the first swap). Given the original issue can be worked around by detaching and then attaching the volume before swapping back to the original volume, and because the original bug only impacts an admin API while here it impacts a user API, it's preferrable to directly revert the regression and then work on the next cycle about the root problem rather than leaving the change and try to fix something which is hard to troubleshoot, also given the lack of functional tests around the volume operations. This reverts commit be553fb15591c6fc212ef3a07c1dd1cbc43d6866. Change-Id: Ibad1afa5860d611e0e0ea0ba5e7dc98ae8f07190 Closes-Bug: #1625660 --- nova/compute/manager.py | 12 +--- nova/tests/unit/compute/test_compute_mgr.py | 66 --------------------- nova/tests/unit/virt/libvirt/test_driver.py | 17 ++---- nova/virt/libvirt/driver.py | 2 +- 4 files changed, 10 insertions(+), 87 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index e3dfbc41cb8a..7a788a19d8d5 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -4848,8 +4848,9 @@ class ComputeManager(manager.Manager): new_volume_id, connector) old_cinfo = jsonutils.loads(bdm['connection_info']) - if 'serial' not in new_cinfo: - new_cinfo['serial'] = new_volume_id + if old_cinfo and 'serial' not in old_cinfo: + old_cinfo['serial'] = old_volume_id + new_cinfo['serial'] = old_cinfo['serial'] return (old_cinfo, new_cinfo) def _swap_volume(self, context, instance, bdm, connector, @@ -4906,13 +4907,6 @@ class ComputeManager(manager.Manager): old_volume_id, new_volume_id, error=failed) - # If Cinder initiated the swap, the serial of new connection info - # is set to old volume ID. - if (new_cinfo is not None and - new_cinfo['serial'] == new_volume_id and - comp_ret['save_volume_id'] == old_volume_id): - new_cinfo['serial'] = old_volume_id - LOG.debug("swap_volume: Cinder migrate_volume_completion " "returned: %(comp_ret)s", {'comp_ret': comp_ret}, context=context, instance=instance) diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index bc32690ec1ce..709c98cda59a 100755 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -1852,72 +1852,6 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): self.assertEqual(volumes[old_volume_id]['status'], 'in-use') self.assertEqual(volumes[new_volume_id]['status'], 'available') - @mock.patch('nova.db.block_device_mapping_get_by_instance_and_volume_id') - @mock.patch.object(fake_driver.FakeDriver, 'get_volume_connector', - return_value={}) - @mock.patch.object(fake_driver.FakeDriver, 'swap_volume') - @mock.patch('nova.volume.cinder.API.get') - @mock.patch('nova.volume.cinder.API.initialize_connection', - return_value={}) - @mock.patch('nova.volume.cinder.API.terminate_connection') - @mock.patch('nova.volume.cinder.API.migrate_volume_completion') - @mock.patch('nova.objects.BlockDeviceMapping.update') - @mock.patch('nova.objects.BlockDeviceMapping.save') - def test_swap_volume_cinder_initiated(self, mock_bdm_save, mock_bdm_update, - mock_migrate_volume_completion, - mock_terminate_connection, - mock_initialize_connection, - mock_get, mock_swap_volume, - mock_get_volume_connector, - mock_bdm_get): - # Check whether the 'serial' in new connection info is equal to - # the old volume ID in the case that cinder initiated - # swapping volumes - mock_get.return_value = {'id': uuids.old_volume, - 'display_name': 'old_volume', - 'status': 'detaching', - 'size': 2} - fake_bdm = fake_block_device.FakeDbBlockDeviceDict( - {'device_name': '/dev/vdb', 'source_type': 'volume', - 'destination_type': 'volume', - 'instance_uuid': uuids.instance, - 'delete_on_termination': True, - 'connection_info': '{"foo": "bar"}'}) - mock_bdm_get.return_value = fake_bdm - mock_migrate_volume_completion.return_value = {'save_volume_id': - uuids.old_volume} - instance = fake_instance.fake_instance_obj(self.context, - **{'uuid': uuids.instance}) - - self.compute.swap_volume( - self.context, uuids.old_volume, uuids.new_volume, instance) - - mock_get_volume_connector.assert_called_once_with(instance) - mock_get.assert_has_calls( - [mock.call(test.MatchType(context.RequestContext), - uuids.old_volume), - mock.call(test.MatchType(context.RequestContext), - uuids.new_volume)]) - mock_initialize_connection.assert_called_once_with( - test.MatchType(context.RequestContext), uuids.new_volume, {}) - mock_swap_volume.assert_called_once_with( - {"foo": "bar"}, {'serial': uuids.old_volume}, instance, - '/dev/vdb', 0) - mock_terminate_connection.assert_called_once_with( - test.MatchType(context.RequestContext), uuids.old_volume, {}) - mock_migrate_volume_completion.assert_called_once_with( - test.MatchType(context.RequestContext), uuids.old_volume, - uuids.new_volume, error=False) - # Check 'serial' in new connection info - mock_bdm_update.assert_called_once_with( - {'connection_info': jsonutils.dumps({'serial': uuids.old_volume}), - 'source_type': 'volume', - 'destination_type': 'volume', - 'snapshot_id': None, - 'volume_id': uuids.old_volume, - 'no_device': None}) - mock_bdm_save.assert_called_once_with() - @mock.patch('nova.db.block_device_mapping_get_by_instance_and_volume_id') @mock.patch('nova.db.block_device_mapping_update') @mock.patch('nova.volume.cinder.API.get') diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 17dd0b1bc3d3..412ac028b529 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -14377,7 +14377,6 @@ class LibvirtConnTestCase(test.NoDBTestCase): srcfile, 1 * units.Gi / units.Ki) mock_define.assert_called_once_with(xmldoc) - @mock.patch('nova.context.get_admin_context', return_value='fake-context') @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.' @@ -14386,13 +14385,12 @@ class LibvirtConnTestCase(test.NoDBTestCase): @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, - get_admin_context, - volume_save, - source_type): + connect_volume, get_volume_config, + get_by_volume_and_instance, + swap_volume, + disconnect_volume, + volume_save, + source_type): conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI()) instance = objects.Instance(**self.test_instance) old_connection_info = {'driver_volume_type': 'fake', @@ -14436,9 +14434,6 @@ class LibvirtConnTestCase(test.NoDBTestCase): get_guest.assert_called_once_with(instance) connect_volume.assert_called_once_with(new_connection_info, disk_info) - get_by_volume_and_instance.assert_called_once_with('fake-context', - 'old-volume-id', - instance.uuid) swap_volume.assert_called_once_with(guest, 'vdb', '/fake-new-volume', 1) diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 1b667e854126..95c2fea93547 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -1217,7 +1217,7 @@ class LibvirtDriver(driver.ComputeDriver): raise NotImplementedError(_("Swap only supports host devices")) # Save updates made in connection_info when connect_volume was called - volume_id = old_connection_info.get('serial') + 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)