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)