Skip to content

Commit

Permalink
Revert "[nhg]: Add support for weight in nexthop group member. (sonic…
Browse files Browse the repository at this point in the history 
…-net#1752)" (sonic-net#1798)

This reverts commit a44e651.

Proper weight initialization is needed when weights are not provided:
https://github.com/Azure/sonic-swss/blob/a44e6513556cfed7de1b70690db90cc09fcef666/orchagent/routeorch.cpp#L638

Warmboot is failing on various platforms: sonic-net#7919

The original PR needs some negative unit testcases when the weight is not present for the nexthop group.
  • Loading branch information
@vaibhavhd
vaibhavhd authored Jun 23, 2021
1 parent e03c677 commit a3bc89b
Show file tree
Hide file tree
Showing 6 changed files with 5 additions and 96 deletions.
39 changes: 0 additions & 39 deletions fpmsyncd/routesync.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,6 @@ void RouteSync::onRouteMsg(int nlmsg_type, struct nl_object *obj, char *vrf)
/* Get nexthop lists */
string nexthops = getNextHopGw(route_obj);
string ifnames = getNextHopIf(route_obj);
string weights = getNextHopWt(route_obj);

vector<string> alsv = tokenize(ifnames, ',');
for (auto alias : alsv)
Expand All @@ -723,11 +722,6 @@ void RouteSync::onRouteMsg(int nlmsg_type, struct nl_object *obj, char *vrf)

fvVector.push_back(nh);
fvVector.push_back(idx);
if (!weights.empty())
{
FieldValueTuple wt("weight", weights);
fvVector.push_back(wt);
}

if (!warmRestartInProgress)
{
Expand Down Expand Up @@ -968,36 +962,3 @@ string RouteSync::getNextHopIf(struct rtnl_route *route_obj)

return result;
}

/*
* Get next hop weights
* @arg route_obj route object
*
* Return concatenation of interface names: wt0 + "," + wt1 + .... + "," + wtN
*/
string RouteSync::getNextHopWt(struct rtnl_route *route_obj)
{
string result = "";

for (int i = 0; i < rtnl_route_get_nnexthops(route_obj); i++)
{
struct rtnl_nexthop *nexthop = rtnl_route_nexthop_n(route_obj, i);
/* Get the weight of next hop */
uint8_t weight = rtnl_route_nh_get_weight(nexthop);
if (weight)
{
result += to_string(weight + 1);
}
else
{
return "";
}

if (i + 1 < rtnl_route_get_nnexthops(route_obj))
{
result += string(",");
}
}

return result;
}
3 changes: 0 additions & 3 deletions fpmsyncd/routesync.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,6 @@ class RouteSync : public NetMsg

/* Get next hop interfaces */
string getNextHopIf(struct rtnl_route *route_obj);

/* Get next hop weights*/
string getNextHopWt(struct rtnl_route *route_obj);
};

}
Expand Down
16 changes: 0 additions & 16 deletions orchagent/nexthopgroupkey.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,22 +31,6 @@ class NextHopGroupKey
}
}

NextHopGroupKey(const std::string &nexthops, const std::string &weights)
{
m_overlay_nexthops = false;
std::vector<std::string> nhv = tokenize(nexthops, NHG_DELIMITER);
std::vector<std::string> wtv = tokenize(weights, NHG_DELIMITER);
for (uint32_t i = 0; i < nhv.size(); i++)
{
NextHopKey nh(nhv[i]);
if (i < wtv.size())
{
nh.weight = (uint32_t)std::stoi(wtv[i]);
}
m_nexthops.insert(nh);
}
}

inline const std::set<NextHopKey> &getNextHops() const
{
return m_nexthops;
Expand Down
8 changes: 3 additions & 5 deletions orchagent/nexthopkey.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,10 @@ struct NextHopKey
string alias; // incoming interface alias
uint32_t vni; // Encap VNI overlay nexthop
MacAddress mac_address; // Overlay Nexthop MAC.
uint32_t weight; // NH weight for NHGs


NextHopKey() : weight(0) {}
NextHopKey(const std::string &ipstr, const std::string &alias) : ip_address(ipstr), alias(alias), vni(0), mac_address(), weight(0) {}
NextHopKey(const IpAddress &ip, const std::string &alias) : ip_address(ip), alias(alias), vni(0), mac_address(), weight(0) {}
NextHopKey() = default;
NextHopKey(const std::string &ipstr, const std::string &alias) : ip_address(ipstr), alias(alias), vni(0), mac_address() {}
NextHopKey(const IpAddress &ip, const std::string &alias) : ip_address(ip), alias(alias), vni(0), mac_address() {}
NextHopKey(const std::string &str)
{
if (str.find(NHG_DELIMITER) != string::npos)
Expand Down
21 changes: 1 addition & 20 deletions orchagent/routeorch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -325,13 +325,6 @@ bool RouteOrch::validnexthopinNextHopGroup(const NextHopKey &nexthop, uint32_t&
nhgm_attr.value.oid = m_neighOrch->getNextHopId(nexthop);
nhgm_attrs.push_back(nhgm_attr);

if (nexthop.weight)
{
nhgm_attr.id = SAI_NEXT_HOP_GROUP_MEMBER_ATTR_WEIGHT;
nhgm_attr.value.s32 = nexthop.weight;
nhgm_attrs.push_back(nhgm_attr);
}

status = sai_next_hop_group_api->create_next_hop_group_member(&nexthop_id, gSwitchId,
(uint32_t)nhgm_attrs.size(),
nhgm_attrs.data());
Expand Down Expand Up @@ -520,7 +513,6 @@ void RouteOrch::doTask(Consumer& consumer)
string aliases;
string vni_labels;
string remote_macs;
string weights;
bool& excp_intfs_flag = ctx.excp_intfs_flag;
bool overlay_nh = false;
bool blackhole = false;
Expand All @@ -543,9 +535,6 @@ void RouteOrch::doTask(Consumer& consumer)

if (fvField(i) == "blackhole")
blackhole = fvValue(i) == "true";

if (fvField(i) == "weight")
weights = fvValue(i);
}

vector<string>& ipv = ctx.ipv;
Expand Down Expand Up @@ -635,7 +624,7 @@ void RouteOrch::doTask(Consumer& consumer)
nhg_str += NHG_DELIMITER + ipv[i] + NH_DELIMITER + alsv[i];
}

nhg = NextHopGroupKey(nhg_str, weights);
nhg = NextHopGroupKey(nhg_str);

}
else
Expand Down Expand Up @@ -1113,7 +1102,6 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops)
for (size_t i = 0; i < npid_count; i++)
{
auto nhid = next_hop_ids[i];
auto weight = nhopgroup_members_set[nhid].weight;

// Create a next hop group member
vector<sai_attribute_t> nhgm_attrs;
Expand All @@ -1127,13 +1115,6 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops)
nhgm_attr.value.oid = nhid;
nhgm_attrs.push_back(nhgm_attr);

if (weight)
{
nhgm_attr.id = SAI_NEXT_HOP_GROUP_MEMBER_ATTR_WEIGHT;
nhgm_attr.value.s32 = weight;
nhgm_attrs.push_back(nhgm_attr);
}

gNextHopGroupMemberBulker.create_entry(&nhgm_ids[i],
(uint32_t)nhgm_attrs.size(),
nhgm_attrs.data());
Expand Down
14 changes: 1 addition & 13 deletions tests/test_nhg.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,7 @@ def test_route_nhg(self, dvs, dvs_route, testlog):

dvs_route.check_asicdb_deleted_route_entries([rtprefix])

fvs = swsscommon.FieldValuePairs([("nexthop","10.0.0.1,10.0.0.3,10.0.0.5"),
("ifname", "Ethernet0,Ethernet4,Ethernet8"),
("weight", "10,30,50")])
fvs = swsscommon.FieldValuePairs([("nexthop","10.0.0.1,10.0.0.3,10.0.0.5"), ("ifname", "Ethernet0,Ethernet4,Ethernet8")])
ps.set(rtprefix, fvs)

# check if route was propagated to ASIC DB
Expand All @@ -70,16 +68,6 @@ def test_route_nhg(self, dvs, dvs_route, testlog):

assert fvs["SAI_NEXT_HOP_GROUP_MEMBER_ATTR_NEXT_HOP_GROUP_ID"] == nhgid

# verify weight attributes in asic db
nhid = fvs["SAI_NEXT_HOP_GROUP_MEMBER_ATTR_NEXT_HOP_ID"]
weight = fvs["SAI_NEXT_HOP_GROUP_MEMBER_ATTR_WEIGHT"]

fvs = asic_db.get_entry("ASIC_STATE:SAI_OBJECT_TYPE_NEXT_HOP", nhid)
nhip = fvs["SAI_NEXT_HOP_ATTR_IP"].split('.')
expected_weight = int(nhip[3]) * 10

assert int(weight) == expected_weight

# bring links down one-by-one
for i in [0, 1, 2]:
dvs.servers[i].runcmd("ip link set down dev eth0") == 0
Expand Down

0 comments on commit a3bc89b

Please sign in to comment.