Allow to attach/detach VIFs to active ironic nodes

This patch enhances attach/detach logic and allows to work with
active ironic nodes. Starting with this change when VIF is attach/detach
neutron port is updated appropriately to reflect changes on hardware
(ie: plug port to specific network)

Change-Id: Idc9dc37b11950e3aa2864a0a90d6ec2e307926a3
Related-Bug: #1659282

ironic/drivers/modules/network/common.py

@@ -23,6 +23,7 @@ from ironic.common import dhcp_factory
 from ironic.common import exception
 from ironic.common.i18n import _, _LW
 from ironic.common import neutron
+from ironic.common import states
 from ironic.common import utils
 from ironic import objects
 
@@ -137,6 +138,78 @@ def get_free_port_like_object(task, vif_id):
     return sorted_free_ports[0]
 
 
+def plug_port_to_tenant_network(task, port_like_obj, client=None):
+    """Plug port like object to tenant network.
+
+    :param task: A TaskManager instance.
+    :param port_like_obj: port-like object to plug.
+    :param client: Neutron client instance.
+    :raises NetworkError: if failed to update Neutron port.
+    :raises VifNotAttached if tenant VIF is not associated with port_like_obj.
+    """
+
+    node = task.node
+    local_link_info = []
+    client_id_opt = None
+
+    vif_id = (
+        port_like_obj.internal_info.get(TENANT_VIF_KEY) or
+        port_like_obj.extra.get('vif_port_id'))
+
+    if not vif_id:
+        obj_name = port_like_obj.__class__.__name__.lower()
+        raise exception.VifNotAttached(
+            _("Tenant VIF is not associated with %(obj_name)s "
+              "%(obj_id)s") % {'obj_name': obj_name,
+                               'obj_id': port_like_obj.uuid})
+
+    LOG.debug('Mapping tenant port %(vif_id)s to node '
+              '%(node_id)s',
+              {'vif_id': vif_id, 'node_id': node.uuid})
+
+    if isinstance(port_like_obj, objects.Portgroup):
+        pg_ports = [p for p in task.ports
+                    if p.portgroup_id == port_like_obj.id]
+        for port in pg_ports:
+            local_link_info.append(port.local_link_connection)
+    else:
+        # We iterate only on ports or portgroups, no need to check
+        # that it is a port
+        local_link_info.append(port_like_obj.local_link_connection)
+        client_id = port_like_obj.extra.get('client-id')
+        if client_id:
+            client_id_opt = ({'opt_name': 'client-id', 'opt_value': client_id})
+
+    # NOTE(sambetts) Only update required binding: attributes,
+    # because other port attributes may have been set by the user or
+    # nova.
+    body = {
+        'port': {
+            'binding:vnic_type': 'baremetal',
+            'binding:host_id': node.uuid,
+            'binding:profile': {
+                'local_link_information': local_link_info,
+            },
+        }
+    }
+    if client_id_opt:
+        body['port']['extra_dhcp_opts'] = [client_id_opt]
+
+    if not client:
+        client = neutron.get_client()
+
+    try:
+        client.update_port(vif_id, body)
+    except neutron_exceptions.ConnectionFailed as e:
+        msg = (_('Could not add public network VIF %(vif)s '
+                 'to node %(node)s, possible network issue. %(exc)s') %
+               {'vif': vif_id,
+                'node': node.uuid,
+                'exc': e})
+        LOG.error(msg)
+        raise exception.NetworkError(msg)
+
+
 class VIFPortIDMixin(object):
 
     def port_changed(self, task, port_obj):
@@ -288,12 +361,12 @@ class VIFPortIDMixin(object):
 
         port_like_obj = get_free_port_like_object(task, vif_id)
 
+        client = neutron.get_client()
         # Address is optional for portgroups
         if port_like_obj.address:
             # Check if the requested vif_id is a neutron port. If it is
             # then attempt to update the port's MAC address.
             try:
-                client = neutron.get_client()
                 client.show_port(vif_id)
             except neutron_exceptions.NeutronClientException:
                 # NOTE(sambetts): If a client error occurs this is because
@@ -318,13 +391,17 @@ class VIFPortIDMixin(object):
         int_info[TENANT_VIF_KEY] = vif_id
         port_like_obj.internal_info = int_info
         port_like_obj.save()
+        # NOTE(vsaienko) allow to attach VIF to active instance.
+        if task.node.provision_state == states.ACTIVE:
+            plug_port_to_tenant_network(task, port_like_obj, client=client)
 
     def vif_detach(self, task, vif_id):
         """Detach a virtual network interface from a node
 
         :param task: A TaskManager instance.
         :param vif_id: A VIF ID to detach
-        :raises: VifNotAttached
+        :raises: VifNotAttached if VIF not attached.
+        :raises: NetworkError: if unbind Neutron port failed.
         """
 
         ports = [p for p in task.ports if p.portgroup_id is None]
@@ -332,8 +409,9 @@ class VIFPortIDMixin(object):
         for port_like_obj in portgroups + ports:
             # FIXME(sambetts) Remove this when we no longer support a nova
             # driver that uses port.extra
-            if (port_like_obj.extra.get("vif_port_id") == vif_id or
+            vif_port_id = port_like_obj.internal_info.get(
-                    port_like_obj.internal_info.get(TENANT_VIF_KEY) == vif_id):
+                TENANT_VIF_KEY, port_like_obj.extra.get("vif_port_id"))
+            if vif_port_id == vif_id:
                 int_info = port_like_obj.internal_info
                 extra = port_like_obj.extra
                 int_info.pop(TENANT_VIF_KEY, None)
@@ -341,6 +419,9 @@ class VIFPortIDMixin(object):
                 port_like_obj.extra = extra
                 port_like_obj.internal_info = int_info
                 port_like_obj.save()
+                # NOTE(vsaienko) allow to unplug VIFs from ACTIVE instance.
+                if task.node.provision_state == states.ACTIVE:
+                    neutron.unbind_neutron_port(vif_port_id)
                 break
         else:
             raise exception.VifNotAttached(vif=vif_id, node=task.node.uuid)

ironic/drivers/modules/network/neutron.py

@@ -14,7 +14,6 @@
 #    under the License.
 
 
-from neutronclient.common import exceptions as neutron_exceptions
 from oslo_config import cfg
 from oslo_log import log
 
@@ -23,7 +22,6 @@ from ironic.common.i18n import _, _LI
 from ironic.common import neutron
 from ironic.drivers import base
 from ironic.drivers.modules.network import common
-from ironic import objects
 
 LOG = log.getLogger(__name__)
 
