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

Change nexthop key to ip & ifname #977

Merged
merged 10 commits into from
Oct 2, 2019
Merged

Change nexthop key to ip & ifname #977

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6735110
Change nexthop key to ip & ifname
Jul 17, 2019
e7ac33f
add back pr953 overwrote
Jul 18, 2019
aa98cc1
Merge branch 'master' of https://github.com/Azure/sonic-swss into nex…
Jul 24, 2019
6042eaa
Revise for acl redirect backward compatibility and review comments
Aug 21, 2019
6b495cd
is_subnet change for nexthop ip fill zero if empty
Sep 17, 2019
c542a6a
Merge branch 'master' of https://github.com/Azure/sonic-swss into nex…
Sep 17, 2019
37a7c49
add changelog for update timestamp
Sep 19, 2019
06ab194
Merge branch 'master' of https://github.com/Azure/sonic-swss into nex…
Sep 20, 2019
698d10a
Revise for review comments
Sep 20, 2019
67aca81
Merge branch 'master' of https://github.com/Azure/sonic-swss into nex…
Sep 23, 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
4 changes: 4 additions & 0 deletions debian/changelog
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
sonic (1.0.0) stable; urgency=medium

* next-hop key add ifname
Copy link
Contributor

Choose a reason for hiding this comment

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

please keep the changelog as it is since right now we're not updating this file routinely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New version dpkg-deb use the newest time in chaneglog to set deb files timestamp. There is a problem that new files with same size and timestamp cannot be saved to image in process of PR build, so in this PR fpmsynd cannot be updated and many test cases failed. I add changelog just for building swss.deb with newer timestamp and pass tests.
THE PROBLEM MUST BE RESOLVED, or else the VS test become meaningless, for the running version may not be the codes compiled.

Copy link
Contributor

Choose a reason for hiding this comment

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

are you referring to the VS test running under this pull request? and the fpmsyncd is not replaced with the latest binary? if that is the case as you described, I'll check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes，I have forward the email which describes this issue detailly to you. It is common issue, not just on this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

could you check right now if the binaries could overwrite the old ones?
I have added Azure/sonic-build-tools@e9e3957 to fix this issue.


-- Tyler Li <tyler.li@mediatek.com> Thu, 19 Sep 2019 04:00:00 -0700

* Initial release.

-- Shuotian Cheng <shuche@microsoft.com> Wed, 09 Mar 2016 12:00:00 -0800
Expand Down
6 changes: 4 additions & 2 deletions doc/swss-schema.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,8 +456,10 @@ Stores rules associated with a specific ACL table on the switch.
; it could be:
: name of physical port. Example: "Ethernet10"
: name of LAG port Example: "PortChannel5"
: next-hop ip address Example: "10.0.0.1"
: next-hop group set of addresses Example: "10.0.0.1,10.0.0.3"
: next-hop ip address (in global) Example: "10.0.0.1"
: next-hop ip address and vrf Example: "10.0.0.2@Vrf2"
: next-hop ip address and ifname Example: "10.0.0.3@Ethernet1"
: next-hop group set of next-hop Example: "10.0.0.1,10.0.0.3@Ethernet1"

redirect_action = 1*255CHAR ; redirect parameter
; This parameter defines a destination for redirected packets
Expand Down
11 changes: 11 additions & 0 deletions fpmsyncd/routesync.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this change? The RouteOrch can delete the entry if nexthop.size == 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As above.

{
result += "0.0.0.0";
}
else
{
result += "::";
}
}

