orchagent: Support port/LAG in multiple VLANs #327

jipanyang · 2017-09-29T07:30:14Z

One port/LAG may join multi-VLAN in tagged/untagged mode.
pvid on LAG is not supported in SAI v1.0, as workaround we deal with each LAG member explicitly for pvid.
bridge port has to be created first for port/LAG to join 1Q VLAN. The same bridge port is used to join all VLANs.
For each VLAN that port/LAG has joined, there is one vlan member id which is the handle for VLAN member attribute get/set and remove.

Signed-off-by: Jipan Yang jipan.yang@alibaba-inc.com

What I did
Support port/LAG in multiple VLANs
Why I did it
VLAN trunk feature implementation
How I verified it
Passed VLAN trunk test cases

Details if related

One port/LAG may join multi-VLAN in tagged/untagged mode. pvid on LAG is not supported in SAI v1.0, as workaround we deal with each LAG member explicitly for pvid. bridge port has to be created first for port/LAG to join 1Q VLAN. The same bridge port is used to join all VLANs. For each VLAN that port/LAG has joined, there is one vlan member id which is the handle for VLAN member attribute get/set and remove. Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

lguohan · 2017-09-30T06:56:21Z

orchagent/port.h

+    sai_vlan_tagging_mode_t    vlan_mode;
+};
+
+typedef std::map<sai_vlan_id_t, VlanMemberEntry> port_vlan_members_t;


port_vlan_members_t [](start = 49, length = 19)

rename to vlan_members_t

lguohan · 2017-09-30T06:58:54Z

orchagent/portsorch.cpp

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


{ [](start = 67, length = 1)

'{' in a separate line.

lguohan · 2017-09-30T06:59:06Z

orchagent/portsorch.cpp

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


f( [](start = 9, length = 2)

keep the space

lguohan · 2017-09-30T07:01:15Z

orchagent/portsorch.cpp

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

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


[](start = 27, length = 1)

no space here.

lguohan · 2017-09-30T07:03:27Z

orchagent/portsorch.cpp

@@ -534,6 +603,10 @@ void PortsOrch::doPortTask(Consumer &consumer)
                if (fvField(i) == "mtu")
                    mtu = (uint32_t)stoul(fvValue(i));

+                /* Set port pvid */
+                if (fvField(i) == "pvid")
+                    pvid = (uint32_t)stoul(fvValue(i));


why do we need this? we can keep the assumption that the untagged mode vlan is the pvid of the port.

The original design is to support pvid setting on port and lag from admin interface, as provided at SAI api. Since it is causing a lot of confusion and no actual use case for it yet, removing support for explicit pvid configuration.

lguohan · 2017-09-30T07:05:39Z

orchagent/portsorch.cpp

+                getPort(vlan_alias, v);
+                v.m_mtu = mtu;
+                // TODO: update MTU for the IP interface of the VLAN
+            }


since it is todo, I suggest to remove it. The rif MTU should be configured under interface_table.

done.
btw, VLAN_TABLE schema might needs some update to remove mtu field.
mtu = 1*4DIGIT ; MTU for the IP interface of the VLAN

lguohan · 2017-09-30T07:14:51Z

orchagent/portsorch.cpp

@@ -1155,6 +1261,9 @@ bool PortsOrch::addBridgePort(Port &port)
 {
    SWSS_LOG_ENTER();

+    if (port.m_bridge_port_id)
+        return true;


use {} even for one line statement.

I think we should print some warning here, since we should not add a port to a bridge if the port is in a bridge already.

In reply to: 142000240 [](ancestors = 142000240)

lguohan · 2017-09-30T07:28:15Z

orchagent/portsorch.cpp


-    if (tagging_mode == "untagged") // set pvlan id for untagged port only
+    /* Use untagged VLAN as pvid of the member port when no pvid has been configured explictly */
+    if (port.m_port_vlan_id == DEFAULT_PORT_VLAN_ID && sai_tagging_mode == SAI_VLAN_TAGGING_MODE_UNTAGGED)


port.m_port_vlan_id == DEFAULT_PORT_VLAN_ID [](start = 8, length = 44)

not sure why we need this condition, can you explain?

As explained previously, it was for supporting pvid configuration from admin interface.
removed.

lguohan · 2017-09-30T07:28:59Z

orchagent/portsorch.cpp

-    status = sai_port_api->set_port_attribute(port.m_port_id, &attr);
-    if (status != SAI_STATUS_SUCCESS)
+    /* Restore to default pvid if no pvid has been configured explictly and this port was in untagged mode */
+    if (port.m_port_vlan_id == DEFAULT_PORT_VLAN_ID && sai_tagging_mode == SAI_VLAN_TAGGING_MODE_UNTAGGED)


port.m_port_vlan_id == DEFAULT_PORT_VLAN_ID && [](start = 8, length = 47)

not sure if we need this.

As explained previously, it was for supporting pvid configuration from admin interface.
removed.

lguohan · 2017-09-30T07:36:09Z

can you also submit the vlan trunk test you mentioned in the PR.

lguohan · 2017-09-30T07:41:35Z

orchagent/portsorch.cpp

@@ -1462,13 +1616,17 @@ bool PortsOrch::addLagMember(Port lag, Port port)

    m_portList[lag.m_alias] = lag;

+    sai_uint32_t pvid = DEFAULT_PORT_VLAN_ID;


= DEFAULT_PORT_VLAN_ID [](start = 21, length = 23)

not needed.

lguohan · 2017-09-30T07:41:48Z

orchagent/portsorch.cpp

@@ -1462,13 +1616,17 @@ bool PortsOrch::addLagMember(Port lag, Port port)

    m_portList[lag.m_alias] = lag;

+    sai_uint32_t pvid = DEFAULT_PORT_VLAN_ID;
+    if (getPortPvid(lag, pvid))
+        setPortPvid (port, pvid);


lguohan · 2017-09-30T07:45:34Z

orchagent/portsorch.cpp

+            return true;
+        }
+    }
+    pvid = DEFAULT_PORT_VLAN_ID;


