Send IPv4 GARP and IPv6 Unsolicited NA in "cmdAdd" #216

wizhaoredhat · 2022-08-10T19:51:14Z

In "cmdAdd", SRIOV-CNI would construct and send IPv4 Gratuitous ARP
and/or Unsolicited Neighbor Advertisement depending on the IP
addresses configured by IPAM. The reason why this change is needed
is for the scenario when an IP address is reused by IPAM with
different interfaces (with different link-layer addresses). This can
occur when pods are deleted and created. For performance reasons,
sending of GARP and/or Unsolicited NA would update invalid ARP/Neighbor
caches in other neighbors/nodes.

Also we set IPv4 ARP Notify and IPv6 Neighbor Discovery Notify in sysfs
for each interface. This will send GARP and/or Unsolicited NA when the
interface is either brought up or the link-layer address changes. This
is useful in cases where an application reenables the interface or the
MAC address configuration is changed.

Some new packages were added, thus go.mod and go.sum were modified
accordingly.

Mocked PciUtils for sriov tests since sriov.go would call PciUtils to
set IPv4 ARP Notify and IPv6 Neighbor Discovery.

Fixes #177

Signed-off-by: William Zhao wizhao@redhat.com

wizhaoredhat · 2022-08-10T19:58:56Z

@adrianchiris @martinkennelly @zshi-redhat @bn222 @Eoghan1232 @s1061123 @SchSeba : When you have an opportunity it would be much appreciated if you reviewed these changes in my PR.

wizhaoredhat · 2022-08-11T18:50:27Z

I believe there was an intermittent problem with checking out some golang libraries. I hope that is what caused the kubernetes ci to fail. Could someone help with reruning the workflow?

coveralls · 2022-08-14T17:36:52Z

Pull Request Test Coverage Report for Build 3003254009

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

12 of 307 (3.91%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-2.4%) to 34.347%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/sriov/sriov.go	3	10	30.0%
pkg/utils/netlink_manager.go	0	39	0.0%
pkg/utils/utils.go	0	39	0.0%
pkg/sriov/mocks/pci_utils_mock.go	9	65	13.85%
pkg/utils/packet.go	0	154	0.0%

Totals
Change from base Build 2992024059:	-2.4%
Covered Lines:	339
Relevant Lines:	987

💛 - Coveralls

pkg/utils/utils.go

pkg/sriov/sriov.go

pkg/utils/utils.go

SchSeba

The PR looks good thanks for the contribution!

I have a general question if we change the sys/net file and then change the nic to up the kernel should send the request for us no?

