From ecd19ce45b61831bdbb42e2176105c735e2667ee Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Thu, 11 Jan 2018 13:06:40 -0500 Subject: [PATCH] Handle swapping to a multiattach volume Since swap volume doesn't attach the new volume using the DriverVolumeBlockDevice.attach flow like normal attach or server create, the new volume connection_info doesn't get the "multiattach" value set in it, which is later read in the virt driver's swap_volume method to build the disk device config. In the case of libvirt it relies on that multiattach flag being set in the connection_info dict to set the 'shareable' element on the LibvirtConfigGuestDisk object. This change modifies the swap volume flow to perform the same checks as in DriverVolumeBlockDevice._volume_attach. In this change, for simplicity, it's just a copy/paste hack, and a TODO is left to move this into common code later. Part of blueprint multi-attach-volume Change-Id: I173c404a54d1306e95c1cc3fbd651b137cb1c6aa --- nova/compute/manager.py | 29 ++++++-- nova/tests/unit/compute/test_compute_mgr.py | 80 ++++++++++++++++++--- 2 files changed, 93 insertions(+), 16 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index a3f5ed69ae03..e97cadc4e68f 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -5373,10 +5373,10 @@ class ComputeManager(manager.Manager): self._detach_volume(context, bdm, instance, attachment_id=attachment_id) - def _init_volume_connection(self, context, new_volume_id, + def _init_volume_connection(self, context, new_volume, old_volume_id, connector, bdm, new_attachment_id, mountpoint): - + new_volume_id = new_volume['id'] if new_attachment_id is None: # We're dealing with an old-style attachment so initialize the # connection so we can get the connection_info. @@ -5384,6 +5384,16 @@ class ComputeManager(manager.Manager): new_volume_id, connector) else: + # Check for multiattach on the new volume and if True, check to + # see if the virt driver supports multiattach. + # TODO(mriedem): This is copied from DriverVolumeBlockDevice + # and should be consolidated into some common code at some point. + vol_multiattach = new_volume.get('multiattach', False) + virt_multiattach = self.driver.capabilities['supports_multiattach'] + if vol_multiattach and not virt_multiattach: + raise exception.MultiattachNotSupportedByVirtDriver( + volume_id=new_volume_id) + # This is a new style attachment and the API created the new # volume attachment and passed the id to the compute over RPC. # At this point we need to update the new volume attachment with @@ -5393,6 +5403,11 @@ class ComputeManager(manager.Manager): context, new_attachment_id, connector, mountpoint)['connection_info'] + if vol_multiattach: + # This will be used by the volume driver to determine the + # proper disk configuration. + new_cinfo['multiattach'] = True + old_cinfo = jsonutils.loads(bdm['connection_info']) if old_cinfo and 'serial' not in old_cinfo: old_cinfo['serial'] = old_volume_id @@ -5404,14 +5419,15 @@ class ComputeManager(manager.Manager): return (old_cinfo, new_cinfo) def _swap_volume(self, context, instance, bdm, connector, - old_volume_id, new_volume_id, resize_to, + old_volume_id, new_volume, resize_to, new_attachment_id, is_cinder_migration): + new_volume_id = new_volume['id'] mountpoint = bdm['device_name'] failed = False new_cinfo = None try: old_cinfo, new_cinfo = self._init_volume_connection( - context, new_volume_id, old_volume_id, connector, + context, new_volume, old_volume_id, connector, bdm, new_attachment_id, mountpoint) # NOTE(lyarwood): The Libvirt driver, the only virt driver # currently implementing swap_volume, will modify the contents of @@ -5552,7 +5568,8 @@ class ComputeManager(manager.Manager): True if old_volume['status'] in ('retyping', 'migrating') else False) old_vol_size = old_volume['size'] - new_vol_size = self.volume_api.get(context, new_volume_id)['size'] + new_volume = self.volume_api.get(context, new_volume_id) + new_vol_size = new_volume['size'] if new_vol_size > old_vol_size: resize_to = new_vol_size @@ -5564,7 +5581,7 @@ class ComputeManager(manager.Manager): bdm, connector, old_volume_id, - new_volume_id, + new_volume, resize_to, new_attachment_id, is_cinder_migration) diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index f39340117341..e6ffdfe8cb3a 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -2067,10 +2067,12 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): attachment_id=uuids.old_attachment_id, connection_info='{"data": {}}', volume_size=1) old_volume = { - 'id': uuids.old_volume_id, 'size': 1, 'status': 'retyping' + 'id': uuids.old_volume_id, 'size': 1, 'status': 'retyping', + 'multiattach': False } new_volume = { - 'id': uuids.new_volume_id, 'size': 1, 'status': 'reserved' + 'id': uuids.new_volume_id, 'size': 1, 'status': 'reserved', + 'multiattach': False } attachment_update.return_value = {"connection_info": {"data": {}}} get_bdm.return_value = bdm @@ -2138,10 +2140,12 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): attachment_id=uuids.old_attachment_id, connection_info='{"data": {}}') old_volume = { - 'id': uuids.old_volume_id, 'size': 1, 'status': 'detaching' + 'id': uuids.old_volume_id, 'size': 1, 'status': 'detaching', + 'multiattach': False } new_volume = { - 'id': uuids.new_volume_id, 'size': 2, 'status': 'reserved' + 'id': uuids.new_volume_id, 'size': 2, 'status': 'reserved', + 'multiattach': False } attachment_update.return_value = {"connection_info": {"data": {}}} get_bdm.return_value = bdm @@ -2181,8 +2185,9 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): self.assertEqual(uuids.new_volume_id, bdm.volume_id) self.assertEqual(uuids.new_attachment_id, bdm.attachment_id) self.assertEqual(2, bdm.volume_size) - self.assertEqual(uuids.new_volume_id, - jsonutils.loads(bdm.connection_info)['serial']) + new_conn_info = jsonutils.loads(bdm.connection_info) + self.assertEqual(uuids.new_volume_id, new_conn_info['serial']) + self.assertNotIn('multiattach', new_conn_info) @mock.patch.object(compute_utils, 'add_instance_fault_from_exc') @mock.patch.object(compute_utils, 'notify_about_volume_swap') @@ -2209,10 +2214,12 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): attachment_id=uuids.old_attachment_id, connection_info='{"data": {}}') old_volume = { - 'id': uuids.old_volume_id, 'size': 1, 'status': 'migrating' + 'id': uuids.old_volume_id, 'size': 1, 'status': 'migrating', + 'multiattach': False } new_volume = { - 'id': uuids.new_volume_id, 'size': 1, 'status': 'reserved' + 'id': uuids.new_volume_id, 'size': 1, 'status': 'reserved', + 'multiattach': False } get_bdm.return_value = bdm get_volume.side_effect = (old_volume, new_volume) @@ -2272,10 +2279,12 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): attachment_id=uuids.old_attachment_id, connection_info='{"data": {}}') old_volume = { - 'id': uuids.old_volume_id, 'size': 1, 'status': 'detaching' + 'id': uuids.old_volume_id, 'size': 1, 'status': 'detaching', + 'multiattach': False } new_volume = { - 'id': uuids.new_volume_id, 'size': 2, 'status': 'reserved' + 'id': uuids.new_volume_id, 'size': 2, 'status': 'reserved', + 'multiattach': False } attachment_update.return_value = {"connection_info": {"data": {}}} get_bdm.return_value = bdm @@ -2317,6 +2326,57 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): # Cinder-initiated call, we don't call migrate_volume_completion. migrate_volume_completion.assert_not_called() + @mock.patch('nova.volume.cinder.API.attachment_update') + def test_swap_volume_with_multiattach(self, attachment_update): + """Tests swap volume where the volume being swapped-to supports + multiattach as well as the compute driver, so the attachment for the + new volume (created in the API) is updated with the host connector + and the new_connection_info is updated with the multiattach flag. + """ + bdm = objects.BlockDeviceMapping( + volume_id=uuids.old_volume_id, device_name='/dev/vda', + attachment_id=uuids.old_attachment_id, + connection_info='{"data": {}}') + new_volume = { + 'id': uuids.new_volume_id, 'size': 2, 'status': 'reserved', + 'multiattach': True + } + attachment_update.return_value = {"connection_info": {"data": {}}} + connector = mock.sentinel.connector + with mock.patch.dict(self.compute.driver.capabilities, + {'supports_multiattach': True}): + _, new_cinfo = self.compute._init_volume_connection( + self.context, new_volume, uuids.old_volume_id, + connector, bdm, uuids.new_attachment_id, bdm.device_name) + self.assertEqual(uuids.new_volume_id, new_cinfo['serial']) + self.assertIn('multiattach', new_cinfo) + self.assertTrue(new_cinfo['multiattach']) + attachment_update.assert_called_once_with( + self.context, uuids.new_attachment_id, connector, + bdm.device_name) + + def test_swap_volume_with_multiattach_no_driver_support(self): + """Tests a swap volume scenario where the new volume being swapped-to + supports multiattach but the virt driver does not, so swap volume + fails. + """ + bdm = objects.BlockDeviceMapping( + volume_id=uuids.old_volume_id, device_name='/dev/vda', + attachment_id=uuids.old_attachment_id, + connection_info='{"data": {}}') + new_volume = { + 'id': uuids.new_volume_id, 'size': 2, 'status': 'reserved', + 'multiattach': True + } + connector = {'host': 'localhost'} + with mock.patch.dict(self.compute.driver.capabilities, + {'supports_multiattach': False}): + self.assertRaises(exception.MultiattachNotSupportedByVirtDriver, + self.compute._init_volume_connection, + self.context, new_volume, uuids.old_volume_id, + connector, bdm, uuids.new_attachment_id, + bdm.device_name) + @mock.patch.object(objects.BlockDeviceMappingList, 'get_by_instance_uuid') @mock.patch.object(fake_driver.FakeDriver, 'check_can_live_migrate_source')