VLAN trunk support: orchagent update. by jipanyang · Pull Request #296 · sonic-net/sonic-swss

jipanyang · 2017-08-25T17:36:04Z

VLAN trunk support. Changes in orchagent:

More than one 802.1Q VLAN could be created.
A VLAN could have members of physical ports or LAG or both types of them.
A physical port or LAG could be added to multiple VLANs in tagged mode.
A physical port or LAG could be added to one VLAN in untagged mode.
It shall be possible to config port VLAN ID (pvid) for individual physical port and LAG.

msftclas · 2017-08-25T17:36:09Z

@JipanYanga,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

stcheng

thanks! please review the comments.

stcheng · 2017-08-25T17:58:06Z

orchagent/intfsorch.cpp

    gPortsOrch->setPort(port.m_alias, port);
-
+    /* Clean this port from default VLAN */
+    gPortsOrch->removeDefaultVlanMembers();


why do you need to call this function here?

After removeRouterIntfs(), the port goes back to default VLAN. Since there is no vlan member id available, this is the only way to get the port out of default VLAN.

Not very clear about the expected behavior at sai level. If port is not supposed to be put back to default VLAN, then it could be libsai implementation issue. This line may be removed.

the port is not supposed to be put back to default VLAN according to the SAI spec. you could remove this line.

Ok, will remove the line of code. Libsai from ASIC vendor should not automatically move the port to default VLAN.

stcheng · 2017-08-25T18:01:36Z

orchagent/port.h

+    sai_object_id_t     vlan_oid;
+    sai_vlan_id_t       vlan_id;
+    std::string         autostate;
+    uint32_t            mtu;


move this mtu to Port structure's variable

stcheng · 2017-08-25T18:05:24Z

orchagent/port.h

-    sai_vlan_id_t       m_vlan_id = 0;
    sai_vlan_id_t       m_port_vlan_id = DEFAULT_PORT_VLAN_ID;  // Port VLAN ID
-    sai_object_id_t     m_vlan_member_id = 0;
+    port_vlan_members_t   m_vlan_members;


align the spaces

stcheng · 2017-08-25T18:06:36Z

orchagent/portsorch.cpp

+{
+    SWSS_LOG_ENTER();
+
+    vector<sai_object_id_t>   idv;


extra spaces

stcheng · 2017-08-25T18:06:58Z

orchagent/portsorch.cpp

+    {
+        Port member;
+        auto it = port.m_members.begin();
+         while (it != port.m_members.end())


align spaces

stcheng · 2017-08-25T18:17:25Z

orchagent/portsorch.cpp

-                it = consumer.m_toSync.erase(it);
-            else
-                it++;
+            string autostate;


just wondering if autostate is a boolean or a string?

It is a value from redis, in type of string. VLAN autostate feature is to be implemented.

stcheng · 2017-08-25T18:22:30Z

orchagent/portsorch.cpp

+     * Port has been removed from 1q bridge at PortsOrch constructor,
+     * also start stipping off VLAN tag.
+     */
+    bool rv = setHostIntfsStripTag(p, SAI_HOSTIF_VLAN_TAG_STRIP);


move this piece of code inside the addHostIntfs function？

After checking it again, bool PortsOrch::setHostIntfsStripTag(Port &port, sai_hostif_vlan_tag_t strip) is taking Port class as first parameter, while bool PortsOrch::addHostIntfs(sai_object_id_t id, string alias, sai_object_id_t &host_intfs_id) is using port id of type sai_object_id_t .

Probably it is better to keep setHostIntfsStripTag() out side of addHostIntfs(), and leave setHostIntfsStripTag() at initializePort().

what shall we do if this function fails here? what about moving this function above under the addHostIntfs function without checking the return value to match the rest of the initialization functions. it seems that if this function fails, there's nothing we could do here. since there's already error message inside this function, we could anyway catch it.

ok, will move the function to addHostIntfs() without checking return value.

stcheng · 2017-08-25T18:24:14Z

orchagent/portsorch.cpp

 }

-bool PortsOrch::addVlanMember(Port vlan, Port port, string& tagging_mode)
+bool PortsOrch::addVlanMember(Port &vlan, Port &port, string& tagging_mode)


align the space, e.g. string &tagging_mode

stcheng · 2017-08-25T18:28:41Z

orchagent/portsorch.cpp

+    assert (vlan_member != port.m_vlan_members.end());
+    sai_tagging_mode = vlan_member->second.vlan_mode;
+    vlan_member_id = vlan_member->second.vlan_member_id;
+    sai_status_t status = sai_vlan_api->remove_vlan_member(vlan_member_id);


switch with the lower empty line 1586.

stcheng · 2017-08-25T18:30:19Z

orchagent/saihelper.cpp

        }
    }

-    /* Disable/enable SwSS recording */


why remove this part?

It duplicates with https://github.com/Azure/sonic-swss/blob/v1.0.3/orchagent/main.cpp#L164, and causes file write issue.

ah! I see. thanks! Let me fix this in a separate pull request!

#298 thanks.

…class, code format and so on.

…gent

stcheng

minor changes. as for the PVID discussion, we could sync on the phone.

stcheng · 2017-09-05T20:03:54Z

orchagent/intfsorch.cpp

    gPortsOrch->setPort(port.m_alias, port);
