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:
parent
8237bb8770
commit
ee2c0a00db
|
@ -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)
|
||||
|
|
|
@ -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')
|
||||
|
|
|
@ -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)
|
||||
|
|
|
@ -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)
|
||||
|
|
Loading…
Reference in New Issue