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

[VRF]: submit vrf feature #943

Merged
merged 46 commits into from
Nov 6, 2019
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
f448a22
FdbOrch getPort failed because fdb_entry have no switch_id
Apr 10, 2018
7f81135
[crmorch]: neighbor used counter increased twice
Apr 16, 2018
91f1f00
Merge pull request #1 from Azure/master
tylerlinp May 22, 2018
5c2af28
[Portsorch]: Add lag-name and oid map to counter table COUNTERS_PORT_…
leoli-nps May 22, 2018
53f2663
[Portsorch]:Add operation to remove lag-name and oid map from counter…
leoli-nps May 24, 2018
fa95f1e
Merge pull request #2 from Azure/master
tylerlinp Aug 21, 2018
03c0472
[Pfc-wd]: Add pfc-wd support for nephos
leoli-nps Aug 21, 2018
e456159
Merge branch 'master' into master
leoli-nps Jan 10, 2019
88e3f4f
Update Makefile.am
leoli-nps Jan 10, 2019
91fdc69
[Pfc-wd]: Remove pfc-wd code for nephos from this branch
leoli-nps Mar 18, 2019
7bfc853
Merge pull request #3 from Azure/master
tylerlinp May 29, 2019
e772a12
Support VRF
Jun 3, 2019
213bca9
revert portsorch to asure
Jun 3, 2019
771d5f1
Loopback support VRF
Jun 6, 2019
90e8000
Merge pull request #4 from Azure/master
tylerlinp Jun 12, 2019
e47cbc4
VS test for VRF
Jun 12, 2019
2c64599
VRF: filter routes pointed to vrf interfaces
Jun 20, 2019
4874983
Merge branch 'master' of https://github.com/Azure/sonic-swss
Jun 21, 2019
6a18696
Add some new VS test for VRF
Jun 21, 2019
40755bd
Filter neighbor netlink state NUD_NOARP for vrf/lo interface
Jun 26, 2019
280c6d3
Interface set new attributes except vrf
Jul 11, 2019
7617947
Merge https://github.com/Azure/sonic-swss
Jul 18, 2019
306a40e
Do overlap checking on all intfs in same vrf
Jul 24, 2019
340b125
intfmgrd throw to log when execute command failed
Jul 24, 2019
c79df41
Merge https://github.com/Azure/sonic-swss
Jul 24, 2019
d2b835e
Revise codes based on review and test
Aug 22, 2019
8396833
Fix vrf delete problem by delaying delLink
Sep 5, 2019
c584517
Merge https://github.com/Azure/sonic-swss
Sep 5, 2019
78c4001
[test]: Fix set interface in configuration database
Sep 6, 2019
778f324
Fix vs test cases failed
Sep 10, 2019
8370676
Merge https://github.com/Azure/sonic-swss
Sep 11, 2019
3e762ac
Fix vrf delete problem by delaying delLink
Sep 16, 2019
1657824
Merge https://github.com/Azure/sonic-swss
Sep 17, 2019
bb61e52
create separate file for NextHopKey and NextHopGroupKey
Sep 25, 2019
ebd624d
change nexthop delimiter to '@'
Sep 25, 2019
bd187bf
Merge https://github.com/Azure/sonic-swss
Sep 25, 2019
a53c365
Revise acl redirect test case
Sep 26, 2019
7f8e447
Merge https://github.com/Azure/sonic-swss
Oct 8, 2019
0652e1d
Merge branch 'master' into master
Oct 10, 2019
bbca607
Address review comments
Oct 11, 2019
d877f23
Merge https://github.com/Azure/sonic-swss
Oct 12, 2019
1618686
Merge https://github.com/Azure/sonic-swss
Oct 28, 2019
854a553
Merge https://github.com/Azure/sonic-swss
Oct 29, 2019
0f55a9b
Merge https://github.com/Azure/sonic-swss
Nov 4, 2019
7987477
fix compile err after merge
Nov 4, 2019
2ee979f
Merge https://github.com/Azure/sonic-swss
Nov 6, 2019
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
152 changes: 121 additions & 31 deletions cfgmgr/intfmgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ using namespace swss;
#define LAG_PREFIX "PortChannel"
#define LOOPBACK_PREFIX "Loopback"
#define VNET_PREFIX "Vnet"
#define VRF_PREFIX "Vrf"

