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

Fix neighbor doesn't update all attribute #2577

Merged
merged 4 commits into from
Jan 3, 2023

Conversation

iris00522
Copy link
Contributor

What I did
Fix neighbor doesn't update all attribute
Why I did it
ipv4 link local neighbor doesn't trigger update mac when kernel change mac
command

root@as9726-32d-1:/home/admin# ip neigh add 169.254.0.222 dev PortChannel102 lladdr 52:54:00:3d:4b:22
root@as9726-32d-1:/home/admin# ip neigh replace 169.254.0.222 dev PortChannel102 lladdr 52:54:00:3d:4b:88

sairedis log

2022-12-19.16:10:46.302551|r|SAI_OBJECT_TYPE_NEIGHBOR_ENTRY:{"ip":"20.20.20.20","rif":"oid:0x600000000058d","switch_id":"oid:0x21000000000000"}
2022-12-19.16:16:40.083417|c|SAI_OBJECT_TYPE_NEIGHBOR_ENTRY:{"ip":"169.254.0.222","rif":"oid:0x600000000058d","switch_id":"oid:0x21000000000000"}|SAI_NEIGHBOR_ENTRY_ATTR_DST_MAC_ADDRESS=52:54:00:3D:4B:22|SAI_NEIGHBOR_ENTRY_ATTR_NO_HOST_ROUTE=true
2022-12-19.16:16:40.086088|c|SAI_OBJECT_TYPE_NEXT_HOP:oid:0x4000000000620|SAI_NEXT_HOP_ATTR_TYPE=SAI_NEXT_HOP_TYPE_IP|SAI_NEXT_HOP_ATTR_IP=169.254.0.222|SAI_NEXT_HOP_ATTR_ROUTER_INTERFACE_ID=oid:0x600000000058d
2022-12-19.16:17:05.746970|s|SAI_OBJECT_TYPE_NEIGHBOR_ENTRY:{"ip":"169.254.0.222","rif":"oid:0x600000000058d","switch_id":"oid:0x21000000000000"}|SAI_NEIGHBOR_ENTRY_ATTR_NO_HOST_ROUTE=true

How I verified it
create a ipv4 link local neighbor and change the neighbor mac
check sairedis did set mac

root@as9726-32d-1:/home/admin# ip neigh add 169.254.0.222 dev PortChannel102 lladdr 52:54:00:3d:4b:22
root@as9726-32d-1:/home/admin# ip neigh replace 169.254.0.222 dev PortChannel102 lladdr 52:54:00:3d:4b:88

sairedis log

2022-12-19.16:22:32.009143|c|SAI_OBJECT_TYPE_NEIGHBOR_ENTRY:{"ip":"169.254.0.222","rif":"oid:0x6000000000589","switch_id":"oid:0x21000000000000"}|SAI_NEIGHBOR_ENTRY_ATTR_DST_MAC_ADDRESS=52:54:00:3D:4B:22|SAI_NEIGHBOR_ENTRY_ATTR_NO_HOST_ROUTE=true
2022-12-19.16:22:32.011333|c|SAI_OBJECT_TYPE_NEXT_HOP:oid:0x4000000000626|SAI_NEXT_HOP_ATTR_TYPE=SAI_NEXT_HOP_TYPE_IP|SAI_NEXT_HOP_ATTR_IP=169.254.0.222|SAI_NEXT_HOP_ATTR_ROUTER_INTERFACE_ID=oid:0x6000000000589
2022-12-19.16:22:45.413179|s|SAI_OBJECT_TYPE_NEIGHBOR_ENTRY:{"ip":"169.254.0.222","rif":"oid:0x6000000000589","switch_id":"oid:0x21000000000000"}|SAI_NEIGHBOR_ENTRY_ATTR_DST_MAC_ADDRESS=52:54:00:3D:4B:88
2022-12-19.16:22:45.415482|s|SAI_OBJECT_TYPE_NEIGHBOR_ENTRY:{"ip":"169.254.0.222","rif":"oid:0x6000000000589","switch_id":"oid:0x21000000000000"}|SAI_NEIGHBOR_ENTRY_ATTR_NO_HOST_ROUTE=true

