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.

Sign up for GitHub

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

Jump to bottom

Add support for Inband VLan #1555

Closed
wants to merge 24 commits into from
Closed

Add support for Inband VLan #1555

Show file tree
Hide file tree
Changes from 22 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 46 additions & 4 deletions cfgmgr/intfmgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -735,11 +735,53 @@ void IntfMgr::doTask(Consumer &consumer)
if((table_name == CFG_VOQ_INBAND_INTERFACE_TABLE_NAME) &&
(op == SET_COMMAND))
{
//No further processing needed. Just relay to orchagent
m_appIntfTableProducer.set(keys[0], data);
m_stateIntfTable.hset(keys[0], "vrf", "");
string inband_type = "";
for (auto idx : data)
{
const auto &field = fvField(idx);
const auto &value = fvValue(idx);
if (field == "inband_type")
{
inband_type = value;
}
}


if (inband_type == "port")
{
//No further processing needed. Just relay to orchagent
m_appIntfTableProducer.set(keys[0], data);
m_stateIntfTable.hset(keys[0], "vrf", "");

it = consumer.m_toSync.erase(it);
it = consumer.m_toSync.erase(it);
}
else
{
vector<FieldValueTuple> temp;
if (!m_stateIntfTable.get(keys[0], temp))
{
m_appIntfTableProducer.set(keys[0], data);
m_stateIntfTable.hset(keys[0], "vrf", "");
}

// Bring up kernel intf of inband Vlan only after it has been created in SAI.
if( m_stateVlanTable.get(keys[0], temp))
{
stringstream cmd;
cmd << IP_CMD << " link set " << keys[0] << " 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("Inband Vlan %s was up", keys[0].c_str());
it = consumer.m_toSync.erase(it);
}
}
}
continue;
}
if (!doIntfGeneralTask(keys, data, op))
Expand Down
34 changes: 31 additions & 3 deletions orchagent/intfsorch.cpp
View file
Open in desktop
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 @@ -42,7 +43,6 @@ const int intfsorch_pri = 35;
#define RIF_FLEX_STAT_COUNTER_POLL_MSECS "1000"
#define UPDATE_MAPS_SEC 1


static const vector<sai_router_interface_stat_t> rifStatIds =
{
SAI_ROUTER_INTERFACE_STAT_IN_PACKETS,
Expand Down Expand Up @@ -100,8 +100,13 @@ 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));

//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 @@ -815,6 +820,30 @@ void IntfsOrch::doTask(Consumer &consumer)
continue;
}

if (inband_type == "vlan")
{
Port inbandVlan;
if (!gPortsOrch->getInbandPort(inbandVlan))
{
it++;
continue;
}

string value;
const auto id = sai_serialize_object_id(inbandVlan.m_rif_id);
if (m_vidToRidTable->hget("", id, value))
{
m_stateVlanTable->hset(inbandVlan.m_alias, "state", "ok");
SWSS_LOG_NOTICE("Added inband VLAN %s to STATE_VLAN_TABLE", inbandVlan.m_alias.c_str());
}
else
{
SWSS_LOG_INFO("Cannot find rid of inband vlan in stateDb");
it++;
continue;
}
}

if (gPortsOrch->getPort(alias, port))
{
/* Set nat zone id */
Expand Down Expand Up @@ -1491,4 +1520,3 @@ void IntfsOrch::voqSyncDelIntf(string &alias)

m_tableVoqSystemInterfaceTable->del(alias);
}

1 change: 1 addition & 0 deletions orchagent/intfsorch.h
View file
Open in desktop
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 Down
86 changes: 74 additions & 12 deletions orchagent/neighorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,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 @@ -507,12 +521,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 @@ -557,23 +579,36 @@ void NeighOrch::doTask(Consumer &consumer)
continue;
}

if(gPortsOrch->isInbandPort(alias))
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)
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())
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;
}
}
}

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


NeighborEntry neighbor_entry = { ip_address, alias };

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

m_remoteNeigh[ip_address] = alias;

it = consumer.m_toSync.erase(it);
}
else
Expand All @@ -1175,6 +1212,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 @@ -1370,6 +1409,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)
{
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;
}

//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 @@ -1418,7 +1472,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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ class NeighOrch : public Orch, public Subject, public Observer
void processFDBFlushUpdate(const FdbFlushUpdate &);

void doTask(Consumer &consumer);

void doVoqSystemNeighTask(Consumer &consumer);

unique_ptr<Table> m_tableVoqSystemNeighTable;
Expand All @@ -110,6 +111,8 @@ class NeighOrch : public Orch, public Subject, public Observer

bool resolveNeighborEntry(const NeighborEntry &, const MacAddress &);
void clearResolvedNeighborEntry(const NeighborEntry &);

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

#endif /* SWSS_NEIGHORCH_H */
Loading