-
Notifications
You must be signed in to change notification settings - Fork 539
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
Changes from 6 commits
9866901
6060060
ff48127
01552f9
d84e5ee
035c6cb
30b3cdf
798348c
2de8cd7
738e5f6
3efaf24
2c7508e
39b703f
9298c9e
8f587fc
c418c43
bfa4adb
3a6967d
cdf5f36
f3dc29d
0ddc5ab
f6d6440
9aff9c2
1d46612
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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 = | ||
{ | ||
|
@@ -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); | ||
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)); | ||
|
@@ -737,6 +752,10 @@ void IntfsOrch::doTask(Consumer &consumer) | |
it++; | ||
continue; | ||
} | ||
if (inband_type == "vlan") | ||
{ | ||
m_inbandVlanTimer->start(); | ||
} | ||
} | ||
|
||
Port port; | ||
|
@@ -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(); ) | ||
|
@@ -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"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @prsunny can you elaborate why we cannot do this in orchagent? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
} | ||
} | ||
} | ||
|
||
bool IntfsOrch::isRemoteSystemPortIntf(string alias) | ||
{ | ||
Port port; | ||
|
@@ -1415,4 +1472,3 @@ void IntfsOrch::voqSyncDelIntf(string &alias) | |
|
||
m_tableVoqSystemInterfaceTable->del(alias); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
} | ||
|
@@ -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); | ||
} | ||
|
@@ -524,23 +546,36 @@ void NeighOrch::doTask(Consumer &consumer) | |
continue; | ||
} | ||
|
||
if(gPortsOrch->isInbandPort(alias)) | ||
IpAddress ip_address(key.substr(found+1)); | ||
|
||
if (gPortsOrch->isInbandPort(alias)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I will rebase and remove comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
{ | ||
Port ibport; | ||
gPortsOrch->getInbandPort(ibport); | ||
if(ibport.m_type != Port::VLAN) | ||
if (ibport.m_type != Port::VLAN) | ||
{ | ||
//For "port" type Inband, the neighbors are only remote neighbors. | ||
//Hence, this is the neigh learned due to the kernel entry added on | ||
//Inband interface for the remote system port neighbors. Skip | ||
it = consumer.m_toSync.erase(it); | ||
continue; | ||
} | ||
//For "vlan" type inband, may identify the remote neighbors and skip | ||
else | ||
{ | ||
/* 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()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} | ||
} | ||
|
||
IpAddress ip_address(key.substr(found+1)); | ||
|
||
|
||
NeighborEntry neighbor_entry = { ip_address, alias }; | ||
|
||
if (op == SET_COMMAND) | ||
|
@@ -1088,6 +1123,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 | ||
|
@@ -1113,6 +1150,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 | ||
|
@@ -1203,6 +1242,21 @@ 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; | ||
} | ||
|
||
// Inband neighbors are not synced to CHASSIS_APP_DB for VLAN type inband interface | ||
// since these neigibors are learned within inband VLAN. | ||
if (inbandPort.m_type == Port::VLAN && inbandPort.m_alias == alias) | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
//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; | ||
|
@@ -1240,7 +1294,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()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
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 timer ? Can you point to design doc that cover 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.
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.
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.
@ysmanman how is this different w.r.t Regular VLAN IP Interface ? We don;t need timer there.
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.
@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.
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.
sai call is synchronized now starting from 202012 release
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.
@lguohan thanks for the comment. I removed the inband vlan timer since sai call is synchronized now.