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 be553fb155.

Change-Id: Ibad1afa5860d611e0e0ea0ba5e7dc98ae8f07190
Closes-Bug: #1625660
This commit is contained in:
Sylvain Bauza 2016-09-20 16:50:16 +02:00
parent 8237bb8770
commit ee2c0a00db
4 changed files with 10 additions and 87 deletions

View File

@ -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)

View File

@ -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')

View File

@ -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)

View File

@ -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)