Rely on portgroup standalone_ports_supported

Some hardware doesn't support portgroup fallback to single interface
fashion. This imposes additional restrictions on ports, for example
such ports will not support booting by PXE.
This patch adds restrictions setting pxe_enabled=True on ports that
are members of portgroups with standalone_ports_supported=False. And
vice versa portgroup.standalone_ports_supported can't be set to False
until portgroup contains ports with pxe_enabled=True.

Setting 'vif_port_id' means that we are using this port in standalone
mode. This patch also ensures that 'vif_port_id' can't be updated on
ports which are members of portgroups with
standalone_ports_supported=False. And vice versa updating portgroup
standalone_ports_supported=False is not allowed if portgroup contains
ports with extra/vif_port_id set.

Partial-bug: #1526403

Change-Id: I9b4682918725bed2da0b7c89666e2c37d8826290

5 files changed, 407 insertions, 4 deletions

diff --git a/ironic/api/controllers/v1/port.py b/ironic/api/controllers/v1/port.py
index a64dbd7be..7fac0da03 100644
--- a/ironic/api/controllers/v1/port.py
+++ b/ironic/api/controllers/v1/port.py
@@ -492,7 +492,7 @@ class PortsController(rest.RestController):
         """Create a new port.
 
         :param port: a port within the request body.
-        :raises: NotAcceptable, HTTPNotFound
+        :raises: NotAcceptable, HTTPNotFound, Conflict
         """
         cdict = pecan.request.context.to_dict()
         policy.authorize('baremetal:port:create', cdict, cdict)
@@ -508,6 +508,20 @@ class PortsController(rest.RestController):
             'portgroup_uuid' in pdict):
             raise exception.NotAcceptable()
 
+        extra = pdict.get('extra')
+        vif = extra.get('vif_port_id') if extra else None
+        if (pdict.get('portgroup_uuid') and
+                (pdict.get('pxe_enabled') or vif)):
+            rpc_pg = objects.Portgroup.get_by_uuid(pecan.request.context,
+                                                   pdict['portgroup_uuid'])
+            if not rpc_pg.standalone_ports_supported:
+                msg = _("Port group %s doesn't support standalone ports. "
+                        "This port cannot be created as a member of that "
+                        "port group because either 'extra/vif_port_id' "
+                        "was specified or 'pxe_enabled' was set to True.")
+                raise exception.Conflict(
+                    msg % pdict['portgroup_uuid'])
+
         new_port = objects.Port(pecan.request.context,
                                 **pdict)
 
diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py
index fd63ce631..35730e933 100644
--- a/ironic/conductor/manager.py
+++ b/ironic/conductor/manager.py
@@ -1644,7 +1644,8 @@ class ConductorManager(base_manager.BaseConductorManager):
                                    exception.FailedToUpdateMacOnPort,
                                    exception.MACAlreadyExists,
                                    exception.InvalidState,
-                                   exception.FailedToUpdateDHCPOptOnPort)
+                                   exception.FailedToUpdateDHCPOptOnPort,
+                                   exception.Conflict)
     def update_port(self, context, port_obj):
         """Update a port.
 
@@ -1658,6 +1659,9 @@ class ConductorManager(base_manager.BaseConductorManager):
         :raises: InvalidState if port connectivity attributes
                  are updated while node not in a MANAGEABLE or ENROLL or
                  INSPECTING state or not in MAINTENANCE mode.
+        :raises: Conflict if trying to set extra/vif_port_id or
+                 pxe_enabled=True on port which is a member of portgroup with
+                 standalone_ports_supported=False.
         """
         port_uuid = port_obj.uuid
         LOG.debug("RPC update_port called for port %s.", port_uuid)
@@ -1665,6 +1669,10 @@ class ConductorManager(base_manager.BaseConductorManager):
         with task_manager.acquire(context, port_obj.node_id,
                                   purpose='port update') as task:
             node = task.node
+            portgroup_obj = None
+            if port_obj.portgroup_id:
+                portgroup_obj = [pg for pg in task.portgroups if
+                                 pg.id == port_obj.portgroup_id][0]
 
             # Only allow updating MAC addresses for active nodes if maintenance
             # mode is on.
