Allow node vif attach to specify port_uuid or portgroup_uuid

Update the function that checks for free ports/portgroups to
accept a port_like_obj_id parameter to match. This parameter
comes from the vif_info dictionary.

Story: 2007723
Task: 39872

Change-Id: I82efa90994325aa37cca865920d656f510a691b2

ironic/api/controllers/v1/node.py

@@ -1729,6 +1729,10 @@ class NodeVIFController(rest.RestController):
             for that VIF.
         """
         rpc_node, topic = self._get_node_and_topic('baremetal:node:vif:attach')
+        if api.request.version.minor >= versions.MINOR_67_NODE_VIF_ATTACH_PORT:
+            if 'port_uuid' in vif and 'portgroup_uuid' in vif:
+                msg = _("Cannot specify both port_uuid and portgroup_uuid")
+                raise exception.Invalid(msg)
         api.request.rpcapi.vif_attach(api.request.context, rpc_node.uuid,
                                       vif_info=vif, topic=topic)
 

ironic/api/controllers/v1/versions.py

@@ -104,6 +104,7 @@ BASE_VERSION = 1
 # v1.64: Add network_type to port.local_link_connection
 # v1.65: Add lessee to the node object.
 # v1.66: Add support for node network_data field.
+# v1.67: Add support for port_uuid/portgroup_uuid in node vif_attach
 
 MINOR_0_JUNO = 0
 MINOR_1_INITIAL_VERSION = 1
@@ -172,6 +173,7 @@ MINOR_63_INDICATORS = 63
 MINOR_64_LOCAL_LINK_CONNECTION_NETWORK_TYPE = 64
 MINOR_65_NODE_LESSEE = 65
 MINOR_66_NODE_NETWORK_DATA = 66
+MINOR_67_NODE_VIF_ATTACH_PORT = 67
 
 # When adding another version, update:
 # - MINOR_MAX_VERSION
@@ -179,7 +181,7 @@ MINOR_66_NODE_NETWORK_DATA = 66
 #   explanation of what changed in the new version
 # - common/release_mappings.py, RELEASE_MAPPING['master']['api']
 
-MINOR_MAX_VERSION = MINOR_66_NODE_NETWORK_DATA
+MINOR_MAX_VERSION = MINOR_67_NODE_VIF_ATTACH_PORT
 
 # String representations of the minor and maximum versions
 _MIN_VERSION_STRING = '{}.{}'.format(BASE_VERSION, MINOR_1_INITIAL_VERSION)

ironic/common/release_mappings.py

@@ -231,7 +231,7 @@ RELEASE_MAPPING = {
         }
     },
     'master': {
-        'api': '1.66',
+        'api': '1.67',
         'rpc': '1.50',
         'objects': {
             'Allocation': ['1.1'],

ironic/drivers/modules/network/common.py

@@ -83,7 +83,7 @@ def _is_port_physnet_allowed(port, physnets):
             or port.physical_network in physnets)
 
 
-def _get_free_portgroups_and_ports(task, vif_id, physnets):
+def _get_free_portgroups_and_ports(task, vif_id, physnets, vif_info={}):
     """Get free portgroups and ports.
 
     It only returns ports or portgroups that can be used for attachment of
@@ -95,6 +95,8 @@ def _get_free_portgroups_and_ports(task, vif_id, physnets):
         attached. This is governed by the segments of the VIF's network. An
         empty set indicates that the ports' physical networks should be
         ignored.
+    :param vif_info: dict that may contain extra information, such as
+        port_uuid
     :returns: list of free ports and portgroups.
     :raises: VifAlreadyAttached, if vif_id is attached to any of the
         node's ports or portgroups.
@@ -109,9 +111,18 @@ def _get_free_portgroups_and_ports(task, vif_id, physnets):
     # at least one port with vif already attached to it
     non_usable_portgroups = set()
 
+    port_uuid = None
+    portgroup_uuid = None
+    if 'port_uuid' in vif_info:
+        port_uuid = vif_info['port_uuid']
+    elif 'portgroup_uuid' in vif_info:
+        portgroup_uuid = vif_info['portgroup_uuid']
+
     for p in task.ports:
+        # If port_uuid is specified in vif_info, check id
         # Validate that port has needed information
