Skip to content

Delete the IPv6 link-local Neighbor when ipv6 link-local mode is disabled #1897

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

Merged
prsunny merged 6 commits into from
Dec 1, 2021
Merged

Delete the IPv6 link-local Neighbor when ipv6 link-local mode is disabled #1897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3a63fff
IPv6 link-local Neighor deletion when ipv6 link-local mode is disabled.
AkhileshSamineni Sep 3, 2021
08a4c8d
Added vs test case.
AkhileshSamineni Oct 22, 2021
75ac751
Added few changes to vs test case
AkhileshSamineni Nov 23, 2021
1b88e1a
Updated vs test case
AkhileshSamineni Nov 24, 2021
0ac9869
Merge branch 'master' into ipv6_ll_neigh_delete
AkhileshSamineni Nov 24, 2021
a77af5e
Added changes to delete the neigh entry from kernel
AkhileshSamineni Nov 30, 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
48 changes: 48 additions & 0 deletions cfgmgr/intfmgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ IntfMgr::IntfMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c
m_stateVrfTable(stateDb, STATE_VRF_TABLE_NAME),
m_stateIntfTable(stateDb, STATE_INTERFACE_TABLE_NAME),
m_appIntfTableProducer(appDb, APP_INTF_TABLE_NAME),
m_neighTable(appDb, APP_NEIGH_TABLE_NAME),
m_appLagTable(appDb, APP_LAG_TABLE_NAME)
{
auto subscriberStateTable = new swss::SubscriberStateTable(stateDb,
Expand Down Expand Up @@ -616,6 +617,35 @@ bool IntfMgr::isIntfStateOk(const string &alias)
return false;
}

void IntfMgr::delIpv6LinkLocalNeigh(const string &alias)
{
vector<string> neighEntries;

SWSS_LOG_INFO("Deleting ipv6 link local neighbors for %s", alias.c_str());

m_neighTable.getKeys(neighEntries);
for (auto neighKey : neighEntries)
{
if (!neighKey.compare(0, alias.size(), alias.c_str()))
{
vector<string> keys = tokenize(neighKey, ':', 1);
if (keys.size() == 2)
{
IpAddress ipAddress(keys[1]);
if (ipAddress.getAddrScope() == IpAddress::AddrScope::LINK_SCOPE)
{
stringstream cmd;
string res;

cmd << IP_CMD << " neigh del dev " << keys[0] << " " << keys[1] ;
Copy link
Copy Markdown
Collaborator

@prsunny prsunny Nov 30, 2021

Choose a reason for hiding this comment

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

why don't we just do a flush of neigh on this interface? why to loop through each of the neighbors especially since this happens during a major config change

Copy link
Copy Markdown
Contributor Author

@AkhileshSamineni AkhileshSamineni Dec 1, 2021

Choose a reason for hiding this comment

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

When we do flushing on a interface, the global ipv6 address also gets deleted along with link-local. For an example

root@sonic:/home/admin# 
root@sonic:/home/admin# ip neigh show
1001::2 dev Ethernet64 lladdr cc:37:ab:17:6c:9a router REACHABLE
fe80::ce37:abff:fe17:6c9a dev Ethernet64 lladdr cc:37:ab:17:6c:9a router REACHABLE
root@sonic:/home/admin# 
root@sonic:/home/admin# sonic-db-cli APPL_DB keys \*NEIGH\*
NEIGH_TABLE:Ethernet64:1001::2
NEIGH_TABLE:Ethernet64:fe80::ce37:abff:fe17:6c9a
root@sonic:/home/admin#

Copy link
Copy Markdown
Collaborator

@prsunny prsunny Dec 1, 2021

Choose a reason for hiding this comment

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

agree.. but this operation is not done frequently, and flushing the interface, it can also relearn, right?

Copy link
Copy Markdown
Contributor

@venkatmahalingam venkatmahalingam Dec 1, 2021

Choose a reason for hiding this comment

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

Looks like this is deleting the link-local neighbor only, given that we will not have many link local neighbors, flushing per interface level may not be required.

Copy link
Copy Markdown
Collaborator

@prsunny prsunny Dec 1, 2021

Choose a reason for hiding this comment

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

ok, i was thinking if we could avoid this Neighbor handling and scanning the APP_DB in this context. @AkhileshSamineni, please make sure if the delete operation fails (for some reason the linklocal is not present in kernel while attempting to delete) does not cause process exit

Copy link
Copy Markdown
Contributor Author

@AkhileshSamineni AkhileshSamineni Dec 1, 2021

Choose a reason for hiding this comment

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

@prsunny If the delete operation fails for some reason, it does not cause the process exit.
we see below INFO message only

Sep  5 13:00:00.887868 sonic INFO swss#/supervisord: intfmgrd RTNETLINK answers: No such file or directory

swss::exec(cmd.str(), res);
SWSS_LOG_INFO("Deleted ipv6 link local neighbor - %s", keys[1].c_str());
}
}
}
}
}

bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,
vector<FieldValueTuple> data,
const string& op)
Expand Down Expand Up @@ -755,6 +785,17 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,
/* Set ipv6 mode */
if (!ipv6_link_local_mode.empty())
{
if ((ipv6_link_local_mode == "enable") && (m_ipv6LinkLocalModeList.find(alias) == m_ipv6LinkLocalModeList.end()))
{
m_ipv6LinkLocalModeList.insert(alias);
SWSS_LOG_INFO("Inserted ipv6 link local mode list for %s", alias.c_str());
}
else if ((ipv6_link_local_mode == "disable") && (m_ipv6LinkLocalModeList.find(alias) != m_ipv6LinkLocalModeList.end()))
{
m_ipv6LinkLocalModeList.erase(alias);
delIpv6LinkLocalNeigh(alias);
SWSS_LOG_INFO("Erased ipv6 link local mode list for %s", alias.c_str());
}
FieldValueTuple fvTuple("ipv6_use_link_local_only", ipv6_link_local_mode);
data.push_back(fvTuple);
}
Expand Down Expand Up @@ -910,6 +951,13 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,
removeSubIntfState(alias);
}

