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

Added Change for given Route ECMP to fallback on Default Route ECMP #3389

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
162 changes: 157 additions & 5 deletions orchagent/routeorch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,70 @@ void RouteOrch::detach(Observer *observer, const IpAddress& dstAddr, sai_object_
}
}

void RouteOrch::updateDefaultRouteSwapSet(const NextHopGroupKey default_nhg_key, std::set<NextHopKey>& active_default_route_nhops)
{
std::set<NextHopKey> current_default_route_nhops;
current_default_route_nhops.clear();

if (default_nhg_key.getSize() == 1)
{
current_default_route_nhops.insert(*default_nhg_key.getNextHops().begin());
Copy link
Collaborator

Choose a reason for hiding this comment

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

fix indentation

}
else
{
auto nhgm = m_syncdNextHopGroups[default_nhg_key].nhopgroup_members;
for (auto nhop = nhgm.begin(); nhop != nhgm.end(); ++nhop)
{
current_default_route_nhops.insert(nhop->first);
}
}

active_default_route_nhops.clear();
std::copy(current_default_route_nhops.begin(), current_default_route_nhops.end(), std::inserter(active_default_route_nhops, active_default_route_nhops.begin()));
}

bool RouteOrch::addDefaultRouteNexthopsInNextHopGroup(NextHopGroupEntry& original_next_hop_group, std::set<NextHopKey>& default_route_next_hop_set)
{
SWSS_LOG_ENTER();
sai_object_id_t nexthop_group_member_id;
sai_status_t status;

for (auto it : default_route_next_hop_set)
{
vector<sai_attribute_t> nhgm_attrs;
sai_attribute_t nhgm_attr;
nhgm_attr.id = SAI_NEXT_HOP_GROUP_MEMBER_ATTR_NEXT_HOP_GROUP_ID;
nhgm_attr.value.oid = original_next_hop_group.next_hop_group_id;
nhgm_attrs.push_back(nhgm_attr);

nhgm_attr.id = SAI_NEXT_HOP_GROUP_MEMBER_ATTR_NEXT_HOP_ID;
nhgm_attr.value.oid = m_neighOrch->getNextHopId(it);
nhgm_attrs.push_back(nhgm_attr);

status = sai_next_hop_group_api->create_next_hop_group_member(&nexthop_group_member_id, gSwitchId,
(uint32_t)nhgm_attrs.size(),
nhgm_attrs.data());

if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_ERROR("Default Route Swap Failed to add next hop member to group %" PRIx64 ": %d\n",
original_next_hop_group.next_hop_group_id, status);
task_process_status handle_status = handleSaiCreateStatus(SAI_API_NEXT_HOP_GROUP, status);
if (handle_status != task_success)
{
return parseHandleSaiStatusFailure(handle_status);
}
}
// Increment the Default Route Active NH Reference Count
m_neighOrch->increaseNextHopRefCount(it);
gCrmOrch->incCrmResUsedCounter(CrmResourceType::CRM_NEXTHOP_GROUP_MEMBER);
original_next_hop_group.default_route_nhopgroup_members[it].next_hop_id = nexthop_group_member_id;
original_next_hop_group.default_route_nhopgroup_members[it].seq_id = 0;
original_next_hop_group.is_default_route_nh_swap = true;
}
return true;
}

bool RouteOrch::validnexthopinNextHopGroup(const NextHopKey &nexthop, uint32_t& count)
{
SWSS_LOG_ENTER();
Expand All @@ -398,6 +462,13 @@ bool RouteOrch::validnexthopinNextHopGroup(const NextHopKey &nexthop, uint32_t&
continue;
}

// Route NHOP Group is swapped by default route nh memeber . do not add Nexthop again.
// Wait for Nexthop Group Cleanup
if (nhopgroup->second.is_default_route_nh_swap)
{
continue;
}

vector<sai_attribute_t> nhgm_attrs;
sai_attribute_t nhgm_attr;

Expand Down Expand Up @@ -444,6 +515,7 @@ bool RouteOrch::validnexthopinNextHopGroup(const NextHopKey &nexthop, uint32_t&
++count;
gCrmOrch->incCrmResUsedCounter(CrmResourceType::CRM_NEXTHOP_GROUP_MEMBER);
nhopgroup->second.nhopgroup_members[nexthop].next_hop_id = nexthop_id;
nhopgroup->second.nh_member_install_count++;
}

if (!m_fgNhgOrch->validNextHopInNextHopGroup(nexthop))
Expand Down Expand Up @@ -484,7 +556,22 @@ bool RouteOrch::invalidnexthopinNextHopGroup(const NextHopKey &nexthop, uint32_t
return parseHandleSaiStatusFailure(handle_status);
}
}