Details if related

@@ -912,15 +912,18 @@ bool NeighOrch::addNeighbor(const NeighborEntry &neighborEntry, const MacAddress
}
else if (isHwConfigured(neighborEntry))
{
status = sai_neighbor_api->set_neighbor_entry_attribute(&neighbor_entry, &neighbor_attr);
if (status != SAI_STATUS_SUCCESS)
for(auto itr : neighbor_attrs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

space after (

@prsunny
Copy link
Collaborator

prsunny commented Dec 19, 2022

@iris00522 , changes lgtm. But is this a valid scenario that IPv4 linklocal neighbor mac gets changed? @venkatmahalingam, to review.

@venkatmahalingam
Copy link
Contributor

venkatmahalingam commented Dec 19, 2022

@iris00522 , changes lgtm. But is this a valid scenario that IPv4 linklocal neighbor mac gets changed? @venkatmahalingam, to review.
@prsunny I have not seen this scenario with link local address.
@iris00522 How did you get into this issue? same IP neighbor got different MAC? what's the use-case? changing the neighbor MAC manually is possible for regular neighbor as well, not sure if that's working? please confirm.

@prsunny
Copy link
Collaborator

prsunny commented Dec 19, 2022

@venkatmahalingam , this issue would be only for link-local ipv4 neighbor since we add two neighbor attributes only for that case. Regular neighbors are not impacted

@gord1306
Copy link
Contributor

@prsunny @venkatmahalingam, when a device is unable to obtain a valid IP address from a DHCP server, it may use an IPv4 link-local address as a fallback option. It has the possibility to encounter the issue. In addition to being used as a fallback option for devices without a valid IP address, IPv4 link-local addresses can also be used by software and services like OpenStack to retrieve internal information. If two devices on the same network are assigned the same link-local address, it is possible for there to be a situation where both devices have the same IP address but different MAC addresses.

{
return parseHandleSaiStatusFailure(handle_status);
SWSS_LOG_ERROR("Failed to update neighbor %s on %s, rv:%d",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we log which attribute failed while setting into the HW?

@venkatmahalingam
Copy link
Contributor

@venkatmahalingam , this issue would be only for link-local ipv4 neighbor since we add two neighbor attributes only for that case. Regular neighbors are not impacted

Got it @prsunny. for regular neighbor only MAC attr set is good enough and link local nbr we have another attr NO_HOST_ROUTE, I have added one minor comment but otherwise I'm good with the new changes.

@prsunny prsunny merged commit 4395cea into sonic-net:master Jan 3, 2023
@iris00522 iris00522 deleted the set_neigh branch January 4, 2023 08:25
dprital added a commit to dprital/sonic-buildimage that referenced this pull request Jan 30, 2023
Update sonic-swss submodule pointer to include the following:
* a2a483d [acl] Add new ACL key BTH_OPCODE and AETH_SYNDROME  ([sonic-net#2617](sonic-net/sonic-swss#2617))
* 9d1f66b [bfdorch] add local discriminator to state DB ([sonic-net#2629](sonic-net/sonic-swss#2629))
* c54b3d1 Vxlan tunnel endpoint custom monitoring APPL DB table. ([sonic-net#2589](sonic-net/sonic-swss#2589))
* 7f03db2 Fix potential risks ([sonic-net#2516](sonic-net/sonic-swss#2516))
* 383ee68 [refactor]Refactoring sai handle status ([sonic-net#2621](sonic-net/sonic-swss#2621))
* cd95972 Fix issue 13341 ARP entry can be out of sync between kernel and APPL_DB if multiple updates are received from RTNL ([sonic-net#2619](sonic-net/sonic-swss#2619))
* a01470f Remove TODO comments that are no longer relevant ([sonic-net#2622](sonic-net/sonic-swss#2622))
* d058390 Changed the BFD default detect multiplier to 10x ([sonic-net#2614](sonic-net/sonic-swss#2614))
* d78b528 [MuxOrch] Enabling neighbor when adding in active state ([sonic-net#2601](sonic-net/sonic-swss#2601))
* 4ebdad1 [routesync] Fix for stale dynamic neighbor ([sonic-net#2553](sonic-net/sonic-swss#2553))
* 8857f92 Added new attributes for Vnet and Vxlan ecmp configurations. ([sonic-net#2584](sonic-net/sonic-swss#2584))
* b6bbc3e Revert [voq][chassis]Add show fabric counters port/queue commands (2522) ([sonic-net#2611](sonic-net/sonic-swss#2611))
* 52406e2 Add missing parameter to on_switch_shutdown_request method. ([sonic-net#2567](sonic-net/sonic-swss#2567))
* 4ac9ad9 Increase diff coverage to 80% ([sonic-net#2599](sonic-net/sonic-swss#2599))
* 8a0bb36 Handle Mac address 'none' ([sonic-net#2593](sonic-net/sonic-swss#2593))
* f496ab3 [vstest] Only collect stdout of orchagent_restart_check in vstest ([sonic-net#2597](sonic-net/sonic-swss#2597))
* 1dab495 Avoid aborting orchagent when setting TUNNEL attributes ([sonic-net#2591](sonic-net/sonic-swss#2591))
* 4395cea Fix neighbor doesn't update all attribute ([sonic-net#2577](sonic-net/sonic-swss#2577))

Signed-off-by: dprital <drorp@nvidia.com>
liat-grozovik pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Feb 1, 2023
Update sonic-swss submodule pointer to include the following:
* a2a483d [acl] Add new ACL key BTH_OPCODE and AETH_SYNDROME  ([#2617](sonic-net/sonic-swss#2617))
* 9d1f66b [bfdorch] add local discriminator to state DB ([#2629](sonic-net/sonic-swss#2629))
* c54b3d1 Vxlan tunnel endpoint custom monitoring APPL DB table. ([#2589](sonic-net/sonic-swss#2589))
* 7f03db2 Fix potential risks ([#2516](sonic-net/sonic-swss#2516))
* 383ee68 [refactor]Refactoring sai handle status ([#2621](sonic-net/sonic-swss#2621))
* cd95972 Fix issue 13341 ARP entry can be out of sync between kernel and APPL_DB if multiple updates are received from RTNL ([#2619](sonic-net/sonic-swss#2619))
* a01470f Remove TODO comments that are no longer relevant ([#2622](sonic-net/sonic-swss#2622))
* d058390 Changed the BFD default detect multiplier to 10x ([#2614](sonic-net/sonic-swss#2614))
* d78b528 [MuxOrch] Enabling neighbor when adding in active state ([#2601](sonic-net/sonic-swss#2601))
* 4ebdad1 [routesync] Fix for stale dynamic neighbor ([#2553](sonic-net/sonic-swss#2553))
* 8857f92 Added new attributes for Vnet and Vxlan ecmp configurations. ([#2584](sonic-net/sonic-swss#2584))
* b6bbc3e Revert [voq][chassis]Add show fabric counters port/queue commands (2522) ([#2611](sonic-net/sonic-swss#2611))
* 52406e2 Add missing parameter to on_switch_shutdown_request method. ([#2567](sonic-net/sonic-swss#2567))
* 4ac9ad9 Increase diff coverage to 80% ([#2599](sonic-net/sonic-swss#2599))
* 8a0bb36 Handle Mac address 'none' ([#2593](sonic-net/sonic-swss#2593))
* f496ab3 [vstest] Only collect stdout of orchagent_restart_check in vstest ([#2597](sonic-net/sonic-swss#2597))
* 1dab495 Avoid aborting orchagent when setting TUNNEL attributes ([#2591](sonic-net/sonic-swss#2591))
* 4395cea Fix neighbor doesn't update all attribute ([#2577](sonic-net/sonic-swss#2577))

Signed-off-by: dprital <drorp@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants