Orchagent: Host interface tag processing by jipanyang · Pull Request #328 · sonic-net/sonic-swss

jipanyang · 2017-09-29T21:29:07Z

Before SAI_HOSTIF_VLAN_TAG_ORIGINAL is supported by libsai from all asic vendors, the VLAN tag on hostif is explicitly controlled with SAI_HOSTIF_VLAN_TAG_STRIP & SAI_HOSTIF_VLAN_TAG_KEEP attributes.

When a port/LAG is in 1Q bridge, SAI_HOSTIF_VLAN_TAG_KEEP is set for SAI_HOSTIF_ATTR_VLAN_TAG, for all other cases SAI_HOSTIF_VLAN_TAG_STRIP will be used.

Signed-off-by: Jipan Yang [email protected]

What I did
Add support for hostif VLAN tag processing support.

Why I did it
With VLAN trunk feature, it is necessary to have packet trapped to host with right VLAN tag.

How I verified it
Tested with whole VLAN trunk feature, please refer to VLAN trunk test cases.

Details if related

jipanyang · 2017-09-29T21:36:04Z

The PR has dependency on #327

lguohan · 2017-09-30T08:17:48Z

orchagent/portsorch.cpp


    m_portList[lag.m_alias] = lag;

+    if (lag.m_vlan_members.size() > 0)


if (lag.m_vlan_members.size() > 0) [](start = 4, length = 34)

this criteria is a little bit indirect, it is better to check if lag is a bridge port or not (do we have an attribute to indicate if the lag is a bridge port or not), if it is then set to keep.

to check if a lag is a bridge port or not, could we check the m_bridge_port_id variable? we're ensuring that once a port is added to a bridge port, we assign this id, once the bridge port is removed, we reset this id.

lguohan

as comments.

stcheng

See comments.

stcheng · 2017-09-30T17:57:22Z

orchagent/portsorch.cpp

+        if (status != SAI_STATUS_SUCCESS)
+        {
+            SWSS_LOG_ERROR("Failed to set %s to host interface %s",
+                        hostif_vlan_tag[strip], p.m_alias.c_str());


align the spaces, or just use 8 spaces. same below.

stcheng · 2017-09-30T17:59:12Z

orchagent/portsorch.cpp

        return false;
    }

+    bool rv = setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_KEEP);


just use if (!setHostIntfsStripTag) is fine. same below.

lguohan · 2017-10-17T19:10:32Z

@JipanYanga , can you rebase your code and resolve the conflict?

jipanyang · 2017-10-18T03:37:20Z

I did a rebase against master branch. Before the possible merge for this PR is done, the libsai from ASIC vendors should support SAI_HOSTIF_VLAN_TAG_STRIP and SAI_HOSTIF_VLAN_TAG_KEEP
for attribute SAI_HOSTIF_ATTR_VLAN_TAG in both sai_create_hostif_fn() and sai_set_hostif_attribute_fn().

The broadcom libsai I tested has support for them from 3.0.3.3. Current version of broadcom libsai in sonic-buildimage repo is 3.0.3.2-6 . https://github.com/Azure/sonic-buildimage/blob/master/platform/broadcom/sai.mk

stcheng

please see my comment

stcheng · 2017-10-18T18:58:47Z

orchagent/portsorch.cpp

+    }
+    else
+    {
+        SWSS_LOG_ERROR("PortsOrch::setPortPvid port type %d not supported", port.m_type);


the log is not correct

lguohan · 2017-10-23T23:02:54Z

@JipanYanga , 3.0.3.2-6 does not support STRIP/KEEP attribute value, it will cause the T0 to break. For normal VLAN scenario, can we still use STRIP option and setup the pvid in the kernel to make it work? That seems to be the only way I can merge this PR right now.

jipanyang · 2017-10-26T01:29:08Z

So the official libsai from some ASIC vendors don't have support for attributes "SAI_HOSTIF_VLAN_TAG_STRIP and SAI_HOSTIF_VLAN_TAG_KEE" in sai_hostif_vlan_tag_t.
While the two attributes or the better attribute SAI_HOSTIF_VLAN_TAG_ORIGINAL is mandatory for VLAN trunk support.

We may put "return true;" at the beginning of bool PortsOrch::setHostIntfsStripTag(Port &port, sai_hostif_vlan_tag_t strip), some the existing access vlan function won't be affected by the libsai issue.
We may just remove that line of code from the function once libsai support is ready which should happen really soon.

lguohan · 2017-10-26T02:08:46Z

orchagent/portsorch.cpp

+     * Port has been removed from 1q bridge at PortsOrch constructor,
+     * also start stripping off VLAN tag.
+     */
+    setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_STRIP);


