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 commitbe553fb155
. Change-Id: Ibad1afa5860d611e0e0ea0ba5e7dc98ae8f07190 Closes-Bug: #1625660 (cherry picked from commitee2c0a00db
)
This commit is contained in:
parent
0cb20bb014
commit
eae8775f87
|
@ -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')
|
||||
|
|
|
@ -14345,7 +14345,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.'
|
||||
|
@ -14354,13 +14353,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',
|
||||
|
@ -14404,9 +14402,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