-
Notifications
You must be signed in to change notification settings - Fork 519
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
orchagent: Support port/LAG in multiple VLANs #327
Conversation
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>
orchagent/port.h
Outdated
sai_vlan_tagging_mode_t vlan_mode; | ||
}; | ||
|
||
typedef std::map<sai_vlan_id_t, VlanMemberEntry> port_vlan_members_t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
port_vlan_members_t [](start = 49, length = 19)
rename to vlan_members_t
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
orchagent/portsorch.cpp
Outdated
|
||
for (auto &vme: port.m_vlan_members) | ||
{ | ||
if(vme.second.vlan_mode == SAI_VLAN_TAGGING_MODE_UNTAGGED) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ [](start = 67, length = 1)
'{' in a separate line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
orchagent/portsorch.cpp
Outdated
|
||
for (auto &vme: port.m_vlan_members) | ||
{ | ||
if(vme.second.vlan_mode == SAI_VLAN_TAGGING_MODE_UNTAGGED) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f( [](start = 9, length = 2)
keep the space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
orchagent/portsorch.cpp
Outdated
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[](start = 27, length = 1)
no space here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
orchagent/portsorch.cpp
Outdated
@@ -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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this? we can keep the assumption that the untagged mode vlan is the pvid of the port.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
orchagent/portsorch.cpp
Outdated
getPort(vlan_alias, v); | ||
v.m_mtu = mtu; | ||
// TODO: update MTU for the IP interface of the VLAN | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since it is todo, I suggest to remove it. The rif MTU should be configured under interface_table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
btw, VLAN_TABLE schema might needs some update to remove mtu field.
mtu = 1*4DIGIT ; MTU for the IP interface of the VLAN
orchagent/portsorch.cpp
Outdated
@@ -1155,6 +1261,9 @@ bool PortsOrch::addBridgePort(Port &port) | |||
{ | |||
SWSS_LOG_ENTER(); | |||
|
|||
if (port.m_bridge_port_id) | |||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use {} even for one line statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
orchagent/portsorch.cpp
Outdated
|
||
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
port.m_port_vlan_id == DEFAULT_PORT_VLAN_ID [](start = 8, length = 44)
not sure why we need this condition, can you explain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As explained previously, it was for supporting pvid configuration from admin interface.
removed.
orchagent/portsorch.cpp
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
port.m_port_vlan_id == DEFAULT_PORT_VLAN_ID && [](start = 8, length = 47)
not sure if we need this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As explained previously, it was for supporting pvid configuration from admin interface.
removed.
can you also submit the vlan trunk test you mentioned in the PR. |
orchagent/portsorch.cpp
Outdated
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
= DEFAULT_PORT_VLAN_ID [](start = 21, length = 23)
not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed.
orchagent/portsorch.cpp
Outdated
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
orchagent/portsorch.cpp
Outdated
return true; | ||
} | ||
} | ||
pvid = DEFAULT_PORT_VLAN_ID; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As explained previously, it was for supporting pvid configuration from admin interface.
Updated.
orchagent/portsorch.cpp
Outdated
SWSS_LOG_ENTER(); | ||
vector<Port> portv; | ||
|
||
if (port.m_type == Port::PHY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
return false; | ||
} | ||
SWSS_LOG_NOTICE("Set pvid %u to port: %s", attr.value.u32, p.m_alias.c_str()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_port_vlan_id = pvid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
orchagent/port.h
Outdated
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is irrelevant to the pr. consider to remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as comments
Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>
vlan trunk test mentioned in this PR is the VLAN trunk test plan reviewed at SONiC community meeting few weeks ago. |
hi could you resolve the conflicts? |
orchagent/portsorch.cpp
Outdated
{ | ||
SWSS_LOG_DEBUG("Port %s still in VLAN %d", port.m_alias.c_str(), vme.first); | ||
} | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
orchagent/portsorch.cpp
Outdated
{ | ||
for (auto &vme: port.m_vlan_members) | ||
{ | ||
SWSS_LOG_DEBUG("Port %s still in VLAN %d", port.m_alias.c_str(), vme.first); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change level to SWSS_LOG_ERROR, this should not happen at orchagent level, the dependency resolution should be handled by the cfg mgr level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As previous explanation.
We may argue there could be different way of implementation if it is too confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the return value of the function needs to be consistent, true mean removed, false means not removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it is a little confusing. Will do the m_vlan_members set empty check outside of this function.
orchagent/portsorch.cpp
Outdated
@@ -1213,9 +1295,11 @@ bool PortsOrch::removeBridgePort(Port port) | |||
port.m_alias.c_str(), status); | |||
return false; | |||
} | |||
port.m_bridge_port_id = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0 -> SAI_NULL_OBJECT_ID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
orchagent/portsorch.cpp
Outdated
assert(port.m_vlan_id && port.m_vlan_member_id); | ||
|
||
if (removeVlanMember(vlan, port)) | ||
if (removeVlanMember(vlan, port) && removeBridgePort(port)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there is no vlan member, then we can removeBridgePort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
orchagent/portsorch.cpp
Outdated
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[](start = 0, length = 2)
make this as space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let me change it again, hope the new code will match what you want.
orchagent/portsorch.cpp
Outdated
@@ -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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be DEBUG LEVEL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
orchagent/portsorch.cpp
Outdated
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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As explained before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved. Add some comments for minor changes.
sai_vlan_tagging_mode_t vlan_mode; | ||
}; | ||
|
||
typedef std::map<sai_vlan_id_t, VlanMemberEntry> vlan_members_t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to VlanMemberMap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
|
||
pvid = port.m_port_vlan_id; | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add an empty line here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty line before "return true;"? It might be good if SONiC has clear specification about the coding style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean between the }
and the next function.
orchagent/portsorch.cpp
Outdated
@@ -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()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comments indicating that only creation is supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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."
orchagent/portsorch.cpp
Outdated
{ | ||
if (!getPort(name, member)) | ||
{ | ||
SWSS_LOG_ERROR("Failed to get port for %s alias\n", name.c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove trailing \n
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
…comment for vlan creation Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>
14031e3
to
8a0e052
Compare
Build finished. No test results found. |
VLAN trunk test plan: https://github.com/Azure/SONiC/wiki/VLAN-trunk-test-plan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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