From b3e14931d6aac6ee5776ce1e6974c75a5a6b1823 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 5 Jun 2019 16:39:45 +0100 Subject: [PATCH] Unplug VIFs as part of cleanup of networks If an instance fails to build, which is possible for a variety of reasons, we may end up in a situation where we have remnants of a plugged VIF (typically files) left on the host. This is because we cleanup from the neutron perspective but don't attempt to unplug the VIF, a call which may have many side-effects depending on the VIF driver. Resolve this by always attempting to unplug VIFs as part of the network cleanup. A now invalid note is also removed and a unit test corrected. Closes-Bug: #1831771 Related-Bug: #1830081 Signed-off-by: Stephen Finucane Change-Id: Ibdbde4ed460a99b0cbe0d6b76e0e5b3c0650f9d9 --- nova/compute/manager.py | 44 ++++++++++ .../regressions/test_bug_1831771.py | 4 +- nova/tests/functional/test_compute_mgr.py | 12 ++- nova/tests/unit/compute/test_compute_mgr.py | 84 ++++++++++++++----- nova/virt/driver.py | 3 - 5 files changed, 118 insertions(+), 29 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 6c4a2b93e401..77e0b5af2a8d 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -2626,6 +2626,50 @@ class ComputeManager(manager.Manager): def _cleanup_allocated_networks(self, context, instance, requested_networks): + """Cleanup networks allocated for instance. + + :param context: nova request context + :param instance: nova.objects.instance.Instance object + :param requested_networks: nova.objects.NetworkRequestList + """ + LOG.debug('Unplugging VIFs for instance', instance=instance) + + network_info = instance.get_network_info() + + # NOTE(stephenfin) to avoid nova destroying the instance without + # unplugging the interface, refresh network_info if it is empty. + if not network_info: + try: + network_info = self.network_api.get_instance_nw_info( + context, instance, + ) + except Exception as exc: + LOG.warning( + 'Failed to update network info cache when cleaning up ' + 'allocated networks. Stale VIFs may be left on this host.' + 'Error: %s', six.text_type(exc) + ) + return + + try: + self.driver.unplug_vifs(instance, network_info) + except NotImplementedError: + # This is an optional method so ignore things if it doesn't exist + LOG.debug( + 'Virt driver does not provide unplug_vifs method, so it ' + 'is not possible determine if VIFs should be unplugged.' + ) + except exception.NovaException as exc: + # It's possible that the instance never got as far as plugging + # VIFs, in which case we would see an exception which can be + # mostly ignored + LOG.warning( + 'Cleaning up VIFs failed for instance. Error: %s', + six.text_type(exc), instance=instance, + ) + else: + LOG.debug('Unplugged VIFs for instance', instance=instance) + try: self._deallocate_network(context, instance, requested_networks) except Exception: diff --git a/nova/tests/functional/regressions/test_bug_1831771.py b/nova/tests/functional/regressions/test_bug_1831771.py index c710410a2121..67056a395b7a 100644 --- a/nova/tests/functional/regressions/test_bug_1831771.py +++ b/nova/tests/functional/regressions/test_bug_1831771.py @@ -94,8 +94,6 @@ class TestDelete(integrated_helpers.ProviderUsageBaseTestCase): # assert that we spawned the instance, and unplugged vifs on # cleanup mock_spawn.assert_called() - # FIXME(mdbooth): We should have called unplug_vifs in cleanup, but - # we didn't due to bug 1831771 - mock_unplug_vifs.assert_not_called() + mock_unplug_vifs.assert_called() # FIXME(mdbooth): Bug 1848666 self.assertTrue(active_after_deleting_error[0]) diff --git a/nova/tests/functional/test_compute_mgr.py b/nova/tests/functional/test_compute_mgr.py index 75a6e9f5970a..151980ab888f 100644 --- a/nova/tests/functional/test_compute_mgr.py +++ b/nova/tests/functional/test_compute_mgr.py @@ -10,12 +10,16 @@ # License for the specific language governing permissions and limitations # under the License. +from __future__ import absolute_import + +import fixtures import mock from nova import context +from nova.network import model as network_model from nova import objects from nova import test -from nova.tests import fixtures +from nova.tests import fixtures as nova_fixtures from nova.tests.unit import cast_as_call from nova.tests.unit import fake_network from nova.tests.unit import fake_server_actions @@ -24,7 +28,7 @@ from nova.tests.unit import fake_server_actions class ComputeManagerTestCase(test.TestCase): def setUp(self): super(ComputeManagerTestCase, self).setUp() - self.useFixture(fixtures.SpawnIsSynchronousFixture()) + self.useFixture(nova_fixtures.SpawnIsSynchronousFixture()) self.useFixture(cast_as_call.CastAsCall(self)) self.conductor = self.start_service('conductor') self.start_service('scheduler') @@ -32,6 +36,10 @@ class ComputeManagerTestCase(test.TestCase): self.context = context.RequestContext('fake', 'fake') fake_server_actions.stub_out_action_events(self) fake_network.set_stub_network_methods(self) + self.useFixture(fixtures.MockPatch( + 'nova.network.neutron.API.get_instance_nw_info', + return_value=network_model.NetworkInfo(), + )) def test_instance_fault_message_no_traceback_with_retry(self): """This test simulates a spawn failure on the last retry attempt. diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index abe248ff6a4a..2ca25849f765 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -6122,7 +6122,11 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase): mock_build_run.side_effect = exception.RescheduledException(reason='', instance_uuid=self.instance.uuid) - self.compute.build_and_run_instance(self.context, self.instance, + with mock.patch.object( + self.compute.network_api, 'get_instance_nw_info', + ): + self.compute.build_and_run_instance( + self.context, self.instance, self.image, request_spec={}, filter_properties=self.filter_properties, injected_files=self.injected_files, @@ -6206,15 +6210,19 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase): mock_build_and_run.side_effect = exception.RescheduledException( reason='', instance_uuid=self.instance.uuid) - self.compute._do_build_and_run_instance(self.context, instance, - self.image, request_spec={}, - filter_properties=self.filter_properties, - injected_files=self.injected_files, - admin_password=self.admin_pass, - requested_networks=self.requested_networks, - security_groups=self.security_groups, - block_device_mapping=self.block_device_mapping, node=self.node, - limits=self.limits, host_list=fake_host_list) + with mock.patch.object( + self.compute.network_api, 'get_instance_nw_info', + ): + self.compute._do_build_and_run_instance( + self.context, instance, + self.image, request_spec={}, + filter_properties=self.filter_properties, + injected_files=self.injected_files, + admin_password=self.admin_pass, + requested_networks=self.requested_networks, + security_groups=self.security_groups, + block_device_mapping=self.block_device_mapping, node=self.node, + limits=self.limits, host_list=fake_host_list) mock_build_and_run.assert_called_once_with(self.context, instance, @@ -6836,7 +6844,11 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase): mock_claim.side_effect = exc self._do_build_instance_update(mock_save, reschedule_update=True) - self.compute.build_and_run_instance(self.context, self.instance, + with mock.patch.object( + self.compute.network_api, 'get_instance_nw_info', + ): + self.compute.build_and_run_instance( + self.context, self.instance, self.image, request_spec={}, filter_properties=self.filter_properties, injected_files=self.injected_files, @@ -7321,18 +7333,48 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase): mock_setup.assert_called_once_with(self.context, instance, instance.host) - def test_cleanup_allocated_networks_instance_not_found(self): + def test__cleanup_allocated_networks__instance_not_found(self): with test.nested( - mock.patch.object(self.compute, '_deallocate_network'), - mock.patch.object(self.instance, 'save', - side_effect=exception.InstanceNotFound(instance_id='')) - ) as (_deallocate_network, save): + mock.patch.object(self.compute.network_api, + 'get_instance_nw_info'), + mock.patch.object(self.compute.driver, 'unplug_vifs'), + mock.patch.object(self.compute, '_deallocate_network'), + mock.patch.object(self.instance, 'save', + side_effect=exception.InstanceNotFound(instance_id='')) + ) as (mock_nwinfo, mock_unplug, mock_deallocate_network, mock_save): # Testing that this doesn't raise an exception - self.compute._cleanup_allocated_networks(self.context, - self.instance, self.requested_networks) - save.assert_called_once_with() - self.assertEqual('False', - self.instance.system_metadata['network_allocated']) + self.compute._cleanup_allocated_networks( + self.context, self.instance, self.requested_networks) + + mock_nwinfo.assert_called_once_with( + self.context, self.instance) + mock_unplug.assert_called_once_with( + self.instance, mock_nwinfo.return_value) + mock_deallocate_network.assert_called_once_with( + self.context, self.instance, self.requested_networks) + mock_save.assert_called_once_with() + self.assertEqual( + 'False', self.instance.system_metadata['network_allocated']) + + @mock.patch('nova.compute.manager.LOG') + def test__cleanup_allocated_networks__error(self, mock_log): + with test.nested( + mock.patch.object( + self.compute.network_api, 'get_instance_nw_info', + side_effect=Exception('some neutron error') + ), + mock.patch.object(self.compute.driver, 'unplug_vifs'), + ) as (mock_nwinfo, mock_unplug): + self.compute._cleanup_allocated_networks( + self.context, self.instance, self.requested_networks) + + mock_nwinfo.assert_called_once_with(self.context, self.instance) + self.assertEqual(1, mock_log.warning.call_count) + self.assertIn( + 'Failed to update network info cache', + mock_log.warning.call_args[0][0], + ) + mock_unplug.assert_not_called() def test_deallocate_network_none_requested(self): # Tests that we don't deallocate networks if 'none' were diff --git a/nova/virt/driver.py b/nova/virt/driver.py index f8f57ecab88b..177a0d4d0f0e 100644 --- a/nova/virt/driver.py +++ b/nova/virt/driver.py @@ -1442,9 +1442,6 @@ class ComputeDriver(object): raise NotImplementedError() def unplug_vifs(self, instance, network_info): - # NOTE(markus_z): 2015-08-18 - # The compute manager doesn't use this interface, which seems odd - # since the manager should be the controlling thing here. """Unplug virtual interfaces (VIFs) from networks. The counter action is :func:`plug_vifs`.