IntfMgr::IntfMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, const vector<string> &tableNames) :
Orch(cfgDb, tableNames),
Expand Down Expand Up @@ -50,7 +51,7 @@ void IntfMgr::setIntfIp(const string &alias, const string &opCmd,
}
}

void IntfMgr::setIntfVrf(const string &alias, const string vrfName)
void IntfMgr::setIntfVrf(const string &alias, const string &vrfName)
{
stringstream cmd;
string res;
Expand All @@ -63,7 +64,90 @@ void IntfMgr::setIntfVrf(const string &alias, const string vrfName)
{
cmd << IP_CMD << " link set " << alias << " nomaster";
}
EXEC_WITH_ERROR_THROW(cmd.str(), res);
int ret = swss::exec(cmd.str(), res);
if (ret)
{
SWSS_LOG_ERROR("Command '%s' failed with rc %d", cmd.str().c_str(), ret);
}
}

void IntfMgr::addLoopbackIntf(const string &alias)
{
stringstream cmd;
string res;

cmd << IP_CMD << " link add " << alias << " mtu 65536 type dummy && ";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please define 65535 and use variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, maybe always keep the max value.

cmd << IP_CMD << " link set " << alias << " up";
int ret = swss::exec(cmd.str(), res);
if (ret)
{
SWSS_LOG_ERROR("Command '%s' failed with rc %d", cmd.str().c_str(), ret);
}
}

void IntfMgr::delLoopbackIntf(const string &alias)
{
stringstream cmd;
string res;

cmd << IP_CMD << " link del " << alias;
int ret = swss::exec(cmd.str(), res);
if (ret)
{
SWSS_LOG_ERROR("Command '%s' failed with rc %d", cmd.str().c_str(), ret);
}
}

int IntfMgr::getIntfIpCount(const string &alias)
{
stringstream cmd;
string res;

cmd << IP_CMD << " address show " << alias << " | grep inet | grep -v 'inet6 fe80:' | wc -l";
int ret = swss::exec(cmd.str(), res);
if (ret)
{
SWSS_LOG_ERROR("Command '%s' failed with rc %d", cmd.str().c_str(), ret);
return 0;
}

return std::stoi(res);
}

bool IntfMgr::isIntfGeneralDone(const string &alias)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Name doesn't align with functionality. How about isIntfCreated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't get a good name all along. Yours is better.

{
vector<FieldValueTuple> temp;

if (m_stateIntfTable.get(alias, temp))
{
SWSS_LOG_DEBUG("Intf %s is ready", alias.c_str());
return true;
}

return false;
}

bool IntfMgr::isIntfChangeVrf(const string &alias, const string &vrfName)
prsunny marked this conversation as resolved.
Show resolved Hide resolved
{
vector<FieldValueTuple> temp;

if (m_stateIntfTable.get(alias, temp))
{
for (auto idx : temp)
{
const auto &field = fvField(idx);
const auto &value = fvValue(idx);
if (field == "vrf")
{
if (value == vrfName)
return false;
else
return true;
}
}
}

return false;
}

bool IntfMgr::isIntfStateOk(const string &alias)
Expand Down Expand Up @@ -94,6 +178,14 @@ bool IntfMgr::isIntfStateOk(const string &alias)
return true;
}
}
else if (!alias.compare(0, strlen(VRF_PREFIX), VRF_PREFIX))
{
if (m_stateVrfTable.get(alias, temp))
{
SWSS_LOG_DEBUG("Vrf %s is ready", alias.c_str());
return true;
}
}
else if (m_statePortTable.get(alias, temp))
{
SWSS_LOG_DEBUG("Port %s is ready", alias.c_str());
Expand Down Expand Up @@ -141,32 +233,38 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,
return false;
}

