From d4ed0d8b7adc350e8962df033c2da892c95561fe Mon Sep 17 00:00:00 2001
From: Arnaud Morin <arnaud.morin@corp.ovh.com>
Date: Wed, 22 May 2019 17:34:20 +0200
Subject: [PATCH] Refresh instance network info on deletion

When deleting an instance, if the network info is empty, we should
refresh the info because we can't be sure the copy of the cache we
have when we fetched the instance to delete is up-to-date, i.e. if
we're racing to delete the server while it's building and the
network info cache was updated in the database after we started the
delete operation and got the instance from the DB, then we could
fail to unplug VIFs.

Closes-Bug: #1830081

Change-Id: I99601773406c61f93002e2f7cbb248cf73cef0ab
Signed-off-by: Arnaud Morin <arnaud.morin@corp.ovh.com>
---
 nova/compute/manager.py                       |  6 ++++
 nova/tests/unit/compute/test_compute_mgr.py   | 33 +++++++++++++++++++
 nova/tests/unit/compute/test_compute_utils.py |  2 +-
 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index 914131d4a389..c8b49c9d66e7 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -2569,6 +2569,12 @@ class ComputeManager(manager.Manager):
 
         network_info = instance.get_network_info()
 
+        # NOTE(arnaudmorin) to avoid nova destroying the instance without
+        # unplugging the interface, refresh network_info if it is empty.
+        if not network_info:
+            network_info = self.network_api.get_instance_nw_info(
+                context, instance)
+
         # NOTE(vish) get bdms before destroying the instance
         vol_bdms = [bdm for bdm in bdms if bdm.is_volume]
         block_device_info = self._get_instance_block_device_info(
diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py
index 04b0ff538009..258ba8a82a5e 100644
--- a/nova/tests/unit/compute/test_compute_mgr.py
+++ b/nova/tests/unit/compute/test_compute_mgr.py
@@ -4842,6 +4842,39 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
         mock_delete_instance.assert_called_once_with(
             self.context, instance, bdms)
 
+    @mock.patch('nova.context.RequestContext.elevated')
+    def test_terminate_instance_no_network_info(self, mock_elevated):
+        # Tests that we refresh the network info if it was empty
+        instance = fake_instance.fake_instance_obj(
+            self.context, vm_state=vm_states.ACTIVE)
+        empty_nw_info = network_model.NetworkInfo()
+        instance.info_cache = objects.InstanceInfoCache(
+            network_info=empty_nw_info)
+        vif = fake_network_cache_model.new_vif()
+        nw_info = network_model.NetworkInfo([vif])
+        bdms = objects.BlockDeviceMappingList()
+        elevated = context.get_admin_context()
+        mock_elevated.return_value = elevated
+
+        # Call shutdown instance
+        with test.nested(
+            mock.patch.object(self.compute.network_api, 'get_instance_nw_info',
+                              return_value=nw_info),
+            mock.patch.object(self.compute, '_get_instance_block_device_info'),
+            mock.patch.object(self.compute.driver, 'destroy')
+        ) as (
+            mock_nw_api_info, mock_get_bdi, mock_destroy
+        ):
+            self.compute._shutdown_instance(self.context, instance, bdms,
+                notify=False, try_deallocate_networks=False)
+
+        # Verify
+        mock_nw_api_info.assert_called_once_with(elevated, instance)
+        mock_get_bdi.assert_called_once_with(elevated, instance, bdms=bdms)
+        # destroy should have been called with the refresh network_info
+        mock_destroy.assert_called_once_with(
+            elevated, instance, nw_info, mock_get_bdi.return_value)
+
     @mock.patch('nova.compute.utils.notify_about_instance_action')
     @mock.patch.object(nova.compute.manager.ComputeManager,
                        '_notify_about_instance_usage')
diff --git a/nova/tests/unit/compute/test_compute_utils.py b/nova/tests/unit/compute/test_compute_utils.py
index 4f9546bba472..366c7b65e5d7 100644
--- a/nova/tests/unit/compute/test_compute_utils.py
+++ b/nova/tests/unit/compute/test_compute_utils.py
@@ -60,7 +60,7 @@ CONF = nova.conf.CONF
 def create_instance(context, user_id='fake', project_id='fake', params=None):
     """Create a test instance."""
     flavor = objects.Flavor.get_by_name(context, 'm1.tiny')
-    net_info = model.NetworkInfo([])
+    net_info = model.NetworkInfo([model.VIF(id=uuids.port_id)])
     info_cache = objects.InstanceInfoCache(network_info=net_info)
     inst = objects.Instance(context=context,
                             image_ref=uuids.fake_image_ref,