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

Add support for Inband VLan #1555

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
9866901
Add support for Inband VLan
ysmanman Dec 17, 2020
6060060
Merge remote-tracking branch 'upstream/master' into inband-vlan-pr
ysmanman Feb 16, 2021
ff48127
Fix typo in merge conflict resolution
ysmanman Feb 16, 2021
01552f9
Do not sync local inband VLAN neigh
ysmanman Feb 16, 2021
d84e5ee
Refactor code and add comment
ysmanman Feb 22, 2021
035c6cb
Stop inband VLAN timer only after inband VLAn is brought up in kernel
ysmanman Feb 22, 2021
30b3cdf
Fix typo
ysmanman Feb 23, 2021
798348c
Add missing semicolon
ysmanman Feb 26, 2021
2de8cd7
Add space after if keyword
ysmanman Feb 26, 2021
738e5f6
Add local and remote cpu ports to inband VLAN in the same for loop.
ysmanman Apr 11, 2021
3efaf24
Merge branch 'master' into inband-vlan-pr
ysmanman Apr 21, 2021
2c7508e
Merge remote-tracking branch 'upstream/master' into inband-vlan-pr
ysmanman Apr 29, 2021
39b703f
Bring up kernel intf of inband Vlan in intfmgr.
ysmanman Apr 30, 2021
9298c9e
Add test for inband Vlan
ysmanman Apr 30, 2021
8f587fc
Fix comments
ysmanman Apr 30, 2021
c418c43
Fix bug in setVoqInbandIntf
ysmanman May 6, 2021
bfa4adb
Separate the handling of VOQ inbandif based on inband type to make co…
ysmanman May 6, 2021
3a6967d
Only check port object for inband port
ysmanman May 7, 2021
cdf5f36
Do not add hostif for inband Vlan in VS
ysmanman May 7, 2021
f3dc29d
Fix inband vlan test
ysmanman May 7, 2021
0ddc5ab
Fix typo
ysmanman May 7, 2021
f6d6440
Get rid of inban vlan timer since sai call is synchronized now.
ysmanman May 11, 2021
9aff9c2
Merge branch 'master' into inband-vlan-pr
ysmanman May 28, 2021
1d46612
Don't add directed broadcast for inband Vlan
ysmanman May 28, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 50 additions & 0 deletions orchagent/intfsorch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "bufferorch.h"
#include "directory.h"
#include "vnetorch.h"
#include "exec.h"

extern sai_object_id_t gVirtualRouterId;
extern Directory<Orch*> gDirectory;
Expand All @@ -38,6 +39,8 @@ const int intfsorch_pri = 35;
#define RIF_FLEX_STAT_COUNTER_POLL_MSECS "1000"
#define UPDATE_MAPS_SEC 1

#define INBAND_INTF_TIMER_SEC 1

#define LOOPBACK_PREFIX "Loopback"

static const vector<sai_router_interface_stat_t> rifStatIds =
Expand Down Expand Up @@ -96,6 +99,15 @@ IntfsOrch::IntfsOrch(DBConnector *db, string tableName, VRFOrch *vrf_orch) :
{
SWSS_LOG_WARN("RIF flex counter group plugins was not set successfully: %s", e.what());
}

if (gMySwitchType == "voq")
{
auto intervT = timespec { .tv_sec = INBAND_INTF_TIMER_SEC , .tv_nsec = 0 };
m_inbandVlanTimer = new SelectableTimer(intervT);
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need timer ? Can you point to design doc that cover this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since SAI create call is not synchronized, we cannot bring up inband vlan in kernel immediately after creating inband vlan with SAI call. We use this timer to periodically check if it's ready to bring up inband vlan in kernel or not. The timer is stopped once inband vlan is brought up in kernel successfully. I think this is implementation details, and thus is not covered in HLD.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ysmanman how is this different w.r.t Regular VLAN IP Interface ? We don;t need timer there.

Copy link
Contributor Author

@ysmanman ysmanman Apr 21, 2021

Choose a reason for hiding this comment

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

@abdosi There are few differences between regular Vlan and Inban Vlan: 0) The members of Inband Vlan are local and remote CPU ports, which don't have kernel devices created today. 1) Inband Vlan is a static configuration and imembership is unlikely user configurable. These are the main reasons that Inband Vlan was implemented different from regular Vlans and therefore we added the timer.

Copy link
Contributor

Choose a reason for hiding this comment

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

sai call is synchronized now starting from 202012 release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lguohan thanks for the comment. I removed the inband vlan timer since sai call is synchronized now.

auto executorInbandVlan = new ExecutableTimer(m_inbandVlanTimer, this,
"INBAND_VLAN_TIMER");
Orch::addExecutor(executorInbandVlan);
}
}