if (i + 1 < rtnl_route_get_nnexthops(route_obj))
{
Expand Down
34 changes: 17 additions & 17 deletions orchagent/aclorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,12 +546,12 @@ void AclRule::decreaseNextHopRefCount()
{
if (!m_redirect_target_next_hop.empty())
{
m_pAclOrch->m_neighOrch->decreaseNextHopRefCount(IpAddress(m_redirect_target_next_hop));
m_pAclOrch->m_neighOrch->decreaseNextHopRefCount(NextHopKey(m_redirect_target_next_hop));
m_redirect_target_next_hop.clear();
}
if (!m_redirect_target_next_hop_group.empty())
{
IpAddresses target = IpAddresses(m_redirect_target_next_hop_group);
NextHopGroupKey target = NextHopGroupKey(m_redirect_target_next_hop_group);
m_pAclOrch->m_routeOrch->decreaseNextHopRefCount(target);
// remove next hop group in case it's not used by anything else
if (m_pAclOrch->m_routeOrch->isRefCounterZero(target))
Expand Down Expand Up @@ -880,44 +880,44 @@ sai_object_id_t AclRuleL3::getRedirectObjectId(const string& redirect_value)
}
}

// Try to parse nexthop ip address
// Try to parse nexthop ip address and interface name
try
{
IpAddress ip(target);
if (!m_pAclOrch->m_neighOrch->hasNextHop(ip))
NextHopKey nh(target);
if (!m_pAclOrch->m_neighOrch->hasNextHop(nh))
{
SWSS_LOG_ERROR("ACL Redirect action target next hop ip: '%s' doesn't exist on the switch", ip.to_string().c_str());
SWSS_LOG_ERROR("ACL Redirect action target next hop ip: '%s' doesn't exist on the switch", nh.to_string().c_str());
return SAI_NULL_OBJECT_ID;
}

m_redirect_target_next_hop = target;
m_pAclOrch->m_neighOrch->increaseNextHopRefCount(ip);
return m_pAclOrch->m_neighOrch->getNextHopId(ip);
m_pAclOrch->m_neighOrch->increaseNextHopRefCount(nh);
return m_pAclOrch->m_neighOrch->getNextHopId(nh);
}
catch (...)
{
// no error, just try next variant
}

// try to parse nh group ip addresses
// try to parse nh group the set of <ip address, interface name>
try
{
IpAddresses ips(target);
if (!m_pAclOrch->m_routeOrch->hasNextHopGroup(ips))
NextHopGroupKey nhg(target);
if (!m_pAclOrch->m_routeOrch->hasNextHopGroup(nhg))
{
SWSS_LOG_INFO("ACL Redirect action target next hop group: '%s' doesn't exist on the switch. Creating it.", ips.to_string().c_str());
SWSS_LOG_INFO("ACL Redirect action target next hop group: '%s' doesn't exist on the switch. Creating it.", nhg.to_string().c_str());

if (!m_pAclOrch->m_routeOrch->addNextHopGroup(ips))
if (!m_pAclOrch->m_routeOrch->addNextHopGroup(nhg))
{
SWSS_LOG_ERROR("Can't create required target next hop group '%s'", ips.to_string().c_str());
SWSS_LOG_ERROR("Can't create required target next hop group '%s'", nhg.to_string().c_str());
return SAI_NULL_OBJECT_ID;
}
SWSS_LOG_DEBUG("Created acl redirect target next hop group '%s'", ips.to_string().c_str());
SWSS_LOG_DEBUG("Created acl redirect target next hop group '%s'", nhg.to_string().c_str());
}

m_redirect_target_next_hop_group = target;
m_pAclOrch->m_routeOrch->increaseNextHopRefCount(ips);
return m_pAclOrch->m_routeOrch->getNextHopGroupId(ips);
m_pAclOrch->m_routeOrch->increaseNextHopRefCount(nhg);
return m_pAclOrch->m_routeOrch->getNextHopGroupId(nhg);
}
catch (...)
{
Expand Down
33 changes: 30 additions & 3 deletions orchagent/intfsorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,35 @@ sai_object_id_t IntfsOrch::getRouterIntfsId(const string &alias)
{
Port port;
gPortsOrch->getPort(alias, port);
assert(port.m_rif_id);
return port.m_rif_id;
}

string IntfsOrch::getRouterIntfsAlias(const IpAddress &ip, const string &vrf_name)
{
sai_object_id_t vrf_id = gVirtualRouterId;

if (!vrf_name.empty())
{
vrf_id = m_vrfOrch->getVRFid(vrf_name);
}

for (const auto &it_intfs: m_syncdIntfses)
{
if (it_intfs.second.vrf_id != vrf_id)
{
continue;
}
for (const auto &prefixIt: it_intfs.second.ip_addresses)
{
if (prefixIt.isAddressInSubnet(ip))
{
return it_intfs.first;
}
}
}
return string();
}

void IntfsOrch::increaseRouterIntfsRefCount(const string &alias)
{
SWSS_LOG_ENTER();
Expand Down Expand Up @@ -154,6 +179,7 @@ bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPre
gPortsOrch->increasePortRefCount(alias);
IntfsEntry intfs_entry;
intfs_entry.ref_count = 0;
intfs_entry.vrf_id = vrf_id;
m_syncdIntfses[alias] = intfs_entry;
}
else
Expand Down Expand Up @@ -328,6 +354,7 @@ void IntfsOrch::doTask(Consumer &consumer)
IntfsEntry intfs_entry;

intfs_entry.ref_count = 0;
intfs_entry.vrf_id = vrf_id;
intfs_entry.ip_addresses.insert(ip_prefix);
m_syncdIntfses[alias] = intfs_entry;
addIp2Me = true;
Expand Down Expand Up @@ -629,7 +656,7 @@ void IntfsOrch::addSubnetRoute(const Port &port, const IpPrefix &ip_prefix)
gCrmOrch->incCrmResUsedCounter(CrmResourceType::CRM_IPV6_ROUTE);
}

gRouteOrch->notifyNextHopChangeObservers(ip_prefix, IpAddresses(), true);
gRouteOrch->notifyNextHopChangeObservers(ip_prefix, NextHopGroupKey(), true);
}

