Set MAC address after renaming the interface #280

zeeke · 2023-09-29T16:29:26Z

Setting the MAC address at the end of SetupVF reduces the chances of race conditions with tools that set MAC i address asynchronously (i.e. iavf).

pkg/sriov/sriov.go

mlguerrero12 · 2023-10-04T07:01:20Z

LGTM

adrianchiris · 2023-10-04T13:28:40Z

@zeeke could you provide more info on the the race condition and how this reduces the chance of race condition ?

Setting the MAC address at the end of SetupVF reduces the chances of race conditions with tools that set MAC i address asynchronously (i.e. iavf).

typo remove "i" in MAC i address

adrianchiris

small nit in commit msg + adding more info on the race condition is appreciated.

overall LGTM

zeeke · 2023-10-04T15:34:36Z

@zeeke could you provide more info on the the race condition and how this reduces the chance of race condition ?

I'm collaborating with kernel engineers to gather more information about this. What we discovered so far is that after recent changes to iavf driver (around c34743daca0eb1dc855831a5210f0800a850088e), setting the MAC address via sriov-cni produced VFs that are not able to ping each other.
Though this will likely be a kernel regression, I could not reproduce it via bare ip link ... commands, and these changes fix the problem with iavf cards.

I'm currently testing this against multiple NIC vendors. I hope I'll be back with more information, or at least regression test results to share with the community.

zeeke · 2023-10-12T10:51:42Z

I managed to reproduce the issue this PR is trying to solve with bare ip ... commands, emulating what the SRIOV CNI does and highlighting the problem with the i40e driver.

test-ip.sh [2] executes steps before this changes: it configures two VFs from the same PF with MAC and IP addresses. Then tries a ping for a connectivity check.

i40e_before.txt [4] shows an error raised by the first MAC address set.

...
+ ip link set dev temp_71 address 20:04:0f:f1:88:A1
RTNETLINK answers: Resource temporarily unavailable
+ ip link set dev temp_71 address 20:04:0f:f1:88:A1
+ ip link set dev temp_71 netns ns_a
...

Though the second call goes well, the network device doesn't have any connectivity:

+ ip netns exec ns_a ping -c 3 10.255.255.2
PING 10.255.255.2 (10.255.255.2) 56(84) bytes of data.

--- 10.255.255.2 ping statistics ---
3 packets transmitted, 0 received, 100% packet loss, time 2069ms

test-ip.with-fix.sh.txt [1] shows this PR's changes fix the problem on i40e [3]:

+ ip link set dev ens1f0 vf 0 vlan 0
+ ip link set dev ens1f0 vf 0 state enable
+ ip link set dev ens1f0 vf 0 mac 20:04:0f:f1:88:A1
+ ip link set dev ens1f0v0 down
+ ip link set ens1f0v0 name temp_2
+ ip link set dev temp_2 netns ns_a
+ ip netns exec ns_a ip link set temp_2 name net1
+ ip netns exec ns_a sh -c 'echo 1 > /proc/sys/net/ipv4/conf/net1/arp_notify'
+ ip netns exec ns_a ip link set dev net1 address 20:04:0f:f1:88:A1
+ ip netns exec ns_a ip link set dev net1 up
+ ip netns exec ns_a ip address add dev net1 10.255.255.1/24

mlx5 driver is not affected by the problem [6], and the fix is not interfering with it [5].

Besides this test script, several tests have been executed against the sriov-operator with this cni fix, showing no regression on Intel E810 and Mellanox ConnectX-5

[1] test-ip.with-fix.sh.txt
[2] test-ip.sh.txt
[3] i40e_after.txt
[4] i40e_before.txt
[5] mlx5_after.txt
[6] mlx5_before.txt

mlguerrero12 · 2023-10-12T11:49:28Z

Nice work @zeeke!

Perhaps we should add a log in the retrying mechanism of SetVFEffectiveMAC to catch transient errors while setting the mac.

zeeke · 2023-10-12T12:24:06Z

Nice work @zeeke!

Perhaps we should add a log in the retrying mechanism of SetVFEffectiveMAC to catch transient errors while setting the mac.

Good point

zeeke · 2023-10-13T08:10:51Z

@Eoghan1232 can you please take a look at this?

mlguerrero12 · 2023-10-13T08:23:14Z

pkg/utils/utils.go

@@ -402,3 +407,15 @@ func Retry(retries int, sleep time.Duration, f func() error) error {
 	}
 	return err
 }