if (nhopgroup->second.nh_member_install_count)
{
nhopgroup->second.nh_member_install_count--;
}

if (nhopgroup->second.nh_member_install_count == 0 && nhopgroup->second.eligible_for_default_route_nh_swap && !nhopgroup->second.is_default_route_nh_swap)
{
if(nexthop.ip_address.isV4())
Copy link
Contributor

Choose a reason for hiding this comment

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

if at this time the default route from bgp is not present. will the v4_active_default_route_nhops have the drop port?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arlakshm : if no default route than existing behavior will happen where nexthop group will not have any members which will cause drop as expected.

{
addDefaultRouteNexthopsInNextHopGroup(nhopgroup->second, v4_active_default_route_nhops);
}
else
{
addDefaultRouteNexthopsInNextHopGroup(nhopgroup->second, v6_active_default_route_nhops);
}
}
++count;
gCrmOrch->decCrmResUsedCounter(CrmResourceType::CRM_NEXTHOP_GROUP_MEMBER);
}
Expand Down Expand Up @@ -632,6 +719,7 @@ void RouteOrch::doTask(Consumer& consumer)
string srv6_segments;
string srv6_source;
bool srv6_nh = false;
bool fallback_to_default_route = false;

for (auto i : kfvFieldsValues(t))
{
Expand Down Expand Up @@ -673,6 +761,10 @@ void RouteOrch::doTask(Consumer& consumer)
{
ctx.protocol = fvValue(i);
}
if (fvField(i) == "fallback_to_default_route")
Copy link
Collaborator

Choose a reason for hiding this comment

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

who sets this attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is being done here: #3353

{
fallback_to_default_route = fvValue(i) == "true";
}
}