void IntfsOrch::removeSubnetRoute(const Port &port, const IpPrefix &ip_prefix)
Expand Down Expand Up @@ -661,7 +688,7 @@ void IntfsOrch::removeSubnetRoute(const Port &port, const IpPrefix &ip_prefix)
gCrmOrch->decCrmResUsedCounter(CrmResourceType::CRM_IPV6_ROUTE);
}

gRouteOrch->notifyNextHopChangeObservers(ip_prefix, IpAddresses(), false);
gRouteOrch->notifyNextHopChangeObservers(ip_prefix, NextHopGroupKey(), false);
}

void IntfsOrch::addIp2MeRoute(sai_object_id_t vrf_id, const IpPrefix &ip_prefix)
Expand Down
2 changes: 2 additions & 0 deletions orchagent/intfsorch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ struct IntfsEntry
{
std::set<IpPrefix> ip_addresses;
int ref_count;
sai_object_id_t vrf_id;
};

typedef map<string, IntfsEntry> IntfsTable;
Expand All @@ -32,6 +33,7 @@ class IntfsOrch : public Orch
IntfsOrch(DBConnector *db, string tableName, VRFOrch *vrf_orch);

sai_object_id_t getRouterIntfsId(const string&);
string getRouterIntfsAlias(const IpAddress &ip, const string &vrf_name = "");

void increaseRouterIntfsRefCount(const string&);
void decreaseRouterIntfsRefCount(const string&);
Expand Down
22 changes: 11 additions & 11 deletions orchagent/mirrororch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ MirrorEntry::MirrorEntry(const string& platform) :
}

nexthopInfo.prefix = IpPrefix("0.0.0.0/0");
nexthopInfo.nexthop = IpAddress("0.0.0.0");
nexthopInfo.nexthop = NextHopKey("0.0.0.0", "");
}

