Skip to content

Commit

Permalink
Restore original VF value when needed (#114)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@adrianchiris
adrianchiris authored Apr 7, 2020
1 parent 1d07d00 commit e051689
Show file tree
Hide file tree
Showing 5 changed files with 285 additions and 101 deletions.
2 changes: 2 additions & 0 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ echo 0 > /sys/class/net/enp2s0f0/device/sriov_numvfs
echo 8 > /sys/class/net/enp2s0f0/device/sriov_numvfs
```

Note: In case spoofchk is enabled for VF, a valid administrative MAC needs to be specified.

## Configuration reference
### Main parameters
* `name` (string, required): the name of the network
Expand Down
29 changes: 19 additions & 10 deletions pkg/config/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,26 +48,35 @@ func LoadConf(bytes []byte) (*sriovtypes.NetConf, error) {
}

if hostIFNames != "" {
n.HostIFNames = hostIFNames
n.OrigVfState.HostIFName = hostIFNames
}

if hostIFNames == "" && !n.DPDKMode {
return nil, fmt.Errorf("LoadConf(): the VF %s does not have a interface name or a dpdk driver", n.DeviceID)
}

// validate vlan id range
if n.Vlan < 0 || n.Vlan > 4094 {
return nil, fmt.Errorf("LoadConf(): vlan id %d invalid: value must be in the range 0-4094", n.Vlan)
if n.Vlan != nil {
// validate vlan id range
if *n.Vlan < 0 || *n.Vlan > 4094 {
return nil, fmt.Errorf("LoadConf(): vlan id %d invalid: value must be in the range 0-4094", *n.Vlan)
}
}

if n.VlanQoS != nil {
// validate that VLAN QoS is in the 0-7 range
if *n.VlanQoS < 0 || *n.VlanQoS > 7 {
return nil, fmt.Errorf("LoadConf(): vlan QoS PCP %d invalid: value must be in the range 0-7", *n.VlanQoS)
}
}

// validate that vlan id is set if vlan qos is configured
if n.Vlan == 0 && n.VlanQoS != 0 {
return nil, fmt.Errorf("LoadConf(): non-zero vlan id must be configured to set vlan QoS")
// validate that vlan id is set if vlan qos is set
if n.VlanQoS != nil && n.Vlan == nil {
return nil, fmt.Errorf(("LoadConf(): vlan id must be configured to set vlan QoS"))
}

// validate that VLAN QoS is in the 0-7 range
if n.VlanQoS < 0 || n.VlanQoS > 7 {
return nil, fmt.Errorf("LoadConf(): vlan QoS PCP %d invalid: value must be in the range 0-7", n.VlanQoS)
// validate non-zero value for vlan id if vlan qos is set to a non-zero value
if (n.VlanQoS != nil && *n.VlanQoS != 0) && *n.Vlan == 0 {
return nil, fmt.Errorf("LoadConf(): non-zero vlan id must be configured to set vlan QoS to a non-zero value")
}

// validate that link state is one of supported values
Expand Down
99 changes: 53 additions & 46 deletions pkg/sriov/sriov.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ func NewSriovManager() Manager {

// SetupVF sets up a VF in Pod netns
func (s *sriovManager) SetupVF(conf *sriovtypes.NetConf, podifName string, cid string, netns ns.NetNS) (string, error) {
linkName := conf.HostIFNames
linkName := conf.OrigVfState.HostIFName

linkObj, err := s.nLink.LinkByName(linkName)
if err != nil {
Expand Down Expand Up @@ -172,7 +172,7 @@ func (s *sriovManager) SetupVF(conf *sriovtypes.NetConf, podifName string, cid s
}

// Save the original effective MAC address before overriding it
conf.EffectiveMAC = linkObj.Attrs().HardwareAddr.String()
conf.OrigVfState.EffectiveMAC = linkObj.Attrs().HardwareAddr.String()

if err = s.nLink.LinkSetHardwareAddr(linkObj, hwaddr); err != nil {
return "", fmt.Errorf("failed to set netlink MAC address to %s: %v", hwaddr, err)
Expand Down Expand Up @@ -215,8 +215,8 @@ func (s *sriovManager) ReleaseVF(conf *sriovtypes.NetConf, podifName string, cid
return fmt.Errorf("failed to get init netns: %v", err)
}

if len(conf.ContIFNames) < 1 && len(conf.ContIFNames) != len(conf.HostIFNames) {
return fmt.Errorf("number of interface names mismatch ContIFNames: %d HostIFNames: %d", len(conf.ContIFNames), len(conf.HostIFNames))
if len(conf.ContIFNames) < 1 && len(conf.ContIFNames) != len(conf.OrigVfState.HostIFName) {
return fmt.Errorf("number of interface names mismatch ContIFNames: %d HostIFNames: %d", len(conf.ContIFNames), len(conf.OrigVfState.HostIFName))
}

return netns.Do(func(_ ns.NetNS) error {
Expand All @@ -233,16 +233,16 @@ func (s *sriovManager) ReleaseVF(conf *sriovtypes.NetConf, podifName string, cid
}

// rename VF device
err = s.nLink.LinkSetName(linkObj, conf.HostIFNames)
err = s.nLink.LinkSetName(linkObj, conf.OrigVfState.HostIFName)
if err != nil {
return fmt.Errorf("failed to rename link %s to host name %s: %q", podifName, conf.HostIFNames, err)
return fmt.Errorf("failed to rename link %s to host name %s: %q", podifName, conf.OrigVfState.HostIFName, err)
}

// reset effective MAC address
if conf.MAC != "" {
hwaddr, err := net.ParseMAC(conf.EffectiveMAC)
hwaddr, err := net.ParseMAC(conf.OrigVfState.EffectiveMAC)
if err != nil {
return fmt.Errorf("failed to parse original effective MAC address %s: %v", conf.EffectiveMAC, err)
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 {
Expand All @@ -252,7 +252,7 @@ func (s *sriovManager) ReleaseVF(conf *sriovtypes.NetConf, podifName string, cid

// move VF device to init netns
if err = s.nLink.LinkSetNsFd(linkObj, int(initns.Fd())); err != nil {
return fmt.Errorf("failed to move interface %s to init netns: %v", conf.HostIFNames, err)
return fmt.Errorf("failed to move interface %s to init netns: %v", conf.OrigVfState.HostIFName, err)
}

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

// Save current the VF state before modifying it
vfState := getVfInfo(pfLink, conf.VFID)
if vfState == nil {
return fmt.Errorf("failed to find vf %d", conf.VFID)
}
conf.OrigVfState.FillFromVfInfo(vfState)

// 1. Set vlan
if conf.Vlan != 0 {
if conf.Vlan != nil {
// set vlan qos if present in the config
if conf.VlanQoS != 0 {
if err = s.nLink.LinkSetVfVlanQos(pfLink, conf.VFID, conf.Vlan, conf.VlanQoS); err != nil {
if conf.VlanQoS != nil {
if err = s.nLink.LinkSetVfVlanQos(pfLink, conf.VFID, *conf.Vlan, *conf.VlanQoS); err != nil {
return fmt.Errorf("failed to set vf %d vlan configuration: %v", conf.VFID, err)
}
} else {
// set vlan id field only
if err = s.nLink.LinkSetVfVlan(pfLink, conf.VFID, conf.Vlan); err != nil {
if err = s.nLink.LinkSetVfVlan(pfLink, conf.VFID, *conf.Vlan); err != nil {
return fmt.Errorf("failed to set vf %d vlan: %v", conf.VFID, err)
}
}
Expand All @@ -299,13 +306,6 @@ func (s *sriovManager) ApplyVFConfig(conf *sriovtypes.NetConf) error {
return fmt.Errorf("failed to parse MAC address %s: %v", conf.MAC, err)
}

// Save the original administrative MAC address before overriding it
vf := getVfInfo(pfLink, conf.VFID)
if vf == nil {
return fmt.Errorf("failed to find vf %d", conf.VFID)
}
conf.AdminMAC = vf.Mac.String()

if err = s.nLink.LinkSetVfHardwareAddr(pfLink, conf.VFID, hwaddr); err != nil {
return fmt.Errorf("failed to set MAC address to %s: %v", hwaddr, err)
}
Expand Down Expand Up @@ -375,57 +375,64 @@ func (s *sriovManager) ApplyVFConfig(conf *sriovtypes.NetConf) error {
return nil
}

// ResetVFConfig reset a VF with default values
// ResetVFConfig reset a VF to its original state
func (s *sriovManager) ResetVFConfig(conf *sriovtypes.NetConf) error {

pfLink, err := s.nLink.LinkByName(conf.Master)
if err != nil {
return fmt.Errorf("failed to lookup master %q: %v", conf.Master, err)
}

// Set vlan to 0
if conf.Vlan != 0 {
if conf.VlanQoS != 0 {
if err = s.nLink.LinkSetVfVlanQos(pfLink, conf.VFID, 0, 0); err != nil {
return fmt.Errorf("failed to set vf %d vlan: %v", conf.VFID, err)
// Restore VLAN
if conf.Vlan != nil {
if conf.VlanQoS != nil {
if err = s.nLink.LinkSetVfVlanQos(pfLink, conf.VFID, conf.OrigVfState.Vlan, conf.OrigVfState.VlanQoS); err != nil {
return fmt.Errorf("failed to restore vf %d vlan: %v", conf.VFID, err)
}
} else if err = s.nLink.LinkSetVfVlan(pfLink, conf.VFID, 0); err != nil {
return fmt.Errorf("failed to set vf %d vlan: %v", conf.VFID, err)
} else if err = s.nLink.LinkSetVfVlan(pfLink, conf.VFID, conf.OrigVfState.Vlan); err != nil {
return fmt.Errorf("failed to restore vf %d vlan: %v", conf.VFID, err)
}
}

// Restore spoofchk
if conf.SpoofChk != "" {
if err = s.nLink.LinkSetVfSpoofchk(pfLink, conf.VFID, conf.OrigVfState.SpoofChk); err != nil {
return fmt.Errorf("failed to restore spoofchk for vf %d: %v", conf.VFID, err)
}
}

// Restore the original administrative MAC address
if conf.MAC != "" {
hwaddr, err := net.ParseMAC(conf.AdminMAC)
hwaddr, err := net.ParseMAC(conf.OrigVfState.AdminMAC)
if err != nil {
return fmt.Errorf("failed to parse original administrative MAC address %s: %v", conf.AdminMAC, err)
return fmt.Errorf("failed to parse original administrative MAC address %s: %v", conf.OrigVfState.AdminMAC, err)
}
if err = s.nLink.LinkSetVfHardwareAddr(pfLink, conf.VFID, hwaddr); err != nil {
return fmt.Errorf("failed to restore original administrative MAC address %s: %v", hwaddr, err)
}
}

// Set spoofchk to on
if err = s.nLink.LinkSetVfSpoofchk(pfLink, conf.VFID, true); err != nil {
return fmt.Errorf("failed to enable spoof checking for vf %d: %v", conf.VFID, err)
}

// Set trust to off
if err = s.nLink.LinkSetVfTrust(pfLink, conf.VFID, false); err != nil {
return fmt.Errorf("failed to disable trust for vf %d: %v", conf.VFID, err)
// Restore VF trust
if conf.Trust != "" {
// TODO: netlink go implementation does not support getting VF trust, need to add support there first
// for now, just set VF trust to off if it was specified by the user in netconf
if err = s.nLink.LinkSetVfTrust(pfLink, conf.VFID, false); err != nil {
return fmt.Errorf("failed to disable trust for vf %d: %v", conf.VFID, err)
}
}

// Disable rate limiting
if err = s.nLink.LinkSetVfRate(pfLink, conf.VFID, 0, 0); err != nil {
return fmt.Errorf("failed to disable rate limiting for vf %d %v", conf.VFID, err)
// Restore rate limiting
if conf.MinTxRate != nil || conf.MaxTxRate != nil {
if err = s.nLink.LinkSetVfRate(pfLink, conf.VFID, conf.OrigVfState.MinTxRate, conf.OrigVfState.MaxTxRate); err != nil {
return fmt.Errorf("failed to disable rate limiting for vf %d %v", conf.VFID, err)
}
}

// Reset link state to `auto`
// Restore link state to `auto`
if conf.LinkState != "" {
// While resetting to `auto` can be a reasonable thing to do regardless of whether it was explicitly
// specified in the network definition, reset only when link_state was explicitly specified, to
// accommodate for drivers / NICs that don't support the netlink command (e.g. igb driver)
if err = s.nLink.LinkSetVfState(pfLink, conf.VFID, 0); err != nil {
// Reset only when link_state was explicitly specified, to accommodate for drivers / NICs
// that don't support the netlink command (e.g. igb driver)
if err = s.nLink.LinkSetVfState(pfLink, conf.VFID, conf.OrigVfState.LinkState); err != nil {
return fmt.Errorf("failed to set link state to auto for vf %d: %v", conf.VFID, err)
}
}
Expand Down
Loading

0 comments on commit e051689

Please sign in to comment.