Merge "Refactor get_physnets_by_portgroup_id"

4 files changed, 112 insertions, 31 deletions

diff --git a/ironic/common/network.py b/ironic/common/network.py
index d36c9cbff..f99e372af 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 48b11bbfc..e69b910b2 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 eaffe6930..c65cf5691 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 7afb9f446..bc5e26b6b 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)