-        if not neutron.validate_port_info(task.node, p):
+        if ((port_uuid and port_uuid != p.uuid)
+            or not neutron.validate_port_info(task.node, p)):
             continue
         if _vif_attached(p, vif_id):
             # Consider such portgroup unusable. The fact that we can have None
@@ -120,18 +131,21 @@ def _get_free_portgroups_and_ports(task, vif_id, physnets):
             continue
         if not _is_port_physnet_allowed(p, physnets):
             continue
-        if p.portgroup_id is None:
+        if p.portgroup_id is None and not portgroup_uuid:
-            # ports without portgroup_id are always considered candidates
             free_port_like_objs.append(p)
         else:
             ports_by_portgroup[p.portgroup_id].append(p)
 
+    if not port_uuid:
         for pg in task.portgroups:
-        if _vif_attached(pg, vif_id):
+            # if portgroup_uuid is specified in vif_info, check id
+            if ((portgroup_uuid and portgroup_uuid != pg.uuid)
+                or _vif_attached(pg, vif_id)):
                 continue
             if pg.id in non_usable_portgroups:
                 # This portgroup has vifs attached to its ports, consider its
                 # ports instead to avoid collisions
+                if not portgroup_uuid:
                     free_port_like_objs.extend(ports_by_portgroup[pg.id])
             # Also ignore empty portgroups
             elif ports_by_portgroup[pg.id]:
@@ -140,7 +154,7 @@ def _get_free_portgroups_and_ports(task, vif_id, physnets):
     return free_port_like_objs
 
 
-def get_free_port_like_object(task, vif_id, physnets):
+def get_free_port_like_object(task, vif_id, physnets, vif_info={}):
     """Find free port-like object (portgroup or port) VIF will be attached to.
 
     Ensures that the VIF is not already attached to this node.  When selecting
@@ -160,6 +174,8 @@ def get_free_port_like_object(task, vif_id, physnets):
         attached. This is governed by the segments of the VIF's network. An
         empty set indicates that the ports' physical networks should be
         ignored.
+    :param vif_info: dict that may contain extra information, such as
+        port_uuid
     :raises: VifAlreadyAttached, if VIF is already attached to the node.
     :raises: NoFreePhysicalPorts, if there is no port-like object VIF can be
         attached to.
@@ -167,8 +183,8 @@ def get_free_port_like_object(task, vif_id, physnets):
              has ports which are not all assigned the same physical network.
     :returns: port-like object VIF will be attached to.
     """
-    free_port_like_objs = _get_free_portgroups_and_ports(task, vif_id,
+    free_port_like_objs = _get_free_portgroups_and_ports(
-                                                         physnets)
+        task, vif_id, physnets, vif_info)
 
     if not free_port_like_objs:
         raise exception.NoFreePhysicalPorts(vif=vif_id)
@@ -552,7 +568,8 @@ class NeutronVIFPortIDMixin(VIFPortIDMixin):
                     raise exception.VifInvalidForAttach(
                         node=task.node.uuid, vif=vif_id, reason=reason)
 
-        port_like_obj = get_free_port_like_object(task, vif_id, physnets)
+        port_like_obj = get_free_port_like_object(
+            task, vif_id, physnets, vif_info)
 
         # Address is optional for portgroups
         if port_like_obj.address:

ironic/tests/unit/api/controllers/v1/test_node.py

@@ -6176,6 +6176,51 @@ class TestAttachDetachVif(test_api_base.BaseApiTest):
         self.assertEqual(http_client.CONFLICT, ret.status_code)
         self.assertTrue(ret.json['error_message'])
 