if (m_ipv6LinkLocalModeList.find(alias) != m_ipv6LinkLocalModeList.end())
{
m_ipv6LinkLocalModeList.erase(alias);
delIpv6LinkLocalNeigh(alias);
SWSS_LOG_INFO("Erased ipv6 link local mode list for %s", alias.c_str());
}

m_appIntfTableProducer.del(alias);
m_stateIntfTable.del(alias);
}
Expand Down
3 changes: 3 additions & 0 deletions cfgmgr/intfmgr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,12 @@ class IntfMgr : public Orch
ProducerStateTable m_appIntfTableProducer;
Table m_cfgIntfTable, m_cfgVlanIntfTable, m_cfgLagIntfTable, m_cfgLoopbackIntfTable;
Table m_statePortTable, m_stateLagTable, m_stateVlanTable, m_stateVrfTable, m_stateIntfTable, m_appLagTable;
Table m_neighTable;

SubIntfMap m_subIntfList;
std::set<std::string> m_loopbackIntfList;
std::set<std::string> m_pendingReplayIntfList;
std::set<std::string> m_ipv6LinkLocalModeList;

void setIntfIp(const std::string &alias, const std::string &opCmd, const IpPrefix &ipPrefix);
void setIntfVrf(const std::string &alias, const std::string &vrfName);
Expand Down Expand Up @@ -65,6 +67,7 @@ class IntfMgr : public Orch
void removeHostSubIntf(const std::string &subIntf);
void setSubIntfStateOk(const std::string &alias);
void removeSubIntfState(const std::string &alias);
void delIpv6LinkLocalNeigh(const std::string &alias);

