Add support for recirc port and everflow #1530

ysmanman · 2020-12-07T20:46:27Z

This PR adds recirc port and Everflow support in distributed VOQ chassis.

What I did

Recirc prot changes:

portsyncd includes recirc ports in total port count.
portsyncd does not add recirc ports to g_portSet because no host intfs are created for recirc ports.
portsorch discovers recirc ports by checking port role setting and adds them correspondingly.
intforch supports adding rif for recirc ports.

Everflow changes:

mirrororch allows mirror dst port to be system port.
mirrororch sets mirror dst port to recirc port in voq switch.
mirrororch sets router mac as mirror dst mac in voq switch.

Why I did it

Support recirc port and Everflow

How I verified it

Tested in a distributed VOQ system with BCM ASICs.

Details if related

This PR has some dependency on PR #1431 and some minor change is needed once that PR is merged.

Recirc prot changes: - portsyncd includes recirc ports in total port count. - portsyncd does not add recirc ports to g_portSet because no host intfs are created for recirc ports. - portsorch discovers recirc ports by checking port role setting and adds them correspondingly. - intforch supports adding rif for recirc ports. Everflow changes: - mirrororch allows mirror dst port to be system port. - mirrororch sets mirror dst port to recirc port in voq switch. - mirrororch sets router mac as mirror dst mac in voq switch.

ghost · 2020-12-07T20:46:40Z

All CLA requirements met.

ysmanman · 2020-12-09T18:11:40Z

orchagent/mirrororch.cpp

@@ -678,6 +679,10 @@ bool MirrorOrch::getNeighborInfo(const string& name, MirrorEntry& session)

            return true;
        }
+        case Port::SYSTEM:


Do we actually need this? Is knowing route existing enough?

mirrororch makes use of getNeighborInfo() to resolve mirror dstIp into neighbor, especially when mirror dstIp is actually a neighbor. Therefore, the change is still required to allow resolving mirror dstIp to remote neighbor in multi-asic.

orchagent/portsorch.cpp

rlhui · 2020-12-10T02:15:14Z

Could we please name it recycle port instead of recirc port?

ysmanman · 2020-12-10T03:11:18Z

We actually named these ports as recycle ports initially. This is in line with the naming in Broadcom SAI/SDK. However, in an earlier discussion with Guohan, he suggested to use recirc, instead of recycle. We are okay to change it back to recycle ports. Please do let us know what you and Guohan decide.

…

On Wed, Dec 9, 2020 at 6:15 PM rlhui ***@***.***> wrote: Could we please name it recycling port instead of recirc port? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1530 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APITCDBIPXWC7UBF5D3KVQLSUAVMHANCNFSM4UQ74QZQ> .

zhenggen-xu · 2020-12-10T06:00:37Z

Do you have a design doc for this feature?

lgtm-com · 2020-12-10T06:18:47Z

This pull request introduces 3 alerts when merging 3185fca into cf27721 - view on LGTM.com

new alerts:

3 for Unused local variable

ysmanman · 2020-12-10T06:19:32Z

This is the PR for Everflow HLD: sonic-net/SONiC#716. The HLD was reviewed in chassis subgroup meeting last week. We will add a section in the HLD to cover recirc port.

…

On Wed, Dec 9, 2020 at 10:00 PM zhenggen-xu ***@***.***> wrote: Do you have a design doc for this feature? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1530 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APITCDEWIUQBGBPSFY2NWZLSUBPZFANCNFSM4UQ74QZQ> .

lgtm-com · 2020-12-10T19:34:36Z

This pull request introduces 3 alerts when merging 9324a32 into 376acfe - view on LGTM.com

new alerts:

3 for Unused local variable

lgtm-com · 2021-02-03T01:32:08Z

This pull request introduces 3 alerts when merging 6e33003 into 288fb40 - view on LGTM.com

new alerts:

3 for Unused local variable

lguohan · 2021-02-12T11:50:37Z

tests/test_port_config.py

+        asic_db = swsscommon.DBConnector(swsscommon.ASIC_DB, dvs.redis_sock, 0)
+        asic_db_lanes_tbl = swsscommon.Table(asic_db, "LANES")
+
+        lanes = asic_db_lanes_tbl.get('')[1]


use wait_for function to improve robustness

Thanks for the suggestion. Fixed.

lgtm-com · 2021-02-13T00:39:12Z

This pull request introduces 3 alerts when merging 5df61d8 into f9d5959 - view on LGTM.com

new alerts:

3 for Unused local variable

…for a given role.

lgtm-com · 2021-02-14T22:56:09Z

This pull request introduces 3 alerts when merging d5dbaf4 into f2854f8 - view on LGTM.com