// Set Interface VRF except for lo
if (!is_lo)
/* if to change vrf then skip */
if (isIntfChangeVrf(alias, vrf_name))
{
if (!vrf_name.empty())
{
setIntfVrf(alias, vrf_name);
}
m_appIntfTableProducer.set(alias, data);
SWSS_LOG_ERROR("%s can not change to %s directly, skipping", alias.c_str(), vrf_name.c_str());
return true;
}
else
if (is_lo)
{
addLoopbackIntf(alias);
}
if (!vrf_name.empty())
{
m_appIntfTableProducer.set("lo", data);
setIntfVrf(alias, vrf_name);
}
m_appIntfTableProducer.set(alias, data);
m_stateIntfTable.hset(alias, "vrf", vrf_name);
}
else if (op == DEL_COMMAND)
{
// Set Interface VRF except for lo
if (!is_lo)
/* make sure all ip addresses associated with interface are removed, otherwise these ip address would
be set with global vrf and it may cause ip address confliction. */
if (getIntfIpCount(alias))
{
setIntfVrf(alias, "");
m_appIntfTableProducer.del(alias);
return false;
}
else
setIntfVrf(alias, "");
if (is_lo)
{
m_appIntfTableProducer.del("lo");
delLoopbackIntf(alias);
}
m_appIntfTableProducer.del(alias);
m_stateIntfTable.del(alias);
}
else
{
Expand All @@ -184,26 +282,21 @@ bool IntfMgr::doIntfAddrTask(const vector<string>& keys,

string alias(keys[0]);
IpPrefix ip_prefix(keys[1]);
bool is_lo = !alias.compare(0, strlen(LOOPBACK_PREFIX), LOOPBACK_PREFIX);
string appKey = (is_lo ? "lo" : keys[0]) + ":" + keys[1];
string appKey = keys[0] + ":" + keys[1];

if (op == SET_COMMAND)
{
/*
* Don't proceed if port/LAG/VLAN is not ready yet.
* Don't proceed if port/LAG/VLAN and intfGeneral are not ready yet.
* The pending task will be checked periodically and retried.
*/
if (!isIntfStateOk(alias))
if (!isIntfStateOk(alias) || !isIntfGeneralDone(alias))
{
SWSS_LOG_DEBUG("Interface is not ready, skipping %s", alias.c_str());
return false;
}

// Set Interface IP except for lo
if (!is_lo)
{
setIntfIp(alias, "add", ip_prefix.to_string(), ip_prefix.isV4());
}
setIntfIp(alias, "add", ip_prefix.to_string(), ip_prefix.isV4());

std::vector<FieldValueTuple> fvVector;
FieldValueTuple f("family", ip_prefix.isV4() ? IPV4_NAME : IPV6_NAME);
Expand All @@ -216,11 +309,8 @@ bool IntfMgr::doIntfAddrTask(const vector<string>& keys,
}
else if (op == DEL_COMMAND)
{
// Set Interface IP except for lo
if (!is_lo)
{
setIntfIp(alias, "del", ip_prefix.to_string(), ip_prefix.isV4());
}
setIntfIp(alias, "del", ip_prefix.to_string(), ip_prefix.isV4());

m_appIntfTableProducer.del(appKey);
m_stateIntfTable.del(keys[0] + state_db_key_delimiter + keys[1]);
}
Expand Down
7 changes: 6 additions & 1 deletion cfgmgr/intfmgr.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,16 @@ class IntfMgr : public Orch
Table m_statePortTable, m_stateLagTable, m_stateVlanTable, m_stateVrfTable, m_stateIntfTable;

void setIntfIp(const string &alias, const string &opCmd, const string &ipPrefixStr, const bool ipv4 = true);
void setIntfVrf(const string &alias, const string vrfName);
void setIntfVrf(const string &alias, const string &vrfName);
bool doIntfGeneralTask(const vector<string>& keys, const vector<FieldValueTuple>& data, const string& op);
bool doIntfAddrTask(const vector<string>& keys, const vector<FieldValueTuple>& data, const string& op);
void doTask(Consumer &consumer);
bool isIntfStateOk(const string &alias);
bool isIntfGeneralDone(const string &alias);
bool isIntfChangeVrf(const string &alias, const string &vrfName);
int getIntfIpCount(const string &alias);
void addLoopbackIntf(const string &alias);
void delLoopbackIntf(const string &alias);
};

}
Expand Down
23 changes: 2 additions & 21 deletions cfgmgr/nbrmgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,6 @@

using namespace swss;

#define VLAN_PREFIX "Vlan"
#define LAG_PREFIX "PortChannel"

static bool send_message(struct nl_sock *sk, struct nl_msg *msg)
{
bool rc = false;
Expand Down Expand Up @@ -66,25 +63,9 @@ bool NbrMgr::isIntfStateOk(const string &alias)
{
vector<FieldValueTuple> temp;

if (!alias.compare(0, strlen(VLAN_PREFIX), VLAN_PREFIX))
{
if (m_stateVlanTable.get(alias, temp))
{
SWSS_LOG_DEBUG("Vlan %s is ready", alias.c_str());
return true;
}
}
else if (!alias.compare(0, strlen(LAG_PREFIX), LAG_PREFIX))
{
if (m_stateLagTable.get(alias, temp))
{
SWSS_LOG_DEBUG("Lag %s is ready", alias.c_str());
return true;
}
}
else if (m_statePortTable.get(alias, temp))
if (m_stateIntfTable.get(alias, temp))
{
SWSS_LOG_DEBUG("Port %s is ready", alias.c_str());
SWSS_LOG_DEBUG("Intf %s is ready", alias.c_str());
return true;
}

Expand Down
17 changes: 17 additions & 0 deletions cfgmgr/vrfmgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,17 @@ bool VrfMgr::setLink(const string& vrfName)
return true;
}

int VrfMgr::getVrfIntfCount(const string& vrfName)
{
stringstream cmd;
string res;

cmd << IP_CMD << " link show " << " | grep 'master " << vrfName << "' | wc -l";
EXEC_WITH_ERROR_THROW(cmd.str(), res);

return std::stoi(res);
}

void VrfMgr::doTask(Consumer &consumer)
{
SWSS_LOG_ENTER();
Expand Down Expand Up @@ -173,6 +184,12 @@ void VrfMgr::doTask(Consumer &consumer)
}
else if (op == DEL_COMMAND)
{
if (getVrfIntfCount(vrfName))
{
it++;
continue;
}

if (!delLink(vrfName))
{
SWSS_LOG_ERROR("Failed to remove vrf netdev %s", vrfName.c_str());
Expand Down
1 change: 1 addition & 0 deletions cfgmgr/vrfmgr.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class VrfMgr : public Orch
private:
bool delLink(const string& vrfName);
bool setLink(const string& vrfName);
int getVrfIntfCount(const string& vrfName);
void recycleTable(uint32_t table);
uint32_t getFreeTable(void);
void handleVnetConfigSet(KeyOpFieldsValuesTuple &t);
Expand Down
51 changes: 49 additions & 2 deletions fpmsyncd/routesync.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ using namespace swss;

#define VXLAN_IF_NAME_PREFIX "Brvxlan"
#define VNET_PREFIX "Vnet"
#define VRF_PREFIX "Vrf"

RouteSync::RouteSync(RedisPipeline *pipeline) :
m_routeTable(pipeline, APP_ROUTE_TABLE_NAME, true),
Expand Down Expand Up @@ -70,10 +71,40 @@ void RouteSync::onRouteMsg(int nlmsg_type, struct nl_object *obj)
{
struct rtnl_route *route_obj = (struct rtnl_route *)obj;
struct nl_addr *dip;
char destipprefix[MAX_ADDR_SIZE + 1] = {0};
char destipprefix[IFNAMSIZ + MAX_ADDR_SIZE + 2] = {0};

/*
* Here vrf_index is not real table id but vrf_id in zebra.
* It is vrf interface index in kernel for vrf route and VRF_DEFAULT(0) for global route.
* Now zebra fpm only fill in a uchar, ifindex limited to 255.
*/
unsigned int vrf_index = rtnl_route_get_table(route_obj);
if (vrf_index)
{
if (!getIfName(vrf_index, destipprefix, IFNAMSIZ))
{
SWSS_LOG_ERROR("Fail to get the VRF name (ifindex %u)", vrf_index);
return;
}
/*
* Now vrf device name is required to start with VRF_PREFIX,
* it is difficult to split vrf_name:ipv6_addr.
*/
if (memcmp(destipprefix, VRF_PREFIX, strlen(VRF_PREFIX)))
{
SWSS_LOG_ERROR("Invalid VRF name %s (ifindex %u)", destipprefix, vrf_index);
return;
}
destipprefix[strlen(destipprefix)] = ':';
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This check is already done in RouteSync::onMsg. Can you please check if that can be used instead of repeating the same. We also have found some performance issues with getIfName

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will optimize it later.


dip = rtnl_route_get_dst(route_obj);
nl_addr2str(dip, destipprefix, MAX_ADDR_SIZE);
nl_addr2str(dip, destipprefix + strlen(destipprefix), MAX_ADDR_SIZE);
/* Full mask route append prefix length, or else resync cannot match. */
if (nl_addr_get_prefixlen(dip) == (8 * nl_addr_get_len(dip)))
{
snprintf(destipprefix + strlen(destipprefix), sizeof(destipprefix) - strlen(destipprefix), "/%u", nl_addr_get_prefixlen(dip));
}
SWSS_LOG_DEBUG("Receive new route message dest ip prefix: %s", destipprefix);

/*
Expand Down Expand Up @@ -186,6 +217,11 @@ void RouteSync::onVnetRouteMsg(int nlmsg_type, struct nl_object *obj, string vne
struct nl_addr *dip = rtnl_route_get_dst(route_obj);
char destipprefix[MAX_ADDR_SIZE + 1] = {0};
nl_addr2str(dip, destipprefix, MAX_ADDR_SIZE);
/* Full mask route append prefix length, or else resync cannot match. */
if (nl_addr_get_prefixlen(dip) == (8 * nl_addr_get_len(dip)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain this change? VNET routes makes some assumptions. Just want to ensure, there is no changes to VNET route handling. Please provide an example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nl_addr2str omit mask when mask is full of bits 1. So here route prefix key is only ip address left for full mask routes. In the process of resync, the dirty routes are all with mask(IpPrefix::to_string()), while the newly received route may have no mask, then they cannot match even though they are same prefix(1.1.1.1/32 and 1.1.1.1 are two entries in m_toSync), so the route would be DEL and SET 2 OPs, even worse no sequence. It doesn't matter for route handling, for IpPrefix constructor can deal with both. For consistency it is better to fill mask back.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove this based on WG discussion

{
snprintf(destipprefix + strlen(destipprefix), sizeof(destipprefix) - strlen(destipprefix), "/%u", nl_addr_get_prefixlen(dip));
}

string vnet_dip = vnet + string(":") + destipprefix;
SWSS_LOG_DEBUG("Receive new vnet route message %s", vnet_dip.c_str());
Expand Down Expand Up @@ -325,6 +361,17 @@ string RouteSync::getNextHopGw(struct rtnl_route *route_obj)
nl_addr2str(addr, gw_ip, MAX_ADDR_SIZE);
result += gw_ip;
}
else
{
if (rtnl_route_get_family(route_obj) == AF_INET)
{
result += "0.0.0.0";
}
else
{
result += "::";
}
}

if (i + 1 < rtnl_route_get_nnexthops(route_obj))
{
Expand Down
Loading