sai_object_id_t IntfsOrch::getRouterIntfsId(const string &alias)
Expand Down Expand Up @@ -1250,6 +1262,11 @@ void IntfsOrch::doTask(SelectableTimer &timer)
{
SWSS_LOG_ENTER();

if (m_inbandVlanTimer == &timer)
{
return processInbandVlanReady();
}

SWSS_LOG_DEBUG("Registering %" PRId64 " new intfs", m_rifsToAdd.size());
string value;
for (auto it = m_rifsToAdd.begin(); it != m_rifsToAdd.end(); )
Expand Down Expand Up @@ -1286,3 +1303,36 @@ void IntfsOrch::doTask(SelectableTimer &timer)
}
}
}

void IntfsOrch::processInbandVlanReady()
{
SWSS_LOG_ENTER();

Port inbandVlan;
if (!gPortsOrch->getInbandPort(inbandVlan))
{
return;
}

string value;
const auto id = sai_serialize_object_id(inbandVlan.m_rif_id);
if (m_vidToRidTable->hget("", id, value))
{
stringstream cmd;
cmd << "ifconfig " << inbandVlan.m_alias << " " << m_inbandAddress.to_string() << " up";

std::string res;
int ret = swss::exec(cmd.str(), res);
if (ret)
{
SWSS_LOG_ERROR("Command '%s' failed with rc %d", cmd.str().c_str(), ret);
}
else
{
SWSS_LOG_NOTICE("Configured Ip on the inband vlan");

}

m_inbandVlanTimer->stop();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to stop the timer when there is failure in bringing up inband vlan in the kernel? Will it be a good idea to leave the timer running for re-tries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right. Fixed.

}
}
4 changes: 4 additions & 0 deletions orchagent/intfsorch.h
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ class IntfsOrch : public Orch

bool setIntfVlanFloodType(const Port &port, sai_vlan_flood_control_type_t vlan_flood_type);
bool setIntfProxyArp(const string &alias, const string &proxy_arp);

SelectableTimer* m_inbandVlanTimer = nullptr;
IpPrefix m_inbandAddress;
void processInbandVlanReady();
};

#endif /* SWSS_INTFSORCH_H */
29 changes: 29 additions & 0 deletions orchagent/neighorch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,35 @@ void NeighOrch::doTask(Consumer &consumer)

IpAddress ip_address(key.substr(found+1));

/* The following change is based on the following PR:
* https://github.com/Azure/sonic-swss/pull/1431
*
* For inband Vlan, a kernel neigh is added for every remote neighbor
* and the interface that the neigh is attached is inband Vlan.
* neighsyncd adds the kernel neigh to neigh table in APPL_DB again.
* The following change is to skip adding these remote neighbor attached
* to inband Vlan.
*
* The collection m_remoteNeigh is added to track remote neighbor. It's
* updated in the following method introduced in the above PR:
*
* doVoqSystemNeighTask()
*/
if (gPortsOrch->isInbandPort(alias))
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the comments. Whole part of this change should be merged with existing Inband port type handling. PR 1431 is merged. Please rebase and adjust the changes accordingly so that the comments can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I will rebase and remove comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

{
Port ibport;
gPortsOrch->getInbandPort(ibport);
if(ibport.m_type == Port::VLAN)
{
/* Only create neighbor attached to inband Vlan */
if (m_remoteNeigh.find(ip_address) != m_remoteNeigh.end())
Copy link
Contributor

Choose a reason for hiding this comment

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

Can Inband Vlan have neighbors other than remote neighbors? If not, we do not need to have collecting remote neigh. Just checking the interface of the neigh itself is enough to skip (as it is done for the Inband port type).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. iBGP peer can be resolved over inband Vlan. This kind of inband vlan neighbor needs to be created. But remote neighbors can be skipped.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vganesan-nokia why same cannot happen with Inband port ? iBGP neighbor can happen on inband port also ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The only neighbors that would be available on the inband port are the static neigbors we add for the remote neighbors (which should be skipped here since these are already added to SAI as part of doVoqSystemNeighTask()). Unlike inband vlan, inband port will not have any other neighbors. So no need to collect or verify whether the neihbor is remote or not. Just check the interface of the neighbor and if the interface is inband interface, skip the neighbor.

For inband port except the ip address of inband port of my asic, all the iBGP neighbors (which are ip address of inband port of other asics) are remote neighbors. These are already programmed in SAI by doVoqSystemNeighTask() so we skip here. But inband vlan will have both the iBGP neighbors (resolved via ARP) which should be programmed here and remote neighbors which are added statically to inband vlan and SAI prgrammed in doVoqSystemNeighTask(). Therefore for inband vlan case, we need to indentify the remote neighbors and skip.

{
it = consumer.m_toSync.erase(it);
continue;
}
}
}

