[ACL] Include service port into upstream neighbors by Gfrom2016 · Pull Request #18671 · sonic-net/sonic-mgmt

Gfrom2016 · 2025-05-28T09:27:25Z

Description of PR

Summary:
Fixes # (issue)
In the topology with service ports, like t0-d18u8s4, t0-isolated-d16u16s1, t0-isolated-d32u32s2. we should include the PT0 neighbor port in the upstream neighbor list. With the enhancement in #18516, we can achieve this by adding pt0 in UPSTREAM_ALL_NEIGHBOR_MAP.

Type of change

Back port request

Approach

What is the motivation for this PR?

To include service ports into upstream neighbors list.

How did you do it?

Update the UPSTREAM_ALL_NEIGHBOR_MAP.

How did you verify/test it?

Run acl test and passed.

Signed-off-by: zitingguo-ms zitingguo@microsoft.com

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

mssonicbld · 2025-05-28T09:27:33Z

/azp run

azure-pipelines · 2025-05-28T09:27:46Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-05-28T09:32:12Z

/azp run

azure-pipelines · 2025-05-28T09:32:28Z

Azure Pipelines successfully started running 1 pipeline(s).

tests/acl/test_acl.py

mssonicbld · 2025-05-29T11:32:34Z

/azp run

azure-pipelines · 2025-05-29T11:32:46Z

Azure Pipelines successfully started running 1 pipeline(s).

r12f · 2025-06-03T03:13:49Z

tests/acl/test_acl.py

            or 't1-isolated' in tbinfo["topo"]["name"]
        )
-        and tbinfo["topo"]["name"] not in TOPOLOGY_WITH_SERVICE_PORT
+        and not re.match(r"t0-.*-s\d+$", tbinfo["topo"]["name"])


.*-s\d+

this might not work, I believe you mean .*s\d+?

mssonicbld · 2025-06-03T03:22:42Z

/azp run

azure-pipelines · 2025-06-03T03:22:48Z

Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

Signed-off-by: zitingguo-ms <zitingguo@microsoft.com>

r12f · 2025-06-03T03:26:03Z

tests/acl/test_acl.py

            or 't1-isolated' in tbinfo["topo"]["name"]
        )
-        and not re.match(r"t0-.*-s\d+$", tbinfo["topo"]["name"])
+        and not re.match(r"t0-.*-s\d+", tbinfo["topo"]["name"])


hi Ziting, I believe this is the actual issue:

Signed-off-by: zitingguo-ms <zitingguo@microsoft.com>

Signed-off-by: zitingguo <zitingguo@microsoft.com>

Signed-off-by: zitingguo-ms <zitingguo@microsoft.com>

mssonicbld · 2025-06-03T03:30:11Z

/azp run

azure-pipelines · 2025-06-03T03:30:25Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-06-03T03:32:56Z

/azp run

azure-pipelines · 2025-06-03T03:33:08Z

Azure Pipelines successfully started running 1 pipeline(s).

sdszhang

lgtm

mssonicbld · 2025-06-05T13:42:13Z

@Gfrom2016 PR conflicts with 202411 branch

mssonicbld · 2025-06-06T06:26:13Z

@Gfrom2016 PR conflicts with 202505 branch

yejianquan · 2025-06-06T06:26:44Z

@Gfrom2016 please fix the conflict and create PR to 202505 branch

Summary: Fixes # (issue) In the topology with service ports, like t0-d18u8s4, t0-isolated-d16u16s1, t0-isolated-d32u32s2. we should include the PT0 neighbor port in the upstream neighbor list. With the enhancement in sonic-net#18516, we can achieve this by adding pt0 in UPSTREAM_ALL_NEIGHBOR_MAP.

