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 4 commits
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
60 changes: 58 additions & 2 deletions orchagent/intfsorch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "directory.h"
#include "vnetorch.h"
#include "subscriberstatetable.h"
#include "exec.h"

extern sai_object_id_t gVirtualRouterId;
extern Directory<Orch*> gDirectory;
Expand All @@ -41,6 +42,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


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

if(gMySwitchType == "voq")
if (gMySwitchType == "voq")
{
//STATE DB connection for setting state of the VoQ inband VLAN interface
unique_ptr<DBConnector> stateDb;
stateDb = make_unique<DBConnector>("STATE_DB", 0);
m_stateVlanTable = unique_ptr<Table>(new Table(stateDb.get(), STATE_VLAN_TABLE_NAME));

//Timer to bring up inband Vlan netdev
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);

//Add subscriber to process VOQ system interface
tableName = CHASSIS_APP_SYSTEM_INTERFACE_TABLE_NAME;
Orch::addExecutor(new Consumer(new SubscriberStateTable(chassisAppDb, tableName, TableConsumable::DEFAULT_POP_BATCH_SIZE, 0), this, tableName));
Expand Down Expand Up @@ -737,6 +752,10 @@ void IntfsOrch::doTask(Consumer &consumer)
it++;
continue;
}
if (inband_type == "vlan")
{
m_inbandVlanTimer->start();
}
}

Port port;
Expand Down Expand Up @@ -1319,6 +1338,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 @@ -1357,6 +1381,39 @@ 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 << "ip link set " << inbandVlan.m_alias << " up";
Copy link
Collaborator

Choose a reason for hiding this comment

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

We cannot do "ip link" command from orchagent. This approach has to be revisited.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prsunny can you elaborate why we cannot do this in orchagent?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no kernel interactions from orchagent. In the current architecture, this is handled by the manager daemons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prsunny Thanks for the explanation. The kernel interaction was moved to intfmgr as suggested.


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
{
m_stateVlanTable->hset(inbandVlan.m_alias, "state", "ok");
SWSS_LOG_INFO("Added inband VLAN %s to STATE_VLAN_TABLE", inbandVlan.m_alias.c_str());
}

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.

}
}

bool IntfsOrch::isRemoteSystemPortIntf(string alias)
{
Port port;
Expand Down Expand Up @@ -1415,4 +1472,3 @@ void IntfsOrch::voqSyncDelIntf(string &alias)

m_tableVoqSystemInterfaceTable->del(alias);
}

4 changes: 4 additions & 0 deletions orchagent/intfsorch.h
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ class IntfsOrch : public Orch
unique_ptr<Table> m_rifNameTable;
unique_ptr<Table> m_rifTypeTable;
unique_ptr<Table> m_vidToRidTable;
unique_ptr<Table> m_stateVlanTable;
unique_ptr<ProducerTable> m_flexCounterTable;
unique_ptr<ProducerTable> m_flexCounterGroupTable;

Expand All @@ -98,6 +99,9 @@ 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;
void processInbandVlanReady();

unique_ptr<Table> m_tableVoqSystemInterfaceTable;
void voqSyncAddIntf(string &alias);
void voqSyncDelIntf(string &alias);
Expand Down
79 changes: 73 additions & 6 deletions orchagent/neighorch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,21 @@ void NeighOrch::decreaseNextHopRefCount(const NextHopKey &nexthop, uint32_t coun

bool NeighOrch::getNeighborEntry(const NextHopKey &nexthop, NeighborEntry &neighborEntry, MacAddress &macAddress)
{
if (!hasNextHop(nexthop))
string alias = nexthop.alias;
if(m_intfsOrch->isRemoteSystemPortIntf(alias))
{
Port inbp;
if (!gPortsOrch->getInbandPort(inbp))
{
SWSS_LOG_ERROR("Failed to find inband port");
return false;
}
alias = inbp.m_alias;
}

NextHopKey updatedNexthop(nexthop.ip_address, alias);

if (!hasNextHop(updatedNexthop))
{
return false;
}
Expand All @@ -474,12 +488,20 @@ bool NeighOrch::getNeighborEntry(const NextHopKey &nexthop, NeighborEntry &neigh

bool NeighOrch::getNeighborEntry(const IpAddress &ipAddress, NeighborEntry &neighborEntry, MacAddress &macAddress)
{
string alias = m_intfsOrch->getRouterIntfsAlias(ipAddress);
if (alias.empty())
string alias;
if (m_remoteNeigh.find(ipAddress) != m_remoteNeigh.end())
{
return false;
alias = m_remoteNeigh[ipAddress];
}

else
{
alias = m_intfsOrch->getRouterIntfsAlias(ipAddress);
if (alias.empty())
{
return false;
}
}

NextHopKey nexthop(ipAddress, alias);
return getNeighborEntry(nexthop, neighborEntry, macAddress);
}
Expand Down Expand Up @@ -541,6 +563,26 @@ void NeighOrch::doTask(Consumer &consumer)

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

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)
{
/* 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.
*/
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;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic can go as else part (after line 560). The getPortOrch->isInbandPort(alias) and getPortsOrch->getInbandPort(ibport) are redundantly called multiple times.

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. Fixed.

}
}

NeighborEntry neighbor_entry = { ip_address, alias };

if (op == SET_COMMAND)
Expand Down Expand Up @@ -1088,6 +1130,8 @@ void NeighOrch::doVoqSystemNeighTask(Consumer &consumer)
fvVector.push_back(mac);
m_stateSystemNeighTable->set(state_key, fvVector);

m_m_remoteNeigh[ip_address] = alias

it = consumer.m_toSync.erase(it);
}
else
Expand All @@ -1113,6 +1157,8 @@ void NeighOrch::doVoqSystemNeighTask(Consumer &consumer)
//neigh successfully deleted from SAI. Set STATE DB to signal to remove entries from kernel
m_stateSystemNeighTable->del(state_key);

m_remoteNeigh.erase(ip_address);

it = consumer.m_toSync.erase(it);
}
else
Expand Down Expand Up @@ -1203,6 +1249,19 @@ void NeighOrch::voqSyncAddNeigh(string &alias, IpAddress &ip_address, const MacA
sai_attribute_t attr;
sai_status_t status;

Port inbandPort;
if (!gPortsOrch->getInbandPort(inbandPort))
{
SWSS_LOG_ERROR("Failed to get inband port/VLAN");
return;
}

if (inbandPort.m_type == Port::VLAN && inbandPort.m_alias == alias)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this case only for inband vlan ? Can you please explain which case it falls as per https://github.com/Azure/SONiC/blob/master/doc/voq/voq_hld.md ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In inband vlan case, each asic (precisely its cpu port) participates in inband vlan. Each asic has its own inband IP address that belongs to the same inband vlan network. Each asic learns the inband IPs of other asics as neighbors. These neighbors are mainly used for the communication between SONiC instances like routing protocol peering. Therefor, we added this change to not add inband neighbors into system neigh table. This is described in inband vlan section (2.5.2) in voq HLD.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks

SWSS_LOG_NOTICE("Skip inband VLAN neigh: %s", ip_address.to_string().c_str());
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It will be great if comments are added for this block of code saying that "inband neighbors are not synced to CHASSIS_APP_DB for inband interface type VLAN since the inband neigibors are learned via arp learning within inband VLAN"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the comment as suggested.

//Sync only local neigh. Confirm for the local neigh and
//get the system port alias for key for syncing to CHASSIS_APP_DB
Port port;
Expand Down Expand Up @@ -1240,7 +1299,15 @@ void NeighOrch::voqSyncAddNeigh(string &alias, IpAddress &ip_address, const MacA
FieldValueTuple eiFv ("encap_index", to_string(attr.value.u32));
attrs.push_back(eiFv);

FieldValueTuple macFv ("neigh", mac.to_string());
FieldValueTuple macFv;
if (inbandPort.m_type == Port::VLAN)
{
macFv = FieldValueTuple("neigh", gMacAddress.to_string());
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 are putting our own mac when sync to chassis app db ? remote side will use this to program SAI . Why we need to send our own mac to remote side ? as per design doc also SAI neighbor tables uses actual mac for vlan inband case. can you please clarify.

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 need to install kernel neighbor entries for remote neighbors. In inband vlan case, the mac of the kernel neighbor entry must be the router mac of remote asic, where the remote neighbor was learnt. This was described in section 2.5.2.2 of voq HLD. Use neighbor nbr-0-1 as an example shown in that section. nbr-0-1 is learnt by asic0. A kernel neighbor entry is installed for the neighbor in asic1 with asic0's mac. This is why local router mac is published in chassis app db here. SAI programming for remote neighbor uses encap index, which is an indirection of the actual mac of remote neighbor.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks. looks like i overlooked it

}
else
{
macFv = FieldValueTuple("neigh", mac.to_string());
}
attrs.push_back(macFv);

string key = alias + m_tableVoqSystemNeighTable->getTableNameSeparator().c_str() + ip_address.to_string();
Expand Down
3 changes: 3 additions & 0 deletions orchagent/neighorch.h
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ class NeighOrch : public Orch, public Subject, public Observer
bool resolveNeighborEntry(const NeighborEntry &, const MacAddress &);

void doTask(Consumer &consumer);

void doVoqSystemNeighTask(Consumer &consumer);

unique_ptr<Table> m_tableVoqSystemNeighTable;
Expand All @@ -104,6 +105,8 @@ class NeighOrch : public Orch, public Subject, public Observer
bool addVoqEncapIndex(string &alias, IpAddress &ip, vector<sai_attribute_t> &neighbor_attrs);
void voqSyncAddNeigh(string &alias, IpAddress &ip_address, const MacAddress &mac, sai_neighbor_entry_t &neighbor_entry);
void voqSyncDelNeigh(string &alias, IpAddress &ip_address);

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

#endif /* SWSS_NEIGHORCH_H */