/*
Expand All @@ -687,6 +779,7 @@ void RouteOrch::doTask(Consumer& consumer)
}

ctx.nhg_index = nhg_index;
ctx.fallback_to_default_route = fallback_to_default_route;

/*
* If the nexthop_group is empty, create the next hop group key
Expand Down Expand Up @@ -975,6 +1068,8 @@ void RouteOrch::doTask(Consumer& consumer)
// Go through the bulker results
auto it_prev = consumer.m_toSync.begin();
m_bulkNhgReducedRefCnt.clear();
NextHopGroupKey v4_default_nhg_key;
NextHopGroupKey v6_default_nhg_key;
while (it_prev != it)
{
KeyOpFieldsValuesTuple t = it_prev->second;
Expand Down Expand Up @@ -1032,6 +1127,18 @@ void RouteOrch::doTask(Consumer& consumer)
it_prev = consumer.m_toSync.erase(it_prev);
else
it_prev++;

if (ip_prefix.isDefaultRoute() && vrf_id == gVirtualRouterId)
{
if (ip_prefix.isV4())
Comment on lines +1132 to +1133
Copy link
Contributor

@arlakshm arlakshm Nov 25, 2024

Choose a reason for hiding this comment

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

nit: indentation #Resolved

{
v4_default_nhg_key = getSyncdRouteNhgKey(gVirtualRouterId, ip_prefix);
}
else
{
v6_default_nhg_key = getSyncdRouteNhgKey(gVirtualRouterId, ip_prefix);
}
}
}
}
else if (op == DEL_COMMAND)
Expand Down Expand Up @@ -1067,9 +1174,23 @@ void RouteOrch::doTask(Consumer& consumer)
}
else if (m_syncdNextHopGroups[it_nhg.first].ref_count == 0)
{
removeNextHopGroup(it_nhg.first);
// Pass the flag to indicate if the NextHop Group as Default Route NH Members as swapped.
removeNextHopGroup(it_nhg.first, m_syncdNextHopGroups[it_nhg.first].is_default_route_nh_swap);
}
}

if (!(v4_default_nhg_key.getSize()) && !(v6_default_nhg_key.getSize()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add { for if checks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

{
return;
}
if (v4_default_nhg_key.getSize())
{
updateDefaultRouteSwapSet(v4_default_nhg_key, v4_active_default_route_nhops);
}
if (v6_default_nhg_key.getSize())
{
updateDefaultRouteSwapSet(v6_default_nhg_key, v6_active_default_route_nhops);
}
}
}

Expand Down Expand Up @@ -1364,6 +1485,7 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops)

NextHopGroupEntry next_hop_group_entry;
next_hop_group_entry.next_hop_group_id = next_hop_group_id;
next_hop_group_entry.nh_member_install_count = 0;

size_t npid_count = next_hop_ids.size();
vector<sai_object_id_t> nhgm_ids(npid_count);
Expand Down Expand Up @@ -1434,6 +1556,7 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops)
{
next_hop_group_entry.nhopgroup_members[nhopgroup_members_set.find(nhid)->second].next_hop_id = nhgm_id;
next_hop_group_entry.nhopgroup_members[nhopgroup_members_set.find(nhid)->second].seq_id = ((uint32_t)i) + 1;
next_hop_group_entry.nh_member_install_count++;
}
}

Expand All @@ -1451,7 +1574,7 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops)
return true;
}

bool RouteOrch::removeNextHopGroup(const NextHopGroupKey &nexthops)
bool RouteOrch::removeNextHopGroup(const NextHopGroupKey &nexthops, const bool is_default_route_nh_swap)
{
SWSS_LOG_ENTER();

Expand All @@ -1472,10 +1595,10 @@ bool RouteOrch::removeNextHopGroup(const NextHopGroupKey &nexthops)
SWSS_LOG_NOTICE("Delete next hop group %s", nexthops.to_string().c_str());

vector<sai_object_id_t> next_hop_ids;
auto& nhgm = next_hop_group_entry->second.nhopgroup_members;
auto& nhgm = is_default_route_nh_swap ? next_hop_group_entry->second.default_route_nhopgroup_members : next_hop_group_entry->second.nhopgroup_members;
for (auto nhop = nhgm.begin(); nhop != nhgm.end();)
{
if (m_neighOrch->isNextHopFlagSet(nhop->first, NHFLAGS_IFDOWN))
if (m_neighOrch->isNextHopFlagSet(nhop->first, NHFLAGS_IFDOWN) && (!is_default_route_nh_swap))
{
SWSS_LOG_WARN("NHFLAGS_IFDOWN set for next hop group member %s with next_hop_id %" PRIx64,
nhop->first.to_string().c_str(), nhop->second.next_hop_id);
Expand Down Expand Up @@ -1566,6 +1689,16 @@ bool RouteOrch::removeNextHopGroup(const NextHopGroupKey &nexthops)
}
}

// Decrement Nexthop Reference Count for Default Route NH Member used as swapped
if (is_default_route_nh_swap)
{
auto& nhgm = next_hop_group_entry->second.default_route_nhopgroup_members;
for (auto nhop = nhgm.begin(); nhop != nhgm.end(); ++nhop)
{
m_neighOrch->decreaseNextHopRefCount(nhop->first);
}
}

m_syncdNextHopGroups.erase(nexthops);

return true;
Expand Down Expand Up @@ -1718,6 +1851,11 @@ void RouteOrch::addTempRoute(RouteBulkContext& ctx, const NextHopGroupKey &nextH
(*it).to_string().c_str(), ipPrefix.to_string().c_str());
it = next_hop_set.erase(it);
}
else if(m_neighOrch->isNextHopFlagSet(*it, NHFLAGS_IFDOWN))
{
SWSS_LOG_INFO("Interface down for NH %s, skip this NH", (*it).to_string().c_str());
it = next_hop_set.erase(it);
}
else
it++;
}
Expand Down Expand Up @@ -1951,6 +2089,11 @@ bool RouteOrch::addRoute(RouteBulkContext& ctx, const NextHopGroupKey &nextHops)
/* Return false since the original route is not successfully added */
return false;
}
else
{
m_syncdNextHopGroups[nextHops].eligible_for_default_route_nh_swap = ctx.fallback_to_default_route;
m_syncdNextHopGroups[nextHops].is_default_route_nh_swap = false;
}
}

next_hop_id = m_syncdNextHopGroups[nextHops].next_hop_group_id;
Expand Down Expand Up @@ -2506,6 +2649,15 @@ bool RouteOrch::removeRoutePost(const RouteBulkContext& ctx)
updateDefRouteState(ipPrefix.to_string());

SWSS_LOG_INFO("Set route %s next hop ID to NULL", ipPrefix.to_string().c_str());