+    @mock.patch.object(objects.Node, 'get_by_uuid')
+    @mock.patch.object(rpcapi.ConductorAPI, 'vif_attach')
+    def test_vif_attach_port_uuid_and_portgroup_uuid(self, mock_attach,
+                                                     mock_get):
+        vif_id = uuidutils.generate_uuid()
+        request_body = {
+            'id': vif_id,
+            'port_uuid': 'port-uuid',
+            'portgroup_uuid': 'portgroup-uuid'
+        }
+
+        mock_get.return_value = self.node
+
+        ret = self.post_json('/nodes/%s/vifs' % self.node.uuid,
+                             request_body, expect_errors=True,
+                             headers={api_base.Version.string:
+                                      "1.67"})
+
+        self.assertEqual(http_client.BAD_REQUEST, ret.status_int)
+        self.assertTrue(ret.json['error_message'])
+
+    @mock.patch.object(objects.Node, 'get_by_uuid')
+    @mock.patch.object(rpcapi.ConductorAPI, 'vif_attach')
+    def test_vif_attach_port_uuid_and_portgroup_uuid_old(self, mock_attach,
+                                                         mock_get):
+        vif_id = uuidutils.generate_uuid()
+        request_body = {
+            'id': vif_id,
+            'port_uuid': 'port-uuid',
+            'portgroup_uuid': 'portgroup-uuid'
+        }
+
+        mock_get.return_value = self.node
+
+        ret = self.post_json('/nodes/%s/vifs' % self.node.uuid,
+                             request_body,
+                             headers={api_base.Version.string:
+                                      self.vif_version})
+
+        self.assertEqual(http_client.NO_CONTENT, ret.status_code)
+        mock_get.assert_called_once_with(mock.ANY, self.node.uuid)
+        mock_attach.assert_called_once_with(mock.ANY, self.node.uuid,
+                                            vif_info=request_body,
+                                            topic='test-topic')
+
     @mock.patch.object(objects.Node, 'get_by_uuid')
     @mock.patch.object(rpcapi.ConductorAPI, 'vif_detach')
     def test_vif_detach(self, mock_detach, mock_get):

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

@@ -207,6 +207,59 @@ class TestCommonFunctions(db_base.DbTestCase):
         self.assertItemsEqual(
             [self.port.uuid], [p.uuid for p in free_port_like_objs])
 
+    def test__get_free_portgroups_and_ports_port_uuid(self):
+        self.node.network_interface = 'flat'
+        self.node.save()
+        pg1, pg1_ports, pg2, pg2_ports, pg3, pg3_ports = self._objects_setup(
+            set_physnets=False)
+        with task_manager.acquire(self.context, self.node.id) as task:
+            free_port_like_objs = (
+                common._get_free_portgroups_and_ports(
+                    task, self.vif_id, {}, {'port_uuid': self.port.uuid}))
+        self.assertItemsEqual(
+            [self.port.uuid],
+            [p.uuid for p in free_port_like_objs])
+
+    def test__get_free_portgroups_and_ports_portgroup_uuid(self):
+        self.node.network_interface = 'flat'
+        self.node.save()
+        pg1, pg1_ports, pg2, pg2_ports, pg3, pg3_ports = self._objects_setup(
+            set_physnets=False)
+        with task_manager.acquire(self.context, self.node.id) as task:
+            free_port_like_objs = (
+                common._get_free_portgroups_and_ports(
+                    task, self.vif_id, {}, {'portgroup_uuid': pg1.uuid}))
+        self.assertItemsEqual(
+            [pg1.uuid],
+            [p.uuid for p in free_port_like_objs])
+
+    def test__get_free_portgroups_and_ports_portgroup_uuid_attached_vifs(self):
+        self.node.network_interface = 'flat'
+        self.node.save()
+        pg1, pg1_ports, pg2, pg2_ports, pg3, pg3_ports = self._objects_setup(
+            set_physnets=False)
+        with task_manager.acquire(self.context, self.node.id) as task:
+            free_port_like_objs = (
+                common._get_free_portgroups_and_ports(
+                    task, self.vif_id, {}, {'portgroup_uuid': pg2.uuid}))
+        self.assertItemsEqual(
+            [],
+            [p.uuid for p in free_port_like_objs])
+
+    def test__get_free_portgroups_and_ports_no_matching_uuid(self):
+        self.node.network_interface = 'flat'
+        self.node.save()
+        pg1, pg1_ports, pg2, pg2_ports, pg3, pg3_ports = self._objects_setup(
+            set_physnets=False)
+        with task_manager.acquire(self.context, self.node.id) as task:
+            free_port_like_objs = (
+                common._get_free_portgroups_and_ports(
+                    task, self.vif_id, {},
+                    {'port_uuid': uuidutils.generate_uuid()}))
+        self.assertItemsEqual(
+            [],
+            [p.uuid for p in free_port_like_objs])
+
     @mock.patch.object(neutron_common, 'validate_port_info', autospec=True,
                        return_value=True)
     def test_get_free_port_like_object_ports(self, vpi_mock):