@@ -160,63 +158,16 @@ class NeutronNetwork(common.VIFPortIDMixin,
         ports = [p for p in ports if not p.portgroup_id]
         portgroups = task.portgroups
 
-        portmap = neutron.get_node_portmap(task)
-
         client = neutron.get_client()
         pobj_without_vif = 0
         for port_like_obj in ports + portgroups:
-            vif_port_id = (
-                port_like_obj.internal_info.get(common.TENANT_VIF_KEY) or
-                port_like_obj.extra.get('vif_port_id'))
-
-            if not vif_port_id:
-                pobj_without_vif += 1
-                continue
-
-            LOG.debug('Mapping tenant port %(vif_port_id)s to node '
-                      '%(node_id)s',
-                      {'vif_port_id': vif_port_id, 'node_id': node.uuid})
-            local_link_info = []
-            client_id_opt = None
-            if isinstance(port_like_obj, objects.Portgroup):
-                pg_ports = [p for p in task.ports
-                            if p.portgroup_id == port_like_obj.id]
-                for port in pg_ports:
-                    local_link_info.append(portmap[port.uuid])
-            else:
-                # We iterate only on ports or portgroups, no need to check
-                # that it is a port
-                local_link_info.append(portmap[port_like_obj.uuid])
-                client_id = port_like_obj.extra.get('client-id')
-                if client_id:
-                    client_id_opt = (
-                        {'opt_name': 'client-id', 'opt_value': client_id})
-
-            # NOTE(sambetts) Only update required binding: attributes,
-            # because other port attributes may have been set by the user or
-            # nova.
-            body = {
-                'port': {
-                    'binding:vnic_type': 'baremetal',
-                    'binding:host_id': node.uuid,
-                    'binding:profile': {
-                        'local_link_information': local_link_info,
-                    },
-                }
-            }
-            if client_id_opt:
-                body['port']['extra_dhcp_opts'] = [client_id_opt]
 
             try:
-                client.update_port(vif_port_id, body)
+                common.plug_port_to_tenant_network(task, port_like_obj,
-            except neutron_exceptions.ConnectionFailed as e:
+                                                   client=client)
-                msg = (_('Could not add public network VIF %(vif)s '
+            except exception.VifNotAttached:
-                         'to node %(node)s, possible network issue. %(exc)s') %
+                pobj_without_vif += 1
-                       {'vif': vif_port_id,
+                continue
-                        'node': node.uuid,
-                        'exc': e})
-                LOG.error(msg)
-                raise exception.NetworkError(msg)
 
         if pobj_without_vif == len(ports + portgroups):
             msg = _("No neutron ports or portgroups are associated with "

ironic/tests/unit/drivers/modules/network/test_common.py

@@ -16,6 +16,7 @@ from oslo_utils import uuidutils
 
 from ironic.common import exception
 from ironic.common import neutron as neutron_common
+from ironic.common import states
 from ironic.common import utils as common_utils
 from ironic.conductor import task_manager
 from ironic.drivers.modules.network import common
@@ -236,6 +237,36 @@ class TestCommonFunctions(db_base.DbTestCase):
                               common.get_free_port_like_object,
                               task, self.vif_id)
 
+    @mock.patch.object(neutron_common, 'get_client', autospec=True)
+    def test_plug_port_to_tenant_network_client(self, mock_gc):
+        self.port.internal_info = {common.TENANT_VIF_KEY: self.vif_id}
+        self.port.save()
+        with task_manager.acquire(self.context, self.node.id) as task:
+            common.plug_port_to_tenant_network(task, self.port,
+                                               client=mock.MagicMock())
+        self.assertFalse(mock_gc.called)
+
+    @mock.patch.object(neutron_common, 'get_client', autospec=True)
+    def test_plug_port_to_tenant_network_no_client(self, mock_gc):
+        self.port.internal_info = {common.TENANT_VIF_KEY: self.vif_id}
+        self.port.save()
+        with task_manager.acquire(self.context, self.node.id) as task:
+            common.plug_port_to_tenant_network(task, self.port)
+        self.assertTrue(mock_gc.called)
+
+    @mock.patch.object(neutron_common, 'get_client', autospec=True)
+    def test_plug_port_to_tenant_network_no_tenant_vif(self, mock_gc):
+        nclient = mock.MagicMock()
+        mock_gc.return_value = nclient
+        self.port.extra = {}
+        self.port.save()
+        with task_manager.acquire(self.context, self.node.id) as task:
+            self.assertRaisesRegex(
+                exception.VifNotAttached,
+                "not associated with port %s" % self.port.uuid,
+                common.plug_port_to_tenant_network,
+                task, self.port)
+
 
 class TestVifPortIDMixin(db_base.DbTestCase):
 
@@ -330,7 +361,7 @@ class TestVifPortIDMixin(db_base.DbTestCase):
             self.assertEqual(vif['id'],
                              pg.internal_info[common.TENANT_VIF_KEY])
             self.assertFalse(mock_upa.called)
-            self.assertFalse(mock_client.called)
+            self.assertTrue(mock_client.called)
 
     @mock.patch.object(neutron_common, 'get_client')
     @mock.patch.object(neutron_common, 'update_port_address')
@@ -354,7 +385,8 @@ class TestVifPortIDMixin(db_base.DbTestCase):
             self.assertFalse('vif_port_id' in self.port.extra)
             self.assertFalse(common.TENANT_VIF_KEY in self.port.internal_info)
 
-    def test_vif_detach_in_internal_info(self):
+    @mock.patch.object(neutron_common, 'unbind_neutron_port', autospec=True)
+    def test_vif_detach_in_internal_info(self, mock_unp):
         self.port.internal_info = {
             common.TENANT_VIF_KEY: self.port.extra['vif_port_id']}
         self.port.extra = {}
@@ -365,6 +397,7 @@ class TestVifPortIDMixin(db_base.DbTestCase):
             self.port.refresh()
             self.assertFalse('vif_port_id' in self.port.extra)
             self.assertFalse(common.TENANT_VIF_KEY in self.port.internal_info)
+        self.assertFalse(mock_unp.called)
 
     def test_vif_detach_in_extra_portgroup(self):
         vif_id = uuidutils.generate_uuid()
@@ -402,6 +435,21 @@ class TestVifPortIDMixin(db_base.DbTestCase):
                 exception.VifNotAttached, "it is not attached to it.",
                 self.interface.vif_detach, task, 'aaa')
 
+    @mock.patch.object(neutron_common, 'unbind_neutron_port', autospec=True)
+    def test_vif_detach_active_node(self, mock_unp):
+        vif_id = self.port.extra['vif_port_id']
+        self.port.internal_info = {
+            common.TENANT_VIF_KEY: vif_id}
+        self.port.extra = {}
+        self.port.save()
+        self.node.provision_state = states.ACTIVE
+        self.node.save()
+        with task_manager.acquire(self.context, self.node.id) as task:
+            self.interface.vif_detach(
+                task, self.port.internal_info[common.TENANT_VIF_KEY])
+            self.port.refresh()
+            mock_unp.assert_called_once_with(vif_id)
+
     def test_get_current_vif_extra_vif_port_id(self):
         extra = {'vif_port_id': 'foo'}
         self.port.extra = extra

releasenotes/notes/allow-to-attach-vif-to-active-node-55963be2ec269043.yaml

@@ -0,0 +1,3 @@
+---
+features:
+  - Adds possibility to attach/detach VIFs to/from active nodes.