+
+func warnOnError(callerFn string, f func() error) func() error {


Let's simplify this. You can simply put the logging in the Retry function.

It would involve many log calls, one for each error returned. Here we catch all of them.

I'm ok with removing all this logging and discussing it in another PR

but I didn't mean to add it inside the func() which is passed as a parameter in the Retry function, but to add it in the Retry function itself. Line 400 of utils.go. You would only add it once.

Oh, I got it. Not a big fan of logging from utility functions. it can lead to a lot of junk in the log files, for example, if the Retry function is called for another job where it's expected to have multiple failures before going well.

since it's not yet used widely, I'd overcome that by making it private

you can also add a new parameter "warnOnError" in the Retry func to control when to log

5th parameter for a utility function. That's going to be even worse.
Thanks for the suggestions, but let's discuss it in a different PR, we went out of topic here.

Reverting

alright, please create an issue to not forget this.

Eoghan1232

I am okay with these changes.

One question, I see you took out the changes to utils:

func warnOnError(callerFn string, f func() error) func() error {

was this intended? @zeeke

Setting the MAC address at the end of SetupVF reduces the chances of race conditions with tools that set MAC address asynchronously (i.e. iavf). This commit solve an issue with i40e driver where calling `SetVFEffectiveMAC` after `SetVFHardwareMAC` may produce a VF with no connectivity even if the last configuration step didn't produce any error: ``` + ip link set dev ens1f0 vf 0 mac 20:04:0f:f1:88:A1 # No error + ip link set dev temp_71 address 20:04:0f:f1:88:A1 # Transient error RTNETLINK answers: Resource temporarily unavailable + ip link set dev temp_71 address 20:04:0f:f1:88:A1 # No error ``` Note: this seems to be a kernel driver regression introduced near torvalds/linux@c34743d Signed-off-by: Andrea Panattoni <[email protected]>

zeeke · 2023-10-13T12:38:25Z

I am okay with these changes.

One question, I see you took out the changes to utils:
func warnOnError(callerFn string, f func() error) func() error {
was this intended? @zeeke

Yes, we didn't find a consensus on how to log setting MAC address retries, so we'll tackle that on a different PR.

small nit in commit msg + adding more info on the race condition is appreciated.

overall LGTM

Updated the commit message

Eoghan1232 · 2023-10-13T12:48:04Z

I am okay with these changes.
One question, I see you took out the changes to utils:
func warnOnError(callerFn string, f func() error) func() error {
was this intended? @zeeke
Yes, we didn't find a consensus on how to log setting MAC address retries, so we'll tackle that on a different PR.

small nit in commit msg + adding more info on the race condition is appreciated.
overall LGTM

Updated the commit message

understood, then I think we are good to merge this.

zeeke · 2023-10-13T13:10:31Z

PR got 2 LGTMs from maintainers, merging.

adrianchiris · 2023-10-15T07:24:26Z

Thx for the detailed information @zeeke !

zeeke force-pushed the ocpbugs-19536-us branch from 11ea0a4 to 312c193 Compare October 2, 2023 11:59

mlguerrero12 reviewed Oct 2, 2023

View reviewed changes

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

zeeke force-pushed the ocpbugs-19536-us branch from 312c193 to e8b7a5e Compare October 3, 2023 15:48

mlguerrero12 approved these changes Oct 4, 2023

View reviewed changes

adrianchiris approved these changes Oct 4, 2023

View reviewed changes

zeeke force-pushed the ocpbugs-19536-us branch from e8b7a5e to da3ca0e Compare October 4, 2023 15:35

zeeke force-pushed the ocpbugs-19536-us branch from da3ca0e to 03df0d8 Compare October 12, 2023 10:11

zeeke force-pushed the ocpbugs-19536-us branch from 1d69ed6 to 634a385 Compare October 12, 2023 15:01

mlguerrero12 reviewed Oct 13, 2023

View reviewed changes

zeeke force-pushed the ocpbugs-19536-us branch 2 times, most recently from 4ef5df7 to 03df0d8 Compare October 13, 2023 09:10

Eoghan1232 approved these changes Oct 13, 2023

View reviewed changes

zeeke force-pushed the ocpbugs-19536-us branch from 03df0d8 to d572508 Compare October 13, 2023 12:37

zeeke merged commit 75f2f5e into k8snetworkplumbingwg:master Oct 13, 2023
10 checks passed

zeeke mentioned this pull request Oct 17, 2023

OCPBUGS-21831: [release-4.14] Set MAC address after renaming the interface openshift/sriov-cni#92

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set MAC address after renaming the interface #280

Set MAC address after renaming the interface #280

zeeke commented Sep 29, 2023

mlguerrero12 commented Oct 4, 2023

adrianchiris commented Oct 4, 2023

adrianchiris left a comment

zeeke commented Oct 4, 2023

zeeke commented Oct 12, 2023

mlguerrero12 commented Oct 12, 2023 •

edited

Loading

zeeke commented Oct 12, 2023

zeeke commented Oct 13, 2023

mlguerrero12 Oct 13, 2023

zeeke Oct 13, 2023 •

edited

Loading

mlguerrero12 Oct 13, 2023 •

edited

Loading

zeeke Oct 13, 2023

mlguerrero12 Oct 13, 2023

zeeke Oct 13, 2023

mlguerrero12 Oct 13, 2023

Eoghan1232 left a comment •

edited

Loading

zeeke commented Oct 13, 2023

Eoghan1232 commented Oct 13, 2023

zeeke commented Oct 13, 2023

adrianchiris commented Oct 15, 2023

Set MAC address after renaming the interface #280

Set MAC address after renaming the interface #280

Conversation

zeeke commented Sep 29, 2023

mlguerrero12 commented Oct 4, 2023

adrianchiris commented Oct 4, 2023

adrianchiris left a comment

Choose a reason for hiding this comment

zeeke commented Oct 4, 2023

zeeke commented Oct 12, 2023

mlguerrero12 commented Oct 12, 2023 • edited Loading

zeeke commented Oct 12, 2023

zeeke commented Oct 13, 2023

mlguerrero12 Oct 13, 2023

Choose a reason for hiding this comment

zeeke Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

mlguerrero12 Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

zeeke Oct 13, 2023

Choose a reason for hiding this comment

mlguerrero12 Oct 13, 2023

Choose a reason for hiding this comment

zeeke Oct 13, 2023

Choose a reason for hiding this comment

mlguerrero12 Oct 13, 2023

Choose a reason for hiding this comment

Eoghan1232 left a comment • edited Loading

Choose a reason for hiding this comment

zeeke commented Oct 13, 2023

Eoghan1232 commented Oct 13, 2023

zeeke commented Oct 13, 2023

adrianchiris commented Oct 15, 2023

mlguerrero12 commented Oct 12, 2023 •

edited

Loading

zeeke Oct 13, 2023 •

edited

Loading

mlguerrero12 Oct 13, 2023 •

edited

Loading

Eoghan1232 left a comment •

edited

Loading