this is complicated, when the port is not a router interface, we should just return port.m_port_vlan_id, and we should make the m_port_vlan_id correct.

As explained previously, it was for supporting pvid configuration from admin interface.

Updated.

lguohan · 2017-09-30T07:47:16Z

orchagent/portsorch.cpp

+    SWSS_LOG_ENTER();
+    vector<Port> portv;
+
+    if (port.m_type == Port::PHY)


if (port.m_type == Port::PHY) [](start = 1, length = 32)

if the port.m_rid_id is not null, we should return false. since we do not allow set pvid for router interface.

lguohan · 2017-09-30T07:47:45Z

orchagent/portsorch.cpp

+            return false;
+        }
+        SWSS_LOG_NOTICE("Set pvid %u to port: %s", attr.value.u32, p.m_alias.c_str());
+    }


m_port_vlan_id = pvid

lguohan · 2017-09-30T07:49:06Z

orchagent/port.h

@@ -53,17 +68,17 @@ class Port
    Type                m_type;
    int                 m_index = 0;    // PHY_PORT: index
    int                 m_ifindex = 0;
+    sai_uint32_t        m_mtu;


this is irrelevant to the pr. consider to remove.

lguohan

as comments

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

jipanyang · 2017-10-02T06:49:36Z

vlan trunk test mentioned in this PR is the VLAN trunk test plan reviewed at SONiC community meeting few weeks ago.
Developer of the VLAN trunk test plan will push the document to github soon.

stcheng · 2017-10-03T23:04:27Z

hi could you resolve the conflicts?

lguohan · 2017-10-04T03:36:04Z

orchagent/portsorch.cpp

+        {
+            SWSS_LOG_DEBUG("Port %s still in VLAN %d", port.m_alias.c_str(), vme.first);
+        }
+        return true;


this should be false.

each time a port/lag leaves a VLAN, " if (removeVlanMember(vlan, port) && removeBridgePort(port))" is called, port.m_vlan_members.erase(vlan_member) is called in removeVlanMember(vlan, port), and here it is to check whether the port/lag is still in any vlan. It is ok to be non-empty.

this make the return value confusing, according to the function name the return value true means you have removed the bridge port, in this case you should return false. The current return value does not match user expectation, we have to avoid this.

lguohan · 2017-10-04T03:37:02Z

orchagent/portsorch.cpp

+    {
+        for (auto &vme: port.m_vlan_members)
+        {
+            SWSS_LOG_DEBUG("Port %s still in VLAN %d", port.m_alias.c_str(), vme.first);


change level to SWSS_LOG_ERROR, this should not happen at orchagent level, the dependency resolution should be handled by the cfg mgr level.

As previous explanation.
We may argue there could be different way of implementation if it is too confusing.

the return value of the function needs to be consistent, true mean removed, false means not removed.

Ok, it is a little confusing. Will do the m_vlan_members set empty check outside of this function.

lguohan · 2017-10-04T04:01:45Z

orchagent/portsorch.cpp

@@ -1213,9 +1295,11 @@ bool PortsOrch::removeBridgePort(Port port)
            port.m_alias.c_str(), status);
        return false;
    }
+    port.m_bridge_port_id = 0;


0 -> SAI_NULL_OBJECT_ID

