From c8d518898e27951123ad7816337d2a0f862ea5b4 Mon Sep 17 00:00:00 2001 From: Vasyl Saienko Date: Thu, 25 Aug 2016 11:25:32 -0400 Subject: [PATCH] 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 --- ironic/api/controllers/v1/port.py | 16 +- ironic/conductor/manager.py | 46 +++- ironic/tests/unit/api/v1/test_ports.py | 109 +++++++++ ironic/tests/unit/conductor/test_manager.py | 215 ++++++++++++++++++ ...lone-ports-supported-8153e1135787828b.yaml | 25 ++ 5 files changed, 407 insertions(+), 4 deletions(-) create mode 100644 releasenotes/notes/rely-on-standalone-ports-supported-8153e1135787828b.yaml diff --git a/ironic/api/controllers/v1/port.py b/ironic/api/controllers/v1/port.py index a64dbd7be1..7fac0da03a 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 fd63ce6314..35730e9333 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 1b8b717c9e..43bcc34090 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 2ee4ebc3d3..b58884f73a 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 0000000000..23fb687e5b --- /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.