Restore original VF state when needed #114

adrianchiris · 2020-02-23T12:38:50Z

Restore original VF value when needed

This commit attempts to sort out (to some extent)
how a VF configuration is restored.

Until today, almost all VF configurations (except MAC)
where restored to some hardcoded value.
These values may not be desired by the system administrator
or may be invalid for some NIC driver implementation.

As an example, The behaviour of A VF where spoofchk is enabled
and admin mac address is zero is not well defined and is left
for the vendor driver implementors to define the behaviour.
This may cause the NIC's embedded switch to block traffic
from the VF, hence a POD will not be able to send traffic on that VF.

Today, when a user does not specify MAC and Spoofchk values in network
configuration will run into the issue described above when the VF is
reused for a POD.

To fix the issue, described above we take a similar approach to
how Administrative MAC is saved and restored today.

This commit:
  1. Defines a new VF state struct which will save VF configurations
     to be used during configuration restoration which shall be saved
     together with the network confguration in cache.
  2. Moves AdminMAC, EffectiveMAC attributes to the new struct
  3. Saves the VF's state during CmdAdd flow
  4. Conditionally restores all values to their saved state during
     ResetVFConfig()
  5. Modifies order when restoring VF confguration to first restore
     spoofchk and then Administrative MAC
  6. Enhances documentation under Mellanox to clarify the spoofchk
     and mac dependency.

adrianchiris · 2020-02-23T12:44:30Z

pkg/sriov/sriov.go

+		if err = s.nLink.LinkSetVfSpoofchk(pfLink, conf.VFID, spoofchk_val); err != nil {
+			return fmt.Errorf("failed to restore spoof checking for vf %d to %s: %v", conf.VFID, conf.OrigSpoofChk, err)
+		}
+	}


we could add an "else" clause here and set spoofchk to false. However, this will not necessarily preserve the original state of the system.

zshi-redhat · 2020-02-25T09:28:41Z

pkg/sriov/sriov.go

+			conf.OrigSpoofChk = "off"
+		}
+
+		if spoofChk && conf.MAC == "" && origVfInfo.Mac.String() == zeroMac {


I think we may not want to enforce spoofchk=on + adminMac=all-zero rule here as this behavior may be different from driver to driver.
I'd suggest to just restore the spoofChk to its original value which shall work across drivers as driver would set the spoofchk and adminMac properly when VF first gets created.

It may also be worth documenting this behavior for user reference.

Seeing that Intel VFs are created with spoofchk on and all-zero admin mac it seems that behaviour of this state (and transitions to this state) is left to the interpretation of each vendor driver.
will remove the check

ahalimx86 · 2020-02-25T10:35:17Z

pkg/types/types.go

+	MinTxRate     *int   `json:"min_tx_rate"`            // Mbps, 0 = disable rate limiting
+	MaxTxRate     *int   `json:"max_tx_rate"`            // Mbps, 0 = disable rate limiting
+	SpoofChk      string `json:"spoofchk,omitempty"`     // on|off
+	OrigSpoofChk  string `json:"origSpoofchk,omitempty"` // on|off, represents the original spoofchk value in the system


I am getting concerned about littering all "non-user-configurable" parameters with corresponding json encoding fields so that we could save them on disk.
Ideally, we should only have fields with json tag if the value is expected from user. I was wondering if there's any better way of doing this.

This regards a more general problem, for this PR we deal with the issue of implicitly changing spoofchk value which was introduced in
020e04d
this causes pod traffic to fail on mellanox VFs. (as described in commit message)

ideally the VF's "restore" state should be in a separate cache entry (maybe different file/ different section in the file).
these values would still have json tags as the state will eventually end up as JSON to be consumed later by cmdDel

I agree with @ahalim-intel that we should not expose a parameter that's not configurable by user. This may better go into the conf file saved in data dir. Shall we address the VF state restoration in one issue, which would solve all the relevent open issues such as #113, #116 etc

I dont understand why this differs from NetConf.AdminMAC , NetConf.EffectiveMAC (These are not user configurable parameters) this patch takes the same approach to deal with what i consider a critical issue as Pod 2 Pod traffic will always fail on VFs from Mellanox NIC due to the fact that the original state of spoofchk is not preserved.

Now, don't get me wrong, i'm all for solving the general issue but it can be regarded as an enhancement (crucial one IMO) to the project that will take some time to agree on design, implementation etc.
This patch is proposed as a triage until #115 is implemented.

Also i'm not sure about backpoting policies for this project in case there was a release with this issue.

I think the idea of not having origSpoofChk in conf also applies to AdminMac and EffectiveMac, it's just that it was not explicitly called out. Maybe we shall put all the VF state related options which are not expected to be configured by user into a separate structure or as a sub-structure of conf so that CNI can cache all the VF states and restore them when cmdDel is called.

Regarding backporting, I think we don't have much request as such before so there is no established rules of how this should be done. but we're open to backport any fix to release branch where needed.

ahalimx86 · 2020-02-25T10:52:48Z

pkg/sriov/sriov.go

@@ -277,6 +279,11 @@ func (s *sriovManager) ApplyVFConfig(conf *sriovtypes.NetConf) error {
 		return fmt.Errorf("failed to lookup master %q: %v", conf.Master, err)
 	}

+	origVfInfo := getVfInfo(pfLink, conf.VFID)


We should try save and restore the original VF states from this. And on the ApplyVFConfig() should only try setting VF configurations that are given in the config. If any config not given let's not touch them.

If we can define a data structure for origVfInfo, then it would be easier to store the retrieved origVfInfo to the defined structure and cache it. This will save us from contional check from here

adrianchiris · 2020-03-10T08:04:25Z

recheck

adrianchiris · 2020-03-11T18:05:31Z

This PR has been modified to address the comments, it now stores the original VF state in a separate struct nested in netconf and conditionally restores all VF configuration values.

zshi-redhat · 2020-03-12T04:28:03Z

pkg/sriov/sriov.go

+	}
+
+	// Restore spoofchk
+	if conf.SpoofChk != "" {


If it restores only when spoofChk is configured, will this resolve the issue that spoofChk is flipped to on while releasing VF on mellanox VF?

Yes, it is assumed that the VF state prior to cmdAdd() call is a valid one for the NIC driver(so no spoofchk=on, MAC=00000...).
then if user chose to change the value, we will restore to the original state.

What if user doesn't specify the value of spoofChk, will it still be turned on automatically when released? (then next time it's used in other pod, traffic blocked)

Maybe im missing something, but who would turn on spoofchk automatically ?
the only entity who modifies these VF attributes should be SR-IOV CNI which, now, does that conditionally.

can you describe the scenario ?

ok, here is what I found on openshift deployment: when VF is released from pod, spoofChk is turned on automatically, the next time new pod get created with the same VF will have traffic blocked as it doesn't explicitly turn off the spoofChk.

ok, here is what I found on openshift deployment: when VF is released from pod, spoofChk is turned on automatically, the next time new pod get created with the same VF will have traffic blocked as it doesn't explicitly turn off the spoofChk.

Current SR-IOV CNI automatically sets it to on on cmdDel(), after this change it will not.
this PR does not address fixing a VF which has ended up in an invalid state (initial configuration is assumed to be valid).

lennybe · 2020-03-12T08:42:58Z

/retest

zshi-redhat · 2020-03-13T07:33:18Z

pkg/config/config.go

 	}

 	// validate that vlan id is set if vlan qos is configured
-	if n.Vlan == 0 && n.VlanQoS != 0 {
+	if (n.Vlan == nil || *n.Vlan == 0) && n.VlanQoS != nil {


I know it's rarely used, but shall we allow combined settings vlan == 0 and vlanQoS == 0?

IMO we should not (at least not in this PR) ill modify the condition to:

if (n.Vlan == nil || *n.Vlan == 0) && (n.VlanQos != nil && *n.VlanQos != 0) {...}

On second thought, i don't think we need to prevent it.

Changing Vlan and VlanQos to a pointer allows us to know if the user specified a value.
so in my opinion we should enforce:

Vlan must be specified by the user if VlanQos is specified

if VlanQos is specified and is a non-zero value then Vlan must be a non zero value

i don't think we should prevent vlan == 0 && vlanQos == 0

This commit attempts to sort out (to some extent) how a VF configuration is restored. Until today, almost all VF configurations (except MAC) where restored to some hardcoded value. These values may not be desired by the system administrator or may be invalid for some NIC driver implementation. As an example, The behaviour of A VF where spoofchk is enabled and admin mac address is zero is not well defined and is left for the vendor driver implementors to define the behaviour. This may cause the NIC's embedded switch to block traffic from the VF, hence a POD will not be able to send traffic on that VF. Today, when a user does not specify MAC and Spoofchk values in network configuration will run into the issue described above when the VF is reused for a POD. To fix the issue, described above we take a similar approach to how Administrative MAC is saved and restored today. This commit: 1. Defines a new VF state struct which will save VF configurations to be used during configuration restoration which shall be saved together with the network confguration in cache. 2. Moves AdminMAC, EffectiveMAC attributes to the new struct 3. Saves the VF's state during CmdAdd flow 4. Conditionally restores all values to their saved state during ResetVFConfig() 5. Modifies order when restoring VF confguration to first restore spoofchk and then Administrative MAC 6. Enhances documentation under Mellanox to clarify the spoofchk and mac dependency.

lennybe · 2020-03-16T14:39:46Z

/retest

adrianchiris · 2020-03-16T16:16:37Z

PR update : allow user to specify the following configuration: vlan == 0 && vlanQos == 0

adrianchiris · 2020-03-23T09:20:56Z

@zshi-redhat @ahalim-intel inputs appreciated :)

zshi-redhat · 2020-03-24T06:18:10Z

@zshi-redhat @ahalim-intel inputs appreciated :)