if (ipPrefix.isV4())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here for {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

{
v4_active_default_route_nhops.clear();
}
else
{
v6_active_default_route_nhops.clear();
}
}
else
{
Expand Down
16 changes: 14 additions & 2 deletions orchagent/routeorch.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ struct NextHopGroupEntry
sai_object_id_t next_hop_group_id; // next hop group id
int ref_count; // reference count
NextHopGroupMembers nhopgroup_members; // ids of members indexed by <ip_address, if_alias>
NextHopGroupMembers default_route_nhopgroup_members; // ids of members indexed by <ip_address, if_alias>
uint32_t nh_member_install_count;
bool eligible_for_default_route_nh_swap;
bool is_default_route_nh_swap;
};

struct NextHopUpdate
Expand Down Expand Up @@ -122,13 +126,15 @@ struct RouteBulkContext
bool excp_intfs_flag;
// using_temp_nhg will track if the NhgOrch's owned NHG is temporary or not
bool using_temp_nhg;
bool fallback_to_default_route;

std::string key; // Key in database table
std::string protocol; // Protocol string
bool is_set; // True if set operation

RouteBulkContext(const std::string& key, bool is_set)
: key(key), excp_intfs_flag(false), using_temp_nhg(false), is_set(is_set)
: key(key), excp_intfs_flag(false), using_temp_nhg(false), is_set(is_set),
fallback_to_default_route(false)
{
}

Expand All @@ -146,6 +152,7 @@ struct RouteBulkContext
using_temp_nhg = false;
key.clear();
protocol.clear();
fallback_to_default_route = false;
}
};

Expand Down Expand Up @@ -197,12 +204,13 @@ class RouteOrch : public Orch, public Subject
bool isRefCounterZero(const NextHopGroupKey&) const;

bool addNextHopGroup(const NextHopGroupKey&);
bool removeNextHopGroup(const NextHopGroupKey&);
bool removeNextHopGroup(const NextHopGroupKey&, const bool is_default_route_nh_swap=false);

void addNextHopRoute(const NextHopKey&, const RouteKey&);
void removeNextHopRoute(const NextHopKey&, const RouteKey&);
bool updateNextHopRoutes(const NextHopKey&, uint32_t&);
bool getRoutesForNexthop(std::set<RouteKey>&, const NextHopKey&);
bool swapnexthopinNextHopGroup(sai_object_id_t next_hop_group_id, sai_object_id_t default_next_hop_id);

bool validnexthopinNextHopGroup(const NextHopKey&, uint32_t&);
bool invalidnexthopinNextHopGroup(const NextHopKey&, uint32_t&);
Expand Down Expand Up @@ -242,6 +250,8 @@ class RouteOrch : public Orch, public Subject
unsigned int m_maxNextHopGroupCount;
bool m_resync;

std::set<NextHopKey> v4_active_default_route_nhops;
std::set<NextHopKey> v6_active_default_route_nhops;
shared_ptr<DBConnector> m_stateDb;
unique_ptr<swss::Table> m_stateDefaultRouteTb;

Expand Down Expand Up @@ -288,6 +298,8 @@ class RouteOrch : public Orch, public Subject
bool isVipRoute(const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops);
void createVipRouteSubnetDecapTerm(const IpPrefix &ipPrefix);
void removeVipRouteSubnetDecapTerm(const IpPrefix &ipPrefix);
bool addDefaultRouteNexthopsInNextHopGroup(NextHopGroupEntry& original_next_hop_group, std::set<NextHopKey>& default_route_next_hop_set);
void updateDefaultRouteSwapSet(const NextHopGroupKey default_nhg_key, std::set<NextHopKey>& active_default_route_nhops);
};

#endif /* SWSS_ROUTEORCH_H */
8 changes: 7 additions & 1 deletion tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -742,13 +742,19 @@ def start_fpmsyncd(self):
self.runcmd(['sh', '-c', 'supervisorctl start fpmsyncd'])

# Let's give fpmsyncd a chance to connect to Zebra.
time.sleep(5)
time.sleep(10)

# deps: warm_reboot
def stop_fpmsyncd(self):
self.runcmd(['sh', '-c', 'pkill -x fpmsyncd'])
time.sleep(1)

def disable_fpmsyncd(self):
self.runcmd(['sh', '-c', 'supervisorctl stop fpmsyncd'])

# Let's give fpmsyncd a chance to connect to Zebra.
time.sleep(5)

# deps: warm_reboot
def SubscribeAppDbObject(self, objpfx):
r = redis.Redis(unix_socket_path=self.redis_sock, db=swsscommon.APPL_DB,
Expand Down
Loading
Loading