NeighborEntry neighbor_entry = { ip_address, alias };

if (op == SET_COMMAND)
Expand Down
2 changes: 2 additions & 0 deletions orchagent/neighorch.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ class NeighOrch : public Orch, public Subject, public Observer
bool resolveNeighborEntry(const NeighborEntry &, const MacAddress &);

void doTask(Consumer &consumer);

std::map<IpAddress, string> m_remoteNeigh;
};

#endif /* SWSS_NEIGHORCH_H */
144 changes: 129 additions & 15 deletions orchagent/portsorch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3326,7 +3326,14 @@ bool PortsOrch::addHostIntfs(Port &port, string alias, sai_object_id_t &host_int
attrs.push_back(attr);

attr.id = SAI_HOSTIF_ATTR_OBJ_ID;
attr.value.oid = port.m_port_id;
if (port.m_type == Port::VLAN)
{
attr.value.oid = port.m_vlan_info.vlan_oid;
}
else
{
attr.value.oid = port.m_port_id;
}
attrs.push_back(attr);

attr.id = SAI_HOSTIF_ATTR_NAME;
Expand Down Expand Up @@ -3378,7 +3385,13 @@ bool PortsOrch::addBridgePort(Port &port)
sai_attribute_t attr;
vector<sai_attribute_t> attrs;

if (port.m_type == Port::PHY)
/*
* Local and remote CPU ports need to be added to inband Vlan.
* Port type and oid setting for these CPU ports are similar to PHY ports.
*/
if (port.m_type == Port::PHY ||
port.m_type == Port::CPU ||
port.m_type == Port::SYSTEM)
{
attr.id = SAI_BRIDGE_PORT_ATTR_TYPE;
attr.value.s32 = SAI_BRIDGE_PORT_TYPE_PORT;
Expand Down Expand Up @@ -3420,22 +3433,28 @@ bool PortsOrch::addBridgePort(Port &port)
}

/* Create a bridge port with admin status set to UP */
attr.id = SAI_BRIDGE_PORT_ATTR_ADMIN_STATE;
attr.value.booldata = true;
attrs.push_back(attr);

/* And with hardware FDB learning mode set to HW (explicit default value) */
attr.id = SAI_BRIDGE_PORT_ATTR_FDB_LEARNING_MODE;
auto found = learn_mode_map.find(port.m_learn_mode);
if (found == learn_mode_map.end())
if (port.m_type != Port::SYSTEM)
{
attr.value.s32 = SAI_BRIDGE_PORT_FDB_LEARNING_MODE_HW;
attr.id = SAI_BRIDGE_PORT_ATTR_ADMIN_STATE;
attr.value.booldata = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

why we skip this for system port ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both local and remote cpu ports need to be added into inband vlan. The port type of remote cpu ports is system port, and thus we have to add the check here to skip the processing that's not required for system port.

Copy link
Contributor

Choose a reason for hiding this comment

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

@michaelli10 Can you help. We can add system port as part of Bridge. Can we set SAI_BRIDGE_PORT_ATTR_ADMIN_STATE for Remote Systrem Port and what does it mean ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@abdosi SAI_BRIDGE_PORT_ATTR_ADMIN_STATE is not currently supported for remote system port.

attrs.push_back(attr);
Comment on lines +4020 to +4022
Copy link
Contributor

Choose a reason for hiding this comment

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

This part of the code can be moved in the if block at line 3444. The check for the port.m_type != Port::SYSTEM is in two places unnecessarily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Will fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

}
else

/* And with hardware FDB learning mode set to HW (explicit default value) */
if (port.m_type != Port::SYSTEM)
{
attr.value.s32 = found->second;
attr.id = SAI_BRIDGE_PORT_ATTR_FDB_LEARNING_MODE;
auto found = learn_mode_map.find(port.m_learn_mode);
if (found == learn_mode_map.end())
{
Copy link
Contributor

Choose a reason for hiding this comment

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

also any reason to skip this for system port ? code here is generic for add systemport as bridge port.
Yes for inband use case this not needed but i feel this code should be kept generic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both local and remote cpu ports need to be added into inband vlan. The port type of remote cpu ports is system port, and thus we have to add the check here to skip the processing that's not required for system port.

attr.value.s32 = SAI_BRIDGE_PORT_FDB_LEARNING_MODE_HW;
}
else
{
attr.value.s32 = found->second;
}
attrs.push_back(attr);
}
attrs.push_back(attr);

sai_status_t status = sai_bridge_api->create_bridge_port(&port.m_bridge_port_id, gSwitchId, (uint32_t)attrs.size(), attrs.data());
if (status != SAI_STATUS_SUCCESS)
Expand All @@ -3445,7 +3464,8 @@ bool PortsOrch::addBridgePort(Port &port)
return false;
}