@@ -1717,12 +1725,12 @@ class ConductorManager(base_manager.BaseConductorManager):
                         "address."),
                         {'port': port_uuid, 'instance': node.instance_uuid})
 
+            vif = port_obj.extra.get('vif_port_id')
             if 'extra' in port_obj.obj_what_changed():
                 orignal_port = objects.Port.get_by_id(context, port_obj.id)
                 updated_client_id = port_obj.extra.get('client-id')
                 if (orignal_port.extra.get('client-id') !=
                     updated_client_id):
-                    vif = port_obj.extra.get('vif_port_id')
                     # DHCP Option with opt_value=None will remove it
                     # from the neutron port
                     if vif:
@@ -1742,6 +1750,19 @@ class ConductorManager(base_manager.BaseConductorManager):
                             {'port': port_uuid,
                              'instance': node.instance_uuid})
 
+            if portgroup_obj and ((set(port_obj.obj_what_changed()) &
+                                  {'pxe_enabled', 'portgroup_id'}) or vif):
+                if ((port_obj.pxe_enabled or vif) and not
+                    portgroup_obj.standalone_ports_supported):
+                    msg = (_("Port group %(portgroup)s doesn't support "
+                             "standalone ports. This port %(port)s cannot be "
+                             " a member of that port group because either "
+                             "'extra/vif_port_id' was specified or "
+                             "'pxe_enabled' was set to True.") %
+                           {"portgroup": portgroup_obj.uuid,
+                            "port": port_uuid})
+                    raise exception.Conflict(msg)
+
             port_obj.save()
 
             return port_obj
@@ -1751,7 +1772,8 @@ class ConductorManager(base_manager.BaseConductorManager):
                                    exception.FailedToUpdateMacOnPort,
                                    exception.PortgroupMACAlreadyExists,
                                    exception.PortgroupNotEmpty,
-                                   exception.InvalidState)
+                                   exception.InvalidState,
+                                   exception.Conflict)
     def update_portgroup(self, context, portgroup_obj):
         """Update a portgroup.
 
@@ -1767,6 +1789,9 @@ class ConductorManager(base_manager.BaseConductorManager):
                  in MAINTENANCE mode.
         :raises: PortgroupNotEmpty if there are ports associated with this
                  portgroup.
+        :raises: Conflict when trying to set standalone_ports_supported=False
+                 on portgroup with ports that has pxe_enabled=True and vice
+                 versa.
         """
         portgroup_uuid = portgroup_obj.uuid
         LOG.debug("RPC update_portgroup called for portgroup %s.",
@@ -1829,6 +1854,21 @@ class ConductorManager(base_manager.BaseConductorManager):
                          'instance': node.instance_uuid,
                          'node': node.uuid})
 
+            if ('standalone_ports_supported' in
+                    portgroup_obj.obj_what_changed()):
+                if not portgroup_obj.standalone_ports_supported:
+                    ports = [p for p in task.ports if
+                             p.portgroup_id == portgroup_obj.id]
+                    for p in ports:
+                        extra = p.extra
+                        vif = extra.get('vif_port_id')
+                        if vif or p.pxe_enabled:
+                            msg = _("standalone_ports_supported can not be "
+                                    "set to False, because the port group %s "
+                                    "contains ports with 'extra/vif_port_id' "
+                                    "or pxe_enabled=True") % portgroup_uuid
+                            raise exception.Conflict(msg)
+
             portgroup_obj.save()
 
             return portgroup_obj
diff --git a/ironic/tests/unit/api/v1/test_ports.py b/ironic/tests/unit/api/v1/test_ports.py
index 1b8b717c9..43bcc3409 100644
--- a/ironic/tests/unit/api/v1/test_ports.py
+++ b/ironic/tests/unit/api/v1/test_ports.py
@@ -955,6 +955,18 @@ class TestPost(test_api_base.BaseApiTest):
         self.assertEqual(urlparse.urlparse(response.location).path,
                          expected_location)
 