I just got chance to test this PR, it worked fine. Thanks for working on it!

carmark · 2020-03-31T09:32:04Z

works well also on my local environment. Without this patch, I may encounter the panic issue, such as :

Mar 31 17:04:47 k8s-worker-4 kubelet[13739]: panic: runtime error: invalid memory address or nil pointer dereference
Mar 31 17:04:47 k8s-worker-4 kubelet[13739]: [signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x543768]
Mar 31 17:04:47 k8s-worker-4 kubelet[13739]: goroutine 1 [running, locked to thread]:
Mar 31 17:04:47 k8s-worker-4 kubelet[13739]: github.com/intel/sriov-cni/pkg/sriov.(*sriovManager).SetupVF(0xc00000c720, 0xc000145040, 0xc00001401b, 0x3, 0xc00001c010, 0x40, 0x5e0e20, 0xc000010a70, 0xc00018000
Mar 31 17:04:47 k8s-worker-4 kubelet[13739]: /root/go/src/github.com/intel/sriov-cni/.gopath/src/github.com/intel/sriov-cni/pkg/sriov/sriov.go:155 +0x98
Mar 31 17:04:47 k8s-worker-4 kubelet[13739]: main.cmdAdd(0xc0001644d0, 0x0, 0x0)
Mar 31 17:04:47 k8s-worker-4 kubelet[13739]: /root/go/src/github.com/intel/sriov-cni/.gopath/src/github.com/intel/sriov-cni/cmd/sriov/main.go:96 +0x7a0
Mar 31 17:04:47 k8s-worker-4 kubelet[13739]: github.com/intel/sriov-cni/vendor/github.com/containernetworking/cni/pkg/skel.(*dispatcher).checkVersionAndCall(0xc00011bec8, 0xc0001644d0, 0x5df660, 0xc000138570,
Mar 31 17:04:47 k8s-worker-4 kubelet[13739]: /root/go/src/github.com/intel/sriov-cni/.gopath/src/github.com/intel/sriov-cni/vendor/github.com/containernetworking/cni/pkg/skel/skel.go:185 +0x258
Mar 31 17:04:47 k8s-worker-4 kubelet[13739]: github.com/intel/sriov-cni/vendor/github.com/containernetworking/cni/pkg/skel.(*dispatcher).pluginMain(0xc00011bec8, 0x5b7aa0, 0x5b7aa8, 0x5b7ab8, 0x5df660, 0xc000
Mar 31 17:04:47 k8s-worker-4 kubelet[13739]: /root/go/src/github.com/intel/sriov-cni/.gopath/src/github.com/intel/sriov-cni/vendor/github.com/containernetworking/cni/pkg/skel/skel.go:221 +0x546
Mar 31 17:04:47 k8s-worker-4 kubelet[13739]: github.com/intel/sriov-cni/vendor/github.com/containernetworking/cni/pkg/skel.PluginMainWithError(...)
Mar 31 17:04:47 k8s-worker-4 kubelet[13739]: /root/go/src/github.com/intel/sriov-cni/.gopath/src/github.com/intel/sriov-cni/vendor/github.com/containernetworking/cni/pkg/skel/skel.go:286
Mar 31 17:04:47 k8s-worker-4 kubelet[13739]: github.com/intel/sriov-cni/vendor/github.com/containernetworking/cni/pkg/skel.PluginMain(0x5b7aa0, 0x5b7aa8, 0x5b7ab8, 0x5df660, 0xc000138570, 0x0, 0x0)
Mar 31 17:04:47 k8s-worker-4 kubelet[13739]: /root/go/src/github.com/intel/sriov-cni/.gopath/src/github.com/intel/sriov-cni/vendor/github.com/containernetworking/cni/pkg/skel/skel.go:301 +0x128

ahalimx86 · 2020-04-01T15:58:27Z

/lgtm
I will wait for others feedback as well.

moshe010 · 2020-04-07T03:17:39Z

can we merged this please?

killianmuldoon · 2020-04-07T11:08:09Z

I tested this across a number of nics and drivers and it's working as described. /LGTM

moshe010 · 2020-04-07T11:23:25Z

thanks :)

moshe010 · 2020-04-08T06:02:46Z

@zshi-redhat, Do we need to backport this for openshift?

zshi-redhat · 2020-04-08T08:32:19Z

@zshi-redhat, Do we need to backport this for openshift?

@moshe010 this is cherry-picked as part of regular update: openshift/sriov-cni#14

This commit attempts to sort out (to some extent) how a VF configuration is restored. Until today, almost all VF configurations (except MAC) where restored to some hardcoded value. These values may not be desired by the system administrator or may be invalid for some NIC driver implementation. As an example, The behaviour of A VF where spoofchk is enabled and admin mac address is zero is not well defined and is left for the vendor driver implementors to define the behaviour. This may cause the NIC's embedded switch to block traffic from the VF, hence a POD will not be able to send traffic on that VF. Today, when a user does not specify MAC and Spoofchk values in network configuration will run into the issue described above when the VF is reused for a POD. To fix the issue, described above we take a similar approach to how Administrative MAC is saved and restored today. This commit: 1. Defines a new VF state struct which will save VF configurations to be used during configuration restoration which shall be saved together with the network confguration in cache. 2. Moves AdminMAC, EffectiveMAC attributes to the new struct 3. Saves the VF's state during CmdAdd flow 4. Conditionally restores all values to their saved state during ResetVFConfig() 5. Modifies order when restoring VF confguration to first restore spoofchk and then Administrative MAC 6. Enhances documentation under Mellanox to clarify the spoofchk and mac dependency.

…ster Merge https://github.com/k8snetworkplumbingwg/sriov-cni:master into master

adrianchiris requested a review from zshi-redhat February 23, 2020 12:38

adrianchiris commented Feb 23, 2020

View reviewed changes

adrianchiris force-pushed the vf-config branch from a495922 to 2a3a55e Compare February 24, 2020 07:32

adrianchiris requested a review from ahalimx86 February 25, 2020 08:14

adrianchiris mentioned this pull request Feb 25, 2020

Clean up any stale configuration on a VF before assigning #113

Closed

zshi-redhat reviewed Feb 25, 2020

View reviewed changes

ahalimx86 reviewed Feb 25, 2020

View reviewed changes

adrianchiris force-pushed the vf-config branch 2 times, most recently from 6583666 to d85544e Compare February 25, 2020 12:19

adrianchiris mentioned this pull request Feb 25, 2020

SR-IOV CNI to properly restore the state of a VF #115

Closed

adrianchiris force-pushed the vf-config branch from d85544e to 599b0bb Compare March 11, 2020 17:56

adrianchiris changed the title ~~Restore original spoofchk value only when needed~~ Restore original VF state when needed Mar 11, 2020

zshi-redhat reviewed Mar 12, 2020

View reviewed changes

zshi-redhat mentioned this pull request Mar 13, 2020

VF configured/attached with unsupported operations in net-attach-def doesn't gets de-configured on pod deletion #119

Closed

zshi-redhat reviewed Mar 13, 2020

View reviewed changes

adrianchiris force-pushed the vf-config branch from 599b0bb to 8eb44fa Compare March 16, 2020 13:36

ahalimx86 merged commit e051689 into k8snetworkplumbingwg:master Apr 7, 2020

zeeke pushed a commit to zeeke/sriov-cni that referenced this pull request Jan 13, 2025

Merge pull request k8snetworkplumbingwg#114 from SchSeba/merge-bot-ma…

98ccb73

…ster Merge https://github.com/k8snetworkplumbingwg/sriov-cni:master into master

Restore original VF state when needed #114

Restore original VF state when needed #114

Conversation

adrianchiris commented Feb 23, 2020 • edited Loading

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 Feb 25, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris commented Mar 10, 2020

adrianchiris commented Mar 11, 2020

zshi-redhat Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

lennybe commented Mar 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lennybe commented Mar 16, 2020

adrianchiris commented Mar 16, 2020

adrianchiris commented Mar 23, 2020

zshi-redhat commented Mar 24, 2020

carmark commented Mar 31, 2020

ahalimx86 commented Apr 1, 2020

moshe010 commented Apr 7, 2020

killianmuldoon commented Apr 7, 2020

moshe010 commented Apr 7, 2020

moshe010 commented Apr 8, 2020

zshi-redhat commented Apr 8, 2020

adrianchiris commented Feb 23, 2020 •

edited

Loading

adrianchiris Feb 25, 2020 •

edited

Loading

zshi-redhat Mar 12, 2020 •

edited

Loading

adrianchiris Mar 12, 2020 •

edited

Loading

adrianchiris Mar 12, 2020 •

edited

Loading