From 355b6da96a7fd8f6384766f8900ce9c37ac6811e Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Fri, 19 Oct 2018 18:01:52 -0400 Subject: [PATCH] Drop legacy live migrate allocation compat code This completes the legacy migration-based allocation compat cleanup from I0851e2d54a1fdc82fe3291fb7e286e790f121e92. That change handled the cold migrate flows. This completes the related cleanup for the live migrate flows. The compat being removed here was added in change I9068a5a5b47cef565802a6d58f37777464644100 in Queens. Change-Id: I8e47cac8bab50a086b98f37c2f9f659b10009cf1 --- nova/compute/manager.py | 36 +++++--------- nova/conductor/tasks/live_migrate.py | 23 +++------ nova/tests/unit/compute/test_compute.py | 19 ++----- nova/tests/unit/compute/test_compute_mgr.py | 49 ------------------- .../unit/conductor/tasks/test_live_migrate.py | 30 +++--------- 5 files changed, 29 insertions(+), 128 deletions(-) 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)