From 43fa6fc294daaf754c426476463e6f41dad6b93b Mon Sep 17 00:00:00 2001 From: Mark Goddard Date: Tue, 11 Jul 2017 13:34:33 +0100 Subject: [PATCH] Refactor get_physnets_by_portgroup_id There is a lot of common code between the functions ironic.conductor.utils.validate_port_physnets() and ironic.drivers.modules.network.common._get_physnets_by_portgroup(). This change refactors the code, adding a new utility method, get_physnets_by_portgroup_id, which returns the physical networks associated with a portgroup. There should be at most one such physical network, and the presence of multiple will cause PortgroupPhysnetInconsistent to be raised. Change-Id: I8f01dcc5eaa0c8511ce77622e41db88e27791327 Related-Bug: #1666009 --- ironic/common/network.py | 30 ++++++++++ ironic/conductor/utils.py | 14 +---- ironic/drivers/modules/network/common.py | 23 +------ ironic/tests/unit/common/test_network.py | 76 ++++++++++++++++++++++++ 4 files changed, 112 insertions(+), 31 deletions(-) diff --git a/ironic/common/network.py b/ironic/common/network.py index d36c9cbff7..f99e372af6 100644 --- a/ironic/common/network.py +++ b/ironic/common/network.py @@ -12,6 +12,8 @@ # License for the specific language governing permissions and limitations # under the License. +from ironic.common import exception + def get_node_vif_ids(task): """Get all VIF ids for a node. @@ -78,3 +80,31 @@ def get_physnets_for_node(task): return set(port.physical_network for port in task.ports if port.physical_network is not None) + + +def get_physnets_by_portgroup_id(task, portgroup_id, exclude_port=None): + """Return the set of physical networks associated with a portgroup. + + :param task: a TaskManager instance. + :param portgroup_id: ID of the portgroup. + :param exclude_port: A Port object to exclude from the determination of the + portgroup's physical network, or None. + :returns: The set of physical networks associated with the portgroup. The + set will contain zero or one physical networks. + :raises: PortgroupPhysnetInconsistent if the portgroup's ports are not + assigned the same physical network. + """ + pg_ports = get_ports_by_portgroup_id(task, portgroup_id) + if exclude_port is not None and 'id' in exclude_port: + exclude_port_id = exclude_port.id + else: + exclude_port_id = None + pg_physnets = set(port.physical_network + for port in pg_ports + if port.id != exclude_port_id) + # Sanity check: all ports should have the same physical network. + if len(pg_physnets) > 1: + portgroup = get_portgroup_by_id(task, portgroup_id) + raise exception.PortgroupPhysnetInconsistent( + portgroup=portgroup.uuid, physical_networks=", ".join(pg_physnets)) + return pg_physnets diff --git a/ironic/conductor/utils.py b/ironic/conductor/utils.py index 48b11bbfc8..e69b910b23 100644 --- a/ironic/conductor/utils.py +++ b/ironic/conductor/utils.py @@ -599,21 +599,13 @@ def validate_port_physnet(task, port_obj): return # Determine the current physical network of the portgroup. - pg_ports = network.get_ports_by_portgroup_id(task, port_obj.portgroup_id) - port_obj_id = port_obj.id if 'id' in port_obj else None - pg_physnets = {port.physical_network - for port in pg_ports if port.id != port_obj_id} + pg_physnets = network.get_physnets_by_portgroup_id(task, + port_obj.portgroup_id, + exclude_port=port_obj) if not pg_physnets: return - # Sanity check that all existing ports in the group have the same - # physical network (should never happen). - if len(pg_physnets) > 1: - portgroup = network.get_portgroup_by_id(task, port_obj.portgroup_id) - raise exception.PortgroupPhysnetInconsistent( - portgroup=portgroup.uuid, physical_networks=", ".join(pg_physnets)) - # Check that the port has the same physical network as any existing # member ports. pg_physnet = pg_physnets.pop() diff --git a/ironic/drivers/modules/network/common.py b/ironic/drivers/modules/network/common.py index d5e5f4afac..01eb750f86 100644 --- a/ironic/drivers/modules/network/common.py +++ b/ironic/drivers/modules/network/common.py @@ -140,25 +140,6 @@ def _get_free_portgroups_and_ports(task, vif_id, physnets): return free_port_like_objs -def _get_physnet_for_portgroup(task, portgroup): - """Return the physical network associated with a portgroup. - - :param task: a TaskManager instance. - :param portgroup: a Portgroup object. - :returns: The physical network associated with the portgroup. - :raises: PortgroupPhysnetInconsistent if the portgroup's ports are not - assigned the same physical network. - """ - pg_ports = network.get_ports_by_portgroup_id(task, portgroup.id) - pg_physnets = {port.physical_network for port in pg_ports} - # Sanity check: there should be at least one port in the portgroup and - # all ports should have the same physical network. - if len(pg_physnets) != 1: - raise exception.PortgroupPhysnetInconsistent( - portgroup=portgroup.uuid, physical_networks=", ".join(pg_physnets)) - return pg_physnets.pop() - - def get_free_port_like_object(task, vif_id, physnets): """Find free port-like object (portgroup or port) VIF will be attached to. @@ -207,7 +188,9 @@ def get_free_port_like_object(task, vif_id, physnets): """ is_pg = isinstance(port_like_obj, objects.Portgroup) if is_pg: - pg_physnet = _get_physnet_for_portgroup(task, port_like_obj) + pg_physnets = network.get_physnets_by_portgroup_id( + task, port_like_obj.id) + pg_physnet = pg_physnets.pop() physnet_matches = pg_physnet in physnets pxe_enabled = True else: diff --git a/ironic/tests/unit/common/test_network.py b/ironic/tests/unit/common/test_network.py index 7afb9f4467..bc5e26b6b5 100644 --- a/ironic/tests/unit/common/test_network.py +++ b/ironic/tests/unit/common/test_network.py @@ -15,6 +15,7 @@ from oslo_utils import uuidutils +from ironic.common import exception from ironic.common import network from ironic.conductor import task_manager from ironic.tests.unit.conductor import mgr_utils @@ -227,3 +228,78 @@ class GetPhysnetsForNodeTestCase(db_base.DbTestCase): with task_manager.acquire(self.context, node.uuid) as task: res = network.get_physnets_for_node(task) self.assertEqual({'physnet1', 'physnet2'}, res) + + +class GetPhysnetsByPortgroupID(db_base.DbTestCase): + + def setUp(self): + super(GetPhysnetsByPortgroupID, self).setUp() + self.node = object_utils.create_test_node(self.context, driver='fake') + self.portgroup = object_utils.create_test_portgroup( + self.context, node_id=self.node.id) + + def _test(self, expected_result, exclude_port=None): + with task_manager.acquire(self.context, self.node.uuid) as task: + result = network.get_physnets_by_portgroup_id(task, + self.portgroup.id, + exclude_port) + self.assertEqual(expected_result, result) + + def test_empty(self): + self._test(set()) + + def test_one_port(self): + object_utils.create_test_port(self.context, node_id=self.node.id, + portgroup_id=self.portgroup.id, + physical_network='physnet1') + self._test({'physnet1'}) + + def test_two_ports(self): + object_utils.create_test_port(self.context, node_id=self.node.id, + portgroup_id=self.portgroup.id, + physical_network='physnet1') + object_utils.create_test_port(self.context, node_id=self.node.id, + uuid=uuidutils.generate_uuid(), + address='00:11:22:33:44:55', + portgroup_id=self.portgroup.id, + physical_network='physnet1') + self._test({'physnet1'}) + + def test_exclude_port(self): + object_utils.create_test_port(self.context, node_id=self.node.id, + portgroup_id=self.portgroup.id, + physical_network='physnet1') + port2 = object_utils.create_test_port(self.context, + node_id=self.node.id, + uuid=uuidutils.generate_uuid(), + address='00:11:22:33:44:55', + portgroup_id=self.portgroup.id, + physical_network='physnet2') + self._test({'physnet1'}, port2) + + def test_exclude_port_no_id(self): + # During port creation there may be no 'id' field. + object_utils.create_test_port(self.context, node_id=self.node.id, + portgroup_id=self.portgroup.id, + physical_network='physnet1') + port2 = object_utils.get_test_port(self.context, + node_id=self.node.id, + uuid=uuidutils.generate_uuid(), + address='00:11:22:33:44:55', + portgroup_id=self.portgroup.id, + physical_network='physnet2') + self._test({'physnet1'}, port2) + + def test_two_ports_inconsistent(self): + object_utils.create_test_port(self.context, node_id=self.node.id, + portgroup_id=self.portgroup.id, + physical_network='physnet1') + object_utils.create_test_port(self.context, node_id=self.node.id, + uuid=uuidutils.generate_uuid(), + address='00:11:22:33:44:55', + portgroup_id=self.portgroup.id, + physical_network='physnet2') + with task_manager.acquire(self.context, self.node.uuid) as task: + self.assertRaises(exception.PortgroupPhysnetInconsistent, + network.get_physnets_by_portgroup_id, + task, self.portgroup.id)