…t into upstream neighbors in ACL tests (#18847) * [M1] Collect all upstream ports for ACL test (#18516) What is the motivation for this PR? In M1 topo, we have 2 upstream neighbor types: MA and MB. We need to collect all upstream ports to avoid packet capture flaky failure. How did you do it? Add function to read all up/down stream neighbor types. How did you verify/test it? Verified by ACL test on M1. * [ACL] Include service port into upstream neighbors (#18671) Summary: Fixes # (issue) In the topology with service ports, like t0-d18u8s4, t0-isolated-d16u16s1, t0-isolated-d32u32s2. we should include the PT0 neighbor port in the upstream neighbor list. With the enhancement in #18516, we can achieve this by adding pt0 in UPSTREAM_ALL_NEIGHBOR_MAP. --------- Co-authored-by: Zhijian Li <zhijianli@microsoft.com>

Summary: Fixes # (issue) In the topology with service ports, like t0-d18u8s4, t0-isolated-d16u16s1, t0-isolated-d32u32s2. we should include the PT0 neighbor port in the upstream neighbor list. With the enhancement in sonic-net#18516, we can achieve this by adding pt0 in UPSTREAM_ALL_NEIGHBOR_MAP. Signed-off-by: opcoder0 <110003254+opcoder0@users.noreply.github.com>

Summary: Fixes # (issue) In the topology with service ports, like t0-d18u8s4, t0-isolated-d16u16s1, t0-isolated-d32u32s2. we should include the PT0 neighbor port in the upstream neighbor list. With the enhancement in sonic-net#18516, we can achieve this by adding pt0 in UPSTREAM_ALL_NEIGHBOR_MAP. Signed-off-by: Aharon Malkin <amalkin@nvidia.com>

Summary: Fixes # (issue) In the topology with service ports, like t0-d18u8s4, t0-isolated-d16u16s1, t0-isolated-d32u32s2. we should include the PT0 neighbor port in the upstream neighbor list. With the enhancement in sonic-net#18516, we can achieve this by adding pt0 in UPSTREAM_ALL_NEIGHBOR_MAP. Signed-off-by: Guy Shemesh <gshemesh@nvidia.com>

Summary: Fixes # (issue) In the topology with service ports, like t0-d18u8s4, t0-isolated-d16u16s1, t0-isolated-d32u32s2. we should include the PT0 neighbor port in the upstream neighbor list. With the enhancement in sonic-net#18516, we can achieve this by adding pt0 in UPSTREAM_ALL_NEIGHBOR_MAP.

Summary: Fixes # (issue) In the topology with service ports, like t0-d18u8s4, t0-isolated-d16u16s1, t0-isolated-d32u32s2. we should include the PT0 neighbor port in the upstream neighbor list. With the enhancement in sonic-net#18516, we can achieve this by adding pt0 in UPSTREAM_ALL_NEIGHBOR_MAP. Signed-off-by: Guy Shemesh <gshemesh@nvidia.com>

Summary: Fixes # (issue) In the topology with service ports, like t0-d18u8s4, t0-isolated-d16u16s1, t0-isolated-d32u32s2. we should include the PT0 neighbor port in the upstream neighbor list. With the enhancement in sonic-net#18516, we can achieve this by adding pt0 in UPSTREAM_ALL_NEIGHBOR_MAP. Signed-off-by: Yael Tzur <ytzur@nvidia.com>

Gfrom2016 changed the title ~~Include service port into upstream neighbors~~ [ACL] Include service port into upstream neighbors May 28, 2025

Gfrom2016 mentioned this pull request May 28, 2025

Bug: [service port] list of upstream/downstream ports not correct in topo with service ports #18061

Closed

Gfrom2016 added Request for 202411 branch Request for 202505 branch labels May 28, 2025

r12f reviewed May 28, 2025

View reviewed changes

tests/acl/test_acl.py Outdated Show resolved Hide resolved

r12f reviewed Jun 3, 2025

View reviewed changes

[ACL] Fix service port not included in upstream neighbor

f9d1ab4

Signed-off-by: zitingguo-ms <zitingguo@microsoft.com>

r12f reviewed Jun 3, 2025

View reviewed changes

Gfrom2016 added 4 commits June 3, 2025 03:27

[test_stress_acl] Fix service port not in upsream neighbor issue

510f6fd

Signed-off-by: zitingguo-ms <zitingguo@microsoft.com>

[ptf_helper] Include PT0 neighbor in upstream_links

422cbdc

Signed-off-by: zitingguo-ms <zitingguo@microsoft.com>

Use regex to find out topo with service port

005e13b

Signed-off-by: zitingguo <zitingguo@microsoft.com>

Fix regex

bbbf1f8

Signed-off-by: zitingguo-ms <zitingguo@microsoft.com>

Gfrom2016 force-pushed the include_service_port_in_acl branch from 2f8d7b3 to bbbf1f8 Compare June 3, 2025 03:30

Fix

b70673e

sdszhang approved these changes Jun 5, 2025

View reviewed changes

StormLiangMS merged commit 5fcd219 into sonic-net:master Jun 5, 2025
18 checks passed

StormLiangMS added the Approved for 202411 branch label Jun 5, 2025

mssonicbld added the Cherry Pick Conflict_202411 label Jun 5, 2025

yejianquan added the Approved for 202505 branch label Jun 6, 2025

mssonicbld added the Cherry Pick Conflict_202505 label Jun 6, 2025

Gfrom2016 mentioned this pull request Jun 6, 2025

[Cherry-pick][ACL] Include service port into upstream neighbors #18846

Merged

Gfrom2016 mentioned this pull request Jun 6, 2025

[Cherry-pick][ACL] Collect all upstream ports and Include service port into upstream neighbors in ACL tests #18847

Merged

yejianquan added Included in 202505 branch and removed Cherry Pick Conflict_202505 labels Jun 6, 2025

sdszhang mentioned this pull request Jun 30, 2025

[202505] [PR #18152,#18683,#18444,#18826 ] Cherry-pick isolated topo changes #19254

Merged

11 tasks

Conversation

Gfrom2016 commented May 28, 2025

Description of PR

Type of change

Back port request

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

Uh oh!

mssonicbld commented May 28, 2025

Uh oh!

azure-pipelines bot commented May 28, 2025

Uh oh!

mssonicbld commented May 28, 2025

Uh oh!

azure-pipelines bot commented May 28, 2025

Uh oh!

Uh oh!

mssonicbld commented May 29, 2025

Uh oh!

azure-pipelines bot commented May 29, 2025

Uh oh!

r12f Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

mssonicbld commented Jun 3, 2025

Uh oh!

azure-pipelines bot commented Jun 3, 2025

Uh oh!

r12f Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

mssonicbld commented Jun 3, 2025

Uh oh!

azure-pipelines bot commented Jun 3, 2025

Uh oh!

mssonicbld commented Jun 3, 2025

Uh oh!

azure-pipelines bot commented Jun 3, 2025

Uh oh!

sdszhang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mssonicbld commented Jun 5, 2025

Uh oh!

mssonicbld commented Jun 6, 2025

Uh oh!

yejianquan commented Jun 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants