Remove vifs upon teardown

Since we removed the ability for nova to cleanly remove
the vif during teardown because that created a race condition,
the removal of all vif attachment records only seems to be the
right thing to do since we can't realistically change nova try
harder, and functionally we are otherwise looking at massive
locking changes.

Removing vif records is the lesser evil until we can reach
consensus on completely revamping locking to allow for greater
concurrency.

Change-Id: I8d683d2d506c97535b5a8f9a5de4c070c7e887df
Story: #1743652
Task: #9275

7 files changed, 129 insertions, 3 deletions

diff --git a/ironic/common/network.py b/ironic/common/network.py
index f99e372af..05906ee43 100644
--- a/ironic/common/network.py
+++ b/ironic/common/network.py
@@ -12,8 +12,12 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+from oslo_log import log
+
 from ironic.common import exception
 
+LOG = log.getLogger(__name__)
+
 
 def get_node_vif_ids(task):
     """Get all VIF ids for a node.
@@ -108,3 +112,34 @@ def get_physnets_by_portgroup_id(task, portgroup_id, exclude_port=None):
         raise exception.PortgroupPhysnetInconsistent(
             portgroup=portgroup.uuid, physical_networks=", ".join(pg_physnets))
     return pg_physnets
+
+
+def remove_vifs_from_node(task):
+    """Remove all vif attachment records from a node.
+
+    :param task: a TaskManager instance.
+    """
+    vifs = task.driver.network.vif_list(task)
+    for vif_entry in vifs:
+        vif = vif_entry.get('id')
+        if not vif:
+            LOG.warning('Incorrect vif entry for %(node)s lacks an ID field, '
+                        'and is thus unsupported. Found: %(found)s.',
+                        {'node': task.node.uuid,
+                         'found': vif_entry})
+            continue
+        try:
+            task.driver.network.vif_detach(task, vif)
+        except exception.VifNotAttached as e:
+            LOG.warning('While removing records of VIF attachments from node '
+                        '%(node)s, we recieved indication that %(vif)s is '
+                        'no longer attached. There should not happen under '
+                        'normal circumstances.',
+                        {'node': task.node.uuid,
+                         'vif': vif})
+
+        except exception.NetworkError as e:
+            LOG.error('An error has been encountered while removing a '
+                      'VIF record for %(node)s. Error: %(error)s',
+                      {'node': task.node.uuid,
+                       'error': e})
diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py
index 951f7c73e..c25933adf 100644
--- a/ironic/conductor/manager.py
+++ b/ironic/conductor/manager.py
@@ -62,6 +62,7 @@ from ironic.common import exception
 from ironic.common.glance_service import service_utils as glance_utils
 from ironic.common.i18n import _
 from ironic.common import images
+from ironic.common import network
 from ironic.common import states
 from ironic.common import swift
 from ironic.conductor import base_manager
@@ -928,6 +929,7 @@ class ConductorManager(base_manager.BaseConductorManager):
             driver_internal_info = node.driver_internal_info
             driver_internal_info.pop('instance', None)
             node.driver_internal_info = driver_internal_info
+            network.remove_vifs_from_node(task)
             node.save()
 
         # Begin cleaning
diff --git a/ironic/drivers/modules/network/common.py b/ironic/drivers/modules/network/common.py
index dd99da141..e9ebabe86 100644
--- a/ironic/drivers/modules/network/common.py
+++ b/ironic/drivers/modules/network/common.py
@@ -583,6 +583,8 @@ class NeutronVIFPortIDMixin(VIFPortIDMixin):
 
         self._clear_vif_from_port_like_obj(port_like_obj)
 
-        # NOTE(vsaienko) allow to unplug VIFs from ACTIVE instance.
-        if task.node.provision_state == states.ACTIVE:
+        # NOTE(vsaienko): allow to unplug VIFs from ACTIVE instance.
+        # NOTE(TheJulia): Also ensure that we delete the vif when in
+        # DELETING state.
+        if task.node.provision_state in [states.ACTIVE, states.DELETING]:
             neutron.unbind_neutron_port(vif_id, context=task.context)
diff --git a/ironic/tests/unit/common/test_network.py b/ironic/tests/unit/common/test_network.py
index d59846385..b7f89ffbe 100644
--- a/ironic/tests/unit/common/test_network.py
+++ b/ironic/tests/unit/common/test_network.py
@@ -13,11 +13,15 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import mock
 from oslo_utils import uuidutils
 
 from ironic.common import exception
 from ironic.common import network
+from ironic.common import neutron as neutron_common
+from ironic.common import states
 from ironic.conductor import task_manager
+from ironic.drivers.modules.network import common as driver_common
 from ironic.tests.unit.conductor import mgr_utils
 from ironic.tests.unit.db import base as db_base
 from ironic.tests.unit.db import utils as db_utils
@@ -156,6 +160,47 @@ class TestNetwork(db_base.DbTestCase):
     def test_get_node_vif_ids_during_rescuing(self):
         self._test_get_node_vif_ids_multitenancy('rescuing_vif_port_id')
 
+    def test_remove_vifs_from_node(self):
+        db_utils.create_test_port(
+            node_id=self.node.id, address='aa:bb:cc:dd:ee:ff',
+            internal_info={driver_common.TENANT_VIF_KEY: 'test-vif-A'})
+        db_utils.create_test_portgroup(
+            node_id=self.node.id, address='dd:ee:ff:aa:bb:cc',
+            internal_info={driver_common.TENANT_VIF_KEY: 'test-vif-B'})
+        with task_manager.acquire(self.context, self.node.uuid) as task:
+            network.remove_vifs_from_node(task)
+        with task_manager.acquire(self.context, self.node.uuid) as task:
+            result = network.get_node_vif_ids(task)
+        self.assertEqual({}, result['ports'])
+        self.assertEqual({}, result['portgroups'])
+
+
+class TestRemoveVifsTestCase(db_base.DbTestCase):
+
+    def setUp(self):
+        super(TestRemoveVifsTestCase, self).setUp()
+        self.node = object_utils.create_test_node(
+            self.context,
+            network_interface='flat',
+            provision_state=states.DELETING)
+
+    @mock.patch.object(neutron_common, 'unbind_neutron_port')
+    def test_remove_vifs_from_node_failure(self, mock_unbind):
+        db_utils.create_test_port(
+            node_id=self.node.id, address='aa:bb:cc:dd:ee:ff',
+            internal_info={driver_common.TENANT_VIF_KEY: 'test-vif-A'})
+        db_utils.create_test_portgroup(
+            node_id=self.node.id, address='dd:ee:ff:aa:bb:cc',
+            internal_info={driver_common.TENANT_VIF_KEY: 'test-vif-B'})
+        mock_unbind.side_effect = [exception.NetworkError, None]
+        with task_manager.acquire(self.context, self.node.uuid) as task:
+            network.remove_vifs_from_node(task)
+        with task_manager.acquire(self.context, self.node.uuid) as task:
+            result = network.get_node_vif_ids(task)
+        self.assertEqual({}, result['ports'])
+        self.assertEqual({}, result['portgroups'])
+        self.assertEqual(2, mock_unbind.call_count)
+
 
 class GetPortgroupByIdTestCase(db_base.DbTestCase):
 
diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py
index 7a2d42608..5f9033609 100644
--- a/ironic/tests/unit/conductor/test_manager.py
+++ b/ironic/tests/unit/conductor/test_manager.py
@@ -1786,9 +1786,13 @@ class DoNodeDeployTearDownTestCase(mgr_utils.ServiceSetUpMixin,
         self.assertEqual({}, node.instance_info)
         mock_tear_down.assert_called_once_with(mock.ANY)
 
+    # TODO(TheJulia): Since we're functionally bound to neutron support
+    # by default, the fake drivers still invoke neutron.
+    @mock.patch('ironic.common.neutron.unbind_neutron_port')
     @mock.patch('ironic.conductor.manager.ConductorManager._do_node_clean')
     @mock.patch('ironic.drivers.modules.fake.FakeDeploy.tear_down')
-    def test__do_node_tear_down_ok(self, mock_tear_down, mock_clean):
+    def test__do_node_tear_down_ok(self, mock_tear_down, mock_clean,
+                                   mock_unbind):
         # test when driver.deploy.tear_down succeeds
         node = obj_utils.create_test_node(
             self.context, driver='fake', provision_state=states.DELETING,
@@ -1797,11 +1801,15 @@ class DoNodeDeployTearDownTestCase(mgr_utils.ServiceSetUpMixin,
             instance_info={'foo': 'bar'},
             driver_internal_info={'is_whole_disk_image': False,
                                   'instance': {'ephemeral_gb': 10}})
+        port = obj_utils.create_test_port(
+            self.context, node_id=node.id,
+            internal_info={'tenant_vif_port_id': 'foo'})
 
         task = task_manager.TaskManager(self.context, node.uuid)
         self._start_service()
         self.service._do_node_tear_down(task, node.provision_state)
         node.refresh()
+        port.refresh()
         # Node will be moved to AVAILABLE after cleaning, not tested here
         self.assertEqual(states.CLEANING, node.provision_state)
         self.assertEqual(states.AVAILABLE, node.target_provision_state)
@@ -1811,6 +1819,8 @@ class DoNodeDeployTearDownTestCase(mgr_utils.ServiceSetUpMixin,
         self.assertNotIn('instance', node.driver_internal_info)
         mock_tear_down.assert_called_once_with(mock.ANY)
         mock_clean.assert_called_once_with(mock.ANY)
+        self.assertEqual({}, port.internal_info)
+        mock_unbind.assert_called_once_with('foo', context=mock.ANY)
 
     @mock.patch('ironic.drivers.modules.fake.FakeRescue.clean_up')
     @mock.patch('ironic.conductor.manager.ConductorManager._do_node_clean')
diff --git a/ironic/tests/unit/drivers/modules/network/test_common.py b/ironic/tests/unit/drivers/modules/network/test_common.py
index a64fc348f..70f3b1307 100644
--- a/ironic/tests/unit/drivers/modules/network/test_common.py
+++ b/ironic/tests/unit/drivers/modules/network/test_common.py
@@ -935,6 +935,20 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase):
     @mock.patch.object(common.VIFPortIDMixin, '_clear_vif_from_port_like_obj')
     @mock.patch.object(neutron_common, 'unbind_neutron_port', autospec=True)
     @mock.patch.object(common.VIFPortIDMixin, '_get_port_like_obj_by_vif_id')
+    def test_vif_detach_deleting_node(self, mock_get, mock_unp, mock_clear):
+        self.node.provision_state = states.DELETING
+        self.node.save()
+        mock_get.return_value = self.port
+        with task_manager.acquire(self.context, self.node.id) as task:
+            self.interface.vif_detach(task, 'fake_vif_id')
+            mock_unp.assert_called_once_with('fake_vif_id',
+                                             context=task.context)
+        mock_get.assert_called_once_with(task, 'fake_vif_id')
+        mock_clear.assert_called_once_with(self.port)
+
+    @mock.patch.object(common.VIFPortIDMixin, '_clear_vif_from_port_like_obj')
+    @mock.patch.object(neutron_common, 'unbind_neutron_port', autospec=True)
+    @mock.patch.object(common.VIFPortIDMixin, '_get_port_like_obj_by_vif_id')
     def test_vif_detach_active_node_failure(self, mock_get, mock_unp,
                                             mock_clear):
         self.node.provision_state = states.ACTIVE
diff --git a/releasenotes/notes/remove-vifs-on-teardown-707c8e40c46b6e64.yaml b/releasenotes/notes/remove-vifs-on-teardown-707c8e40c46b6e64.yaml
new file mode 100644
index 000000000..e0e7cb821
--- /dev/null
+++ b/releasenotes/notes/remove-vifs-on-teardown-707c8e40c46b6e64.yaml
@@ -0,0 +1,18 @@
+---
+upgrade:
+  - |
+    The behavior for retention of VIF interface attachments has changed.
+
+    If your use of the BareMetal service is reliant upon the behavior of
+    the VIFs being retained, which was introduced as a behavior change
+    during the Ocata cycle, then you must update your tooling to explicitly
+    re-add the the VIF attachments prior to deployment.
+fixes:
+  - |
+    Removes all records of VIF attachments upon the teardown of a deployed
+    node. This is in order to resolve issues related to where it is
+    operationally impossible in some circumstances to remove a VIF
+    attachment while a node is being undeployed as the Compute service
+    will only attempt to remove the VIF for five minutes.
+
+    See `bug 1743652 <https://bugs.launchpad.net/ironic/+bug/1743652>`_ for more details.