[sonic-swss:TPID CONFIG]Added Orchagent TPID config support for Port/LAG #1747

gechiang · 2021-05-17T17:55:30Z

Signed-off-by: Gen-Hwa Chiang [email protected]

What I did
This PR is part of the TPID CONFIG SONiC feature set to support port/LAG TPID configuration.
TPID HLD PR: (sonic-net/SONiC#681)

There are total of 3 PRs across different submodules to achieve this feature support.

SONiC-Utilities ([TPID CONFIG] Added TPID configuration CLI support sonic-utilities#1618) - To handle TPID CLI related changes.
SONiC Yang Model and Minigraph default ([TPID CONFIG]TPID attribute Yang model and default TPID for Minigraph to configDB Changes sonic-buildimage#7630) - for the new port/LAG TPID attribute and Minigraph to configDB changes.
This PR - To handle Orchagent changes triggered from TPID configuration changes to interact with SYNCd SAI.

Please NOTE this PR has a functional dependency on (sonic-net/sonic-buildimage#7630)

Please refer to the TPID HLD listed above for more details on the TPID configuration feature support.
Here is a quick summary for this PR changes:

By default TPID for port/LAG is 0x8100. With HW SKU that has capability to handle TPID configuration user is allowed to configure the port/LAG to non default TPID values such as (0x9100, 0x9200, 0x88A8).
Orchagent is responsible to query HW SKU TPID attribute capability and store this information onto the StateDB so that the CLI handler can first validate if HW is capable before allowing any TPID CLI configuration to take place.
Orchagent listens to CLI cionfiguration changes and performs the SAI API call to perform the TPID attribute setting on the Port/LAG accordingly.

Why I did it
Having the capability to set TPID configuration it allows the HW to ignore the VALN TAG field if the TPID of received packet does not match the configured TPID value and treat it as Untagged packet. This allows many desired feature such as the fanout switch configuration in SONiC DUT PTF testing topology as well as potential 802.1Q Tunneling feature.

How I verified it
See the HLD for details on all the testing requirement.
Added unit testing on existing unit test modules (port and portchannel) to include the TPID config validation testing.

Details if related

Signed-off-by: Gen-Hwa Chiang <[email protected]>

prsunny · 2021-05-17T22:21:10Z

cfgmgr/teammgr.cpp

+    fvs.push_back(fv);
+    m_appLagTable.set(alias, fvs);
+
+    SWSS_LOG_NOTICE("Set port channel %s TPID to %s",


Please have it in same line

Will change the logging to same line.

prsunny · 2021-05-17T22:27:09Z

orchagent/orchdaemon.cpp

@@ -275,6 +280,56 @@ bool OrchDaemon::init()

    gMacsecOrch = new MACsecOrch(m_applDb, m_stateDb, macsec_app_tables, gPortsOrch);

+    Table m_switchTable(stateDbSwitchTable.first, stateDbSwitchTable.second);
+    // Check if SAI is capable of handling TPID config and store result in StateDB switch capability table
+    {


Please move this section to switch init and make this part of switchorch class.

Changed per suggestion in my new commit.

prsunny · 2021-05-17T22:27:38Z

orchagent/orchdaemon.cpp

+            }
+            SWSS_LOG_NOTICE("LAG TPID capability %d", capability.set_implemented);
+        }
+        m_switchTable.set("switch", fvVector);


You can use the API SwitchOrch::set_switch_capability

Got it. Will change this per your suggestion.

Changed per suggestion in my new commit.

prsunny · 2021-05-17T22:30:15Z

orchagent/portsorch.cpp

@@ -2699,6 +2740,22 @@ void PortsOrch::doPortTask(Consumer &consumer)
                    }
                }

+                if (tpid != 0 && tpid != p.m_tpid)


How do we reset? I think the check should be just tpid != p.m_tpid, correct?

In this case "tpid !=0" check serves the purpose of identifying whether it is handling non-TPID operations in which case variable tpid is initialized to "0" and there is no need to perform TPID operation. This is inline with how MTU and speed setting is being handled. Hope this explains why this check is required.

…orch class

prsunny

lgtm, minor comments

prsunny · 2021-05-26T01:21:06Z

orchagent/portsorch.cpp

+                {
+                    tpid_string = fvValue(i);
+                    // Need to get rid of the leading 0x
+                    tpid_string.erase(0,2);


Is it better to use str.find and then str.erase(0, pos)? Do we always expect a 0x?

The CLI enforces that the TPID values must be one of the following: "0x8100", "0x9100", "0x9200", or "0x88A8".
So yes it will always have the "0x" which we can get rid of.

prsunny · 2021-05-26T01:23:54Z

orchagent/portsorch.cpp

@@ -917,6 +917,36 @@ bool PortsOrch::setPortMtu(sai_object_id_t id, sai_uint32_t mtu)
    return true;
 }

+
+bool PortsOrch::setPortTpid(sai_object_id_t id, sai_uint16_t tpid)


Can we combine this with Lag to have single function and pass is_lag as boolean since only two lines are different and rest are the same? Just a suggestion.

There is a chance that we may end up having to implement the LAG handling by having Orchagent apply the LAG TPID config to all its LAG members. This is because currently only one SAI vendor supports TPID setting on Port only and not yet for LAG. For our fanout switch usage, this is already good enough. But if we ever want to apply it to LAG while SAI vendor not supporting it, it will be much easier to have the port/LAG handling having its own function to do the handling. So for now, let's keep the way it is just in case the enhancement is needed in the future.

prsunny · 2021-05-26T01:30:14Z

tests/test_port.py

+        tpid = "0x0000"
+        for fv in fvs:
+            if fv[0] == "tpid":
+                tpid = fv[1]


By default, there is no tpid field in the port table until you set, correct? IMO, this check doesn't serve a purpose

sure. let me take it out.

prsunny · 2021-05-27T19:22:34Z

/azp run

azure-pipelines · 2021-05-27T19:22:49Z

Azure Pipelines successfully started running 1 pipeline(s).

lgtm-com · 2021-05-31T01:54:22Z

This pull request introduces 1 alert when merging 88eea45 into 6c4c9ad - view on LGTM.com

new alerts:

1 for Unused local variable

… attempt to address other non-TPID related test failures

prsunny · 2021-06-01T16:11:35Z

/azp run

azure-pipelines · 2021-06-01T16:11:50Z

Azure Pipelines successfully started running 1 pipeline(s).

…LAG (sonic-net#1747) * [sonic-swss:TPID CONFIG]Added Orchagent TPID config support for Port/LAG Signed-off-by: Gen-Hwa Chiang <[email protected]> * Changed per review comment to move TPID capability query under switchorch class

…1747) What I did Have independent subdirs for each mounted dir to avoid any collisions of files/dirs by same name. Adopt for older version of python3 How I did it Changes: Individual subdirs for each dir to be mounted subprocess args made compatible with older version of python3 (tested in version 3.5.3) How to verify it Simulate read-only state Run this script Test ssh via new tacacs user (who had not logged in earlier)

[sonic-swss:TPID CONFIG]Added Orchagent TPID config support for Port/LAG

7c2768c

Signed-off-by: Gen-Hwa Chiang <[email protected]>

This was referenced May 17, 2021

[TPID CONFIG]TPID attribute Yang model and default TPID for Minigraph to configDB Changes sonic-net/sonic-buildimage#7630

Merged

[TPID CONFIG] Added TPID configuration CLI support sonic-net/sonic-utilities#1618

Merged

gechiang requested review from abdosi, jleveque, lguohan, rlhui and prsunny May 17, 2021 18:53

gechiang mentioned this pull request May 17, 2021

SONiC TPID Configuration Support sonic-net/SONiC#681

Merged

prsunny reviewed May 17, 2021

View reviewed changes

Changed per review comment to move TPID capability query under switch…

3522e81

…orch class

gechiang requested a review from prsunny May 19, 2021 02:00

prsunny previously approved these changes May 26, 2021

View reviewed changes

Resolved merge conflict

37285d1

gechiang dismissed prsunny’s stale review via 37285d1 May 26, 2021 18:27

Removed unnecessary application DB check before the TPID test begins

9bc5391

gechiang requested a review from prsunny May 26, 2021 18:39

prsunny previously approved these changes May 26, 2021

View reviewed changes

Fix test case issue

88619b0

gechiang dismissed prsunny’s stale review via 88619b0 May 29, 2021 07:34

gechiang added 2 commits May 29, 2021 07:48

Fix test case issue

a4855a7

remove unnecessary check

88eea45

gechiang added 3 commits June 1, 2021 00:45

Merge remote-tracking branch 'upstream/master'

ef088bc

Re-enable Portchannel TPID testing after merged latest from master in…

090a2b7

… attempt to address other non-TPID related test failures

Fix missing Producer DB that accidentally deleted on previous merge

67b0f87

prsunny approved these changes Jun 1, 2021

View reviewed changes

gechiang merged commit 7e4c3c0 into sonic-net:master Jun 1, 2021

shlomibitton mentioned this pull request Jun 14, 2021

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sonic-swss:TPID CONFIG]Added Orchagent TPID config support for Port/LAG #1747

[sonic-swss:TPID CONFIG]Added Orchagent TPID config support for Port/LAG #1747

gechiang commented May 17, 2021 •

edited

Loading

prsunny May 17, 2021

gechiang May 18, 2021

prsunny May 17, 2021

gechiang May 19, 2021

prsunny May 17, 2021

gechiang May 18, 2021

gechiang May 19, 2021

prsunny May 17, 2021

gechiang May 18, 2021

prsunny left a comment

prsunny May 26, 2021

gechiang May 26, 2021

prsunny May 26, 2021

gechiang May 26, 2021

prsunny May 26, 2021

prsunny May 26, 2021

gechiang May 26, 2021

prsunny commented May 27, 2021

azure-pipelines bot commented May 27, 2021

lgtm-com bot commented May 31, 2021

prsunny commented Jun 1, 2021

azure-pipelines bot commented Jun 1, 2021

[sonic-swss:TPID CONFIG]Added Orchagent TPID config support for Port/LAG #1747

[sonic-swss:TPID CONFIG]Added Orchagent TPID config support for Port/LAG #1747

Conversation

gechiang commented May 17, 2021 • edited Loading

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

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

Choose a reason for hiding this comment

prsunny left a comment

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

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

prsunny commented May 27, 2021

azure-pipelines bot commented May 27, 2021

lgtm-com bot commented May 31, 2021

prsunny commented Jun 1, 2021

azure-pipelines bot commented Jun 1, 2021

gechiang commented May 17, 2021 •

edited

Loading