@@ -712,7 +765,8 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase):
             mock_upa.assert_called_once_with(
                 "fake_vif_id", self.port.address, context=task.context)
         self.assertFalse(mock_gpbpi.called)
-        mock_gfp.assert_called_once_with(task, 'fake_vif_id', set())
+        mock_gfp.assert_called_once_with(task, 'fake_vif_id', set(),
+                                         {'id': 'fake_vif_id'})
         mock_save.assert_called_once_with(self.port, "fake_vif_id")
 
     @mock.patch.object(common.VIFPortIDMixin, '_save_vif_to_port_like_obj')
@@ -728,7 +782,8 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase):
         with task_manager.acquire(self.context, self.node.id) as task:
             self.assertRaises(exception.NoFreePhysicalPorts,
                               self.interface.vif_attach, task, vif)
-        mock_gfp.assert_called_once_with(task, 'fake_vif_id', set())
+        mock_gfp.assert_called_once_with(task, 'fake_vif_id', set(),
+                                         {'id': 'fake_vif_id'})
         self.assertFalse(mock_save.called)
 
     @mock.patch.object(common.VIFPortIDMixin, '_save_vif_to_port_like_obj')
@@ -751,7 +806,8 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase):
                 "fake_vif_id", self.port.address, context=task.context)
         mock_gpbpi.assert_called_once_with(mock_client.return_value,
                                            'fake_vif_id')
-        mock_gfp.assert_called_once_with(task, 'fake_vif_id', {'physnet1'})
+        mock_gfp.assert_called_once_with(task, 'fake_vif_id', {'physnet1'},
+                                         {'id': 'fake_vif_id'})
         mock_save.assert_called_once_with(self.port, "fake_vif_id")
 
     @mock.patch.object(common.VIFPortIDMixin, '_save_vif_to_port_like_obj')
@@ -773,7 +829,8 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase):
             mock_upa.assert_called_once_with(
                 "fake_vif_id", self.port.address, context=task.context)
         self.assertFalse(mock_gpbpi.called)
-        mock_gfp.assert_called_once_with(task, 'fake_vif_id', set())
+        mock_gfp.assert_called_once_with(task, 'fake_vif_id', set(),
+                                         {'id': 'fake_vif_id'})
         mock_save.assert_called_once_with(self.port, "fake_vif_id")
         mock_plug.assert_called_once_with(task, self.port, mock.ANY)
 
@@ -799,7 +856,8 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase):
             mock_upa.assert_called_once_with(
                 "fake_vif_id", self.port.address, context=task.context)
         self.assertFalse(mock_gpbpi.called)
-        mock_gfp.assert_called_once_with(task, 'fake_vif_id', set())
+        mock_gfp.assert_called_once_with(task, 'fake_vif_id', set(),
+                                         {'id': 'fake_vif_id'})
         mock_save.assert_called_once_with(self.port, "fake_vif_id")
         mock_plug.assert_called_once_with(task, self.port, mock.ANY)
 
@@ -819,7 +877,8 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase):
             self.interface.vif_attach(task, vif)
             mock_client.assert_called_once_with(context=task.context)
         self.assertFalse(mock_gpbpi.called)
-        mock_gfp.assert_called_once_with(task, 'fake_vif_id', set())
+        mock_gfp.assert_called_once_with(task, 'fake_vif_id', set(),
+                                         {'id': 'fake_vif_id'})
         self.assertFalse(mock_client.return_value.show_port.called)
         self.assertFalse(mock_upa.called)
         mock_save.assert_called_once_with(pg, "fake_vif_id")

releasenotes/notes/vif-port-attach-17a9993bf5c21d69.yaml

@@ -0,0 +1,8 @@
+---
+features:
+  - |
+    Adds the ability for Ironic to attach a node to a specific port or portgroup.
+    This is accomplished by having the node vif_attach API accept a port_uuid or
+    portgroup_uuid key within vif_info. If one is specified, then Ironic will
+    attempt to attach to the specified port/portgroup. Specifying both returns
+    an error.