if (!setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_KEEP))
if (port.m_type != Port::SYSTEM &&
!setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_KEEP))
{
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());
Expand Down Expand Up @@ -4631,3 +4651,97 @@ bool PortsOrch::initGearboxPort(Port &port)
return true;
}

/*
* The following method is called from setVoqInbandIntf() introduced in the following PR:
* https://github.com/Azure/sonic-swss/pull/1431
*/
bool PortsOrch::addInbandVlan(const string &alias, const string &type)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the comments. PR 1431 is merged. setVoqInbandIntf() can be modified to call this api if the inband type is "vlan". Please rebase and modify setVoqInbandIntf() to make this complete and the comments can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I will rebase and remove the comment.

{
if (m_inbandPortName == alias)
{
/* Inband vlan already exists with this name */
SWSS_LOG_WARN("Interface %s is already configured as inband!", alias.c_str());
return true;
}

if (type != "vlan")
{
SWSS_LOG_WARN("Inband type %s is not vlan.", type.c_str());
return false;
}

/* Create inband Vlan and also local and remote CPU ports to inband Vlan */

Port cpuPort;
Port inbandVlan;
string tagged_mode("tagged");

if (!addVlan(alias))
{
SWSS_LOG_ERROR("Failed to add inband VLAN %s", alias.c_str());
return false;
}

if (!getPort(alias, inbandVlan))
{
SWSS_LOG_ERROR("Failed to get port object of inband VLAN %s", alias.c_str());
return false;
}
Comment on lines +5767 to +5777
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not do this vlan addition processing via vlan configuration by having entry in VLAN table in config_db? So that this will be similar to inband port type. For port type, the VOQ_INBAND_INTERFACE configuration is just to designate a port or vlan for interface for cpu-to-cpu communication. We actually do not configure the port or vlan itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We thought about this too. But inband VLAN is not exactly the same as regular SONiC VLANs. For example, the inband VLAN members are cpu ports, which do not have host intfs being created today. Therefore, inband VLAN processing is different from VLAN processing.


getCpuPort(cpuPort);

if (!addBridgePort(cpuPort))
{
SWSS_LOG_ERROR("Failed to add CPU port %s as bridge port", cpuPort.m_alias.c_str());
return false;
}

if (!addVlanMember(inbandVlan, cpuPort, tagged_mode))
{
SWSS_LOG_ERROR("Failed to add CPU port %s in inband VLAN", cpuPort.m_alias.c_str());
return false;
}

SWSS_LOG_NOTICE("add port %s to inbandVlan %s", cpuPort.m_alias.c_str(), inbandVlan.m_alias.c_str());

if (!addHostIntfs(inbandVlan, inbandVlan.m_alias, inbandVlan.m_hif_id))
{
SWSS_LOG_ERROR("Failed to add host intf for inband VLAN");
return false;
}

if (!setHostIntfsOperStatus(inbandVlan, true))
{
SWSS_LOG_ERROR("Failed to set host intf oper status for inband VLAN");
return false;
}

for (auto &it: m_portList)
{
Port port = it.second;
if (port.m_type != Port::SYSTEM ||
port.m_system_port_info.type == SAI_SYSTEM_PORT_TYPE_LOCAL ||
Copy link
Contributor

Choose a reason for hiding this comment

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

can local cpu port be covered in this same loop ? basically this will make code common w.r.t API addBridgePort() and addVlanMemeber()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good point. I will try to see if I can move local cpu port processing into the for loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed as what you suggested.

port.m_alias.find("Cpu") == string::npos)
{
continue;
}

if (!addBridgePort(port))
{
SWSS_LOG_ERROR("Failed to add port %s as bridge port", port.m_alias.c_str());
continue;
}

if (!addVlanMember(inbandVlan, port, tagged_mode))
{
SWSS_LOG_ERROR("Failed to add port %s in inband VLAN", port.m_alias.c_str());
continue;
}

SWSS_LOG_NOTICE("add sysport %s to inbandVlan %s", port.m_alias.c_str(), inbandVlan.m_alias.c_str());
}

m_inbandPortName = alias;

return true;
}
9 changes: 8 additions & 1 deletion orchagent/portsorch.h
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,14 @@ class PortsOrch : public Orch, public Subject
sai_acl_bind_point_type_t &sai_acl_bind_type);
void initGearbox();
bool initGearboxPort(Port &port);


/*
* This is actually added by the following PR:
* https://github.com/Azure/sonic-swss/pull/1431
*/
string m_inbandPortName;

bool addInbandVlan(const string &alias, const string &type);
};
#endif /* SWSS_PORTSORCH_H */