From 2d68fbe56582cebf4b0d8286e9821f863e452699 Mon Sep 17 00:00:00 2001 From: John Griffith Date: Tue, 5 Dec 2017 18:39:57 -0500 Subject: [PATCH] Add new style volume attachment support to block_device.py This plumbs in the new-style volume attachment support to the virt driver block device code which does the actual work of attaching the volume via the driver and completing the attachment with cinder (the thing that makes the volume status "in-use"). At this point, none of the new flow is exercised outside of tests because we are not yet setting the attachment_id when attaching volumes, that comes in a later change to the API. It's worth noting that when nova creates a volume during boot from volume for a source_type=blank/image/snapshot BDM, we won't have an attachment_id for that volume so we'll attach using the old flow. For now this is OK, but might be something we need to revisit in the future when we eventually want to remove the old flow. The structure here is to separate the old/new attach flows into separate methods, despite quite a bit of copy/paste code in both places. A single method using conditionals in different places is arguably uglier (as seen in the test code) and we could likely consolidate some of the common code into helper methods later. Note that test_refresh_connection_info_with_attachment_id is removed since it's now redundant with test_refresh_connection. Co-Authored-By: Matt Riedemann Part of blueprint cinder-new-attach-apis Change-Id: Ib1b6b223c9d04579828d47607006ecd98b472e5a --- nova/tests/unit/virt/test_block_device.py | 130 +++++++++++++--------- nova/virt/block_device.py | 111 ++++++++++++++++-- 2 files changed, 182 insertions(+), 59 deletions(-) diff --git a/nova/tests/unit/virt/test_block_device.py b/nova/tests/unit/virt/test_block_device.py index 4dced98e0b32..6cbc0692f790 100644 --- a/nova/tests/unit/virt/test_block_device.py +++ b/nova/tests/unit/virt/test_block_device.py @@ -32,6 +32,10 @@ from nova.volume import cinder class TestDriverBlockDevice(test.NoDBTestCase): + # This is used to signal if we're dealing with a new style volume + # attachment (Cinder v3.44 flow). + attachment_id = None + driver_classes = { 'swap': driver_block_device.DriverSwapBlockDevice, 'ephemeral': driver_block_device.DriverEphemeralBlockDevice, @@ -101,7 +105,6 @@ class TestDriverBlockDevice(test.NoDBTestCase): 'boot_index': 0}) volume_driver_bdm = { - 'attachment_id': None, 'mount_device': '/dev/sda1', 'connection_info': {"fake": "connection_info"}, 'delete_on_termination': False, @@ -130,7 +133,6 @@ class TestDriverBlockDevice(test.NoDBTestCase): 'boot_index': -1}) snapshot_driver_bdm = { - 'attachment_id': None, 'mount_device': '/dev/sda2', 'connection_info': {"fake": "connection_info"}, 'delete_on_termination': True, @@ -159,7 +161,6 @@ class TestDriverBlockDevice(test.NoDBTestCase): 'boot_index': -1}) image_driver_bdm = { - 'attachment_id': None, 'mount_device': '/dev/sda2', 'connection_info': {"fake": "connection_info"}, 'delete_on_termination': True, @@ -188,7 +189,6 @@ class TestDriverBlockDevice(test.NoDBTestCase): 'boot_index': -1}) blank_driver_bdm = { - 'attachment_id': None, 'mount_device': '/dev/sda2', 'connection_info': {"fake": "connection_info"}, 'delete_on_termination': True, @@ -222,6 +222,14 @@ class TestDriverBlockDevice(test.NoDBTestCase): self.blank_bdm = fake_block_device.fake_bdm_object( self.context, self.blank_bdm_dict) + # Set the attachment_id on our fake class variables which we have + # to do in setUp so that any attachment_id set by a subclass will + # be used properly. + for name in ('volume', 'snapshot', 'image', 'blank'): + for attr in ('%s_bdm', '%s_driver_bdm'): + bdm = getattr(self, attr % name) + bdm['attachment_id'] = self.attachment_id + def test_no_device_raises(self): for name, cls in self.driver_classes.items(): bdm = fake_block_device.fake_bdm_object( @@ -443,13 +451,20 @@ class TestDriverBlockDevice(test.NoDBTestCase): fake_volume, instance=instance).AndRaise( test.TestingException) + # The @update_db decorator will save any changes. driver_bdm._bdm_obj.save().AndReturn(None) return instance, expected_conn_info self.virt_driver.get_volume_connector(instance).AndReturn(connector) - self.volume_api.initialize_connection( - elevated_context, fake_volume['id'], - connector).AndReturn(connection_info) + if self.attachment_id is None: + self.volume_api.initialize_connection( + elevated_context, fake_volume['id'], + connector).AndReturn(connection_info) + else: + self.volume_api.attachment_update( + elevated_context, self.attachment_id, connector).AndReturn( + {'connection_info': connection_info}) + if driver_attach: encryptors.get_encryption_metadata( elevated_context, self.volume_api, fake_volume['id'], @@ -468,34 +483,52 @@ class TestDriverBlockDevice(test.NoDBTestCase): disk_bus=bdm_dict['disk_bus'], device_type=bdm_dict['device_type'], encryption=enc_data).AndRaise(test.TestingException) - self.volume_api.terminate_connection( - elevated_context, fake_volume['id'], - connector).AndReturn(None) + if self.attachment_id is None: + self.volume_api.terminate_connection( + elevated_context, fake_volume['id'], + connector).AndReturn(None) + else: + self.volume_api.attachment_delete( + elevated_context, self.attachment_id).AndReturn(None) + # The @update_db decorator will save any changes. driver_bdm._bdm_obj.save().AndReturn(None) return instance, expected_conn_info if volume_attach: + # save updates before marking the volume as in-use driver_bdm._bdm_obj.save().AndReturn(None) if not fail_volume_attach: - self.volume_api.attach(elevated_context, fake_volume['id'], - uuids.uuid, bdm_dict['device_name'], - mode=access_mode).AndReturn(None) + if self.attachment_id is None: + self.volume_api.attach(elevated_context, fake_volume['id'], + uuids.uuid, bdm_dict['device_name'], + mode=access_mode).AndReturn(None) + else: + self.volume_api.attachment_complete( + elevated_context, self.attachment_id).AndReturn(None) else: - self.volume_api.attach(elevated_context, fake_volume['id'], - uuids.uuid, bdm_dict['device_name'], - mode=access_mode).AndRaise( - test.TestingException) - if driver_attach: - self.virt_driver.detach_volume( + if self.attachment_id is None: + self.volume_api.attach(elevated_context, fake_volume['id'], + uuids.uuid, bdm_dict['device_name'], + mode=access_mode).AndRaise( + test.TestingException) + if driver_attach: + self.virt_driver.detach_volume( expected_conn_info, instance, bdm_dict['device_name'], encryption=enc_data).AndReturn(None) - self.volume_api.terminate_connection( + self.volume_api.terminate_connection( elevated_context, fake_volume['id'], connector).AndReturn(None) - self.volume_api.detach(elevated_context, - fake_volume['id']).AndReturn(None) + self.volume_api.detach(elevated_context, + fake_volume['id']).AndReturn(None) + else: + self.volume_api.attachment_complete( + elevated_context, self.attachment_id).AndRaise( + test.TestingException) + self.volume_api.attachment_delete( + elevated_context, self.attachment_id).AndReturn(None) + # The @update_db decorator will save any changes. driver_bdm._bdm_obj.save().AndReturn(None) return instance, expected_conn_info @@ -648,10 +681,16 @@ class TestDriverBlockDevice(test.NoDBTestCase): self.mox.StubOutWithMock(test_bdm._bdm_obj, 'save') - self.virt_driver.get_volume_connector(instance).AndReturn(connector) - self.volume_api.initialize_connection( - self.context, test_bdm.volume_id, - connector).AndReturn(connection_info) + if self.attachment_id is None: + self.virt_driver.get_volume_connector(instance).AndReturn( + connector) + self.volume_api.initialize_connection( + self.context, test_bdm.volume_id, + connector).AndReturn(connection_info) + else: + self.volume_api.attachment_get( + self.context, self.attachment_id).AndReturn( + {'connection_info': connection_info}) test_bdm._bdm_obj.save().AndReturn(None) self.mox.ReplayAll() @@ -661,36 +700,14 @@ class TestDriverBlockDevice(test.NoDBTestCase): self.assertThat(test_bdm['connection_info'], matchers.DictMatches(expected_conn_info)) - def test_refresh_connection_info_with_attachment_id(self): - """Tests refreshing connection info when the DriverVolumeBlockDevice - has a new style attachment_id set, which is a call to attachment_get - rather than initialize_connection. - """ - test_bdm = self.driver_classes['volume'](self.volume_bdm) - test_bdm['attachment_id'] = uuids.attachment_id - connection_info = {'data': {'multipath_id': 'fake_multipath_id'}} - fake_attachment = dict(connection_info=connection_info) - expected_conn_info = {'data': {'multipath_id': 'fake_multipath_id'}, - 'serial': self.volume_bdm.volume_id} - - with mock.patch('nova.volume.cinder.API') as volume_api: - with mock.patch.object(test_bdm, 'save') as bdm_save: - volume_api.attachment_get.return_value = fake_attachment - test_bdm.refresh_connection_info( - self.context, mock.sentinel.instance, volume_api, - mock.sentinel.virt_driver) - - volume_api.attachment_get.assert_called_once_with( - self.context, uuids.attachment_id) - self.assertDictEqual(expected_conn_info, test_bdm['connection_info']) - bdm_save.assert_called_once_with() - def test_snapshot_attach_no_volume(self): no_volume_snapshot = self.snapshot_bdm_dict.copy() no_volume_snapshot['volume_id'] = None test_bdm = self.driver_classes['snapshot']( fake_block_device.fake_bdm_object( self.context, no_volume_snapshot)) + # When we create a volume, we attach it using the old flow. + self.attachment_id = None snapshot = {'id': 'fake-volume-id-1', 'attach_status': 'detached'} @@ -721,6 +738,8 @@ class TestDriverBlockDevice(test.NoDBTestCase): test_bdm = self.driver_classes['snapshot']( fake_block_device.fake_bdm_object( self.context, no_volume_snapshot)) + # When we create a volume, we attach it using the old flow. + self.attachment_id = None snapshot = {'id': 'fake-volume-id-1', 'attach_status': 'detached'} @@ -809,6 +828,8 @@ class TestDriverBlockDevice(test.NoDBTestCase): test_bdm = self.driver_classes['image']( fake_block_device.fake_bdm_object( self.context, no_volume_image)) + # When we create a volume, we attach it using the old flow. + self.attachment_id = None image = {'id': 'fake-image-id-1'} volume = {'id': 'fake-volume-id-2', @@ -836,6 +857,8 @@ class TestDriverBlockDevice(test.NoDBTestCase): test_bdm = self.driver_classes['image']( fake_block_device.fake_bdm_object( self.context, no_volume_image)) + # When we create a volume, we attach it using the old flow. + self.attachment_id = None image = {'id': 'fake-image-id-1'} volume = {'id': 'fake-volume-id-2', @@ -1131,3 +1154,10 @@ class TestDriverBlockDevice(test.NoDBTestCase): # can't assert_not_called if the method isn't in the spec. self.assertFalse(hasattr(test_eph, 'refresh_connection_info')) self.assertFalse(hasattr(test_swap, 'refresh_connection_info')) + + +class TestDriverBlockDeviceNewFlow(TestDriverBlockDevice): + """Virt block_device tests for the Cinder 3.44 volume attach flow + where a volume BDM has an attachment_id. + """ + attachment_id = uuids.attachment_id diff --git a/nova/virt/block_device.py b/nova/virt/block_device.py index 12e275406fe7..c432988b9e63 100644 --- a/nova/virt/block_device.py +++ b/nova/virt/block_device.py @@ -358,17 +358,11 @@ class DriverVolumeBlockDevice(DriverBlockDevice): else: volume_api.attachment_delete(context, self['attachment_id']) - @update_db - def attach(self, context, instance, volume_api, virt_driver, - do_driver_attach=False, **kwargs): - volume = volume_api.get(context, self.volume_id) - volume_api.check_availability_zone(context, volume, - instance=instance) - + def _legacy_volume_attach(self, context, volume, connector, instance, + volume_api, virt_driver, + do_driver_attach=False): volume_id = volume['id'] - context = context.elevated() - connector = virt_driver.get_volume_connector(instance) connection_info = volume_api.initialize_connection(context, volume_id, connector) @@ -434,6 +428,96 @@ class DriverVolumeBlockDevice(DriverBlockDevice): # happen, the detach request will be ignored. volume_api.detach(context, volume_id) + def _volume_attach(self, context, volume, connector, instance, + volume_api, virt_driver, attachment_id, + do_driver_attach=False): + # This is where we actually (finally) make a call down to the device + # driver and actually create/establish the connection. We'll go from + # here to block driver-->os-brick and back up. + + volume_id = volume['id'] + if self.volume_size is None: + self.volume_size = volume.get('size') + + LOG.debug("Updating existing volume attachment record: %s", + attachment_id, instance=instance) + connection_info = volume_api.attachment_update( + context, attachment_id, connector)['connection_info'] + if 'serial' not in connection_info: + connection_info['serial'] = self.volume_id + self._preserve_multipath_id(connection_info) + + if do_driver_attach: + encryption = encryptors.get_encryption_metadata( + context, volume_api, volume_id, connection_info) + + try: + virt_driver.attach_volume( + context, connection_info, instance, + self['mount_device'], disk_bus=self['disk_bus'], + device_type=self['device_type'], encryption=encryption) + except Exception: + with excutils.save_and_reraise_exception(): + LOG.exception("Driver failed to attach volume " + "%(volume_id)s at %(mountpoint)s", + {'volume_id': volume_id, + 'mountpoint': self['mount_device']}, + instance=instance) + volume_api.attachment_delete(context, + attachment_id) + + # NOTE(mriedem): save our current state so connection_info is in + # the database before the volume status goes to 'in-use' because + # after that we can detach and connection_info is required for + # detach. + # TODO(mriedem): Technically for the new flow, we shouldn't have to + # rely on the BlockDeviceMapping.connection_info since it's stored + # with the attachment in Cinder (see refresh_connection_info). + # Therefore we should phase out code that relies on the + # BDM.connection_info and get it from Cinder if it's needed. + self['connection_info'] = connection_info + self.save() + + try: + # This marks the volume as "in-use". + volume_api.attachment_complete(context, attachment_id) + except Exception: + with excutils.save_and_reraise_exception(): + if do_driver_attach: + # Disconnect the volume from the host. + try: + virt_driver.detach_volume(connection_info, + instance, + self['mount_device'], + encryption=encryption) + except Exception: + LOG.warning("Driver failed to detach volume " + "%(volume_id)s at %(mount_point)s.", + {'volume_id': volume_id, + 'mount_point': self['mount_device']}, + exc_info=True, instance=instance) + # Delete the attachment to mark the volume as "available". + volume_api.attachment_delete(context, self['attachment_id']) + + @update_db + def attach(self, context, instance, volume_api, virt_driver, + do_driver_attach=False, **kwargs): + volume = volume_api.get(context, self.volume_id) + volume_api.check_availability_zone(context, volume, + instance=instance) + context = context.elevated() + connector = virt_driver.get_volume_connector(instance) + + if not self['attachment_id']: + self._legacy_volume_attach(context, volume, connector, instance, + volume_api, virt_driver, + do_driver_attach) + else: + self._volume_attach(context, volume, connector, instance, + volume_api, virt_driver, + self['attachment_id'], + do_driver_attach) + @update_db def refresh_connection_info(self, context, instance, volume_api, virt_driver): @@ -502,6 +586,9 @@ class DriverSnapshotBlockDevice(DriverVolumeBlockDevice): self.volume_id = vol['id'] + # TODO(mriedem): Create an attachment to reserve the volume and + # make us go down the new-style attach flow. + # Call the volume attach now super(DriverSnapshotBlockDevice, self).attach( context, instance, volume_api, virt_driver) @@ -524,6 +611,9 @@ class DriverImageBlockDevice(DriverVolumeBlockDevice): self.volume_id = vol['id'] + # TODO(mriedem): Create an attachment to reserve the volume and + # make us go down the new-style attach flow. + super(DriverImageBlockDevice, self).attach( context, instance, volume_api, virt_driver) @@ -545,6 +635,9 @@ class DriverBlankBlockDevice(DriverVolumeBlockDevice): self.volume_id = vol['id'] + # TODO(mriedem): Create an attachment to reserve the volume and + # make us go down the new-style attach flow. + super(DriverBlankBlockDevice, self).attach( context, instance, volume_api, virt_driver)