new alerts:

3 for Unused local variable

- Fix doProcessRecircPort to bail out if port Id is not found - Fix a couple of issues in test_recirc_port and also address review comments

lgtm-com · 2021-02-16T00:22:26Z

This pull request introduces 2 alerts when merging e50cbd5 into de03dd7 - view on LGTM.com

new alerts:

2 for Unused local variable

lgtm-com · 2021-02-16T18:34:24Z

This pull request introduces 2 alerts when merging ec03925 into de03dd7 - view on LGTM.com

new alerts:

2 for Unused local variable

lgtm-com · 2021-04-20T03:59:03Z

This pull request introduces 2 alerts when merging 95377ac into 500e2e9 - view on LGTM.com

new alerts:

2 for Unused local variable

lgtm-com · 2021-04-20T05:34:21Z

This pull request introduces 2 alerts when merging e0d9c79 into 500e2e9 - view on LGTM.com

new alerts:

2 for Unused local variable

ysmanman · 2021-04-20T16:40:43Z

Looks fine to me.

Please fix LGTM and resolve conflict. Will approve then.

@abdosi Thanks for review. Conflict was resolved and all checks have passed now.

lgtm-com · 2021-04-28T17:43:07Z

This pull request introduces 2 alerts when merging f55823f into ee7a735 - view on LGTM.com

new alerts:

2 for Unused local variable

ysmanman · 2021-04-28T22:11:00Z

Conflict was resolved and all checks have passed now.

ysmanman · 2021-04-28T22:12:37Z

@abdosi Conflict was resolved and all checks have passed now. Can you take a look and approve?

anshuv-mfst · 2021-05-03T21:41:34Z

Hi @lguohan , @abdosi - could you please review and approve.

abdosi · 2021-05-19T00:15:40Z

@ysmanman can you fix the conflict.

ysmanman · 2021-05-19T00:20:47Z

@ysmanman can you fix the conflict.

@abdosi Resolve the conflict just now.

abdosi · 2021-05-19T00:22:06Z

tests/test_port_config.py

+        # Add recirc ports to port config in configDB
+        recirc_port_lane_name_map = {}
+        for i in range(2):
+            name = alias = "Ethernet-Rec%s" % i


does it not need Ethernet-IB here ?

@abdosi thanks for review. The name do not really matter here. What matters is the port role. We covered both port roles: Rec and Inb in the test (Line188).

abdosi · 2021-05-19T00:22:26Z

tests/test_port_config.py

+
+        # Verify recirc ports in port table in applDB
+        for i in range(2):
+            name = alias = "Ethernet-Rec%s" % i


same as above

@abdosi The name do not really matter here. What matters is the port role. We covered both port roles: Rec and Inb in the test (Line188 and 205).

lgtm-com · 2021-05-19T20:43:07Z

This pull request introduces 2 alerts when merging 19ec921 into 3629d70 - view on LGTM.com

new alerts:

2 for Unused local variable

ysmanman · 2021-05-19T22:15:13Z

@abdosi Resolved conflicts again. Swss tests passed.

anshuv-mfst · 2021-05-26T16:06:56Z

@abdosi - could you please re-approve.

This reverts commit 5ef4b38.

This PR adds recirc port and Everflow support in distributed VOQ chassis. What I did Recirc prot changes: portsyncd includes recirc ports in total port count. portsyncd does not add recirc ports to g_portSet because no host intfs are created for recirc ports. portsorch discovers recirc ports by checking port role setting and adds them correspondingly. intforch supports adding rif for recirc ports. Everflow changes: mirrororch allows mirror dst port to be system port. mirrororch sets mirror dst port to recirc port in voq switch. mirrororch sets router mac as mirror dst mac in voq switch.