lguohan · 2017-10-04T04:08:22Z

orchagent/portsorch.cpp

-                assert(port.m_vlan_id && port.m_vlan_member_id);
-
-                if (removeVlanMember(vlan, port))
+                if (removeVlanMember(vlan, port) && removeBridgePort(port))


if there is no vlan member, then we can removeBridgePort.

lguohan · 2017-10-04T04:19:59Z

orchagent/portsorch.cpp

        return false;
    }

-    SWSS_LOG_NOTICE("Remove VLAN %s vid:%hu", vlan.m_alias.c_str(), vlan.m_vlan_id);
+    SWSS_LOG_NOTICE("Remove VLAN %s vid:%hu", vlan.m_alias.c_str(),
+		vlan.m_vlan_info.vlan_id);


[](start = 0, length = 2)

make this as space.

lguohan · 2017-10-04T04:22:41Z

orchagent/portsorch.cpp

    SWSS_LOG_NOTICE("Add bridge port %s to default 1Q bridge", port.m_alias.c_str());

    return true;
 }

-bool PortsOrch::removeBridgePort(Port port)
+bool PortsOrch::removeBridgePort(Port &port)


BridgePo [](start = 22, length = 8)

symmetric to addBridgePort, we can first check if the port is already in bridge or not, if the port is not in bridge, we can return true.

there is back and forth for addBridgePort() and removeBridgePort().
Hope the following implementation is ok:
"Calling addBridgePort() only when bridge port not created for port/lag, and calling removeBridgePort() only when port/lag not in any VLAN"

there is no back and forth, what I meant is that

for addBridgePort() if it is already a bridge port, you can return true. For removeBridgePort, if it is not a bridge, you can also return true.

they are different than when the port is still in vlan, you should return false.

Ok, let me change it again, hope the new code will match what you want.

lguohan · 2017-10-04T04:23:33Z

orchagent/portsorch.cpp

@@ -1155,6 +1208,12 @@ bool PortsOrch::addBridgePort(Port &port)
 {
    SWSS_LOG_ENTER();

+    if (port.m_bridge_port_id)
+    {
+        SWSS_LOG_WARN("port %s already in 1Q bridge", port.m_alias.c_str());


can be DEBUG LEVEL.

there is back and forth for addBridgePort() and removeBridgePort().
Hope the following implementation is ok:
"Calling addBridgePort() only when bridge port not created for port/lag, and calling removeBridgePort() only when port/lag not in any VLAN"

lguohan · 2017-10-04T04:24:05Z

orchagent/portsorch.cpp

-            assert(!port.m_vlan_id && !port.m_vlan_member_id);
-
-            if (addBridgePort(port) && addVlanMember(vlan, port, tagging_mode))
+            if ((port.m_bridge_port_id || addBridgePort(port)) && addVlanMember(vlan, port, tagging_mode))


port.m_bridge_port_id || [](start = 17, length = 25)

since you check this in addBridgePort, you do not need to check the bridge_port_id here.

As explained before.

lguohan

as comments.

…g, and calling removeBridgePort() only when port/lag not in any VLAN Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

…removeBridgePort, if it is not a bridge, return true. Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

…rt-lag Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

stcheng

Approved. Add some comments for minor changes.

stcheng · 2017-10-06T07:32:41Z

orchagent/port.h

+    sai_vlan_tagging_mode_t    vlan_mode;
+};
+
+typedef std::map<sai_vlan_id_t, VlanMemberEntry> vlan_members_t;


rename to VlanMemberMap?

it was changed to this name according to comment:
"
lguohan 6 days ago Contributor
port_vlan_members_t [](start = 49, length = 19)
rename to vlan_members_t
"

stcheng · 2017-10-06T07:35:16Z

orchagent/portsorch.cpp

+
+    pvid = port.m_port_vlan_id;
+    return true;
+}


add an empty line here.

empty line before "return true;"? It might be good if SONiC has clear specification about the coding style.

I mean between the } and the next function.

stcheng · 2017-10-06T07:41:37Z

orchagent/portsorch.cpp