base on (https://sysctl-explorer.net/net/ipv6/ndisc_notify/)

pkg/utils/utils.go

wizhaoredhat · 2022-08-16T21:15:20Z

The PR looks good thanks for the contribution!

I have a general question if we change the sys/net file and then change the nic to up the kernel should send the request for us no?

base on (https://sysctl-explorer.net/net/ipv6/ndisc_notify/)

@SchSeba
Glad to be able to contribute!

To your question, yes that is correct. This is the method that Zenghui was first prototyping. However after some debugging and testing. We found some issues:

The interface must be up for IPAM to function correctly. E.g. dhcp IPAM.
(Thus we have to down the interface after IPAM config and bring it back up)
IP addresses can be lost or changed. E.g By default sysctl "keep_addr_on_down" for IPv6 is set to zero. This means that if the interface is "downed", the IPv6 address(es) configured statically are lost.
Also flapping a interface link when the majority of the network configuration was completed by SRIOV-CNI and IPAM to send a gratuitous ARP or unsolicited NA seemed a bit hacky in my opinion.

Constructing the packets manually and sending them seemed like the correct path to go.

cmd/sriov/main.go

pkg/utils/packet.go

wizhaoredhat · 2022-08-22T18:40:41Z

@adrianchiris @zshi-redhat @bn222 @s1061123 @SchSeba : Please review again at your convenience. I would like to have this feature in soon. Thanks!

pkg/utils/packet.go

adrianchiris

Thanks for working on this @wizhaoredhat !

apologies for the late review.

cmd/sriov/main.go

pkg/sriov/sriov_test.go

pkg/utils/utils.go

pkg/utils/packet.go

bn222 · 2022-08-29T08:43:40Z

The commit has become quite large. Would it be possible to split it up a bit?

wizhaoredhat · 2022-08-30T16:56:36Z

Hi All,
Could this PR be merged? I think I addressed everyone's comments. Regarding the checks being not successful, I think the Linter has some issues, the problem area doesn't seem to be with my changes.

@adrianchiris @zshi-redhat @bn222 @s1061123 @SchSeba

SchSeba · 2022-09-01T10:52:10Z

For the CI issue I have an open PR for it

#223

cmd/sriov/main.go

pkg/utils/netlink_manager.go

pkg/utils/utils.go

pkg/sriov/sriov.go

adrianchiris

Overall looks good, once @SchSeba comments are addressed and PR rebased im LGTM

pkg/sriov/mocks/netlink_manager_mock.go

In "cmdAdd", SRIOV-CNI would construct and send IPv4 Gratuitous ARP and/or Unsolicited Neighbor Advertisement depending on the IP addresses configured by IPAM. The reason why this change is needed is for the scenario when an IP address is reused by IPAM with different interfaces (with different link-layer addresses). This can occur when pods are deleted and created. For performance reasons, sending of GARP and/or Unsolicited NA would update invalid ARP/Neighbor caches in other neighbors/nodes. Also we set IPv4 ARP Notify and IPv6 Neighbor Discovery Notify in sysfs for each interface. This will send GARP and/or Unsolicited NA when the interface is either brought up or the link-layer address changes. This is useful in cases where an application reenables the interface or the MAC address configuration is changed. Some new packages were added, thus go.mod and go.sum were modified accordingly. Mocked PciUtils for sriov tests since sriov.go would call PciUtils to set IPv4 ARP Notify and IPv6 Neighbor Discovery. Fixes k8snetworkplumbingwg#177 Signed-off-by: William Zhao <wizhao@redhat.com>

adrianchiris · 2022-09-08T06:04:38Z

/retest

adrianchiris

lets wait for CI to pass, otherwise LGTM

thanks for addressing my comments !

Eoghan1232 · 2022-09-08T07:37:24Z

/retest

@adrianchiris could you kick off ci again ?

wizhaoredhat · 2022-09-08T12:32:04Z

I am not sure why the Mellanox CI is failing when we only changed the test file and mocks... Any idea @adrianchiris ?

adrianchiris · 2022-09-08T13:16:32Z

/retest

adrianchiris · 2022-09-08T13:16:49Z

it should run properly now

wizhaoredhat · 2022-09-08T13:37:01Z

it should run properly now

@adrianchiris
Great, could we merge this pull request now?

Eoghan1232

lgtm

bn222 · 2022-09-08T14:05:29Z

/lgtm

Eoghan1232 · 2022-09-08T14:09:45Z

Merging now.

wizhaoredhat force-pushed the send_garp_and_na branch from b3da09b to 274b033 Compare August 11, 2022 18:19

adrianchiris reviewed Aug 15, 2022

View reviewed changes

pkg/utils/utils.go Outdated Show resolved Hide resolved

adrianchiris reviewed Aug 15, 2022

View reviewed changes

pkg/utils/utils.go Outdated Show resolved Hide resolved

adrianchiris reviewed Aug 15, 2022

View reviewed changes

pkg/sriov/sriov.go Outdated Show resolved Hide resolved

s1061123 reviewed Aug 15, 2022

View reviewed changes

pkg/utils/utils.go Outdated Show resolved Hide resolved

SchSeba reviewed Aug 16, 2022

View reviewed changes

bn222 reviewed Aug 16, 2022

View reviewed changes

pkg/utils/utils.go Outdated Show resolved Hide resolved

wizhaoredhat force-pushed the send_garp_and_na branch from 274b033 to fcfbcb6 Compare August 16, 2022 20:43

bn222 reviewed Aug 17, 2022

View reviewed changes

cmd/sriov/main.go Outdated Show resolved Hide resolved

wizhaoredhat force-pushed the send_garp_and_na branch 2 times, most recently from 2d8259b to 7450a17 Compare August 19, 2022 01:02

bn222 reviewed Aug 19, 2022

View reviewed changes

pkg/utils/packet.go Outdated Show resolved Hide resolved

wizhaoredhat force-pushed the send_garp_and_na branch from 7450a17 to d5f81ce Compare August 22, 2022 22:44

wizhaoredhat requested review from s1061123, SchSeba and adrianchiris and removed request for s1061123 and SchSeba August 22, 2022 22:45

bn222 reviewed Aug 23, 2022

View reviewed changes

pkg/utils/packet.go Outdated Show resolved Hide resolved

wizhaoredhat force-pushed the send_garp_and_na branch from d5f81ce to caabb16 Compare August 23, 2022 16:38

adrianchiris reviewed Aug 25, 2022

View reviewed changes

wizhaoredhat force-pushed the send_garp_and_na branch from caabb16 to fb811b7 Compare August 26, 2022 23:47

wizhaoredhat requested a review from adrianchiris August 31, 2022 13:32

SchSeba reviewed Sep 4, 2022

View reviewed changes

cmd/sriov/main.go Outdated Show resolved Hide resolved

pkg/utils/netlink_manager.go Show resolved Hide resolved

pkg/utils/utils.go Outdated Show resolved Hide resolved

pkg/sriov/sriov.go Show resolved Hide resolved

adrianchiris reviewed Sep 5, 2022

View reviewed changes

wizhaoredhat force-pushed the send_garp_and_na branch 3 times, most recently from ec3a420 to 17c092e Compare September 6, 2022 20:42

adrianchiris reviewed Sep 7, 2022

View reviewed changes

pkg/sriov/mocks/netlink_manager_mock.go Outdated Show resolved Hide resolved

wizhaoredhat force-pushed the send_garp_and_na branch from 17c092e to c241dcb Compare September 7, 2022 21:41

adrianchiris approved these changes Sep 8, 2022

View reviewed changes

Eoghan1232 approved these changes Sep 8, 2022

View reviewed changes

Eoghan1232 merged commit f177b8e into k8snetworkplumbingwg:master Sep 8, 2022

cyclinder mentioned this pull request Nov 7, 2022

Request: publish a new release #230

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send IPv4 GARP and IPv6 Unsolicited NA in "cmdAdd" #216

Send IPv4 GARP and IPv6 Unsolicited NA in "cmdAdd" #216

wizhaoredhat commented Aug 10, 2022 •

edited

Loading

wizhaoredhat commented Aug 10, 2022 •

edited

Loading

wizhaoredhat commented Aug 11, 2022

coveralls commented Aug 14, 2022 •

edited

Loading

SchSeba left a comment

wizhaoredhat commented Aug 16, 2022 •

edited

Loading

wizhaoredhat commented Aug 22, 2022

adrianchiris left a comment

bn222 commented Aug 29, 2022

wizhaoredhat commented Aug 30, 2022

SchSeba commented Sep 1, 2022 •

edited

Loading

adrianchiris left a comment

adrianchiris commented Sep 8, 2022

adrianchiris left a comment

Eoghan1232 commented Sep 8, 2022 •

edited

Loading

wizhaoredhat commented Sep 8, 2022

adrianchiris commented Sep 8, 2022

adrianchiris commented Sep 8, 2022

wizhaoredhat commented Sep 8, 2022 •

edited

Loading

Eoghan1232 left a comment

bn222 commented Sep 8, 2022

Eoghan1232 commented Sep 8, 2022

Send IPv4 GARP and IPv6 Unsolicited NA in "cmdAdd" #216

Send IPv4 GARP and IPv6 Unsolicited NA in "cmdAdd" #216

Conversation

wizhaoredhat commented Aug 10, 2022 • edited Loading

wizhaoredhat commented Aug 10, 2022 • edited Loading

wizhaoredhat commented Aug 11, 2022

coveralls commented Aug 14, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3003254009

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

SchSeba left a comment

Choose a reason for hiding this comment

wizhaoredhat commented Aug 16, 2022 • edited Loading

wizhaoredhat commented Aug 22, 2022

adrianchiris left a comment

Choose a reason for hiding this comment

bn222 commented Aug 29, 2022

wizhaoredhat commented Aug 30, 2022

SchSeba commented Sep 1, 2022 • edited Loading

adrianchiris left a comment

Choose a reason for hiding this comment

adrianchiris commented Sep 8, 2022

adrianchiris left a comment

Choose a reason for hiding this comment

Eoghan1232 commented Sep 8, 2022 • edited Loading

wizhaoredhat commented Sep 8, 2022

adrianchiris commented Sep 8, 2022

adrianchiris commented Sep 8, 2022

wizhaoredhat commented Sep 8, 2022 • edited Loading

Eoghan1232 left a comment

Choose a reason for hiding this comment

bn222 commented Sep 8, 2022

Eoghan1232 commented Sep 8, 2022

wizhaoredhat commented Aug 10, 2022 •

edited

Loading

wizhaoredhat commented Aug 10, 2022 •

edited

Loading

coveralls commented Aug 14, 2022 •

edited

Loading

wizhaoredhat commented Aug 16, 2022 •

edited

Loading

SchSeba commented Sep 1, 2022 •

edited

Loading

Eoghan1232 commented Sep 8, 2022 •

edited

Loading

wizhaoredhat commented Sep 8, 2022 •

edited

Loading