-
+    /* Clean this port from default VLAN */
+    gPortsOrch->removeDefaultVlanMembers();


the port is not supposed to be put back to default VLAN according to the SAI spec. you could remove this line.

stcheng · 2017-09-05T20:04:49Z

orchagent/portsorch.cpp

+bool PortsOrch::setPortPvid (Port &port, sai_uint32_t pvid)
+{
+    SWSS_LOG_ENTER();
+    vector<Port>   portv;


extra spaces.

Ok, will remove it.

stcheng · 2017-09-05T20:09:52Z

orchagent/portsorch.cpp

+
+bool PortsOrch::getPortPvid(Port &port, sai_uint32_t &pvid)
+{
+


extra empty line

Ok, will remove it.

stcheng · 2017-09-05T20:17:38Z

orchagent/portsorch.cpp

+
+    for (auto &vme: port.m_vlan_members)
+    {
+        if(vme.second.vlan_mode == SAI_VLAN_TAGGING_MODE_UNTAGGED) {


here the port VLAN ID is valid only when we are querying the physical port, is that right? a physical port will not have VLAN members. I'm not sure about why the for loop is here. Do we need to add checks at the entrance of this function to ensure the type of the port? If a sync is needed, let's discuss this on the phone.

stcheng · 2017-09-05T20:18:02Z

orchagent/portsorch.cpp

+bool PortsOrch::setHostIntfsStripTag(Port &port, sai_hostif_vlan_tag_t strip)
+{
+    SWSS_LOG_ENTER();
+    vector<Port>   portv;


extra spaces

Ok, will remove it.

stcheng · 2017-09-05T20:18:37Z

orchagent/portsorch.cpp

+            return false;
+        }
+        SWSS_LOG_NOTICE("Set %s to host interface: %s",
+                          hostif_vlan_tag[strip], p.m_alias.c_str());


align the code or use 8 spaces

stcheng · 2017-09-05T20:25:32Z

orchagent/portsorch.cpp

+     * Port has been removed from 1q bridge at PortsOrch constructor,
+     * also start stipping off VLAN tag.
+     */
+    bool rv = setHostIntfsStripTag(p, SAI_HOSTIF_VLAN_TAG_STRIP);


what shall we do if this function fails here? what about moving this function above under the addHostIntfs function without checking the return value to match the rest of the initialization functions. it seems that if this function fails, there's nothing we could do here. since there's already error message inside this function, we could anyway catch it.

stcheng · 2017-09-05T20:36:10Z

orchagent/portsorch.cpp


-    sai_status_t status = sai_vlan_api->remove_vlan_member(port.m_vlan_member_id);
+    sai_object_id_t            vlan_member_id;
+    sai_vlan_tagging_mode_t    sai_tagging_mode;


no need to align spaces here

stcheng · 2017-09-05T20:36:31Z

orchagent/portsorch.cpp

-        SWSS_LOG_ERROR("Failed to reset port VLAN ID to DEFAULT_PORT_VLAN_ID pid:%lx",
-                port.m_port_id);
-        return false;
+        if(!setPortPvid(port, DEFAULT_PORT_VLAN_ID))


add one space after if

stcheng · 2017-09-05T20:36:50Z

orchagent/portsorch.cpp

+{
+    Port member;
+
+    for(auto &name: lag.m_members)


one space after for

… alignment fix.

jipanyang · 2017-09-16T05:58:21Z

target branch not active.

* Enhanced "show arp/ndp" output to have vlan member info * Fixes and restructure * Addressed review comments, pep8online comments

…c-net#296) Signed-off-by: Shu0T1an ChenG <shuche@microsoft.com>

VLAN trunk support: orchagent update.

a1bbfc0

msftclas added the cla-already-signed label Aug 25, 2017

jipanyang mentioned this pull request Aug 25, 2017

V1.0.3 VLAN trunk implmentation #294

Closed

stcheng added the Enhancement ➕ label Aug 25, 2017

stcheng suggested changes Aug 25, 2017

View reviewed changes

jipanyang added 2 commits August 25, 2017 21:30

Address various review comments such as putting mtu variable at Port …

818dbda

…class, code format and so on.

Merge remote-tracking branch 'upstream/v1.0.3' into v1.0.3_vlan_orcha…

02323a7

…gent

stcheng reviewed Sep 5, 2017

View reviewed changes

Don't set pvid for lag with router interface. Coding style, space and…

c438b7e

… alignment fix.

jipanyang closed this Sep 16, 2017

jipanyang deleted the v1.0.3_vlan_orchagent branch September 16, 2017 06:03

EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022

Enhanced "show arp/ndp" output (sonic-net#296)

506712e

* Enhanced "show arp/ndp" output to have vlan member info * Fixes and restructure * Addressed review comments, pep8online comments

jianyuewu pushed a commit to jianyuewu/sonic-swss that referenced this pull request Dec 24, 2025

[schema]: Change CFG_ACL_TABLE_NAME to CFG_ACL_TABLE_TABLE_NAME (soni…

935155a

…c-net#296) Signed-off-by: Shu0T1an ChenG <shuche@microsoft.com>


		bool PortsOrch::getPortPvid(Port &port, sai_uint32_t &pvid)
		{

Conversation

jipanyang commented Aug 25, 2017

Uh oh!

msftclas commented Aug 25, 2017

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!

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!

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!

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!

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!

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!

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!

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