Skip to content

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

Open
nicholasching wants to merge 3 commits into
opencomputeproject:masterfrom
nicholasching:sai_vpp_ut_saitest_config
Open

sai_test config: drop redundant bridge_id, env-gated port bring-up, fix v4/v6 NHG port-list aliasing#2301
nicholasching wants to merge 3 commits into
opencomputeproject:masterfrom
nicholasching:sai_vpp_ut_saitest_config

Conversation

@nicholasching

Copy link
Copy Markdown

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.

Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
…default option unchanged

Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
@tjchadaga

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

for index, item in enumerate(port_list):
port_bp = sai_thrift_create_bridge_port(
self.client,
bridge_id=bridge_id,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How will this change affect other platforms? Can this be fixed in vpp platform?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the SAI spec, SAI_BRIDGE_PORT_ATTR_BRIDGE_ID is not valid for SAI_BRIDGE_PORT_TYPE_PORT.

SAI/inc/saibridge.h

Lines 182 to 190 in 0aba236

/**
* @brief Associated bridge id
*
* @type sai_object_id_t
* @flags MANDATORY_ON_CREATE | CREATE_AND_SET
* @objects SAI_OBJECT_TYPE_BRIDGE
* @condition SAI_BRIDGE_PORT_ATTR_TYPE == SAI_BRIDGE_PORT_TYPE_SUB_PORT or SAI_BRIDGE_PORT_ATTR_TYPE == SAI_BRIDGE_PORT_TYPE_1D_ROUTER or SAI_BRIDGE_PORT_ATTR_TYPE == SAI_BRIDGE_PORT_TYPE_TUNNEL or SAI_BRIDGE_PORT_ATTR_TYPE == SAI_BRIDGE_PORT_TYPE_BRIDGE_PORT_NEXT_HOP_GROUP
*/
SAI_BRIDGE_PORT_ATTR_BRIDGE_ID,

For this type, the bridge port is implicitly associated with the switch’s default 1Q bridge and bridge_id should not be passed on create.

VPP (via saiserver) was using the sonic-sairedis meta, enforcing the @condition above and rejected the extra bridge_id for PORT type. This was causing issues during the tests where create failed at SAI API validation. Since bridge_id should not be included for PORT type according to saibridge.h, I believe VPP's enforcement is correct and the fix would be relevant to other platforms as well; they were likely ignoring the extra attribute before.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it. I see create_bridge_ports has only one caller and the type is PORT. We can safely remove bridge id.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants