diff --git a/nova/compute/manager.py b/nova/compute/manager.py index bc0eff7204d8..aa816afea70b 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -2514,6 +2514,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 df961f371df8..cf168a541559 100644 --- a/nova/tests/functional/regressions/test_bug_1831771.py +++ b/nova/tests/functional/regressions/test_bug_1831771.py @@ -97,8 +97,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 84ec634a15e9..ee9c2e2dabbd 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.base_api.NetworkAPI.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 00c38b82d461..64d501a82dea 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -94,8 +94,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): @mock.patch.object(manager.ComputeManager, '_sync_instance_power_state') @mock.patch.object(objects.Instance, 'get_by_uuid') @mock.patch.object(objects.Migration, 'get_by_instance_and_status') - @mock.patch.object(nova.network.neutronv2.api.API, - 'migrate_instance_start') + @mock.patch('nova.network.neutronv2.api.API.migrate_instance_start') def _test_handle_lifecycle_event(self, migrate_instance_start, mock_get_migration, mock_get, mock_sync, mock_get_power_state, @@ -5567,9 +5566,14 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase): mock_build_run.side_effect = exception.RescheduledException(reason='', instance_uuid=self.instance.uuid) - with mock.patch.object( - self.compute.network_api, - 'cleanup_instance_network_on_host') as mock_clean: + with test.nested( + mock.patch.object( + self.compute.network_api, 'cleanup_instance_network_on_host', + ), + mock.patch.object( + self.compute.network_api, 'get_instance_nw_info', + ), + ) as (mock_clean, _): self.compute.build_and_run_instance(self.context, self.instance, self.image, request_spec={}, filter_properties=self.filter_properties, @@ -5658,9 +5662,14 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase): mock_build_and_run.side_effect = exception.RescheduledException( reason='', instance_uuid=self.instance.uuid) - with mock.patch.object( - self.compute.network_api, - 'cleanup_instance_network_on_host') as mock_cleanup_network: + with test.nested( + mock.patch.object( + self.compute.network_api, 'cleanup_instance_network_on_host', + ), + mock.patch.object( + self.compute.network_api, 'get_instance_nw_info', + ), + ) as (mock_cleanup_network, _): self.compute._do_build_and_run_instance(self.context, instance, self.image, request_spec={}, filter_properties=self.filter_properties, @@ -6311,9 +6320,14 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase): mock_claim.side_effect = exc self._do_build_instance_update(mock_save, reschedule_update=True) - with mock.patch.object( - self.compute.network_api, - 'cleanup_instance_network_on_host') as mock_clean: + with test.nested( + mock.patch.object( + self.compute.network_api, 'cleanup_instance_network_on_host', + ), + mock.patch.object( + self.compute.network_api, 'get_instance_nw_info', + ), + ) as (mock_clean, _): self.compute.build_and_run_instance(self.context, self.instance, self.image, request_spec={}, filter_properties=self.filter_properties, @@ -6807,18 +6821,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 5e6e78552aa2..864c577b6696 100644 --- a/nova/virt/driver.py +++ b/nova/virt/driver.py @@ -1330,9 +1330,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`.