diff --git a/nova/conductor/tasks/migrate.py b/nova/conductor/tasks/migrate.py index 1d644628086..0b233657873 100644 --- a/nova/conductor/tasks/migrate.py +++ b/nova/conductor/tasks/migrate.py @@ -363,10 +363,11 @@ def _execute(self): # Convert image/local roots before the BFV-only prep_resize path. # Revert keeps the instance BFV-on-VMware. if self.instance.system_metadata.get('cross_hv_resize') == 'true': - root_bdm = self._get_root_bdm() + bdms = self._get_bdms() + root_bdm = self._get_root_bdm(bdms) self._validate_cross_hv_root_bdm(root_bdm) + self._validate_cross_hv_attached_volume_types(bdms) if self._is_image_backed_local_root(root_bdm): - self._validate_cross_hv_manage_config() self._convert_image_backed_root_to_bfv(root_bdm) # NOTE: set after the conversion above, which saves the instance @@ -415,12 +416,30 @@ def _validate_cross_hv_manage_config(): raise exception.CrossHVConfigurationMissing( option='fcd_volume_type') - def _get_root_bdm(self): - """Load and return the root BDM for the instance, or None.""" - bdms = objects.BlockDeviceMappingList.get_by_instance_uuid( + def _get_bdms(self): + """Load and return all BDMs for the instance.""" + return objects.BlockDeviceMappingList.get_by_instance_uuid( self.context, self.instance.uuid) + + def _get_root_bdm(self, bdms=None): + """Load and return the root BDM for the instance, or None.""" + if bdms is None: + bdms = self._get_bdms() return bdms.root_bdm() + def _get_cross_hv_expected_volume_type(self): + """Resolve the configured FCD volume type.""" + self._validate_cross_hv_manage_config() + configured_type = CONF.cross_hv.fcd_volume_type + + for volume_type in self.volume_api.get_all_volume_types(self.context): + if configured_type in (volume_type['id'], volume_type['name']): + return volume_type + + raise exception.InvalidCrossHvResizePrecondition( + reason='Configured [cross_hv] fcd_volume_type %s was not found ' + 'in Cinder volume types.' % configured_type) + def _is_image_backed_local_root(self, root_bdm): """Return True if root_bdm is an image-backed local ephemeral disk.""" if root_bdm is None: @@ -456,6 +475,27 @@ def _validate_cross_hv_root_bdm(self, root_bdm): 'or */volume root disks are supported.' % (root_bdm.source_type, root_bdm.destination_type)) + def _validate_cross_hv_attached_volume_types(self, bdms): + """Ensure every attached Cinder volume matches the supported type.""" + expected_type = self._get_cross_hv_expected_volume_type() + expected_types = {expected_type['id'], expected_type['name']} + expected_type_name = expected_type['name'] + unsupported = [] + + for bdm in bdms: + if bdm.destination_type != 'volume' or not bdm.volume_id: + continue + volume = self.volume_api.get(self.context, bdm.volume_id) + volume_type = volume.get('volume_type_id') or '' + if volume_type not in expected_types: + unsupported.append('%s (%s)' % (bdm.volume_id, volume_type)) + + if unsupported: + raise exception.InvalidCrossHvResizePrecondition( + reason='All attached Cinder volumes must use volume type %s ' + 'for cross-HV resize. Unsupported volumes: %s' % + (expected_type_name, ', '.join(unsupported))) + def _set_cross_hv_root_detach_state(self, value): """Set or clear cross_hv_root_detach_state and save the instance. diff --git a/nova/tests/unit/conductor/tasks/test_migrate.py b/nova/tests/unit/conductor/tasks/test_migrate.py index a72bd442f3d..bd7a9faa9b5 100644 --- a/nova/tests/unit/conductor/tasks/test_migrate.py +++ b/nova/tests/unit/conductor/tasks/test_migrate.py @@ -419,10 +419,13 @@ def test_is_selected_host_in_source_cell_false(self): @mock.patch.object(scheduler_utils, 'setup_instance_group') @mock.patch.object(query.SchedulerQueryClient, 'select_destinations') @mock.patch.object(compute_rpcapi.ComputeAPI, 'prep_resize') - @mock.patch.object(migrate.MigrationTask, '_get_root_bdm') + @mock.patch.object(migrate.MigrationTask, '_get_bdms') + @mock.patch.object(migrate.MigrationTask, + '_validate_cross_hv_attached_volume_types') def test_execute_calls_sanitize_and_stashes_journal( - self, mock_get_bdm, prep_resize_mock, sel_dest_mock, sig_mock, - az_mock, gmv_mock, cm_mock, sm_mock, cn_mock, rc_mock, gbf_mock): + self, mock_validate_vol_types, mock_get_bdms, prep_resize_mock, + sel_dest_mock, sig_mock, az_mock, gmv_mock, cm_mock, sm_mock, + cn_mock, rc_mock, gbf_mock): """Verify sanitize is called for cross-HV and result stashed.""" sel_dest_mock.return_value = self.host_lists az_mock.return_value = 'myaz' @@ -436,7 +439,9 @@ def test_execute_calls_sanitize_and_stashes_journal( bfv_bdm = mock.Mock() bfv_bdm.destination_type = 'volume' bfv_bdm.source_type = 'volume' - mock_get_bdm.return_value = bfv_bdm + bdms = mock.Mock() + bdms.root_bdm.return_value = bfv_bdm + mock_get_bdms.return_value = bdms fake_journal = {'img_hv_type': 'vmware', 'hw_disk_bus': 'scsi'} self.sanitize_mock.return_value = fake_journal @@ -483,9 +488,12 @@ def set_migration_uuid(*a, **k): @mock.patch.object(scheduler_utils, 'setup_instance_group') @mock.patch.object(query.SchedulerQueryClient, 'select_destinations') @mock.patch.object(compute_rpcapi.ComputeAPI, 'prep_resize') + @mock.patch.object(migrate.MigrationTask, + '_validate_cross_hv_attached_volume_types') def test_execute_skips_sanitize_for_same_hv( - self, prep_resize_mock, sel_dest_mock, sig_mock, az_mock, - gmv_mock, cm_mock, sm_mock, cn_mock, rc_mock, gbf_mock): + self, mock_validate_vol_types, prep_resize_mock, sel_dest_mock, + sig_mock, az_mock, gmv_mock, cm_mock, sm_mock, cn_mock, rc_mock, + gbf_mock): """Verify sanitize is NOT called for same-HV resize.""" sel_dest_mock.return_value = self.host_lists az_mock.return_value = 'myaz' @@ -505,6 +513,7 @@ def set_migration_uuid(*a, **k): task.execute() self.sanitize_mock.assert_not_called() + mock_validate_vol_types.assert_not_called() self.assertEqual({}, task._old_image_properties) @@ -650,6 +659,23 @@ def setUp(self): report_client=mock.Mock(), host_list=None, network_api=mock.Mock()) + try: + cfg.CONF.cross_hv.fcd_volume_type + except (cfg.NoSuchGroupError, cfg.NoSuchOptError): + try: + cfg.CONF.register_group(cfg.OptGroup('cross_hv')) + except cfg.DuplicateOptError: + pass + try: + cfg.CONF.register_opt( + cfg.StrOpt('fcd_volume_type', default='vmware'), + group='cross_hv') + except cfg.DuplicateOptError: + pass + self.flags(fcd_volume_type='vmware', group='cross_hv') + self.task.volume_api = mock.Mock() + self.task.volume_api.get_all_volume_types.return_value = [ + {'id': uuids.vmware_type, 'name': 'vmware'}] def _make_bdm(self, source_type='image', destination_type='local', boot_index=0, volume_id=None, image_id='fake-image'): @@ -661,6 +687,15 @@ def _make_bdm(self, source_type='image', destination_type='local', image_id=image_id) return bdm + @mock.patch('nova.objects.BlockDeviceMappingList.get_by_instance_uuid') + def test_get_bdms_returns_instance_bdms(self, mock_get_bdms): + bdms = objects.BlockDeviceMappingList(objects=[self._make_bdm()]) + mock_get_bdms.return_value = bdms + + result = self.task._get_bdms() + + self.assertIs(result, bdms) + @mock.patch('nova.objects.BlockDeviceMappingList.get_by_instance_uuid') def test_get_root_bdm_returns_root(self, mock_get_bdms): root = self._make_bdm(boot_index=0) @@ -733,6 +768,77 @@ def test_validate_cross_hv_root_bdm_rejects_blank_local(self): exception.InvalidCrossHvResizePrecondition, self.task._validate_cross_hv_root_bdm, bdm) + def test_validate_volume_types_allows_expected_type(self): + root = self._make_bdm(source_type='volume', destination_type='volume', + volume_id=uuids.root_volume) + data = self._make_bdm(source_type='blank', destination_type='volume', + boot_index=1, volume_id=uuids.data_volume) + bdms = objects.BlockDeviceMappingList(objects=[root, data]) + self.task.volume_api.get.side_effect = [ + {'volume_type_id': uuids.vmware_type}, + {'volume_type_id': 'vmware'}, + ] + + self.task._validate_cross_hv_attached_volume_types(bdms) + + self.assertEqual(2, self.task.volume_api.get.call_count) + + def test_validate_volume_types_accepts_config_name_and_id(self): + root = self._make_bdm(source_type='volume', destination_type='volume', + volume_id=uuids.root_volume) + bdms = objects.BlockDeviceMappingList(objects=[root]) + self.task.volume_api.get.return_value = { + 'volume_type_id': uuids.vmware_type} + + self.task._validate_cross_hv_attached_volume_types(bdms) + + self.task.volume_api.get_all_volume_types.assert_called_once_with( + self.task.context) + + def test_validate_volume_types_rejects_bfv_root_mismatch(self): + root = self._make_bdm(source_type='volume', destination_type='volume', + volume_id=uuids.root_volume) + bdms = objects.BlockDeviceMappingList(objects=[root]) + self.task.volume_api.get.return_value = { + 'volume_type_id': uuids.premium_type} + + ex = self.assertRaises( + exception.InvalidCrossHvResizePrecondition, + self.task._validate_cross_hv_attached_volume_types, bdms) + + self.assertIn(str(uuids.root_volume), str(ex)) + self.assertIn('volume type vmware', str(ex)) + + def test_validate_volume_types_rejects_unknown_config(self): + root = self._make_bdm(source_type='volume', destination_type='volume', + volume_id=uuids.root_volume) + bdms = objects.BlockDeviceMappingList(objects=[root]) + self.flags(fcd_volume_type='missing-type', group='cross_hv') + self.task.volume_api.get_all_volume_types.return_value = [ + {'id': uuids.vmware_type, 'name': 'vmware'}] + + ex = self.assertRaises( + exception.InvalidCrossHvResizePrecondition, + self.task._validate_cross_hv_attached_volume_types, bdms) + + self.assertIn('Configured [cross_hv] fcd_volume_type missing-type', + str(ex)) + + def test_validate_cross_hv_attached_volume_types_rejects_mismatch(self): + root = self._make_bdm(source_type='image', destination_type='local') + data = self._make_bdm(source_type='blank', destination_type='volume', + boot_index=1, volume_id=uuids.data_volume) + bdms = objects.BlockDeviceMappingList(objects=[root, data]) + self.task.volume_api.get.return_value = { + 'volume_type_id': uuids.premium_type} + + ex = self.assertRaises( + exception.InvalidCrossHvResizePrecondition, + self.task._validate_cross_hv_attached_volume_types, bdms) + + self.assertIn('volume type vmware', str(ex)) + self.assertIn(str(uuids.data_volume), str(ex)) + class MigrationTaskAllocationUtils(test.NoDBTestCase): @mock.patch('nova.objects.ComputeNode.get_by_host_and_nodename') @@ -1067,6 +1173,8 @@ def setUp(self): self.mock_compute_rpcapi = mock.Mock() self.mock_volume_api = mock.Mock() + self.mock_volume_api.get_all_volume_types.return_value = [ + {'id': uuids.vmware_type, 'name': 'vmware'}] self.mock_network_api = mock.Mock() self.mock_network_api.get_requested_resource_for_instance \ .return_value = ([], objects.RequestLevelParams()) @@ -1096,7 +1204,13 @@ def _make_bdm(self, source_type, destination_type, volume_id=None): bdm.volume_id = volume_id return bdm - def _execute_with_mocks(self, root_bdm): + def _make_bdms(self, *bdms): + bdms_obj = mock.MagicMock() + bdms_obj.root_bdm.return_value = bdms[0] if bdms else None + bdms_obj.__iter__.return_value = iter(bdms) + return bdms_obj + + def _execute_with_mocks(self, bdms): """Run _execute() with all external dependencies patched.""" patches = [ mock.patch('nova.compute.utils.heal_reqspec_is_bfv'), @@ -1112,8 +1226,7 @@ def _execute_with_mocks(self, root_bdm): mock.patch.object(self.task, '_is_selected_host_in_source_cell', return_value=True), mock.patch.object(self.task, '_persist_image_properties_journal'), - mock.patch.object(self.task, '_get_root_bdm', - return_value=root_bdm), + mock.patch.object(self.task, '_get_bdms', return_value=bdms), mock.patch('nova.scheduler.utils.setup_instance_group'), mock.patch('nova.scheduler.utils.populate_retry'), mock.patch('nova.scheduler.utils.populate_filter_properties'), @@ -1129,12 +1242,13 @@ def test_image_backed_triggers_conversion_before_prep_resize( self, mock_convert): """image/local root: conversion called, then prep_resize.""" root_bdm = self._make_bdm('image', 'local') + bdms = self._make_bdms(root_bdm) call_order = [] mock_convert.side_effect = lambda _: call_order.append('convert') self.mock_compute_rpcapi.prep_resize.side_effect = ( lambda *a, **k: call_order.append('prep_resize')) - self._execute_with_mocks(root_bdm) + self._execute_with_mocks(bdms) self.assertEqual(['convert', 'prep_resize'], call_order) @@ -1143,8 +1257,11 @@ def test_image_backed_triggers_conversion_before_prep_resize( def test_bfv_root_skips_conversion(self, mock_convert): """volume-backed root: conversion skipped, prep_resize called.""" root_bdm = self._make_bdm('volume', 'volume', volume_id=uuids.vol) + bdms = self._make_bdms(root_bdm) + self.mock_volume_api.get.return_value = { + 'volume_type_id': uuids.vmware_type} - self._execute_with_mocks(root_bdm) + self._execute_with_mocks(bdms) mock_convert.assert_not_called() self.mock_compute_rpcapi.prep_resize.assert_called_once() @@ -1152,10 +1269,11 @@ def test_bfv_root_skips_conversion(self, mock_convert): def test_unsupported_root_raises_before_prep_resize(self): """blank/local root: ValidationError before prep_resize.""" root_bdm = self._make_bdm('blank', 'local') + bdms = self._make_bdms(root_bdm) self.assertRaises( exception.InvalidCrossHvResizePrecondition, - self._execute_with_mocks, root_bdm) + self._execute_with_mocks, bdms) self.mock_compute_rpcapi.prep_resize.assert_not_called() @@ -1179,8 +1297,9 @@ def test_cross_hv_cross_cell_raises_before_conversion(self): mock.patch.object(self.task, '_is_selected_host_in_source_cell', return_value=False), mock.patch.object(self.task, '_persist_image_properties_journal'), - mock.patch.object(self.task, '_get_root_bdm', - return_value=self._make_bdm('image', 'local')), + mock.patch.object(self.task, '_get_bdms', + return_value=self._make_bdms( + self._make_bdm('image', 'local'))), mock.patch('nova.scheduler.utils.setup_instance_group'), mock.patch('nova.scheduler.utils.populate_retry'), mock.patch('nova.scheduler.utils.populate_filter_properties'), @@ -1203,13 +1322,63 @@ def test_cross_hv_cross_cell_raises_before_conversion(self): def test_conversion_failure_blocks_prep_resize(self, mock_convert): """If conversion raises, prep_resize must not be called.""" root_bdm = self._make_bdm('image', 'local') + bdms = self._make_bdms(root_bdm) mock_convert.side_effect = exception.VolumeMigrationError( volume_id=uuids.vol, reason='manage failed') self.assertRaises( exception.VolumeMigrationError, - self._execute_with_mocks, root_bdm) + self._execute_with_mocks, bdms) + + self.mock_compute_rpcapi.prep_resize.assert_not_called() + + def test_mismatched_attached_volume_type_blocks_prep_resize(self): + root_bdm = self._make_bdm('volume', 'volume', volume_id=uuids.root) + data_bdm = self._make_bdm('blank', 'volume', volume_id=uuids.data) + bdms = self._make_bdms(root_bdm, data_bdm) + self.mock_volume_api.get.side_effect = [ + {'volume_type_id': uuids.vmware_type}, + {'volume_type_id': uuids.premium_type}, + ] + + ex = self.assertRaises( + exception.InvalidCrossHvResizePrecondition, + self._execute_with_mocks, bdms) + + self.assertIn('volume type vmware', str(ex)) + self.mock_compute_rpcapi.prep_resize.assert_not_called() + + @mock.patch.object(migrate.MigrationTask, + '_convert_image_backed_root_to_bfv') + def test_ephemeral_attached_type_mismatch_blocks_before_conversion( + self, mock_convert): + root_bdm = self._make_bdm('image', 'local') + data_bdm = self._make_bdm('blank', 'volume', volume_id=uuids.data) + bdms = self._make_bdms(root_bdm, data_bdm) + self.mock_volume_api.get.return_value = { + 'volume_type_id': uuids.premium_type} + + ex = self.assertRaises( + exception.InvalidCrossHvResizePrecondition, + self._execute_with_mocks, bdms) + + self.assertIn(str(uuids.data), str(ex)) + self.assertIn('volume type vmware', str(ex)) + mock_convert.assert_not_called() + self.mock_compute_rpcapi.prep_resize.assert_not_called() + + def test_mismatched_bfv_root_volume_type_blocks_prep_resize(self): + root_bdm = self._make_bdm('volume', 'volume', volume_id=uuids.root) + bdms = self._make_bdms(root_bdm) + self.mock_volume_api.get.return_value = { + 'volume_type_id': uuids.premium_type} + + ex = self.assertRaises( + exception.InvalidCrossHvResizePrecondition, + self._execute_with_mocks, bdms) + self.assertIn(str(uuids.root), str(ex)) + self.assertIn('volume type vmware', str(ex)) self.mock_compute_rpcapi.prep_resize.assert_not_called() def _prep_result(self): @@ -1227,13 +1396,14 @@ def test_rollback_clears_cross_hv_markers_when_prep_rejected(self): could mistake this instance for one still mid conversion. """ root_bdm = self._make_bdm('image', 'local') + bdms = self._make_bdms(root_bdm) self.mock_compute_rpcapi.prep_cross_hv_conversion.side_effect = ( exception.UnsupportedRPCVersion(api='compute', required='6.2.1')) self.assertRaises( exception.UnsupportedRPCVersion, - self._execute_with_mocks, root_bdm) + self._execute_with_mocks, bdms) self.assertNotIn('cross_hv_resize', self.instance.system_metadata) self.assertNotIn( @@ -1248,12 +1418,13 @@ def test_rollback_preserves_markers_when_prep_outcome_unknown(self): markers for manual recovery. """ root_bdm = self._make_bdm('image', 'local') + bdms = self._make_bdms(root_bdm) self.mock_compute_rpcapi.prep_cross_hv_conversion.side_effect = ( messaging.MessagingTimeout('timed out')) self.assertRaises( messaging.MessagingTimeout, - self._execute_with_mocks, root_bdm) + self._execute_with_mocks, bdms) self.assertEqual( 'true', self.instance.system_metadata['cross_hv_resize']) @@ -1273,6 +1444,7 @@ def test_rollback_preserves_markers_after_volume_manage_failed_no_abort( rollback() must not erase the markers. """ root_bdm = self._make_bdm('image', 'local') + bdms = self._make_bdms(root_bdm) self.mock_compute_rpcapi.prep_cross_hv_conversion.return_value = ( self._prep_result()) self.mock_volume_api.manage_existing.side_effect = ( @@ -1281,7 +1453,7 @@ def test_rollback_preserves_markers_after_volume_manage_failed_no_abort( self.assertRaises( exception.VolumeManageFailedNoAbort, - self._execute_with_mocks, root_bdm) + self._execute_with_mocks, bdms) self.assertEqual( 'true', self.instance.system_metadata['cross_hv_resize']) @@ -1302,6 +1474,7 @@ def test_rollback_clears_markers_after_successful_abort(self): and the migration context. """ root_bdm = self._make_bdm('image', 'local') + bdms = self._make_bdms(root_bdm) self.mock_compute_rpcapi.prep_cross_hv_conversion.return_value = ( self._prep_result()) self.mock_volume_api.manage_existing.side_effect = ( @@ -1310,7 +1483,7 @@ def test_rollback_clears_markers_after_successful_abort(self): self.assertRaises( exception.VolumeManageFailed, - self._execute_with_mocks, root_bdm) + self._execute_with_mocks, bdms) self.mock_compute_rpcapi.abort_cross_hv_conversion.\ assert_called_once() @@ -1325,6 +1498,7 @@ def test_rollback_preserves_markers_when_abort_itself_fails(self): so keep the markers instead of assuming the abort worked. """ root_bdm = self._make_bdm('image', 'local') + bdms = self._make_bdms(root_bdm) self.mock_compute_rpcapi.prep_cross_hv_conversion.return_value = ( self._prep_result()) self.mock_volume_api.manage_existing.side_effect = ( @@ -1335,7 +1509,7 @@ def test_rollback_preserves_markers_when_abort_itself_fails(self): self.assertRaises( messaging.MessagingTimeout, - self._execute_with_mocks, root_bdm) + self._execute_with_mocks, bdms) self.assertEqual( 'true', self.instance.system_metadata['cross_hv_resize']) @@ -1352,6 +1526,7 @@ def test_rollback_preserves_markers_when_attachment_create_fails(self): markers must survive rollback(). """ root_bdm = self._make_bdm('image', 'local') + bdms = self._make_bdms(root_bdm) self.mock_compute_rpcapi.prep_cross_hv_conversion.return_value = ( self._prep_result()) self.mock_volume_api.manage_existing.return_value = {'id': uuids.vol} @@ -1360,7 +1535,7 @@ def test_rollback_preserves_markers_when_attachment_create_fails(self): self.assertRaises( exception.NovaException, - self._execute_with_mocks, root_bdm) + self._execute_with_mocks, bdms) self.assertEqual( 'true', self.instance.system_metadata['cross_hv_resize']) @@ -1375,6 +1550,7 @@ def test_rollback_restores_image_properties_from_journal(self): is not left with VMware-only values pointed at a KVM/CH host. """ root_bdm = self._make_bdm('image', 'local') + bdms = self._make_bdms(root_bdm) self.mock_compute_rpcapi.prep_cross_hv_conversion.side_effect = ( exception.UnsupportedRPCVersion(api='compute', required='6.2.1')) @@ -1390,7 +1566,7 @@ def test_rollback_restores_image_properties_from_journal(self): ) as mock_restore: self.assertRaises( exception.UnsupportedRPCVersion, - self._execute_with_mocks, root_bdm) + self._execute_with_mocks, bdms) mock_restore.assert_called_once_with( request_spec=self.request_spec, @@ -1430,6 +1606,7 @@ def test_failed_conversion_does_not_persist_destination_az(self): """ self.instance.availability_zone = 'az-source' root_bdm = self._make_bdm('image', 'local') + bdms = self._make_bdms(root_bdm) self.mock_compute_rpcapi.prep_cross_hv_conversion.side_effect = ( exception.UnsupportedRPCVersion(api='compute', required='6.2.1')) @@ -1439,7 +1616,7 @@ def test_failed_conversion_does_not_persist_destination_az(self): self.assertRaises( exception.UnsupportedRPCVersion, - self._execute_with_mocks, root_bdm) + self._execute_with_mocks, bdms) self.assertTrue(seen, 'expected at least one instance.save()') self.assertEqual(['az-source'] * len(seen), seen)