Add both hardware and nic mac allocation retry #239

SchSeba · 2023-01-30T13:28:34Z

This commit add a retry function for all the mac address allocation functions.

this is needed because some of the drivers have async mac allocation or get a device or resouce busy errors until the vf device is fully configured on the system so instead of failing the CNI we just wait 100 miliseconds

Signed-off-by: Sebastian Sch sebassch@gmail.com

cgoncalves · 2023-01-30T13:57:29Z

pkg/sriov/sriov.go

-			if err = s.nLink.LinkSetHardwareAddr(linkObj, hwaddr); err != nil {
-				return fmt.Errorf("failed to restore original effective netlink MAC address %s: %v", hwaddr, err)
-			}
+			err = utils.Retry(5, 200*time.Millisecond, func() error {


Should the CNI at least output a warning message if this fails?

we return error sorry I miss that :)

coveralls · 2023-01-31T13:20:50Z

Pull Request Test Coverage Report for Build 4607738372

19 of 32 (59.38%) changed or added relevant lines in 3 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.2%) to 37.838%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/sriov/sriov.go	5	7	71.43%
pkg/utils/utils.go	12	23	52.17%

Files with Coverage Reduction	New Missed Lines	%
pkg/sriov/sriov.go	1	36.64%

Totals
Change from base Build 4284585998:	-0.2%
Covered Lines:	420
Relevant Lines:	1110

💛 - Coveralls

cgoncalves · 2023-01-31T13:37:25Z

/lgtm

pkg/sriov/sriov.go

adrianchiris · 2023-02-08T09:05:58Z

pkg/sriov/sriov.go

 		// reset effective MAC address
 		if conf.MAC != "" {
 			hwaddr, err := net.ParseMAC(conf.OrigVfState.EffectiveMAC)
 			if err != nil {
 				return fmt.Errorf("failed to parse original effective MAC address %s: %v", conf.OrigVfState.EffectiveMAC, err)
 			}

-			if err = s.nLink.LinkSetHardwareAddr(linkObj, hwaddr); err != nil {
+			err = utils.Retry(5, 200*time.Millisecond, func() error {


should we try to do things in reverse(ish) of setupVF ?

that is:

bring interface down

restore mac

set netdev orig name

move ifc back to host ns

WDYT ?

this way you may not need the extra call to LinkByName

In addition, can you add same comment for the Retry call as in SetupVF ?

I change the solution a bit when you have time please take a look.

in general I prefer to leave the rename before because if for some reason, in any case, we failed to reset the mac address the container runtime will remove the network namespace and the VF will go back to the node with the pod nic name.
how it's today it will always have the right vf name even if for some driver issues we are not able to recover the right mac address.

in general I prefer to leave the rename before because if for some reason, in any case, we failed to reset the mac address the container runtime will remove the network namespace and the VF will go back to the node with the pod nic name.
how it's today it will always have the right vf name even if for some driver issues we are not able to recover the right mac address.

ok

adrianchiris · 2023-02-26T12:05:00Z

pkg/sriov/sriov.go

+			   so to cover both cases we will retry the set function
+			   and also check the mac address on the VF after the configuration.
+			*/
+			err = utils.Retry(10, 100*time.Millisecond, func() error {


we have two places where we set the MAC netdev mac address (here and in L104)

shall we use the same logic in both ? it seems the "fail silently" part may happen on set as well WDYT ?

that is just have a : setVFEffectiveMAC(netdevName, MAC) and call it with utils.Retry

done can you please have a look?

SchSeba · 2023-04-04T12:32:36Z

@adrianchiris all green now please take a look when you have some time :)

adrianchiris · 2023-04-24T06:42:44Z

/test-all

pkg/sriov/sriov.go

adrianchiris · 2023-04-24T08:51:41Z

pkg/sriov/sriov.go

-			if err = s.nLink.LinkSetHardwareAddr(linkObj, hwaddr); err != nil {
-				return fmt.Errorf("failed to restore original effective netlink MAC address %s: %v", hwaddr, err)
-			}
+		err = utils.SetVFEffectiveMAC(s.nLink, conf.OrigVfState.HostIFName, conf.OrigVfState.EffectiveMAC)


why is this bit not conditioned by conf.MAC as before ?

note that conf.OrigVfState.EffectiveMAC is saved at L88 and is conditioned by conf.MAC

we don't need it as we always have a mac address it can be the original one or the one saved in line 88

from main.go

err = sm.FillOriginalVfInfo(netConf)

FillOriginalVfInfo does not save effective MAC, see [1][2]

also in case conf.MAC was not specified i dont think we need to restore it as we did not set it to begin with.
my POV here is why add un-needed steps that can theoretically fail.

[1]

sriov-cni/pkg/sriov/sriov.go

Line 304 in 77c66ac

func (s *sriovManager) FillOriginalVfInfo(conf *sriovtypes.NetConf) error {

[2]

sriov-cni/pkg/types/types.go

Line 22 in 77c66ac

func (vs *VfState) FillFromVfInfo(info *netlink.VfInfo) {

you are right nice catch!

SchSeba · 2023-05-02T08:30:38Z

Hi @adrianchiris let me know if the changes are good now. thanks!

adrianchiris

a couple of comments, otherwise we are good 2 go.

pkg/utils/utils.go

adrianchiris · 2023-05-16T14:36:01Z

pkg/sriov/sriov.go

-			if err = s.nLink.LinkSetHardwareAddr(linkObj, hwaddr); err != nil {
-				return fmt.Errorf("failed to restore original effective netlink MAC address %s: %v", hwaddr, err)
-			}
+		err = utils.SetVFEffectiveMAC(s.nLink, conf.OrigVfState.HostIFName, conf.OrigVfState.EffectiveMAC)


FillOriginalVfInfo does not save effective MAC, see [1][2]

also in case conf.MAC was not specified i dont think we need to restore it as we did not set it to begin with.
my POV here is why add un-needed steps that can theoretically fail.

[1]

sriov-cni/pkg/sriov/sriov.go

Line 304 in 77c66ac

func (s *sriovManager) FillOriginalVfInfo(conf *sriovtypes.NetConf) error {

[2]

sriov-cni/pkg/types/types.go

Line 22 in 77c66ac

func (vs *VfState) FillFromVfInfo(info *netlink.VfInfo) {

SchSeba · 2023-05-22T11:41:09Z

@adrianchiris done :)

pkg/utils/utils.go

korroot · 2023-05-24T09:17:31Z

Hi @SchSeba
I have some question regarding this PR, when using SRIOV + vfio-pci driver, VPP sets its own MAC on VF inside the container. I see this error message in dmesg:

[23544.190430] i40e 0000:06:00.2: MAC addr e2:0d:d2:15:f5:78 has been set by PF, cannot delete it for VF 8, reset VF to change MAC addr
[23544.190468] i40e 0000:06:00.2: VF 8 failed opcode 11, retval: -5

The network card is Intel x710.

To workaround the above warning message, a manual reset of VF MAC is done on node:

  for vf in $(ip link show dev "$nic"|grep 'vf '|awk '{print $2}'); do
    ip link set "$nic" vf "$vf" mac 00:00:00:00:00:00
  done

I wonder if this PR should set VF MAC to 00:00:00:00:00:00 when SRIOV-CNI assigns the VF to pod?

SchSeba · 2023-05-24T10:58:21Z

Hi @korroot there is another solution that I think is better.

just enable in the sriov-cni the trust:on it will allow the vpp application to control the administrative mac address

korroot · 2023-05-24T11:00:36Z

vf 14     link/ether 5a:6e:ac:6e:65:fb brd ff:ff:ff:ff:ff:ff, spoof checking off, link-state auto, trust on

I have trust on and spoof checking off in NAD.

mlguerrero12 · 2023-05-24T11:01:00Z

pkg/sriov/sriov_test.go

@@ -1,6 +1,7 @@
 package sriov

 import (
+	"github.com/k8snetworkplumbingwg/sriov-cni/pkg/utils"


nit: perhaps it goes better with the other github imports

adrianchiris · 2023-05-24T10:57:59Z

pkg/utils/utils.go

+
+		linkObj, err := netLinkManager.LinkByName(netDeviceName)
+		if err != nil {
+			return fmt.Errorf("failed to get netlink device with name %s: %q", linkObj.Attrs().Name, err)


i would not reference linkObj if err occurred.

you can use another name for the local var, then you can still use linkObj

right I add a orgLinkObj that will never change even if there was an error in the LinkByName functions

pkg/utils/utils.go

mlguerrero12 · 2023-05-24T11:13:41Z

@SchSeba, there is a retry in the sriov.go file line 359 that in my view, shouldn't be there. The retry is intended for LinkSetHardwareAddr not for LinkSetVfHardwareAddr. I think that it was mistankely added the other way around (ReleaseVF and ResetVFConfig).

Do you agree? Should we remove it in this PR as well?

SchSeba · 2023-05-24T12:19:03Z

Hi @mlguerrero12 thanks for the comment that retry was to fix another issue not related to this one where the mac address change in the vf administrative mac address failed

SchSeba · 2023-05-24T12:21:10Z

Hi @korroot this sounds like a bug in the driver as using trust: on should allow the vf to change the mac address.
this is done also inside VMs when we allocate the vfio device

can you try to check a newer kernel version maybe??

mlguerrero12 · 2023-05-24T12:51:05Z

It seems a bit odd that a previous PR added the retry in the add command for "LinkSetHardwareAddr" and in the del command for "LinkSetVfHardwareAddr". Both have the same comment which it seems to indicate issues with setting the netdev VF MAC address (LinkSetHardwareAddr function).

Now, in this PR, you are adding the retry in the del command for "LinkSetHardwareAddr" which was missing. So, the only one that doesn't have the retry now is in the add command for "LinkSetVfHardwareAddr".

What am I missing?

c.c. @cgoncalves

mlguerrero12 · 2023-05-24T17:19:59Z

Just to explain myself better.

First PR was meant to solve the issue while setting the netdev VF MAC address (LinkSetHardwareAddr) after setting the MAC via PF (LinkSetVfHardwareAddr).

Add command (ApplyVFConfig and SetupVF functions)

LinkSetVfHardwareAddr - No retry
LinkSetHardwareAddr - Retry

Del Command (ResetVFConfig and ReleaseVF functions)

LinkSetVfHardwareAddr - Retry - shouldn't be according to the comment
LinkSetHardwareAddr - No retry - Missing

This PR adds the missing retry for del command LinkSetHardwareAddr. So now we have.

Add command (ApplyVFConfig and SetupVF functions)

LinkSetVfHardwareAddr - No retry
LinkSetHardwareAddr - Retry

Del Command (ResetVFConfig and ReleaseVF functions)

LinkSetVfHardwareAddr - Retry - shouldn't be according to the comment
LinkSetHardwareAddr - Retry - Not longer missing.

So, we either delete the retry for del command LinkSetVfHardwareAddr or add it for add command LinkSetVfHardwareAddr to have consistency.

Please correct me if I'm seeing things wrong.

SchSeba · 2023-05-25T08:21:01Z

Thanks for the amazing review @mlguerrero12 !

I will add it also for the add so we consist with all the others :)

ivan4th · 2023-05-25T08:44:34Z

@SchSeba we had a problem with MAC address setting on this setup:

root@worker-2:/# root@worker-2:/# uname -a
Linux worker-2 5.4.0-148-generic #165-Ubuntu SMP Tue Apr 18 08:53:12 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
root@worker-2:/# cat /etc/lsb-release
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=20.04
DISTRIB_CODENAME=focal
DISTRIB_DESCRIPTION="Ubuntu 20.04.4 LTS"

The errors in dmesg looked like this:

[4618379.295961] i40e 0000:06:00.0: Setting VLAN 1052, QOS 0x0 on VF 21
[4618379.399152] i40e 0000:06:00.0: VF 21 is now trusted
[4618387.462708] vfio-pci 0000:06:04.5: enabling device (0000 -> 0002)
[4618389.933770] i40e 0000:06:00.0: MAC addr 72:df:95:0c:e1:04 has been set by PF, cannot delete it for VF 21, reset VF to change MAC addr
[4618389.936110] i40e 0000:06:00.0: VF 21 failed opcode 11, retval: -5
[4618390.582407] i40e 0000:06:00.0: VF 21 successfully set multicast promiscuous mode

As you can see, the device is set to be trusted but this doesn't prevent the error from happening.
The effect of this was that VPP (with DPDK driver) was unable to set the MAC address, in effect, VPP and the NIC VF had different ideas of the MAC address, resulting in connectivity issues.
Resetting the VF by setting the MAC address to all zeros via ip link set "$nic" vf "$vf" mac 00:00:00:00:00:00 before the VF got passed to the pod did work as a workaround for this situation.

SchSeba · 2023-05-25T09:05:36Z

Hi @ivan4th thanks for the update!

you are right about putting the mac address under the PF to be 00:00:00:00:00:00 will disable any mac validation (it may be a security issue)

another possible solution is if you know what is going to be the mac address that vpp will allocate you can configure it in the pod definition this way it will match.

or you can also do it the other way around to tell vpp to use the mac address allocated by this sriov-cni if it's possible.

The last option will be to expose the option to have mac address and adminMacAddress in the network attachment for the sriov-cni this way you will be able to configure the 00:00:00:00:00:00 for the admin mac address.

SchSeba · 2023-05-25T09:24:52Z

@ivan4th I put it on the next community meeting agenda https://docs.google.com/document/d/1sJQMHbxZdeYJPgAWK1aSt6yzZ4K_8es7woVIrwinVwI/edit#

if you can join to talk about the use-case it will be good :)

ivan4th · 2023-05-25T10:03:34Z

@SchSeba thanks, I will join the meeting. Our use case is an in-house VPP-based network layer for Kubernetes, in some ways resembling but also rather unlike Calico-VPP, and the assumption was that VPP has full control over the VFs it gets, including setting the MAC address according to one configured in some CRs of our own (these correspond to VPP's idea of network interfaces)

SchSeba · 2023-05-25T11:07:42Z

@mlguerrero12 can you take another look?

mlguerrero12 · 2023-05-25T12:31:05Z

My only remark is about the comment "Some NIC drivers (i.e. i40e/iavf) set VF MAC ......". Please delete it in the retries for LinkSetVfHardwareAddr and add it to SetVFEffectiveMAC function.

That comment is for LinkSetHardwareAddr not LinkSetVfHardwareAddr.

SchSeba · 2023-05-28T09:38:10Z

@mlguerrero12 done :)

adrianchiris · 2023-05-28T10:30:20Z

/test-all

adrianchiris

This PR has evolved since its conception.

it does not deal only with MAC reset on cmd DEL.

it affects how we set admin and effective mac on both ADD and DEL flows.
commit msg (and PR topic) should be updated to reflect this.

this will also help get reviewers in the proper mindset when reviewing this.

adrianchiris · 2023-05-28T10:33:14Z

pkg/utils/utils.go

+		return fmt.Errorf("failed to parse MAC address %s: %v", macAddress, err)
+	}
+
+	OrgLinkObj, err := netLinkManager.LinkByName(netDeviceName)


nit: O -> o

adrianchiris · 2023-05-28T10:35:38Z

pkg/utils/utils.go

@@ -284,6 +284,51 @@ func CleanCachedNetConf(cRefPath string) error {
 	return nil
 }

+/*
+	SetVFEffectiveMAC will try to set the mac address on a specific VF interface


indentation looks odd for the first line. should it be just a single space ?

when you use /* for a function title it put the first line like that :)
I check it to //

adrianchiris · 2023-05-28T10:45:14Z

pkg/sriov/sriov.go

@@ -231,7 +210,10 @@ func (s *sriovManager) ApplyVFConfig(conf *sriovtypes.NetConf) error {
 			return fmt.Errorf("failed to parse MAC address %s: %v", conf.MAC, err)
 		}

-		if err = s.nLink.LinkSetVfHardwareAddr(pfLink, conf.VFID, hwaddr); err != nil {
+		err = utils.Retry(5, 200*time.Millisecond, func() error {


Thats a new one since i last reviewed (i think .. :) )

I assume its to align with how we do things in ResetVFConfig.

its unclear why we are retrying here... (and also after removing the comment at L350 also in resetVFConfig)

either:

define a wrapper func "SetVFHardwareAddress" in utils which will do this and add comment to that method. (like u did with setting effective mac)

add comments here and below to explain why its being retired.

it was to be equal with the other functions base on this comment #239 (comment)

I create the function and also add unit tests for that new function

I add a small comment

SchSeba · 2023-05-29T09:59:41Z

I hope this time the PR is really done :)

This commit add a retry function for all the mac address allocation functions. this is needed because some of the drivers have async mac allocation or get a device or resouce busy errors until the vf device is fully configured on the system so instead of failing the CNI we just wait 100 miliseconds Signed-off-by: Sebastian Sch <sebassch@gmail.com>

mlguerrero12 · 2023-05-30T11:51:47Z

pkg/utils/utils.go

+		if err != nil {
+			return fmt.Errorf("failed to get netlink device with name %s: %q", orgLinkObj.Attrs().Name, err)
+		}
+		if linkObj.Attrs().Vfs[vfID].Mac.String() != macAddress {


I don't think we can safely assume that Vfs[vfID] will always exist during the del command. The config is read from the cache and the vf might not be present. This will panic.

there is no way we use the vfID many times before we call this function in the ResetVFConfig

if you don't set vlan or spoofchk, it is possible

that is right but today we have the assumption that the cache is configured as expected.
but I agree we can have a check in the cmfDel before we call any function that we have all the stuff needed before we call the release one.

we can have a follow-up PR for it if that sounds good to you :)

mlguerrero12 · 2023-05-30T13:23:21Z

LGTM

adrianchiris · 2023-06-05T13:39:14Z

LGTM, @SchSeba please add an issue to followup on to check vfid exists as stated here: #239 (comment)

feel free to merge this one once created.

SchSeba · 2023-06-08T11:25:43Z

created #271

cgoncalves suggested changes Jan 30, 2023

View reviewed changes

SchSeba force-pushed the fix_release_vf branch from 64567bd to 06aef2e Compare January 31, 2023 13:19

SchSeba mentioned this pull request Feb 2, 2023

expose mac address as part of networks-status in pod yaml #240

Merged

adrianchiris reviewed Feb 7, 2023

View reviewed changes

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

adrianchiris reviewed Feb 8, 2023

View reviewed changes

SchSeba force-pushed the fix_release_vf branch 2 times, most recently from a4f2384 to 3cd9071 Compare February 21, 2023 14:26

adrianchiris reviewed Feb 26, 2023

View reviewed changes

SchSeba force-pushed the fix_release_vf branch 2 times, most recently from 461ce7b to a43748c Compare April 4, 2023 11:55

adrianchiris reviewed Apr 24, 2023

View reviewed changes

SchSeba force-pushed the fix_release_vf branch 2 times, most recently from ff3bd14 to ed3500f Compare May 1, 2023 12:11

adrianchiris reviewed May 16, 2023

View reviewed changes

SchSeba force-pushed the fix_release_vf branch from ed3500f to 4aa7b58 Compare May 22, 2023 09:42

mlguerrero12 reviewed May 23, 2023

View reviewed changes

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

mlguerrero12 reviewed May 23, 2023

View reviewed changes

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

SchSeba force-pushed the fix_release_vf branch from 4aa7b58 to 96cff89 Compare May 24, 2023 10:57

mlguerrero12 reviewed May 24, 2023

View reviewed changes

adrianchiris reviewed May 24, 2023

View reviewed changes

SchSeba force-pushed the fix_release_vf branch from 96cff89 to 32d6518 Compare May 24, 2023 12:16

SchSeba force-pushed the fix_release_vf branch from 32d6518 to 728125d Compare May 25, 2023 08:24

SchSeba force-pushed the fix_release_vf branch from 728125d to 7835c26 Compare May 28, 2023 09:38

adrianchiris reviewed May 28, 2023

View reviewed changes

SchSeba force-pushed the fix_release_vf branch from 7835c26 to 11a7aba Compare May 29, 2023 09:59

SchSeba force-pushed the fix_release_vf branch from 11a7aba to 6890d00 Compare May 29, 2023 11:13

SchSeba force-pushed the fix_release_vf branch from 6890d00 to 04bd07c Compare May 29, 2023 14:04

SchSeba changed the title ~~fix sriov-vf release mac reset~~ Add both hardware and nic mac allocation retry May 29, 2023

mlguerrero12 reviewed May 30, 2023

View reviewed changes

adrianchiris approved these changes Jun 5, 2023

View reviewed changes

SchSeba mentioned this pull request Jun 8, 2023

Validate cache file exist before cmdDel procedure #271

Closed

SchSeba merged commit 4e6eab8 into k8snetworkplumbingwg:master Jun 8, 2023

Add both hardware and nic mac allocation retry #239

Add both hardware and nic mac allocation retry #239

Conversation

SchSeba commented Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 31, 2023 • edited Loading

Pull Request Test Coverage Report for Build 4607738372

💛 - Coveralls

cgoncalves commented Jan 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris Feb 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SchSeba commented Apr 4, 2023

adrianchiris commented Apr 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris May 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SchSeba commented May 2, 2023

adrianchiris left a comment

Choose a reason for hiding this comment

adrianchiris May 16, 2023 • edited Loading

Choose a reason for hiding this comment

SchSeba commented May 22, 2023

korroot commented May 24, 2023

SchSeba commented May 24, 2023

korroot commented May 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlguerrero12 commented May 24, 2023

SchSeba commented May 24, 2023

SchSeba commented May 24, 2023

mlguerrero12 commented May 24, 2023

mlguerrero12 commented May 24, 2023 • edited Loading

SchSeba commented May 25, 2023

ivan4th commented May 25, 2023 • edited Loading

SchSeba commented May 25, 2023

SchSeba commented May 25, 2023

ivan4th commented May 25, 2023

SchSeba commented May 25, 2023

mlguerrero12 commented May 25, 2023

SchSeba commented May 28, 2023

adrianchiris commented May 28, 2023

adrianchiris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris May 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SchSeba commented May 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlguerrero12 commented May 30, 2023

adrianchiris commented Jun 5, 2023 • edited Loading

SchSeba commented Jun 8, 2023

SchSeba commented Jan 30, 2023 •

edited

Loading

coveralls commented Jan 31, 2023 •

edited

Loading

adrianchiris Feb 26, 2023 •

edited

Loading

adrianchiris May 16, 2023 •

edited

Loading

adrianchiris May 16, 2023 •

edited

Loading

mlguerrero12 commented May 24, 2023 •

edited

Loading

ivan4th commented May 25, 2023 •

edited

Loading

adrianchiris May 28, 2023 •

edited

Loading

adrianchiris commented Jun 5, 2023 •

edited

Loading