Leverage _detach_interface for destroying networks

During network task revert, we should unplug vifs as well.

Change-Id: I26b1ba8ffba167683613d901bcbee65fde1f4b2d
Closes-Bug: #1712511

mogan/baremetal/ironic/driver.py

@@ -196,11 +196,9 @@ class IronicDriver(base_driver.BaseEngineDriver):
                    'server_nics': str(server.nics),
                    'port_id': port_id})
         node = self._get_node(server.node_uuid)
-        self._unplug_vif(node, server, port_id)
+        self._unplug_vif(node, port_id)
 
-    def _unplug_vif(self, node, server, port_id):
+    def _unplug_vif(self, node, port_id):
-        for vif in server.nics:
-            if port_id == vif['port_id']:
         try:
             self.ironicclient.call("node.vif_detach", node.uuid,
                                    port_id)
@@ -209,12 +207,6 @@ class IronicDriver(base_driver.BaseEngineDriver):
                 "VIF %(vif)s isn't attached to Ironic node %(node)s",
                 {'vif': port_id, 'node': node.uuid})
 
-    def _cleanup_deploy(self, context, node, server):
-        # NOTE(liusheng): here we may need to stop firewall if we have
-        # implemented in ironic like what Nova dose.
-        for vif in server.nics:
-            self.unplug_vif(context, server, vif['port_id'])
-
     def spawn(self, context, server, configdrive_value):
         """Deploy a server.
 
@@ -240,8 +232,6 @@ class IronicDriver(base_driver.BaseEngineDriver):
         validate_chk = self.ironicclient.call("node.validate", node_uuid)
         if (not validate_chk.deploy.get('result')
                 or not validate_chk.power.get('result')):
-            # something is wrong. undo what we have done
-            self._cleanup_deploy(context, node, server)
             raise exception.ValidationError(_(
                 "Ironic node: %(id)s failed to validate."
                 " (deploy: %(deploy)s, power: %(power)s)")

mogan/db/api.py

@@ -102,12 +102,14 @@ class Connection(object):
         This query the Nics info of the specified server.
         """
 
+    @abc.abstractmethod
     def server_nic_update_or_create(self, context, port_id, values):
         """Update/Create a nic db entry.
 
         This creates or updates a nic db entry.
         """
 
+    @abc.abstractmethod
     def server_nic_delete(self, context, port_id):
         """Delete a nic db entry.
 

mogan/engine/manager.py

@@ -284,9 +284,8 @@ class EngineManager(base_manager.BaseEngineManager):
 
     def destroy_networks(self, context, server):
         ports = server.nics.get_port_ids()
-        server.nics.delete(context)
         for port in ports:
-            self.network_api.delete_port(context, port, server.uuid)
+            self._detach_interface(context, server, port)
 
     def _rollback_servers_quota(self, context, number):
         reserve_opts = {'servers': number}
@@ -432,8 +431,6 @@ class EngineManager(base_manager.BaseEngineManager):
                 LOG.error("Destroy networks for server %(uuid)s failed. "
                           "Exception: %(exception)s",
                           {"uuid": server.uuid, "exception": e})
-        for vif in server.nics:
-            self.driver.unplug_vif(context, server, vif['port_id'])
         self.driver.destroy(context, server)
 
     @wrap_server_fault
@@ -555,9 +552,7 @@ class EngineManager(base_manager.BaseEngineManager):
             raise exception.InterfaceAttachFailed(message=six.text_type(e))
         LOG.info('Attaching interface successfully')
 
-    def detach_interface(self, context, server, port_id):
+    def _detach_interface(self, context, server, port_id):
-        LOG.debug("Detaching interface %(port_id) from server %(server)s",
-                  {'port_id': port_id, 'server': server})
         try:
             self.driver.unplug_vif(context, server, port_id)
         except exception.MoganException as e:
@@ -571,9 +566,15 @@ class EngineManager(base_manager.BaseEngineManager):
             except Exception as e:
                 raise exception.InterfaceDetachFailed(server_uuid=server.uuid)
 
-        for nic in server.nics:
+        try:
-            if nic.port_id == port_id:
+            objects.ServerNic.delete_by_port_id(context, port_id)
-                nic.delete(context)
+        except exception.PortNotFound:
+            pass
+
+    def detach_interface(self, context, server, port_id):
+        LOG.debug("Detaching interface %(port_id) from server %(server)s",
+                  {'port_id': port_id, 'server': server.uuid})
+        self._detach_interface(context, server, port_id)
 
         LOG.info('Interface was successfully detached')
 

mogan/objects/server_nic.py

@@ -58,6 +58,10 @@ class ServerNic(base.MoganObject, object_base.VersionedObjectDictCompat):
         obj.obj_reset_changes()
         return obj
 
+    @classmethod
+    def delete_by_port_id(cls, context, port_id):
+        cls.dbapi.server_nic_delete(context, port_id)
+
     def save(self, context):
         updates = self.obj_get_changes()
         self.dbapi.server_nic_update_or_create(
@@ -69,8 +73,7 @@ class ServerNic(base.MoganObject, object_base.VersionedObjectDictCompat):
             context, self.port_id, values)
 
     def delete(self, context):
-        self.dbapi.server_nic_delete(
+        self.dbapi.server_nic_delete(context, self.port_id)
-            context, self.port_id)
 
 
 @base.MoganObjectRegistry.register

mogan/tests/unit/engine/test_manager.py

@@ -37,31 +37,30 @@ class ManageServerTestCase(mgr_utils.ServiceSetUpMixin,
                            tests_db_base.DbTestCase):
 
     @mock.patch.object(network_api.API, 'delete_port')
-    def test_destroy_networks(self, delete_port_mock):
+    @mock.patch.object(IronicDriver, 'unplug_vif')
+    def test_destroy_networks(self, unplug_vif_mock, delete_port_mock):
         server = obj_utils.create_test_server(self.context)
         server_port_id = server.nics[0].port_id
         delete_port_mock.side_effect = None
-        port = mock.MagicMock()
+        unplug_vif_mock.side_effect = None
-        port.extra = {'vif_port_id': 'fake-vif'}
-        port.uuid = 'fake-uuid'
         self._start_service()
 
         self.service.destroy_networks(self.context, server)
         self._stop_service()
 
+        unplug_vif_mock.assert_called_once_with(
+            self.context, server, server_port_id)
         delete_port_mock.assert_called_once_with(
             self.context, server_port_id, server.uuid)
 
     @mock.patch.object(IronicDriver, 'destroy')
-    @mock.patch.object(IronicDriver, 'unplug_vif')
     @mock.patch.object(manager.EngineManager, 'destroy_networks')
-    def _test__delete_server(self, destroy_networks_mock, unplug_mock,
+    def _test__delete_server(self, destroy_networks_mock,
                              destroy_node_mock, state=None):
         fake_node = mock.MagicMock()
         fake_node.provision_state = state
         server = obj_utils.create_test_server(self.context)
         destroy_networks_mock.side_effect = None
-        unplug_mock.side_effect = None
         destroy_node_mock.side_effect = None
         self._start_service()
 
@@ -69,7 +68,6 @@ class ManageServerTestCase(mgr_utils.ServiceSetUpMixin,
         self._stop_service()
 
         destroy_networks_mock.assert_called_once_with(self.context, server)
-        self.assertEqual(unplug_mock.call_count, len(server.nics))
         destroy_node_mock.assert_called_once_with(self.context, server)
 
     def test__delete_server_cleaning(self):