sai_test config: drop redundant bridge_id, env-gated port bring-up, fix v4/v6 NHG port-list aliasing (#2301)

Context / motivation

Part of the SAIVPP unit-test framework (see PR 1 / our docker-sai-test-vpp/devdocs/, esp. the 6-19 entry). Three independent correctness/robustness fixes in the OCP sai_test config helpers that the suite needs when run against the VPP SAI backend.

What this change does

test/sai_test/config/port_configer.py — drop bridge_id on create_bridge_port. Passing bridge_id to sai_thrift_create_bridge_port caused a create failure in our backend; the default 1Q bridge is used, so the argument is redundant. Removing it lets bridge-port creation succeed.

test/sai_test/config/port_configer.py — env-gated, bounded port bring-up wait. turn_up_and_get_checked_ports() waited per port, serially (retries × sleep) for oper-status UP. On a 32-port topology where oper-status is slow to settle, this is ~60s+ of dead time per common-config build. The wait is now tunable via env (SAI_PORT_UP_RETRIES, SAI_PORT_UP_POLL_INTERVAL, SAI_PORT_UP_SHARED_WAIT) and can poll all ports together in one bounded window. Defaults preserve the original behavior (per-port wait), so real-HW/other OCP consumers are unchanged unless they opt in; only our harness sets the fast values.

test/sai_test/config/route_configer.py — give v4/v6 NHGs independent member_port_indexs. create_nexthop_group_by_nexthops() constructed the v4 and v6 NexthopGroup objects sharing one Python list object for member_port_indexs. A mutation on one group (member remove/re-add tests) then corrupted the other's port list (ValueError: list.remove(x): x not in list). Each group now gets its own list(...) copy. (Latent aliasing bug, independent of any harness specifics.)

Scope / risk

2 files, +64/−25 — test-config helpers only; no change to SAI/backend code or packet semantics.
The port-up change is opt-in via env with original defaults, so it does not alter timing for existing consumers.
The bridge_id removal relies on the default 1Q bridge (already how these tests are used).
Dependencies
None. Related to #2299 and #2300, however, their edits are isolated and each target master.

Author: nicholasching

test/sai_test/config/port_configer.py

@@ -18,6 +18,7 @@
 #
 #
 
+import os
 from collections import OrderedDict
 from ptf import config
 from sai_utils import *  # pylint: disable=wildcard-import; lgtm[py/polluting-import]
@@ -120,7 +121,6 @@ def create_bridge_ports(self, bridge_id, port_list: List['Port']):
         for index, item in enumerate(port_list):
             port_bp = sai_thrift_create_bridge_port(
                 self.client,
-                bridge_id=bridge_id,
                 port_id=item.oid,
                 type=SAI_BRIDGE_PORT_TYPE_PORT,
                 admin_state=True)
@@ -498,7 +498,22 @@ def turn_up_and_get_checked_ports(self, port_list: List['Port']):
         '''
 
         # For brcm devices, need to init and setup the ports at once after start the switch.
-        retries = 10
+        #
+        # Waiting strategy is configurable via environment, defaulting to the original
+        # behavior (retries=2, 1s poll interval). The original loop waited PER PORT
+        # serially (retries x interval seconds for each of N ports), which on a large
+        # port count where oper-status is slow to settle costs N * retries * interval
+        # seconds (e.g. 32 ports x 2 x 1s ~= 64s). The shared-wait path below issues
+        # admin-up to every port once, then polls ALL ports together for at most
+        # (retries x interval) seconds total, re-asserting admin-up on stragglers each
+        # round. Semantics are preserved: every port is set admin-up, oper-status is
+        # polled, and down_port_list / the returned list are identical; only the total
+        # wall-clock wait changes. Real-HW/ASIC consumers that do not set these env
+        # vars keep the exact original timing.
+        retries = int(os.environ.get('SAI_PORT_UP_RETRIES', '2'))
+        poll_interval = float(os.environ.get('SAI_PORT_UP_POLL_INTERVAL', '1'))
+        # Opt-in: poll all ports together in one bounded loop instead of per-port.
+        shared_wait = os.environ.get('SAI_PORT_UP_SHARED_WAIT', '0') == '1'
         down_port_list = []
         test_port_list:List[Port] = []
 
@@ -514,29 +529,49 @@ def turn_up_and_get_checked_ports(self, port_list: List['Port']):
                         port_oid=port.oid,
                         admin_state=True)
 
-        for index, port in enumerate(test_port_list):
-            port_attr = sai_thrift_get_port_attribute(
-                self.client, port.oid, oper_status=True)
-            print("Turn up port {}".format(index))
-            port_up = True
-            if port_attr['oper_status'] != SAI_PORT_OPER_STATUS_UP:
-                port_up = False
-                for num_of_tries in range(retries):
+        if shared_wait:
+            # Single bounded wait shared across all ports.
+            pending = list(enumerate(test_port_list))
+            for num_of_tries in range(retries + 1):
+                still_down = []
+                for index, port in pending:
                     port_attr = sai_thrift_get_port_attribute(
                         self.client, port.oid, oper_status=True)
-                    if port_attr['oper_status'] == SAI_PORT_OPER_STATUS_UP:
-                        port_up = True
-                        break
-                    time.sleep(3)
-                    self.log_port_state(port, index)
-                    print("port {} , local index {} id {} is not up, status: {}. Retry. Reset Admin State.".format(
-                        index, port.port_index, port.oid, port_attr['oper_status']))
-                    sai_thrift_set_port_attribute(
-                        self.client,
-                        port_oid=port.oid,
-                        admin_state=True)
-            if not port_up:
-                down_port_list.append(index)
+                    if port_attr['oper_status'] != SAI_PORT_OPER_STATUS_UP:
+                        still_down.append((index, port))
+                pending = still_down
+                if not pending:
+                    break
+                if num_of_tries < retries:
+                    time.sleep(poll_interval)
+                    for index, port in pending:
+                        sai_thrift_set_port_attribute(
+                            self.client, port_oid=port.oid, admin_state=True)
+            down_port_list = [index for index, _ in pending]
+        else:
+            for index, port in enumerate(test_port_list):
+                port_attr = sai_thrift_get_port_attribute(
+                    self.client, port.oid, oper_status=True)
+                print("Turn up port {}".format(index))
+                port_up = True
+                if port_attr['oper_status'] != SAI_PORT_OPER_STATUS_UP:
+                    port_up = False
+                    for num_of_tries in range(retries):
+                        port_attr = sai_thrift_get_port_attribute(
+                            self.client, port.oid, oper_status=True)
+                        if port_attr['oper_status'] == SAI_PORT_OPER_STATUS_UP:
+                            port_up = True
+                            break
+                        time.sleep(poll_interval)
+                        self.log_port_state(port, index)
+                        print("port {} , local index {} id {} is not up, status: {}. Retry. Reset Admin State.".format(
+                            index, port.port_index, port.oid, port_attr['oper_status']))
+                        sai_thrift_set_port_attribute(
+                            self.client,
+                            port_oid=port.oid,
+                            admin_state=True)
+                if not port_up:
+                    down_port_list.append(index)
         if down_port_list:
             print("Ports {} are  down after retries.".format(down_port_list))
         return test_port_list

test/sai_test/config/route_configer.py

@@ -639,9 +639,13 @@ def create_nexthop_group_by_nexthops(self, nexthopv4_list: List[Nexthop], nextho
             nhp_grpv4_members.append(nhp_grpv4_member)
             nhp_grpv6_members.append(nhp_grpv6_member)
 
+        # Give each NHG its own copy of the member port-index list. The v4 and v6
+        # groups are mutated independently (remove/re-add member tests), so sharing
+        # one list object would let a v4 mutation corrupt the v6 group's port list
+        # (ValueError: x not in list) when both run in the same config group.
         member_port_indexs = [17, 18, 19, 20, 21, 22, 23, 24]
-        nhp_grpv4: NexthopGroup = NexthopGroup(nhop_groupv4_id, nhp_grpv4_members, member_port_indexs)
-        nhp_grpv6: NexthopGroup = NexthopGroup(nhop_groupv6_id, nhp_grpv6_members, member_port_indexs)
+        nhp_grpv4: NexthopGroup = NexthopGroup(nhop_groupv4_id, nhp_grpv4_members, list(member_port_indexs))
+        nhp_grpv6: NexthopGroup = NexthopGroup(nhop_groupv6_id, nhp_grpv6_members, list(member_port_indexs))
 
         self.test_obj.dut.nhp_grpv4_list.append(nhp_grpv4)
         self.test_obj.dut.nhp_grpv6_list.append(nhp_grpv6)