@@ -826,24 +882,23 @@ void PortsOrch::doVlanTask(Consumer &consumer)

        if (op == SET_COMMAND)
        {
-            /* Duplicate entry */
-            if (m_portList.find(vlan_alias) != m_portList.end())
+            if (m_portList.find(vlan_alias) == m_portList.end())


Add comments indicating that only creation is supported

According to SAI specification, there are some VLAN attributes like mac learning enable/disable, flood control which are configurable though we don't support them for now.
We may add more comments here.

stcheng · 2017-10-06T07:49:50Z

orchagent/portsorch.cpp

@@ -952,13 +1004,18 @@ void PortsOrch::doVlanMemberTask(Consumer &consumer)
        {
            if (vlan.m_members.find(port_alias) != vlan.m_members.end())
            {
-                /* Assert the port belongs the a VLAN */
-                assert(port.m_vlan_id && port.m_vlan_member_id);
-
                if (removeVlanMember(vlan, port))


could you make this DEL part symmetric to SET part? e.g. if (removeVlanMember(vlan, port) && removeBridgePort(port)) and move the condition statement into the removeBridgePort function.
This will be easier for readers to understand the logic.

That was the original implementation, changed to current format to address comment of below:
"lguohan 3 days ago Contributor
if there is no vlan member, then we can removeBridgePort."

stcheng · 2017-10-06T08:09:43Z

orchagent/portsorch.cpp

+    {
+        if (!getPort(name, member))
+        {
+            SWSS_LOG_ERROR("Failed to get port for %s alias\n", name.c_str());


Remove trailing \n

…comment for vlan creation Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

svc-acs · 2017-10-10T19:43:40Z

Build finished. No test results found.

jipanyang · 2017-10-12T17:36:27Z

VLAN trunk test plan: https://github.com/Azure/SONiC/wiki/VLAN-trunk-test-plan

lguohan

One port/LAG may join multi-VLAN in tagged/untagged mode. pvid on LAG is not supported in SAI v1.0, as workaround we deal with each LAG member explicitly for pvid. Bridge port has to be created first for port/LAG to join 1Q VLAN. The same bridge port is used to join all VLANs. For each VLAN that port/LAG has joined, there is one vlan member id which is the handle for VLAN member attribute get/set and remove.

* [watermarks] add watermarkstat, watermarkcfg and aliases * [watermark] watermark clear require root, add counterpoll ability to conf/show watermark counters polling interval Signed-off-by: Mykola Faryma <mykolaf@mellanox.com> * resolve conflict Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

* Sync to SAI 1.3 header * SAI v1.3 pointer update * Updated queue deadlock to enque notification * Update SAI pointer

jipanyang mentioned this pull request Sep 29, 2017

Orchagent: Host interface tag processing #328

Merged

lguohan reviewed Sep 30, 2017

View reviewed changes

lguohan requested changes Sep 30, 2017

View reviewed changes

Remove processing for pvid and mtu from VLAN

cf3e0ea

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

lguohan reviewed Oct 4, 2017

View reviewed changes

lguohan requested changes Oct 4, 2017

View reviewed changes

jipanyang added 3 commits October 4, 2017 11:14

Calling addBridgePort() only when bridge port not created for port/la…

cdab82f

…g, and calling removeBridgePort() only when port/lag not in any VLAN Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

For addBridgePort() if it is already a bridge port, return true. For …

228dd48

…removeBridgePort, if it is not a bridge, return true. Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

Merge remote-tracking branch 'upstream/master' into multi-vlan-per-po…

52ea678

…rt-lag Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

stcheng approved these changes Oct 6, 2017

View reviewed changes

Add empty line between functions, remove unnecessary \n and add some …

8a0e052

…comment for vlan creation Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

jipanyang force-pushed the multi-vlan-per-port-lag branch from 14031e3 to 8a0e052 Compare October 10, 2017 19:39

lguohan approved these changes Oct 17, 2017

View reviewed changes

lguohan merged commit e5164f9 into sonic-net:master Oct 17, 2017

jipanyang deleted the multi-vlan-per-port-lag branch June 2, 2018 02:04

oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023

SAI 1.3 changes (sonic-net#327)

8585fc8

* Sync to SAI 1.3 header * SAI v1.3 pointer update * Updated queue deadlock to enque notification * Update SAI pointer

		@@ -1462,13 +1616,17 @@ bool PortsOrch::addLagMember(Port lag, Port port)

		m_portList[lag.m_alias] = lag;

		sai_uint32_t pvid = DEFAULT_PORT_VLAN_ID;

orchagent: Support port/LAG in multiple VLANs #327

orchagent: Support port/LAG in multiple VLANs #327

Conversation

jipanyang commented Sep 29, 2017

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

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

Choose a reason for hiding this comment

lguohan commented Sep 30, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lguohan left a comment

Choose a reason for hiding this comment

jipanyang commented Oct 2, 2017 • edited Loading

stcheng commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lguohan Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

lguohan Oct 4, 2017 • 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

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

lguohan left a comment

Choose a reason for hiding this comment

stcheng 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

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

svc-acs commented Oct 10, 2017

jipanyang commented Oct 2, 2017 •

edited

Loading

lguohan Oct 4, 2017 •

edited

Loading

lguohan Oct 4, 2017 •

edited

Loading