Set MAC address after renaming the interface

[zeeke]

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).

[mlguerrero12]

LGTM

[adrianchiris]

@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

[zeeke]

@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]

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]

Nice work @zeeke!

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

[zeeke]

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]

@Eoghan1232 can you please take a look at this?

[zeeke]

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]

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]

PR got 2 LGTMs from maintainers, merging.

[adrianchiris]

Thx for the detailed information @zeeke !