-
Notifications
You must be signed in to change notification settings - Fork 549
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
Delete the IPv6 link-local Neighbor when ipv6 link-local mode is disabled #1897
Delete the IPv6 link-local Neighbor when ipv6 link-local mode is disabled #1897
Conversation
Signed-off-by: Akhilesh Samineni <[email protected]>
@prsunny Please review the changes. |
@dgsudharsan , please review |
cfgmgr/intfmgr.cpp
Outdated
if (keys.size() == 2) | ||
{ | ||
IpAddress ipAddress(keys[1]); | ||
if ((ipAddress.isV4() == false) && (ipAddress.getAddrScope() == IpAddress::AddrScope::LINK_SCOPE)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this isV4 check be a problem if the changes mentioned in these PR are merged? #1903
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the isV4 check.
@venkatmahalingam Can you also please review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add vs test case covering the scenario?
Signed-off-by: Akhilesh Samineni <[email protected]>
Added vs test case for this scenario. |
@dgsudharsan Most of the vs test cases are failing due to some connection issue
Please re-run it. |
|
||
found_entry = False | ||
for key in neigh_entries: | ||
if (key.find("Ethernet4:2001::2") or key.find("Ethernet0:2000::2")): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
@dgsudharsan Please retest it. |
Akhilesh, I don't permissions. You can run it yourself by /azpw run (pipeline) e.g. /azpw run Azure.sonic-swss (Test vstest) |
Even I don't have permissions. |
@prsunny Please retest it. |
Hi Akhilesh |
/AzurePipelines run Azure.sonic-swss |
Commenter does not have sufficient privileges for PR 1897 in repo Azure/sonic-swss |
@AkhileshSamineni , you could give "/azpw run" |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@AkhileshSamineni The UT around the changes are failing. Can you please run them locally and fix? |
@dgsudharsan Yeah sure, I have pushed the changes only after verified it locally. Test case is failed as seeing 4 neighbor entries before ping operation.
E assert 4 == 0 |
@dgsudharsan @prsunny Corrected the vs test case.
Please check once. |
@preetham-singh , can you please check the test failures. Its now blocking other PRs. Is there any dependency that is missed? |
/azpw run |
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
@prsunny @dgsudharsan Please approve and merge it. |
cfgmgr/intfmgr.cpp
Outdated
IpAddress ipAddress(keys[1]); | ||
if (ipAddress.getAddrScope() == IpAddress::AddrScope::LINK_SCOPE) | ||
{ | ||
m_neighTableProducer.del(neighKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a good approach as to have multiple producers for the same table APP_NEIGH_TABLE_NAME (Owned by neighsyncd). I think, intfmgr should delete this from kernel directly. Is that feasible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prsunny I do agree about multiple producers to the same table.
If intfmgr deletes the kernel entry, then neighsync have to delete that entry from APPL_DB even though the link-local mode is disabled right. Added those changes, please continue the review.
stringstream cmd; | ||
string res; | ||
|
||
cmd << IP_CMD << " neigh del dev " << keys[0] << " " << keys[1] ; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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#
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
@dgsudharsan , @venkatmahalingam , please signoff |
691c37b [Route bulk] Fix bugs in case a SET operation follows a DEL operation in the same bulk (sonic-net/sonic-swss#2086) a4c80c3 patch for issue sonic-net/sonic-swss#1971 - enable Rx Drop handling for cisco-8000 (sonic-net/sonic-swss#2041) 71751d1 [macsec] Support setting IPG by gearbox_config.json (sonic-net/sonic-swss#2051) 5d5c169 [bulk mode] Fix bulk conflict when in case there are both remove and set operations (sonic-net/sonic-swss#2071) 8bbdbd2 Fix SRV6 NHOP CRM object type (sonic-net/sonic-swss#2072) ef5b35f [vstest] VS test failure fix after fabric port orch PR merge (sonic-net/sonic-swss#1811) 89ea538 Supply the missing ingress/egress port profile list in document (sonic-net/sonic-swss#2064) 8123437 [pfc_detect] fix RedisReply errors (sonic-net/sonic-swss#2040) b38f527 [swss][CRM][MPLS] MPLS CRM Nexthop - switch back to using SAI OBJECT rather than SWITCH OBJECT ae061e5 create debug_shell_enable config to enable debug shell (sonic-net/sonic-swss#2060) 45e446d [cbf] Fix max FC value (sonic-net/sonic-swss#2049) b1b5b29 Initial p4orch pytest code. (sonic-net/sonic-swss#2054) d352d5a Update default route status to state DB (sonic-net/sonic-swss#2009) 24a64d6 Orchagent: Integrate P4Orch (sonic-net/sonic-swss#2029) 15a3b6c Delete the IPv6 link-local Neighbor when ipv6 link-local mode is disabled (sonic-net/sonic-swss#1897) ed783e1 [orchagent] Add trap flow counter support (sonic-net/sonic-swss#1951) e9b05a3 [vnetorch] ECMP for vnet tunnel routes with endpoint health monitor (sonic-net/sonic-swss#1955) bcb7d61 P4Orch: inital add of source (sonic-net/sonic-swss#1997) f6f6f86 [mclaglink] fix acl out ports (sonic-net/sonic-swss#2026) fd887bf [Reclaim buffer] Reclaim unused buffer for dynamic buffer model (sonic-net/sonic-swss#1910) 9258978 [orchagent, cfgmgr] Add response publisher and state recording (sonic-net/sonic-swss#1992) 3d862a7 Fixing subport vs test script for subport under VNET (sonic-net/sonic-swss#2048) fb0a5fd Don't handle buffer pool watermark during warm reboot reconciling (sonic-net/sonic-swss#1987) 16d4bcd Routed subinterface enhancements (sonic-net/sonic-swss#1907) 9639db7 [vstest/subintf] Add vs test to validate sub interface ingress to a vnet (sonic-net/sonic-swss#1642) Signed-off-by: Stephen Sun [email protected]
691c37b [Route bulk] Fix bugs in case a SET operation follows a DEL operation in the same bulk (sonic-net/sonic-swss#2086) a4c80c3 patch for issue sonic-net/sonic-swss#1971 - enable Rx Drop handling for cisco-8000 (sonic-net/sonic-swss#2041) 71751d1 [macsec] Support setting IPG by gearbox_config.json (sonic-net/sonic-swss#2051) 5d5c169 [bulk mode] Fix bulk conflict when in case there are both remove and set operations (sonic-net/sonic-swss#2071) 8bbdbd2 Fix SRV6 NHOP CRM object type (sonic-net/sonic-swss#2072) ef5b35f [vstest] VS test failure fix after fabric port orch PR merge (sonic-net/sonic-swss#1811) 89ea538 Supply the missing ingress/egress port profile list in document (sonic-net/sonic-swss#2064) 8123437 [pfc_detect] fix RedisReply errors (sonic-net/sonic-swss#2040) b38f527 [swss][CRM][MPLS] MPLS CRM Nexthop - switch back to using SAI OBJECT rather than SWITCH OBJECT ae061e5 create debug_shell_enable config to enable debug shell (sonic-net/sonic-swss#2060) 45e446d [cbf] Fix max FC value (sonic-net/sonic-swss#2049) b1b5b29 Initial p4orch pytest code. (sonic-net/sonic-swss#2054) d352d5a Update default route status to state DB (sonic-net/sonic-swss#2009) 24a64d6 Orchagent: Integrate P4Orch (sonic-net/sonic-swss#2029) 15a3b6c Delete the IPv6 link-local Neighbor when ipv6 link-local mode is disabled (sonic-net/sonic-swss#1897) ed783e1 [orchagent] Add trap flow counter support (sonic-net/sonic-swss#1951) e9b05a3 [vnetorch] ECMP for vnet tunnel routes with endpoint health monitor (sonic-net/sonic-swss#1955) bcb7d61 P4Orch: inital add of source (sonic-net/sonic-swss#1997) f6f6f86 [mclaglink] fix acl out ports (sonic-net/sonic-swss#2026) fd887bf [Reclaim buffer] Reclaim unused buffer for dynamic buffer model (sonic-net/sonic-swss#1910) 9258978 [orchagent, cfgmgr] Add response publisher and state recording (sonic-net/sonic-swss#1992) 3d862a7 Fixing subport vs test script for subport under VNET (sonic-net/sonic-swss#2048) fb0a5fd Don't handle buffer pool watermark during warm reboot reconciling (sonic-net/sonic-swss#1987) 16d4bcd Routed subinterface enhancements (sonic-net/sonic-swss#1907) 9639db7 [vstest/subintf] Add vs test to validate sub interface ingress to a vnet (sonic-net/sonic-swss#1642) Signed-off-by: Stephen Sun [email protected]
691c37b [Route bulk] Fix bugs in case a SET operation follows a DEL operation in the same bulk (sonic-net/sonic-swss#2086) a4c80c3 patch for issue sonic-net/sonic-swss#1971 - enable Rx Drop handling for cisco-8000 (sonic-net/sonic-swss#2041) 71751d1 [macsec] Support setting IPG by gearbox_config.json (sonic-net/sonic-swss#2051) 5d5c169 [bulk mode] Fix bulk conflict when in case there are both remove and set operations (sonic-net/sonic-swss#2071) 8bbdbd2 Fix SRV6 NHOP CRM object type (sonic-net/sonic-swss#2072) ef5b35f [vstest] VS test failure fix after fabric port orch PR merge (sonic-net/sonic-swss#1811) 89ea538 Supply the missing ingress/egress port profile list in document (sonic-net/sonic-swss#2064) 8123437 [pfc_detect] fix RedisReply errors (sonic-net/sonic-swss#2040) b38f527 [swss][CRM][MPLS] MPLS CRM Nexthop - switch back to using SAI OBJECT rather than SWITCH OBJECT ae061e5 create debug_shell_enable config to enable debug shell (sonic-net/sonic-swss#2060) 45e446d [cbf] Fix max FC value (sonic-net/sonic-swss#2049) b1b5b29 Initial p4orch pytest code. (sonic-net/sonic-swss#2054) d352d5a Update default route status to state DB (sonic-net/sonic-swss#2009) 24a64d6 Orchagent: Integrate P4Orch (sonic-net/sonic-swss#2029) 15a3b6c Delete the IPv6 link-local Neighbor when ipv6 link-local mode is disabled (sonic-net/sonic-swss#1897) ed783e1 [orchagent] Add trap flow counter support (sonic-net/sonic-swss#1951) e9b05a3 [vnetorch] ECMP for vnet tunnel routes with endpoint health monitor (sonic-net/sonic-swss#1955) bcb7d61 P4Orch: inital add of source (sonic-net/sonic-swss#1997) f6f6f86 [mclaglink] fix acl out ports (sonic-net/sonic-swss#2026) fd887bf [Reclaim buffer] Reclaim unused buffer for dynamic buffer model (sonic-net/sonic-swss#1910) 9258978 [orchagent, cfgmgr] Add response publisher and state recording (sonic-net/sonic-swss#1992) 3d862a7 Fixing subport vs test script for subport under VNET (sonic-net/sonic-swss#2048) fb0a5fd Don't handle buffer pool watermark during warm reboot reconciling (sonic-net/sonic-swss#1987) 16d4bcd Routed subinterface enhancements (sonic-net/sonic-swss#1907) 9639db7 [vstest/subintf] Add vs test to validate sub interface ingress to a vnet (sonic-net/sonic-swss#1642) Signed-off-by: Stephen Sun [email protected]
cherry-picking to 202106 is failing. Please create a new pr for the 202106 branch. |
@AkhileshSamineni Can you please create a new PR? |
…bled (sonic-net#1897) * IPv6 link-local Neighbor deletion when ipv6 link-local mode is disabled. Signed-off-by: Akhilesh Samineni <[email protected]>
IPv6 link-local Neighbor deletion when ipv6 link-local mode is disabled
What I did
Fix for Issue => sonic-net/sonic-buildimage#8571:
Issue :
Step 1 : Enable the ipv6 link-local mode on Ethernet48 :
Step 2 : Ping the ipv6 link-local neighbor :
Step 3 : Disable the ipv6 link-local mode on Ethernet48 :
Observed RIF is not getting deleted, as interface (Ethernet48) had an reference to ipv6 link-local neighbor.
Why I did it
The ipv6 link local neighbors are getting added to APPL_DB only when ipv6 link local mode is enabled (Here : https://github.com/Azure/sonic-swss/blob/master/neighsyncd/neighsync.cpp#L85) and the added neighbor entry is not deleted when link-local mode is disabled. Because of APPL_DB neigh entry mapped to interface, the interface RIF is not getting deleted when ipv6 link-local mode is disabled.
Added code changes to delete the ipv6 link-local neigh entries when ipv6 link-local is disabled.
How I verified it
Enable and Disable link-local mode on a interface when global ipv6 address is not configured on an interface :
Enable and Disable link-local mode on a interface when global ipv6 address is configured on the same interface :
Signed-off-by: Akhilesh Samineni [email protected]