bool setIntfProxyArp(const std::string &alias, const std::string &proxy_arp);
bool setIntfGratArp(const std::string &alias, const std::string &grat_arp);
Expand Down
2 changes: 1 addition & 1 deletion neighsyncd/neighsync.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ void NeighSync::onMsg(int nlmsg_type, struct nl_object *obj)
/* Ignore IPv6 link-local addresses as neighbors, if ipv6 link local mode is disabled */
if (family == IPV6_NAME && IN6_IS_ADDR_LINKLOCAL(nl_addr_get_binary_addr(rtnl_neigh_get_dst(neigh))))
{
if (isLinkLocalEnabled(intfName) == false)
if ((isLinkLocalEnabled(intfName) == false) && (nlmsg_type != RTM_DELNEIGH))
{
return;
}
Expand Down
112 changes: 112 additions & 0 deletions tests/test_ipv6_link_local.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
import time
import json
import pytest

from swsscommon import swsscommon

class TestIPv6LinkLocal(object):
def setup_db(self, dvs):
self.pdb = dvs.get_app_db()
self.adb = dvs.get_asic_db()
self.cdb = dvs.get_config_db()

def set_admin_status(self, interface, status):
self.cdb.update_entry("PORT", interface, {"admin_status": status})

def add_ip_address(self, interface, ip):
self.cdb.create_entry("INTERFACE", interface + "|" + ip, {"NULL": "NULL"})
time.sleep(2)

def remove_ip_address(self, interface, ip):
self.cdb.delete_entry("INTERFACE", interface + "|" + ip)
time.sleep(2)

def create_ipv6_link_local_intf(self, interface):
self.cdb.create_entry("INTERFACE", interface, {"ipv6_use_link_local_only": "enable"})
time.sleep(2)

def remove_ipv6_link_local_intf(self, interface):
self.cdb.delete_entry("INTERFACE", interface)
time.sleep(2)

def test_NeighborAddRemoveIpv6LinkLocal(self, dvs, testlog):
self.setup_db(dvs)

# create ipv6 link local intf
self.create_ipv6_link_local_intf("Ethernet0")
self.create_ipv6_link_local_intf("Ethernet4")

# bring up interface
self.set_admin_status("Ethernet0", "up")
self.set_admin_status("Ethernet4", "up")

# set ip address
self.add_ip_address("Ethernet0", "2000::1/64")
self.add_ip_address("Ethernet4", "2001::1/64")
dvs.runcmd("sysctl -w net.ipv6.conf.all.forwarding=1")
dvs.runcmd("sysctl -w net.ipv6.conf.default.forwarding=1")

# set ip address and default route
dvs.servers[0].runcmd("ip -6 address add 2000::2/64 dev eth0")
dvs.servers[0].runcmd("ip -6 route add default via 2000::1")

dvs.servers[1].runcmd("ip -6 address add 2001::2/64 dev eth0")
dvs.servers[1].runcmd("ip -6 route add default via 2001::1")
time.sleep(2)

# get neighbor entry
dvs.servers[0].runcmd("ping -6 -c 1 2001::2")
time.sleep(2)

# Neigh entries should contain Ipv6-link-local neighbors, should be 4
neigh_entries = self.pdb.get_keys("NEIGH_TABLE")
assert (len(neigh_entries) == 4)

found_entry = False
for key in neigh_entries:
if (key.find("Ethernet4:2001::2") or key.find("Ethernet0:2000::2")):
Copy link
Copy Markdown
Collaborator

@dgsudharsan dgsudharsan Nov 3, 2021

Choose a reason for hiding this comment

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

Why is this an 'or' check? Shouldn't it be mandatory that both entries exist?

Copy link
Copy Markdown
Contributor Author

@AkhileshSamineni AkhileshSamineni Nov 5, 2021

Choose a reason for hiding this comment

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

Both entries should exist, but here moving ahead by checking 1 entry only. Anyways checking number of entries as 4 right.

found_entry = True

assert found_entry

# remove ip address
self.remove_ip_address("Ethernet0", "2000::1/64")
self.remove_ip_address("Ethernet4", "2001::1/64")

# remove ipv6 link local intf
self.remove_ipv6_link_local_intf("Ethernet0")
self.remove_ipv6_link_local_intf("Ethernet4")

# Neigh entries should not contain Ipv6-link-local neighbors, should be 2
Copy link
Copy Markdown
Collaborator

@dgsudharsan dgsudharsan Nov 3, 2021

Choose a reason for hiding this comment

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

Wouldn't removing the global IPv6 remove both IP link local as well as global neighbor entries? Can you please clarify?

Copy link
Copy Markdown
Contributor Author

@AkhileshSamineni AkhileshSamineni Nov 5, 2021

Choose a reason for hiding this comment

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

Global ipv6 remove will not remove the link local neighbor entries because ipv6 link local is always enable in kernel. The ipv6 link local remove command will remove the neighbors from APPL_DB only.

Copy link
Copy Markdown
Collaborator

@dgsudharsan dgsudharsan Nov 16, 2021

Choose a reason for hiding this comment

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

If it removes the neighbors from APPL_DB and remove_ip_address remove the global neighbors shouldn't the number of entries be zero? My earlier question was why 2 entries are present since the interface now has no IPv6 address (global) or has link local enabled.

Copy link
Copy Markdown
Collaborator

@dgsudharsan dgsudharsan Nov 22, 2021

Choose a reason for hiding this comment

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

@AkhileshSamineni Can you please let me know if my understanding is correct?

Copy link
Copy Markdown
Contributor Author

@AkhileshSamineni AkhileshSamineni Nov 22, 2021

Choose a reason for hiding this comment

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

@dgsudharsan Global neighbor entries gets deleted when interface is set to down.
When interface set to up/down, then only the netlink message are generated (https://github.com/Azure/sonic-swss/blob/master/tests/test_neighbor.py#L79).

neigh_after_entries = self.pdb.get_keys("NEIGH_TABLE")
print(neigh_after_entries)
assert (len(neigh_after_entries) == 2)

found_existing_entry = False
for key in neigh_entries:
if (key.find("Ethernet4:2001::2") or key.find("Ethernet0:2000::2")):
found_existing_entry = True

assert found_existing_entry

self.set_admin_status("Ethernet0", "down")
self.set_admin_status("Ethernet4", "down")

# remove ip address and default route
dvs.servers[0].runcmd("ip -6 route del default dev eth0")
dvs.servers[0].runcmd("ip -6 address del 2000::2/64 dev eth0")

dvs.servers[1].runcmd("ip -6 route del default dev eth0")
dvs.servers[1].runcmd("ip -6 address del 2001::2/64 dev eth0")

dvs.runcmd("sysctl -w net.ipv6.conf.all.forwarding=0")
dvs.runcmd("sysctl -w net.ipv6.conf.default.forwarding=0")

neigh_entries = self.pdb.get_keys("NEIGH_TABLE")
assert (len(neigh_entries) == 0)

# Add Dummy always-pass test at end as workaroud
# for issue when Flaky fail on final test it invokes module tear-down before retrying
def test_nonflaky_dummy():
pass