… are no broken changes with future versions (sonic-net#1530) #### What I did Fixes sonic-net/sonic-buildimage#7152 #### How I did it Relax the install_requires

ysmanman commented Dec 9, 2020

View reviewed changes

orchagent/portsorch.cpp Show resolved Hide resolved

rlhui requested review from abdosi, prsunny, daall and zhenggen-xu December 9, 2020 18:17

ysmanman commented Dec 9, 2020

View reviewed changes

orchagent/portsorch.cpp Outdated Show resolved Hide resolved

ysmanman commented Dec 9, 2020

View reviewed changes

orchagent/portsorch.cpp Outdated Show resolved Hide resolved

Extend test_port_config to cover recirc ports

3185fca

Add comments to address review comments

9324a32

Merge remote-tracking branch 'upstream/master' into chassis-everflow-pr

6e33003

lguohan reviewed Feb 12, 2021

View reviewed changes

ysmanman added 2 commits February 12, 2021 16:15

Create host intf for recycle ports.

a348eb6

Revert change that's no needed anymore.

5df61d8

Add mapping from recirc port name to its role and return recirc port …

d5dbaf4

…for a given role.

portsorch and recirc port test fix

e50cbd5

- Fix doProcessRecircPort to bail out if port Id is not found - Fix a couple of issues in test_recirc_port and also address review comments

Add missing import

ec03925

Merge branch 'master' into chassis-everflow-pr

95377ac

Fix typo

e0d9c79

Merge branch 'master' into chassis-everflow-pr

f55823f

ysmanman closed this Apr 28, 2021

ysmanman reopened this Apr 28, 2021

Merge branch 'master' into chassis-everflow-pr

921bb61

abdosi reviewed May 19, 2021

View reviewed changes

abdosi previously approved these changes May 19, 2021

View reviewed changes

Merge branch 'master' into chassis-everflow-pr

19ec921

ysmanman dismissed abdosi’s stale review via 19ec921 May 19, 2021 18:46

abdosi approved these changes Jun 2, 2021

View reviewed changes

abdosi merged commit 5ef4b38 into sonic-net:master Jun 2, 2021

shlomibitton mentioned this pull request Jun 14, 2021

[submodule] Update sonic-swss submodule sonic-net/sonic-buildimage#7861

Merged

ysmanman mentioned this pull request Jun 30, 2021

Revert recirc port change #1813

Merged

qiluo-msft added a commit to qiluo-msft/sonic-swss that referenced this pull request Sep 16, 2021

Revert "Add support for recirc port and everflow (sonic-net#1530)"

bb9f17f

This reverts commit 5ef4b38.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for recirc port and everflow #1530

Add support for recirc port and everflow #1530

ysmanman commented Dec 7, 2020 •

edited

Loading

ghost commented Dec 7, 2020 •

edited by ghost

Loading

ysmanman Dec 9, 2020

ysmanman Dec 10, 2020

rlhui commented Dec 10, 2020 •

edited

Loading

ysmanman commented Dec 10, 2020 via email

zhenggen-xu commented Dec 10, 2020

lgtm-com bot commented Dec 10, 2020

ysmanman commented Dec 10, 2020 via email

lgtm-com bot commented Dec 10, 2020

lgtm-com bot commented Feb 3, 2021

lguohan Feb 12, 2021

ysmanman Feb 15, 2021

lgtm-com bot commented Feb 13, 2021

lgtm-com bot commented Feb 14, 2021

lgtm-com bot commented Feb 16, 2021

lgtm-com bot commented Feb 16, 2021

lgtm-com bot commented Apr 20, 2021

lgtm-com bot commented Apr 20, 2021

ysmanman commented Apr 20, 2021

lgtm-com bot commented Apr 28, 2021

ysmanman commented Apr 28, 2021

ysmanman commented Apr 28, 2021

anshuv-mfst commented May 3, 2021

abdosi commented May 19, 2021

ysmanman commented May 19, 2021

abdosi May 19, 2021

ysmanman May 19, 2021

abdosi May 19, 2021

ysmanman May 19, 2021

lgtm-com bot commented May 19, 2021

ysmanman commented May 19, 2021

anshuv-mfst commented May 26, 2021

Add support for recirc port and everflow #1530

Add support for recirc port and everflow #1530

Conversation

ysmanman commented Dec 7, 2020 • edited Loading

ghost commented Dec 7, 2020 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rlhui commented Dec 10, 2020 • edited Loading

ysmanman commented Dec 10, 2020 via email

zhenggen-xu commented Dec 10, 2020

lgtm-com bot commented Dec 10, 2020

ysmanman commented Dec 10, 2020 via email

lgtm-com bot commented Dec 10, 2020

lgtm-com bot commented Feb 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Feb 13, 2021

lgtm-com bot commented Feb 14, 2021

lgtm-com bot commented Feb 16, 2021

lgtm-com bot commented Feb 16, 2021

lgtm-com bot commented Apr 20, 2021

lgtm-com bot commented Apr 20, 2021

ysmanman commented Apr 20, 2021

lgtm-com bot commented Apr 28, 2021

ysmanman commented Apr 28, 2021

ysmanman commented Apr 28, 2021

anshuv-mfst commented May 3, 2021

abdosi commented May 19, 2021

ysmanman commented May 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented May 19, 2021

ysmanman commented May 19, 2021

anshuv-mfst commented May 26, 2021

ysmanman commented Dec 7, 2020 •

edited

Loading

ghost commented Dec 7, 2020 •

edited by ghost

Loading

rlhui commented Dec 10, 2020 •

edited

Loading