Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 10 commits into from
Jun 1, 2021

Conversation

gechiang
Copy link
Contributor

@gechiang gechiang commented May 17, 2021

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.

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:

  1. 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).
  2. 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.
  3. 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

fvs.push_back(fv);
m_appLagTable.set(alias, fvs);

SWSS_LOG_NOTICE("Set port channel %s TPID to %s",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please have it in same line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change the logging to same line.

@@ -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
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed per suggestion in my new commit.

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

Choose a reason for hiding this comment

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

You can use the API SwitchOrch::set_switch_capability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Will change this per your suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed per suggestion in my new commit.

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

if (tpid != 0 && tpid != p.m_tpid)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@gechiang gechiang requested a review from prsunny May 19, 2021 02:00
prsunny
prsunny previously approved these changes May 26, 2021
Copy link
Collaborator

@prsunny prsunny left a comment

Choose a reason for hiding this comment

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

lgtm, minor comments

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok

tpid = "0x0000"
for fv in fvs:
if fv[0] == "tpid":
tpid = fv[1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure. let me take it out.

@gechiang gechiang requested a review from prsunny May 26, 2021 18:39
prsunny
prsunny previously approved these changes May 26, 2021
@prsunny
Copy link
Collaborator

prsunny commented May 27, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lgtm-com
Copy link

lgtm-com bot commented May 31, 2021

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

new alerts:

  • 1 for Unused local variable

@prsunny
Copy link
Collaborator

prsunny commented Jun 1, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@gechiang gechiang merged commit 7e4c3c0 into sonic-net:master Jun 1, 2021
raphaelt-nvidia pushed a commit to raphaelt-nvidia/sonic-swss that referenced this pull request Oct 5, 2021
…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
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
…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)
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.

2 participants