+    def test_create_port_min_api_version(self):
+        pdict = post_get_test_port(
+            node_uuid=self.node.uuid)
+        pdict.pop('local_link_connection')
+        pdict.pop('pxe_enabled')
+        pdict.pop('extra')
+        headers = {api_base.Version.string: str(api_v1.MIN_VER)}
+        response = self.post_json('/ports', pdict, headers=headers)
+        self.assertEqual('application/json', response.content_type)
+        self.assertEqual(http_client.CREATED, response.status_int)
+        self.assertEqual(self.node.uuid, response.json['node_uuid'])
+
     def test_create_port_doesnt_contain_id(self):
         with mock.patch.object(self.dbapi, 'create_port',
                                wraps=self.dbapi.create_port) as cp_mock:
@@ -1194,6 +1206,103 @@ class TestPost(test_api_base.BaseApiTest):
         self.assertEqual('application/json', response.content_type)
         self.assertEqual(http_client.FORBIDDEN, response.status_int)
 
+    def _test_create_port(self, has_vif=False, in_portgroup=False,
+                          pxe_enabled=True, standalone_ports=True,
+                          http_status=http_client.CREATED):
+        extra = {}
+        if has_vif:
+            extra = {'vif_port_id': uuidutils.generate_uuid()}
+        pdict = post_get_test_port(
+            node_uuid=self.node.uuid,
+            pxe_enabled=pxe_enabled,
+            extra=extra)
+
+        if not in_portgroup:
+            pdict.pop('portgroup_uuid')
+        else:
+            self.portgroup.standalone_ports_supported = standalone_ports
+            self.portgroup.save()
+
+        expect_errors = http_status != http_client.CREATED
+
+        response = self.post_json('/ports', pdict, headers=self.headers,
+                                  expect_errors=expect_errors)
+        self.assertEqual('application/json', response.content_type)
+        self.assertEqual(http_status, response.status_int)
+        if not expect_errors:
+            expected_portgroup_uuid = pdict.get('portgroup_uuid', None)
+            self.assertEqual(expected_portgroup_uuid,
+                             response.json['portgroup_uuid'])
+            self.assertEqual(extra, response.json['extra'])
+
+    def test_create_port_novif_pxe_noportgroup(self):
+        self._test_create_port(has_vif=False, in_portgroup=False,
+                               pxe_enabled=True,
+                               http_status=http_client.CREATED)
+
+    def test_create_port_novif_nopxe_noportgroup(self):
+        self._test_create_port(has_vif=False, in_portgroup=False,
+                               pxe_enabled=False,
+                               http_status=http_client.CREATED)
+
+    def test_create_port_vif_pxe_noportgroup(self):
+        self._test_create_port(has_vif=True, in_portgroup=False,
+                               pxe_enabled=True,
+                               http_status=http_client.CREATED)
+
+    def test_create_port_vif_nopxe_noportgroup(self):
+        self._test_create_port(has_vif=True, in_portgroup=False,
+                               pxe_enabled=False,
+                               http_status=http_client.CREATED)
+
+    def test_create_port_novif_pxe_portgroup_standalone_ports(self):
+        self._test_create_port(has_vif=False, in_portgroup=True,
+                               pxe_enabled=True,
+                               standalone_ports=True,
+                               http_status=http_client.CREATED)
+
+    def test_create_port_novif_pxe_portgroup_nostandalone_ports(self):
+        self._test_create_port(has_vif=False, in_portgroup=True,
+                               pxe_enabled=True,
+                               standalone_ports=False,
+                               http_status=http_client.CONFLICT)
+
+    def test_create_port_novif_nopxe_portgroup_standalone_ports(self):
+        self._test_create_port(has_vif=False, in_portgroup=True,
+                               pxe_enabled=False,
+                               standalone_ports=True,
+                               http_status=http_client.CREATED)
+
+    def test_create_port_novif_nopxe_portgroup_nostandalone_ports(self):
+        self._test_create_port(has_vif=False, in_portgroup=True,
+                               pxe_enabled=False,
+                               standalone_ports=False,
+                               http_status=http_client.CREATED)
+
+    def test_create_port_vif_pxe_portgroup_standalone_ports(self):
+        self._test_create_port(has_vif=True, in_portgroup=True,
+                               pxe_enabled=True,
+                               standalone_ports=True,
+                               http_status=http_client.CREATED)
+
+    def test_create_port_vif_pxe_portgroup_nostandalone_ports(self):
+        self._test_create_port(has_vif=True, in_portgroup=True,
+                               pxe_enabled=True,
+                               standalone_ports=False,
+                               http_status=http_client.CONFLICT)
+
+    def test_create_port_vif_nopxe_portgroup_standalone_ports(self):
+        self._test_create_port(has_vif=True, in_portgroup=True,
+                               pxe_enabled=False,
+                               standalone_ports=True,
+                               http_status=http_client.CREATED)
+
+    def test_create_port_vif_nopxe_portgroup_nostandalone_ports(self):
+        self._test_create_port(has_vif=True, in_portgroup=True,
+                               pxe_enabled=False,
+                               standalone_ports=False,
+                               http_status=http_client.CONFLICT)
+
 
 @mock.patch.object(rpcapi.ConductorAPI, 'destroy_port')
 class TestDelete(test_api_base.BaseApiTest):
diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py
index 2ee4ebc3d..b58884f73 100644
--- a/ironic/tests/unit/conductor/test_manager.py
+++ b/ironic/tests/unit/conductor/test_manager.py
@@ -3110,6 +3110,143 @@ class UpdatePortTestCase(mgr_utils.ServiceSetUpMixin,
         port.refresh()
         self.assertEqual(True, port.pxe_enabled)
 
+    def _test_update_port(self, has_vif=False, in_portgroup=False,
+                          pxe_enabled=True, standalone_ports=True,
+                          expect_errors=False):
+        node = obj_utils.create_test_node(self.context, driver='fake',
+                                          provision_state=states.ENROLL)
+
+        pg = obj_utils.create_test_portgroup(
+            self.context, node_id=node.id,
+            standalone_ports_supported=standalone_ports)
+
+        extra_vif = {'vif_port_id': uuidutils.generate_uuid()}
+        if has_vif:
+            extra = extra_vif
+            opposite_extra = {}
+        else:
+            extra = {}
+            opposite_extra = extra_vif
+        opposite_pxe_enabled = not pxe_enabled
+
+        pg_id = None
+        if in_portgroup:
+            pg_id = pg.id
+
+        ports = []
+
+        # Update only portgroup id on existed port with different
+        # combinations of pxe_enabled/vif_port_id
+        p1 = obj_utils.create_test_port(self.context, node_id=node.id,
+                                        uuid=uuidutils.generate_uuid(),
+                                        address="aa:bb:cc:dd:ee:01",
+                                        extra=extra,
+                                        pxe_enabled=pxe_enabled)
+        p1.portgroup_id = pg_id
+        ports.append(p1)
+
+        # Update portgroup_id/pxe_enabled/vif_port_id in one request
+        p2 = obj_utils.create_test_port(self.context, node_id=node.id,
+                                        uuid=uuidutils.generate_uuid(),
+                                        address="aa:bb:cc:dd:ee:02",
+                                        extra=opposite_extra,
+                                        pxe_enabled=opposite_pxe_enabled)
+        p2.extra = extra
+        p2.pxe_enabled = pxe_enabled
+        p2.portgroup_id = pg_id
+        ports.append(p2)
+
+        # Update portgroup_id and pxe_enabled
+        p3 = obj_utils.create_test_port(self.context, node_id=node.id,
+                                        uuid=uuidutils.generate_uuid(),
+                                        address="aa:bb:cc:dd:ee:03",
+                                        extra=extra,
+                                        pxe_enabled=opposite_pxe_enabled)
+        p3.pxe_enabled = pxe_enabled
+        p3.portgroup_id = pg_id
+        ports.append(p3)
+
+        # Update portgroup_id and vif_port_id
+        p4 = obj_utils.create_test_port(self.context, node_id=node.id,
+                                        uuid=uuidutils.generate_uuid(),
+                                        address="aa:bb:cc:dd:ee:04",
+                                        pxe_enabled=pxe_enabled,
+                                        extra=opposite_extra)
+        p4.extra = extra
+        p4.portgroup_id = pg_id
+        ports.append(p4)
+
+        for port in ports:
+            if not expect_errors:
+                res = self.service.update_port(self.context, port)
+                self.assertEqual(port.pxe_enabled, res.pxe_enabled)
+                self.assertEqual(port.portgroup_id, res.portgroup_id)
+                self.assertEqual(port.extra, res.extra)
+            else:
+                self.assertRaises(messaging.rpc.ExpectedException,
+                                  self.service.update_port,
+                                  self.context, port)
+
+    def test_update_port_novif_pxe_noportgroup(self):
+        self._test_update_port(has_vif=False, in_portgroup=False,
+                               pxe_enabled=True,
+                               expect_errors=False)
+
+    def test_update_port_novif_nopxe_noportgroup(self):
+        self._test_update_port(has_vif=False, in_portgroup=False,
+                               pxe_enabled=False,
+                               expect_errors=False)
+
+    def test_update_port_vif_pxe_noportgroup(self):
+        self._test_update_port(has_vif=True, in_portgroup=False,
+                               pxe_enabled=True,
+                               expect_errors=False)
+
+    def test_update_port_vif_nopxe_noportgroup(self):
+        self._test_update_port(has_vif=True, in_portgroup=False,
+                               pxe_enabled=False,
+                               expect_errors=False)
+
+    def test_update_port_novif_pxe_portgroup_standalone_ports(self):
+        self._test_update_port(has_vif=False, in_portgroup=True,
+                               pxe_enabled=True, standalone_ports=True,
+                               expect_errors=False)
+
+    def test_update_port_novif_pxe_portgroup_nostandalone_ports(self):
+        self._test_update_port(has_vif=False, in_portgroup=True,
+                               pxe_enabled=True, standalone_ports=False,
+                               expect_errors=True)
+
+    def test_update_port_novif_nopxe_portgroup_standalone_ports(self):
+        self._test_update_port(has_vif=False, in_portgroup=True,
+                               pxe_enabled=False, standalone_ports=True,
+                               expect_errors=False)
+
+    def test_update_port_novif_nopxe_portgroup_nostandalone_ports(self):
+        self._test_update_port(has_vif=False, in_portgroup=True,
+                               pxe_enabled=False, standalone_ports=False,
+                               expect_errors=False)
+
+    def test_update_port_vif_pxe_portgroup_standalone_ports(self):
+        self._test_update_port(has_vif=True, in_portgroup=True,
+                               pxe_enabled=True, standalone_ports=True,
+                               expect_errors=False)
+
+    def test_update_port_vif_pxe_portgroup_nostandalone_ports(self):
+        self._test_update_port(has_vif=True, in_portgroup=True,
+                               pxe_enabled=True, standalone_ports=False,
+                               expect_errors=True)
+
+    def test_update_port_vif_nopxe_portgroup_standalone_ports(self):
+        self._test_update_port(has_vif=True, in_portgroup=True,
+                               pxe_enabled=True, standalone_ports=True,
+                               expect_errors=False)
+
+    def test_update_port_vif_nopxe_portgroup_nostandalone_ports(self):
+        self._test_update_port(has_vif=True, in_portgroup=True,
+                               pxe_enabled=False, standalone_ports=False,
+                               expect_errors=True)
+
     def test__filter_out_unsupported_types_all(self):
         self._start_service()
         CONF.set_override('send_sensor_data_types', ['All'], group='conductor')
@@ -3469,6 +3606,84 @@ class UpdatePortgroupTestCase(mgr_utils.ServiceSetUpMixin,
         self.assertEqual(new_address, pg.address)
         self.assertFalse(mac_update_mock.called)
 
+    def _test_update_portgroup(self, has_vif=False, with_ports=False,
+                               pxe_enabled=True, standalone_ports=True,
+                               expect_errors=False):
+        node = obj_utils.create_test_node(self.context, driver='fake',
+                                          provision_state=states.ENROLL)
+
+        # NOTE(vsaienko) make sure that old values are opposite to new,
+        # to guarantee that object.what_changes() returns true.
+        old_standalone_ports_supported = not standalone_ports
+
+        pg = obj_utils.create_test_portgroup(
+            self.context, node_id=node.id,
+            standalone_ports_supported=old_standalone_ports_supported)
+
+        if with_ports:
+            extra = {}
+            if has_vif:
+                extra = {'vif_port_id': uuidutils.generate_uuid()}
+
+            obj_utils.create_test_port(
+                self.context, node_id=node.id, extra=extra,
+                pxe_enabled=pxe_enabled, portgroup_id=pg.id)
+
+        pg.standalone_ports_supported = standalone_ports
+
+        if not expect_errors:
+            res = self.service.update_portgroup(self.context, pg)
+            self.assertEqual(pg.standalone_ports_supported,
+                             res.standalone_ports_supported)
+        else:
+            self.assertRaises(messaging.rpc.ExpectedException,
+                              self.service.update_portgroup,
+                              self.context, pg)
+
+    def test_update_portgroup_standalone_ports_noports(self):
+        self._test_update_portgroup(with_ports=False, standalone_ports=True,
+                                    expect_errors=False)
+
+    def test_update_portgroup_standalone_ports_novif_pxe_ports(self):
+        self._test_update_portgroup(with_ports=True, standalone_ports=True,
+                                    has_vif=False, pxe_enabled=True,
+                                    expect_errors=False)
+
+    def test_update_portgroup_nostandalone_ports_novif_pxe_ports(self):
+        self._test_update_portgroup(with_ports=True, standalone_ports=False,
+                                    has_vif=False, pxe_enabled=True,
+                                    expect_errors=True)
+
+    def test_update_portgroup_nostandalone_ports_novif_nopxe_ports(self):
+        self._test_update_portgroup(with_ports=True, standalone_ports=False,
+                                    has_vif=False, pxe_enabled=False,
+                                    expect_errors=False)
+
+    def test_update_portgroup_standalone_ports_novif_nopxe_ports(self):
+        self._test_update_portgroup(with_ports=True, standalone_ports=True,
+                                    has_vif=False, pxe_enabled=False,
+                                    expect_errors=False)
+
+    def test_update_portgroup_standalone_ports_vif_pxe_ports(self):
+        self._test_update_portgroup(with_ports=True, standalone_ports=True,
+                                    has_vif=True, pxe_enabled=True,
+                                    expect_errors=False)
+
+    def test_update_portgroup_nostandalone_ports_vif_pxe_ports(self):
+        self._test_update_portgroup(with_ports=True, standalone_ports=False,
+                                    has_vif=True, pxe_enabled=True,
+                                    expect_errors=True)
+
+    def test_update_portgroup_standalone_ports_vif_nopxe_ports(self):
+        self._test_update_portgroup(with_ports=True, standalone_ports=True,
+                                    has_vif=True, pxe_enabled=False,
+                                    expect_errors=False)
+
+    def test_update_portgroup_nostandalone_ports_vif_nopxe_ports(self):
+        self._test_update_portgroup(with_ports=True, standalone_ports=False,
+                                    has_vif=True, pxe_enabled=False,
+                                    expect_errors=True)
+
 
 @mgr_utils.mock_record_keepalive
 class RaidTestCases(mgr_utils.ServiceSetUpMixin, tests_db_base.DbTestCase):
diff --git a/releasenotes/notes/rely-on-standalone-ports-supported-8153e1135787828b.yaml b/releasenotes/notes/rely-on-standalone-ports-supported-8153e1135787828b.yaml
new file mode 100644
index 000000000..23fb687e5
--- /dev/null
+++ b/releasenotes/notes/rely-on-standalone-ports-supported-8153e1135787828b.yaml
@@ -0,0 +1,25 @@
+---
+other:
+  - |
+    Some combinations of port group protocols and hardware might not support
+    falling back to single interface mode. If a static port group was created
+    under such circumstances (where
+    ``portgroup.standalone_ports_supported = False``), additional restrictions
+    apply to such ports and port groups, for example such ports will not
+    support booting over PXE.
+
+    Certain restrictions are imposed on values of port properties for ports
+    belonging to a port group:
+
+    * ``port.pxe_enabled`` cannot be set to True if the port is a member of
+      a port group with portgroup.standalone_ports_supported already
+      set to False.
+    * ``portgroup.standalone_ports_supported`` cannot be set to False on a
+      portgroup if at least one port in that port group has
+      ``port.pxe_enabled=True``
+    * ``port.extra.vif_port_id`` cannot be set on a port that is a member of
+      a port group with ``portgroup.standalone_ports_supported=False`` as
+      setting it means that we using port in single interface mode.
+    * ``portgroup.standalone_ports_supported`` cannot be set to False on a
+      port group if it has at least one port with ``port.extra.vif_port_id``
+      set.