diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 9f35a4064518..beec08f96b38 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -6647,33 +6647,19 @@ class ComputeManager(manager.Manager): if migrate_data and migrate_data.obj_attr_is_set('migration'): migrate_data.migration.status = 'completed' migrate_data.migration.save() - migration = migrate_data.migration - rc = self.scheduler_client.reportclient - # Check to see if our migration has its own allocations - allocs = rc.get_allocations_for_consumer(ctxt, migration.uuid) - else: - # We didn't have data on a migration, which means we can't - # look up to see if we had new-style migration-based - # allocations. This should really only happen in cases of - # a buggy virt driver or some really old component in the - # system. Log a warning so we know it happened. - allocs = None - LOG.warning('Live migration ended with no migrate_data ' - 'record. Unable to clean up migration-based ' - 'allocations which is almost certainly not ' - 'an expected situation.') - - if allocs: - # We had a migration-based allocation that we need to handle self._delete_allocation_after_move(ctxt, instance, migrate_data.migration) else: - # No migration-based allocations, so do the old thing and - # attempt to clean up any doubled per-instance allocation - rt = self._get_resource_tracker() - rt.delete_allocation_for_migrated_instance( - ctxt, instance, source_node) + # We didn't have data on a migration, which means we can't + # look up to see if we had new-style migration-based + # allocations. This should really only happen in cases of + # a buggy virt driver. Log a warning so we know it happened. + LOG.warning('Live migration ended with no migrate_data ' + 'record. Unable to clean up migration-based ' + 'allocations for node %s which is almost certainly ' + 'not an expected situation.', source_node, + instance=instance) def _consoles_enabled(self): """Returns whether a console is enable.""" @@ -6805,8 +6791,8 @@ class ComputeManager(manager.Manager): if migration: # Remove allocations created in Placement for the dest node. - # If migration is None, we must be so old we don't have placement, - # so no need to do something else. + # If migration is None, the virt driver didn't pass it which is + # a bug. self._revert_allocation(context, instance, migration) else: LOG.error('Unable to revert allocations during live migration ' diff --git a/nova/conductor/tasks/live_migrate.py b/nova/conductor/tasks/live_migrate.py index acb29b7095a2..f14a7a07dcc1 100644 --- a/nova/conductor/tasks/live_migrate.py +++ b/nova/conductor/tasks/live_migrate.py @@ -30,12 +30,6 @@ LOG = logging.getLogger(__name__) CONF = nova.conf.CONF -def should_do_migration_allocation(context): - minver = objects.Service.get_minimum_version_multi(context, - ['nova-compute']) - return minver >= 25 - - def supports_extended_port_binding(context, host): """Checks if the compute host service is new enough to support the neutron port binding-extended details. @@ -73,15 +67,14 @@ class LiveMigrationTask(base.TaskBase): self._check_instance_is_active() self._check_host_is_up(self.source) - if should_do_migration_allocation(self.context): - self._source_cn, self._held_allocations = ( - # NOTE(danms): This may raise various exceptions, which will - # propagate to the API and cause a 500. This is what we - # want, as it would indicate internal data structure corruption - # (such as missing migrations, compute nodes, etc). - migrate.replace_allocation_with_migration(self.context, - self.instance, - self.migration)) + self._source_cn, self._held_allocations = ( + # NOTE(danms): This may raise various exceptions, which will + # propagate to the API and cause a 500. This is what we + # want, as it would indicate internal data structure corruption + # (such as missing migrations, compute nodes, etc). + migrate.replace_allocation_with_migration(self.context, + self.instance, + self.migration)) if not self.destination: # Either no host was specified in the API request and the user diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 869bef3802b6..0ebdfd5d76a0 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -6256,8 +6256,6 @@ class ComputeTestCase(BaseTestCase, migrate_data=test.MatchType( migrate_data_obj.XenapiLiveMigrateData)) - @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - 'remove_provider_tree_from_instance_allocation') @mock.patch.object(compute_rpcapi.ComputeAPI, 'pre_live_migration') @mock.patch.object(compute_rpcapi.ComputeAPI, 'post_live_migration_at_destination') @@ -6266,7 +6264,7 @@ class ComputeTestCase(BaseTestCase, @mock.patch.object(compute_utils, 'EventReporter') @mock.patch('nova.objects.Migration.save') def test_live_migration_works_correctly(self, mock_save, mock_event, - mock_clear, mock_post, mock_pre, mock_remove_allocs): + mock_clear, mock_post, mock_pre): # Confirm live_migration() works as expected correctly. # creating instance testdata c = context.get_admin_context() @@ -6313,8 +6311,6 @@ class ComputeTestCase(BaseTestCase, 'host'], 'dest_compute': dest}) mock_post.assert_called_once_with(c, instance, False, dest) mock_clear.assert_called_once_with(mock.ANY) - mock_remove_allocs.assert_called_once_with( - c, instance.uuid, self.rt.compute_nodes[NODENAME].uuid) @mock.patch.object(compute_rpcapi.ComputeAPI, 'pre_live_migration') @mock.patch.object(compute_rpcapi.ComputeAPI, @@ -6361,15 +6357,13 @@ class ComputeTestCase(BaseTestCase, # cleanup instance.destroy() - @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - 'remove_provider_tree_from_instance_allocation') @mock.patch.object(fake.FakeDriver, 'unfilter_instance') @mock.patch.object(compute_rpcapi.ComputeAPI, 'post_live_migration_at_destination') @mock.patch.object(compute_manager.InstanceEvents, 'clear_events_for_instance') def test_post_live_migration_no_shared_storage_working_correctly(self, - mock_clear, mock_post, mock_unfilter, mock_remove_allocs): + mock_clear, mock_post, mock_unfilter): """Confirm post_live_migration() works correctly as expected for non shared storage migration. """ @@ -6423,14 +6417,9 @@ class ComputeTestCase(BaseTestCase, mock_migrate.assert_called_once_with(c, instance, migration) mock_post.assert_called_once_with(c, instance, False, dest) mock_clear.assert_called_once_with(mock.ANY) - mock_remove_allocs.assert_called_once_with( - c, instance.uuid, self.rt.compute_nodes[NODENAME].uuid) - @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' - 'remove_provider_tree_from_instance_allocation') @mock.patch('nova.compute.utils.notify_about_instance_action') - def test_post_live_migration_working_correctly(self, mock_notify, - mock_remove_allocs): + def test_post_live_migration_working_correctly(self, mock_notify): # Confirm post_live_migration() works as expected correctly. dest = 'desthost' srchost = self.compute.host @@ -6505,8 +6494,6 @@ class ComputeTestCase(BaseTestCase, self.assertIn( 'Migrating instance to desthost finished successfully.', self.stdlog.logger.output) - mock_remove_allocs.assert_called_once_with( - c, instance.uuid, self.rt.compute_nodes[NODENAME].uuid) def test_post_live_migration_exc_on_dest_works_correctly(self): """Confirm that post_live_migration() completes successfully diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index ff687326d459..53dc820877fd 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -7896,55 +7896,6 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase): self.instance, migration) - def test_post_live_migration_old_allocations(self): - # We have a migrate_data with a migration... - migration = objects.Migration(uuid=uuids.migration) - migration.save = mock.MagicMock() - md = objects.LibvirtLiveMigrateData(migration=migration, - is_shared_instance_path=False, - is_shared_block_storage=False) - with test.nested( - mock.patch.object(self.compute.scheduler_client, - 'reportclient'), - mock.patch.object(self.compute, - '_delete_allocation_after_move'), - mock.patch.object(self.compute, - '_get_resource_tracker'), - ) as ( - mock_report, mock_delete, mock_rt, - ): - # ...and that migration does not have allocations... - mock_report.get_allocations_for_consumer.return_value = None - self._call_post_live_migration(migrate_data=md) - # ...so we should have called the old style delete - mock_delete.assert_not_called() - fn = mock_rt.return_value.delete_allocation_for_migrated_instance - fn.assert_called_once_with(self.context, self.instance, - self.instance.node) - - def test_post_live_migration_legacy(self): - # We have no migrate_data... - md = None - with test.nested( - mock.patch.object(self.compute.scheduler_client, - 'reportclient'), - mock.patch.object(self.compute, - '_delete_allocation_after_move'), - mock.patch.object(self.compute, - '_get_resource_tracker'), - ) as ( - mock_report, mock_delete, mock_rt, - ): - self._call_post_live_migration(migrate_data=md) - # ...without migrate_data, no migration allocations check... - ga = mock_report.get_allocations_for_consumer - self.assertFalse(ga.called) - # ...so we should have called the old style delete - mock_delete.assert_not_called() - fn = mock_rt.return_value.delete_allocation_for_migrated_instance - fn.assert_called_once_with(self.context, self.instance, - self.instance.node) - def test_post_live_migration_cinder_v3_api(self): # Because live migration has succeeded, _post_live_migration # should call attachment_delete with the original/old attachment_id diff --git a/nova/tests/unit/conductor/tasks/test_live_migrate.py b/nova/tests/unit/conductor/tasks/test_live_migrate.py index 22d0da0a090a..b2b11504b133 100644 --- a/nova/tests/unit/conductor/tasks/test_live_migrate.py +++ b/nova/tests/unit/conductor/tasks/test_live_migrate.py @@ -74,7 +74,7 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase): servicegroup.API(), scheduler_client.SchedulerClient(), self.fake_spec) - def test_execute_with_destination(self, new_mode=True): + def test_execute_with_destination(self): dest_node = objects.ComputeNode(hypervisor_hostname='dest_node') with test.nested( mock.patch.object(self.task, '_check_host_is_up'), @@ -87,21 +87,15 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase): mock.patch.object(self.task.compute_rpcapi, 'live_migration'), mock.patch('nova.conductor.tasks.migrate.' 'replace_allocation_with_migration'), - mock.patch('nova.conductor.tasks.live_migrate.' - 'should_do_migration_allocation') ) as (mock_check_up, mock_check_dest, mock_claim, mock_save, mock_mig, - m_alloc, mock_sda): + m_alloc): mock_mig.return_value = "bob" m_alloc.return_value = (mock.MagicMock(), mock.sentinel.allocs) - mock_sda.return_value = new_mode self.assertEqual("bob", self.task.execute()) mock_check_up.assert_called_once_with(self.instance_host) mock_check_dest.assert_called_once_with() - if new_mode: - allocs = mock.sentinel.allocs - else: - allocs = None + allocs = mock.sentinel.allocs mock_claim.assert_called_once_with( self.context, self.task.scheduler_client.reportclient, self.instance, mock.sentinel.source_node, dest_node, @@ -121,12 +115,9 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase): self.migration.dest_node) self.assertEqual(self.task.destination, self.migration.dest_compute) - if new_mode: - m_alloc.assert_called_once_with(self.context, - self.instance, - self.migration) - else: - m_alloc.assert_not_called() + m_alloc.assert_called_once_with(self.context, + self.instance, + self.migration) # When the task is executed with a destination it means the host is # being forced and we don't call the scheduler, so we don't need to # heal the request spec. @@ -137,9 +128,6 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase): # modify the request spec self.ensure_network_metadata_mock.assert_not_called() - def test_execute_with_destination_old_school(self): - self.test_execute_with_destination(new_mode=False) - def test_execute_without_destination(self): self.destination = None self._generate_task() @@ -152,14 +140,10 @@ class LiveMigrationTaskTestCase(test.NoDBTestCase): mock.patch.object(self.migration, 'save'), mock.patch('nova.conductor.tasks.migrate.' 'replace_allocation_with_migration'), - mock.patch('nova.conductor.tasks.live_migrate.' - 'should_do_migration_allocation'), - ) as (mock_check, mock_find, mock_mig, mock_save, mock_alloc, - mock_sda): + ) as (mock_check, mock_find, mock_mig, mock_save, mock_alloc): mock_find.return_value = ("found_host", "found_node") mock_mig.return_value = "bob" mock_alloc.return_value = (mock.MagicMock(), mock.MagicMock()) - mock_sda.return_value = True self.assertEqual("bob", self.task.execute()) mock_check.assert_called_once_with(self.instance_host)