by default, it is stripped, so you do not call this.

lguohan · 2017-10-26T02:15:17Z

orchagent/portsorch.cpp

+        SWSS_LOG_ERROR("Failed to set %s for hostif of port %s",
+                hostif_vlan_tag[SAI_HOSTIF_VLAN_TAG_KEEP], port.m_alias.c_str());
+        return false;
+    }


let's move this to addVlanMember(), in the untagged mode, we do not need to call KEEP.

lguohan · 2017-10-26T02:15:37Z

orchagent/portsorch.cpp

+        SWSS_LOG_ERROR("Failed to set %s for hostif of port %s",
+                hostif_vlan_tag[SAI_HOSTIF_VLAN_TAG_STRIP], port.m_alias.c_str());
+        return false;
+    }


move this to removeVlanMember()

lguohan · 2017-10-26T02:23:00Z

orchagent/portsorch.cpp


    m_portList[lag.m_alias] = lag;

+    if (lag.m_bridge_port_id > 0)


also check if the port belongs to multiple vlans or not, if yes then add keep, otherwise do not need to do anything.

lguohan

avoid set vlan_tag attribute when the port is in untagged vlan mode.

Before SAI_HOSTIF_VLAN_TAG_ORIGINAL is supported by libsai from all asic vendors, the VLAN tag on hostif is explicitly controlled with SAI_HOSTIF_VLAN_TAG_STRIP & SAI_HOSTIF_VLAN_TAG_KEEP attributes. When a port/LAG is in 1Q bridge, SAI_HOSTIF_VLAN_TAG_KEEP is set for SAI_HOSTIF_ATTR_VLAN_TAG, for all other cases SAI_HOSTIF_VLAN_TAG_STRIP will be used. Signed-off-by: Jipan Yang <[email protected]>

…TRIP explicitly at init. Signed-off-by: Jipan Yang <[email protected]>

lguohan · 2017-12-07T20:37:41Z

retest this please

config portchannel add <portchannel_name> config portchannel del <portchannel_name> config portchannel member add <portchannel_name> <port_name> config portchannel member del <portchannel_name> <port_name> Signed-off-by: Shu0T1an ChenG <[email protected]>

@runtime

* initial barefoot checkin october 2017 * Revert "Merge branch 'master' of https://github.com/Azure/sonic-sairedis into rel_6_0" This reverts commit c48a7a20de2fb9e615535481f8727029440413bc, reversing changes made to aa5bf640db7b7fca245c1669eeb02198a50cb5c1. * missed integration diffs * Added new attr type support to sairedis. Also, some fixes for compilation issues * Changes to add new DTel api support in sairedis * Add new file to generate Dtel specific SAI stub API * Missed adding a file in the last commit * Fix ref point for SAI * Updated SAI repo to point to dtel_exp * Changes to handle new additions to DTel experimental SAI. Not compiled yet * handle platform specific lins in different directory (@runtime) * force order of library path to look for platform dir before lib dir * Change SAI branch refpoint * Update SAI submodule refpoint * enable fast-boot for barefoot platforms * Update ref point for SAI * SONiC changes due to DTel experimental SAI changes * allow Makefile to build for other platforms - restore it original for non-bfn platforms * allow clean build * Revert "allow clean build" This reverts commit adfdb8642fe0b5dbb4e19d13871172a03a8c883b. * Revert "allow Makefile to build for other platforms - restore it original for non-bfn platforms" This reverts commit d6b0ca34fcf63d0b79c801cd95d7ee2fe58d21ac. * makefile cleanup towards upstream * Support for platforms based on Barefoot Networks' device (sonic-net#304) * search for exact string - newer onie versions match multiple lines * Will need to revert this Should work without this change. Untested for now * SONiC sairedis changes needed to work with SAIv1.3 * Fix SAI path in gitmodules and add a comment * Remove sai thrift build hack * enable fast-boot for barefoot platforms * enable fast-boot for barefoot platforms * Add missing sai rpc hdr file path for bfn * Update SAI ref points * Multi p4 profile support for bfn sde (#5) * Remove code duplicated in merge * Keep fn in same order as azure master (sonic-net#8) * Address review comment * Link just bfn sai lib and shorten linking command by removing other libraries linked at compile time (#6) * Link only bfn sai lib * all unresolved symbols resolved at runtime * Preload some bfn libraries on syncd startup * Build fix * also remove commented code * Convert tabs to spaces * Fix alignment * Address upstream comment * BFN: link with just sai library for saisdkdump * Address review comments

