Fix ngs_allowed_vlans validation

The validation was added in [0] was not covering all cases
as port parameters may be updated when port is actually bound
and they may be propagated via update_port_postcommit function.
Move check added in [0] to _is_link_valid method which is
called in all pleases where we handle ports.

Make bind_port lighweigh and do not call heavy switch functions
as it stated in docstring. All actual work will should be done in
update_port_postcommit.

[0] https://review.opendev.org/c/openstack/networking-generic-switch/+/888047

Change-Id: Ib6e6557fd14621c628fd80158fd4974408ae63dc

Author: jumpojoy

networking_generic_switch/generic_switch_mech.py

@@ -466,6 +466,10 @@ def bind_port(self, context):
         can use with ports.
         """
 
+        # NOTE(vsaienko): Important that this method remains lightweight.
+        # The actual port handling is done in update_port_postcommit.
+        # For more info please read docstring.
+
         port = context.current
         network = context.network.current
         binding_profile = port['binding:profile']
@@ -477,44 +481,7 @@ def bind_port(self, context):
             if not self._is_link_valid(port, network):
                 return
 
-            is_802_3ad = self._is_802_3ad(port)
-            for link in local_link_information:
-                port_id = link.get('port_id')
-                switch_info = link.get('switch_info')
-                switch_id = link.get('switch_id')
-                switch = device_utils.get_switch_device(
-                    self.switches, switch_info=switch_info,
-                    ngs_mac_address=switch_id)
-
-                segments = context.segments_to_bind
-                # If segmentation ID is None, set vlan 1
-                segmentation_id = segments[0].get('segmentation_id') or 1
-
-                # Fail if port or vlan not in allow list
-                if not switch.is_allowed(port_id, segmentation_id):
-                    LOG.warn("Skipped binding port %(port_id)s, "
-                             "port %(port)s in segment "
-                             "%(segment_id)s on device %(device)s, as either "
-                             "the port or vlan is not on the allow list",
-                             {'port_id': port['id'], 'port': port_id,
-                              'device': switch_info,
-                              'segment_id': segmentation_id})
-                    return
-
-                LOG.debug("Putting port %(port_id)s on %(switch_info)s "
-                          "to vlan: %(segmentation_id)s",
-                          {'port_id': port_id, 'switch_info': switch_info,
-                           'segmentation_id': segmentation_id})
-                # Move port to network
-                if is_802_3ad and hasattr(switch, 'plug_bond_to_network'):
-                    switch.plug_bond_to_network(port_id, segmentation_id)
-                else:
-                    switch.plug_port_to_network(port_id, segmentation_id)
-                LOG.info("Successfully bound port %(port_id)s in segment "
-                         "%(segment_id)s on device %(device)s",
-                         {'port_id': port['id'], 'device': switch_info,
-                          'segment_id': segmentation_id})
-
+            segments = context.segments_to_bind
             context.set_binding(segments[0][api.ID],
                                 portbindings.VIF_TYPE_OTHER, {})
 
@@ -540,6 +507,7 @@ def _is_link_valid(self, port, network):
         local_link_information = binding_profile.get('local_link_information')
 
         for link in local_link_information:
+            port_id = link.get('port_id')
             switch_info = link.get('switch_info')
             switch_id = link.get('switch_id')
             switch = device_utils.get_switch_device(
@@ -555,6 +523,7 @@ def _is_link_valid(self, port, network):
 
             physnet = network['provider:physical_network']
             switch_physnets = switch._get_physical_networks()
+            segmentation_id = network.get('provider:segmentation_id') or 1
 
             if switch_physnets and physnet not in switch_physnets:
                 LOG.error("Cannot bind port %(port)s as device %(device)s "
@@ -563,6 +532,17 @@ def _is_link_valid(self, port, network):
                           {'port': port['id'], 'device': switch_info,
                            'physnet': physnet})
                 return False
+
+            # Fail if port or vlan not in allow list
+            if not switch.is_allowed(port_id, segmentation_id):
+                LOG.warn("Skipped binding port %(port_id)s, "
+                         "port %(port)s in segment "
+                         "%(segment_id)s on device %(device)s, as either "
+                         "the port or vlan is not on the allow list",
+                         {'port_id': port['id'], 'port': port_id,
+                          'device': switch_info,
+                          'segment_id': segmentation_id})
+                return False
         return True
 
     @staticmethod

networking_generic_switch/tests/unit/test_generic_switch_mech.py

@@ -904,7 +904,7 @@ def test_bind_port(self, m_apc, m_list):
                                       mock_context.current['id'],
                                       resources.PORT,
                                       'GENERICSWITCH')
-        self.switch_mock.plug_port_to_network.assert_called()
+        self.switch_mock.plug_port_to_network.assert_not_called()
 
     @mock.patch.object(provisioning_blocks, 'add_provisioning_component',
                        autospec=True)
@@ -946,7 +946,7 @@ def test_bind_portgroup(self, m_apc, m_list):
                                           mock_context.current['id'],
                                           resources.PORT,
                                           'GENERICSWITCH')])
-        self.switch_mock.plug_port_to_network.assert_called()
+        self.switch_mock.plug_port_to_network.assert_not_called()
 
     @mock.patch.object(provisioning_blocks, 'add_provisioning_component',
                        autospec=True)
@@ -993,7 +993,7 @@ def test_bind_portgroup_802_3ad(self, m_apc, m_list):
                                           resources.PORT,
                                           'GENERICSWITCH')])
         self.switch_mock.plug_port_to_network.assert_not_called()
-        self.switch_mock.plug_bond_to_network.assert_called()
+        self.switch_mock.plug_bond_to_network.assert_not_called()
 
     @mock.patch.object(provisioning_blocks, 'add_provisioning_component',
                        autospec=True)
@@ -1030,7 +1030,8 @@ def test_bind_port_with_physnet(self, m_apc, m_list):
                                       mock_context.current['id'],
                                       resources.PORT,
                                       'GENERICSWITCH')
-        self.switch_mock.plug_port_to_network.assert_called()
+        # NOTE(vsaienko): make sure we do not call heavy methods in bind_port
+        self.switch_mock.plug_port_to_network.assert_not_called()
 
     @mock.patch.object(provisioning_blocks, 'add_provisioning_component',
                        autospec=True)