MirrorOrch::MirrorOrch(TableConnector stateDbConnector, TableConnector confDbConnector,
Expand Down Expand Up @@ -447,7 +447,7 @@ bool MirrorOrch::getNeighborInfo(const string& name, MirrorEntry& session)
// 3) Otherwise, return false.
if (!m_neighOrch->getNeighborEntry(session.dstIp,
session.neighborInfo.neighbor, session.neighborInfo.mac) &&
(session.nexthopInfo.nexthop.isZero() ||
(session.nexthopInfo.nexthop.ip_address.isZero() ||
!m_neighOrch->getNeighborEntry(session.nexthopInfo.nexthop,
session.neighborInfo.neighbor, session.neighborInfo.mac)))
{
Expand Down Expand Up @@ -885,8 +885,8 @@ void MirrorOrch::updateNextHop(const NextHopUpdate& update)

// This is the ECMP scenario that the new next hop group contains the previous
// next hop. There is no need to update this session's monitor port.
if (update.nexthopGroup != IpAddresses() &&
update.nexthopGroup.getIpAddresses().count(session.nexthopInfo.nexthop))
if (update.nexthopGroup != NextHopGroupKey() &&
update.nexthopGroup.getNextHops().count(session.nexthopInfo.nexthop))

{
continue;
Expand All @@ -895,18 +895,18 @@ void MirrorOrch::updateNextHop(const NextHopUpdate& update)
SWSS_LOG_NOTICE("Updating mirror session %s with route %s",
name.c_str(), update.prefix.to_string().c_str());

if (update.nexthopGroup != IpAddresses())
if (update.nexthopGroup != NextHopGroupKey())
{
SWSS_LOG_NOTICE(" next hop IPs: %s", update.nexthopGroup.to_string().c_str());

// Recover the session based on the state database information
if (m_recoverySessionMap.find(name) != m_recoverySessionMap.end())
{
IpAddress nexthop = IpAddress(tokenize(m_recoverySessionMap[name],
NextHopKey nexthop = NextHopKey(tokenize(m_recoverySessionMap[name],
state_db_key_delimiter, 1)[1]);

// Check if recovered next hop IP is within the update's next hop IPs
if (update.nexthopGroup.getIpAddresses().count(nexthop))
if (update.nexthopGroup.getNextHops().count(nexthop))
{
SWSS_LOG_NOTICE("Recover mirror session %s with next hop %s",
name.c_str(), nexthop.to_string().c_str());
Expand All @@ -918,18 +918,18 @@ void MirrorOrch::updateNextHop(const NextHopUpdate& update)
SWSS_LOG_NOTICE("Correct mirror session %s next hop from %s to %s",
name.c_str(), session.nexthopInfo.nexthop.to_string().c_str(),
nexthop.to_string().c_str());
session.nexthopInfo.nexthop = *update.nexthopGroup.getIpAddresses().begin();
session.nexthopInfo.nexthop = *update.nexthopGroup.getNextHops().begin();
}
}
else
{
// Pick the first one from the next hop group
session.nexthopInfo.nexthop = *update.nexthopGroup.getIpAddresses().begin();
session.nexthopInfo.nexthop = *update.nexthopGroup.getNextHops().begin();
}
}
else
{
session.nexthopInfo.nexthop = IpAddress(0);
session.nexthopInfo.nexthop = NextHopKey("0.0.0.0", "");
}

// Resolve the neighbor of the new next hop
Expand All @@ -951,7 +951,7 @@ void MirrorOrch::updateNeighbor(const NeighborUpdate& update)
// Check if the session's destination IP matches the neighbor's update IP
// or if the session's next hop IP matches the neighbor's update IP
if (session.dstIp != update.entry.ip_address &&
session.nexthopInfo.nexthop != update.entry.ip_address)
session.nexthopInfo.nexthop.ip_address != update.entry.ip_address)
{
continue;
}
Expand Down
2 changes: 1 addition & 1 deletion orchagent/mirrororch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ struct MirrorEntry
struct
{
IpPrefix prefix;
IpAddress nexthop;
NextHopKey nexthop;
} nexthopInfo;

struct
Expand Down
Loading