* Host interface tag processing Before SAI_HOSTIF_VLAN_TAG_ORIGINAL is supported by libsai from all asic vendors, the VLAN tag on hostif is explicitly controlled with SAI_HOSTIF_VLAN_TAG_STRIP & SAI_HOSTIF_VLAN_TAG_KEEP attributes. When a port/LAG is in 1Q bridge, SAI_HOSTIF_VLAN_TAG_KEEP is set for SAI_HOSTIF_ATTR_VLAN_TAG, for all other cases SAI_HOSTIF_VLAN_TAG_STRIP will be used. Signed-off-by: Jipan Yang <[email protected]> * By default vlan tag is stripped, no need to set SAI_HOSTIF_VLAN_TAG_STRIP explicitly at init. Signed-off-by: Jipan Yang <[email protected]>

In file included from /usr/include/gtest/gtest.h:57:0, from macaddress_ut.cpp:2: macaddress_ut.cpp: In member function ‘virtual void MacAddress_parse_Test::TestBody()’: macaddress_ut.cpp:64:48: error: ‘invalid_argument’ does not name a type EXPECT_THROW(MacAddress("52:54:00:25:E9"), invalid_argument); ^ macaddress_ut.cpp:64:5: error: expected ‘)’ before ‘const’ EXPECT_THROW(MacAddress("52:54:00:25:E9"), invalid_argument); ^ macaddress_ut.cpp:64:5: error: expected ‘{’ before ‘const’ macaddress_ut.cpp:64:5: error: expected unqualified-id before ‘)’ token EXPECT_THROW(MacAddress("52:54:00:25:E9"), invalid_argument); ^ macaddress_ut.cpp:64:5: error: expected initializer before ‘)’ token macaddress_ut.cpp:64:5: error: expected primary-expression before ‘catch’ EXPECT_THROW(MacAddress("52:54:00:25:E9"), invalid_argument); ^

lguohan reviewed Sep 30, 2017

View reviewed changes

lguohan requested changes Sep 30, 2017

View reviewed changes

stcheng reviewed Sep 30, 2017

View reviewed changes

jipanyang force-pushed the hostif_tag_processing branch from cd53052 to 30c5722 Compare October 2, 2017 06:54

jipanyang force-pushed the hostif_tag_processing branch from 30c5722 to 6385898 Compare October 18, 2017 03:30

stcheng approved these changes Oct 18, 2017

View reviewed changes

jipanyang force-pushed the hostif_tag_processing branch from 6385898 to 24e3f6d Compare October 19, 2017 21:49

stcheng added the Enhancement ➕ label Oct 19, 2017

jipanyang mentioned this pull request Oct 21, 2017

Add support for vlanconfd and intfconfd sonic-net/sonic-buildimage#1063

Merged

lguohan reviewed Oct 26, 2017

View reviewed changes

lguohan requested changes Oct 26, 2017

View reviewed changes

jipanyang added 2 commits November 29, 2017 11:01

By default vlan tag is stripped, no need to set SAI_HOSTIF_VLAN_TAG_S…

1a473c3

…TRIP explicitly at init. Signed-off-by: Jipan Yang <[email protected]>

jipanyang force-pushed the hostif_tag_processing branch from 24e3f6d to 1a473c3 Compare November 29, 2017 19:05

Merge branch 'master' into hostif_tag_processing

f248a2a

lguohan approved these changes Dec 7, 2017

View reviewed changes

lguohan merged commit ff90429 into sonic-net:master Dec 7, 2017

jipanyang deleted the hostif_tag_processing branch June 5, 2018 00:30

lukasstockner pushed a commit to genesiscloud/sonic-swss that referenced this pull request Apr 2, 2023

Use github code scanning instead of LGTM (sonic-net#328)

d11e983


		m_portList[lag.m_alias] = lag;

		if (lag.m_vlan_members.size() > 0)

Conversation

jipanyang commented Sep 29, 2017

Uh oh!

jipanyang commented Sep 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan left a comment

Choose a reason for hiding this comment

Uh oh!

stcheng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan commented Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jipanyang commented Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stcheng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan commented Oct 23, 2017

Uh oh!

jipanyang commented Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan left a comment

Choose a reason for hiding this comment

Uh oh!

lguohan commented Dec 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lguohan commented Oct 17, 2017 •

edited

Loading

jipanyang commented Oct 18, 2017 •

edited

Loading

jipanyang commented Oct 26, 2017 •

edited

Loading