From 92bf69bfa52f56b359cb3a6f0871f7cc4ec6bdf9 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Tue, 3 Dec 2019 12:51:50 -0500 Subject: [PATCH] Implement cleanup_instance_network_on_host for neutron API This implements the cleanup_instance_network_on_host method in the neutron API which will delete port bindings for the given instance and the given host, similar to how setup_networks_on_host works when teardown=True and the instance.host does not match the host provided to that method. This allows removing the hacks in the _confirm_snapshot_based_resize_delete_port_bindings and _revert_snapshot_based_resize_at_dest methods. Change-Id: Iff8194c868580facb1cc81b5567d66d4093c5274 --- nova/compute/manager.py | 63 ++++++++-------- nova/network/neutronv2/api.py | 84 +++++++++++++-------- nova/tests/unit/compute/test_compute_mgr.py | 54 +++++-------- nova/tests/unit/compute/test_shelve.py | 2 +- 4 files changed, 105 insertions(+), 98 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 69f202bd4b..e9ca1d5d04 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -4492,20 +4492,16 @@ class ComputeManager(manager.Manager): :param instance: Instance object that was resized/cold migrated :param migration: Migration object for the resize/cold migrate """ - # setup_networks_on_host relies on the instance.host not being the same - # as the host we pass in, so we have to mutate the instance to - # effectively trick this code. - with utils.temporary_mutation(instance, host=migration.dest_compute): - LOG.debug('Deleting port bindings for source host.', - instance=instance) - try: - self.network_api.setup_networks_on_host( - ctxt, instance, host=self.host, teardown=True) - except exception.PortBindingDeletionFailed as e: - # Do not let this stop us from cleaning up since the guest - # is already gone. - LOG.error('Failed to delete port bindings from source host. ' - 'Error: %s', six.text_type(e), instance=instance) + LOG.debug('Deleting port bindings for source host.', + instance=instance) + try: + self.network_api.cleanup_instance_network_on_host( + ctxt, instance, self.host) + except exception.PortBindingDeletionFailed as e: + # Do not let this stop us from cleaning up since the guest + # is already gone. + LOG.error('Failed to delete port bindings from source host. ' + 'Error: %s', six.text_type(e), instance=instance) def _delete_volume_attachments(self, ctxt, bdms): """Deletes volume attachment records for the given bdms. @@ -4606,22 +4602,19 @@ class ComputeManager(manager.Manager): self.network_api.migrate_instance_start( ctxt, instance, migration) - # Delete port bindings for the target host. This relies on the - # instance.host not being the same as the host we pass in, so we - # have to mutate the instance to effectively trick this code. - with utils.temporary_mutation(instance, host=migration.source_compute): - LOG.debug('Deleting port bindings for target host %s.', - self.host, instance=instance) - try: - # Note that deleting the destination host port bindings does - # not automatically activate the source host port bindings. - self.network_api.setup_networks_on_host( - ctxt, instance, host=self.host, teardown=True) - except exception.PortBindingDeletionFailed as e: - # Do not let this stop us from cleaning up since the guest - # is already gone. - LOG.error('Failed to delete port bindings from target host. ' - 'Error: %s', six.text_type(e), instance=instance) + # Delete port bindings for the target host. + LOG.debug('Deleting port bindings for target host %s.', + self.host, instance=instance) + try: + # Note that deleting the destination host port bindings does + # not automatically activate the source host port bindings. + self.network_api.cleanup_instance_network_on_host( + ctxt, instance, self.host) + except exception.PortBindingDeletionFailed as e: + # Do not let this stop us from cleaning up since the guest + # is already gone. + LOG.error('Failed to delete port bindings from target host. ' + 'Error: %s', six.text_type(e), instance=instance) # Delete any volume attachments remaining for this target host. LOG.debug('Deleting volume attachments for target host.', @@ -6425,9 +6418,13 @@ class ComputeManager(manager.Manager): self._power_off_instance(context, instance, clean_shutdown) current_power_state = self._get_power_state(context, instance) - - self.network_api.cleanup_instance_network_on_host(context, instance, - instance.host) + # NOTE(mriedem): cleanup_instance_network_on_host assumes there are + # multiple host bindings per port which is not the case with the + # shelve/unshelve flow. Doing so will result in failures to update the + # port binding on unshelve (since we would have deleted it here). + if not utils.is_neutron(): + self.network_api.cleanup_instance_network_on_host( + context, instance, instance.host) network_info = self.network_api.get_instance_nw_info(context, instance) block_device_info = self._get_instance_block_device_info(context, diff --git a/nova/network/neutronv2/api.py b/nova/network/neutronv2/api.py index 8eab0e90a9..7f3022e3a4 100644 --- a/nova/network/neutronv2/api.py +++ b/nova/network/neutronv2/api.py @@ -340,32 +340,41 @@ class API(base_api.NetworkAPI): # the current instance.host. has_binding_ext = self.supports_port_binding_extension(context) if port_migrating and has_binding_ext: - # Attempt to delete all port bindings on the host and raise - # any errors at the end. - failed_port_ids = [] - for port in ports: - # This call is safe in that 404s for non-existing - # bindings are ignored. - try: - self.delete_port_binding( - context, port['id'], host) - except exception.PortBindingDeletionFailed: - # delete_port_binding will log an error for each - # failure but since we're iterating a list we want - # to keep track of all failures to build a generic - # exception to raise - failed_port_ids.append(port['id']) - if failed_port_ids: - msg = (_("Failed to delete binding for port(s) " - "%(port_ids)s and host %(host)s.") % - {'port_ids': ','.join(failed_port_ids), - 'host': host}) - raise exception.PortBindingDeletionFailed(msg) + self._delete_port_bindings(context, ports, host) elif port_migrating: # Setup the port profile self._setup_migration_port_profile( context, instance, host, admin_client, ports) + def _delete_port_bindings(self, context, ports, host): + """Attempt to delete all port bindings on the host. + + :param context: The user request context. + :param ports: list of port dicts to cleanup; the 'id' field is required + per port dict in the list + :param host: host from which to delete port bindings + :raises: PortBindingDeletionFailed if port binding deletion fails. + """ + failed_port_ids = [] + for port in ports: + # This call is safe in that 404s for non-existing + # bindings are ignored. + try: + self.delete_port_binding( + context, port['id'], host) + except exception.PortBindingDeletionFailed: + # delete_port_binding will log an error for each + # failure but since we're iterating a list we want + # to keep track of all failures to build a generic + # exception to raise + failed_port_ids.append(port['id']) + if failed_port_ids: + msg = (_("Failed to delete binding for port(s) " + "%(port_ids)s and host %(host)s.") % + {'port_ids': ','.join(failed_port_ids), + 'host': host}) + raise exception.PortBindingDeletionFailed(msg) + def _get_available_networks(self, context, project_id, net_ids=None, neutron=None, auto_allocate=False): @@ -3319,15 +3328,30 @@ class API(base_api.NetworkAPI): context, instance, host, migration, provider_mappings) def cleanup_instance_network_on_host(self, context, instance, host): - """Cleanup network for specified instance on host.""" - # TODO(mriedem): This should likely be implemented at least for the - # shelve offload operation because the instance is being removed from - # a compute host, VIFs are unplugged, etc, so the ports should also - # be unbound, albeit still logically attached to the instance (for the - # shelve scenario). If _unbind_ports was going to be leveraged here, it - # would have to be adjusted a bit since it currently clears the - # device_id field on the port which is not what we'd want for shelve. - pass + """Cleanup network for specified instance on host. + + Port bindings for the given host are deleted. The ports associated + with the instance via the port device_id field are left intact. + + :param context: The user request context. + :param instance: Instance object with the associated ports + :param host: host from which to delete port bindings + :raises: PortBindingDeletionFailed if port binding deletion fails. + """ + # First check to see if the port binding extension is supported. + if not self.supports_port_binding_extension(context): + LOG.info("Neutron extension '%s' is not supported; not cleaning " + "up port bindings for host %s.", + constants.PORT_BINDING_EXTENDED, host, instance=instance) + return + # Now get the ports associated with the instance. We go directly to + # neutron rather than rely on the info cache just like + # setup_networks_on_host. + search_opts = {'device_id': instance.uuid, + 'tenant_id': instance.project_id, + 'fields': ['id']} # we only need the port id + data = self.list_ports(context, **search_opts) + self._delete_port_bindings(context, data['ports'], host) def _get_pci_mapping_for_migration(self, instance, migration): if not instance.migration_context: diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index e8175e2955..72aae41ae0 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -11036,19 +11036,13 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase, """Happy path test for _confirm_snapshot_based_resize_delete_port_bindings. """ - - def stub_setup_networks_on_host(ctxt, instance, *args, **kwargs): - # Make sure the instance.host was mutated to point at the dest host - self.assertEqual(self.migration.dest_compute, instance.host) - with mock.patch.object( - self.compute.network_api, 'setup_networks_on_host', - side_effect=stub_setup_networks_on_host) as setup_networks: + self.compute.network_api, + 'cleanup_instance_network_on_host') as cleanup_networks: self.compute._confirm_snapshot_based_resize_delete_port_bindings( self.context, self.instance, self.migration) - setup_networks.assert_called_once_with( - self.context, self.instance, host=self.compute.host, - teardown=True) + cleanup_networks.assert_called_once_with( + self.context, self.instance, self.compute.host) def test_confirm_snapshot_based_resize_delete_port_bindings_errors(self): """Tests error handling for @@ -11056,29 +11050,27 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase, """ # PortBindingDeletionFailed will be caught and logged. with mock.patch.object( - self.compute.network_api, 'setup_networks_on_host', + self.compute.network_api, 'cleanup_instance_network_on_host', side_effect=exception.PortBindingDeletionFailed( port_id=uuids.port_id, host=self.compute.host) - ) as setup_networks: + ) as cleanup_networks: self.compute._confirm_snapshot_based_resize_delete_port_bindings( self.context, self.instance, self.migration) - setup_networks.assert_called_once_with( - self.context, self.instance, host=self.compute.host, - teardown=True) + cleanup_networks.assert_called_once_with( + self.context, self.instance, self.compute.host) self.assertIn('Failed to delete port bindings from source host', self.stdlog.logger.output) # Anything else is re-raised. func = self.compute._confirm_snapshot_based_resize_delete_port_bindings with mock.patch.object( - self.compute.network_api, 'setup_networks_on_host', + self.compute.network_api, 'cleanup_instance_network_on_host', side_effect=test.TestingException('neutron down') - ) as setup_networks: + ) as cleanup_networks: self.assertRaises(test.TestingException, func, self.context, self.instance, self.migration) - setup_networks.assert_called_once_with( - self.context, self.instance, host=self.compute.host, - teardown=True) + cleanup_networks.assert_called_once_with( + self.context, self.instance, self.compute.host) def test_delete_volume_attachments(self): """Happy path test for _delete_volume_attachments.""" @@ -11209,15 +11201,6 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase, # at the source compute. self.assertEqual(migration.source_compute, migration.dest_compute) - def stub_setup_networks_on_host(ctxt, instance, *args, **kwargs): - # The instance.host should have been mutated to point at the - # source compute. - self.assertEqual(self.migration.source_compute, instance.host) - # Raise PortBindingDeletionFailed to make sure it's caught and - # logged but not fatal. - raise exception.PortBindingDeletionFailed(port_id=uuids.port_id, - host=self.compute.host) - with test.nested( mock.patch.object(self.compute, 'network_api'), mock.patch.object(self.compute, '_get_instance_block_device_info'), @@ -11230,8 +11213,11 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase, ): mock_network_api.migrate_instance_start.side_effect = \ stub_migrate_instance_start - mock_network_api.setup_networks_on_host.side_effect = \ - stub_setup_networks_on_host + # Raise PortBindingDeletionFailed to make sure it's caught and + # logged but not fatal. + mock_network_api.cleanup_instance_network_on_host.side_effect = \ + exception.PortBindingDeletionFailed(port_id=uuids.port_id, + host=self.compute.host) # Run the code. self.compute._revert_snapshot_based_resize_at_dest( self.context, self.instance, self.migration) @@ -11246,9 +11232,9 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase, block_device_info=mock_get_bdi.return_value) mock_network_api.migrate_instance_start.assert_called_once_with( self.context, self.instance, self.migration) - mock_network_api.setup_networks_on_host.assert_called_once_with( - self.context, self.instance, host=self.compute.host, - teardown=True) + mock_network_api.cleanup_instance_network_on_host.\ + assert_called_once_with( + self.context, self.instance, self.compute.host) # Assert that even though setup_networks_on_host raised # PortBindingDeletionFailed it was handled and logged. self.assertIn('Failed to delete port bindings from target host.', diff --git a/nova/tests/unit/compute/test_shelve.py b/nova/tests/unit/compute/test_shelve.py index a15c054a9f..4cc9118564 100644 --- a/nova/tests/unit/compute/test_shelve.py +++ b/nova/tests/unit/compute/test_shelve.py @@ -170,7 +170,7 @@ class ShelveComputeManagerTestCase(test_compute.BaseTestCase): mock_notify_instance_usage.assert_has_calls( mock_notify_instance_usage_call_list) mock_power_off.assert_has_calls(mock_power_off_call_list) - mock_cleanup.assert_has_calls(mock_cleanup_call_list) + mock_cleanup.assert_not_called() mock_snapshot.assert_called_once_with(self.context, instance, 'fake_image_id', mock.ANY) mock_get_power_state.assert_has_calls(mock_get_power_state_call_list)