Merge pull request #587 from vasrem/fix/enhance-checks-on-generic-plugin

Add more checks on generic plugin to discover discrepancies from the desired state

api/v1/helper.go

@@ -272,6 +272,12 @@ func NeedToUpdateSriov(ifaceSpec *Interface, ifaceStatus *InterfaceExt) bool {
  log.V(2).Info("NeedToUpdateSriov(): NumVfs needs update", "desired", ifaceSpec.NumVfs, "current", ifaceStatus.NumVfs)
  return true
  }
+
+ if ifaceStatus.LinkAdminState == consts.LinkAdminStateDown {
+ log.V(2).Info("NeedToUpdateSriov(): PF link status needs update", "desired to include", "up", "current", ifaceStatus.LinkAdminState)
+ return true
+ }
+
  if ifaceSpec.NumVfs > 0 {
  for _, vfStatus := range ifaceStatus.VFs {
  ingroup := false
@@ -301,6 +307,16 @@ func NeedToUpdateSriov(ifaceSpec *Interface, ifaceStatus *InterfaceExt) bool {
  return true
  }
 
+ if (strings.EqualFold(ifaceStatus.LinkType, consts.LinkTypeETH) && groupSpec.IsRdma) || strings.EqualFold(ifaceStatus.LinkType, consts.LinkTypeIB) {
+ // We do this check only if a Node GUID is set to ensure that we were able to read the
+ // Node GUID. We intentionally skip empty Node GUID in vfStatus because this may happen
+ // when the VF is allocated to a workload.
+ if vfStatus.GUID == consts.UninitializedNodeGUID {
+ log.V(2).Info("NeedToUpdateSriov(): VF GUID needs update",
+ "vf", vfStatus.VfID, "current", vfStatus.GUID)
+ return true
+ }
+ }
  // this is needed to be sure the admin mac address is configured as expected
  if ifaceSpec.ExternallyManaged {
  log.V(2).Info("NeedToUpdateSriov(): need to update the device as it's externally manage",

api/v1/sriovnetworknodestate_types.go

@@ -64,6 +64,7 @@ type InterfaceExt struct {
  NumVfs int `json:"numVfs,omitempty"`
  LinkSpeed string `json:"linkSpeed,omitempty"`
  LinkType string `json:"linkType,omitempty"`
+ LinkAdminState string `json:"linkAdminState,omitempty"`
  EswitchMode string `json:"eSwitchMode,omitempty"`
  ExternallyManaged bool `json:"externallyManaged,omitempty"`
  TotalVfs int `json:"totalvfs,omitempty"`
@@ -84,6 +85,7 @@ type VirtualFunction struct {
  VfID int `json:"vfID"`
  VdpaType string `json:"vdpaType,omitempty"`
  RepresentorName string `json:"representorName,omitempty"`
+ GUID string `json:"guid,omitempty"`
 }
 
 // Bridges contains list of bridges

config/crd/bases/sriovnetwork.openshift.io_sriovnetworknodestates.yaml

@@ -268,6 +268,8 @@ spec:
  type: string
  driver:
  type: string
+ guid:
+ type: string
  mac:
  type: string
  mtu:
@@ -297,6 +299,8 @@ spec:
  type: string
  externallyManaged:
  type: boolean
+ linkAdminState:
+ type: string
  linkSpeed:
  type: string
  linkType:

deployment/sriov-network-operator/crds/sriovnetwork.openshift.io_sriovnetworknodestates.yaml

@@ -268,6 +268,8 @@ spec:
  type: string
  driver:
  type: string
+ guid:
+ type: string
  mac:
  type: string
  mtu:
@@ -297,6 +299,8 @@ spec:
  type: string
  externallyManaged:
  type: boolean
+ linkAdminState:
+ type: string
  linkSpeed:
  type: string
  linkType:

pkg/consts/constants.go

@@ -43,6 +43,11 @@ const (
  LinkTypeIB = "IB"
  LinkTypeETH = "ETH"
 
+ LinkAdminStateUp = "up"
+ LinkAdminStateDown = "down"
+
+ UninitializedNodeGUID = "0000:0000:0000:0000"
+
  DeviceTypeVfioPci = "vfio-pci"
  DeviceTypeNetDevice = "netdevice"
  VdpaTypeVirtio = "virtio"

pkg/host/internal/lib/netlink/netlink.go

@@ -58,6 +58,11 @@ type NetlinkLib interface {
  // cmode argument should contain valid cmode value as uint8, modes are define in nl.DEVLINK_PARAM_CMODE_* constants
  // value argument should have one of the following types: uint8, uint16, uint32, string, bool
  DevlinkSetDeviceParam(bus string, device string, param string, cmode uint8, value interface{}) error
+ // RdmaLinkByName finds a link by name and returns a pointer to the object if
+ // found and nil error, otherwise returns error code.
+ RdmaLinkByName(name string) (*netlink.RdmaLink, error)
+ // IsLinkAdminStateUp checks if the admin state of a link is up
+ IsLinkAdminStateUp(link Link) bool
 }
 
 type libWrapper struct{}
@@ -142,3 +147,14 @@ func (w *libWrapper) DevlinkGetDeviceParamByName(bus string, device string, para
 func (w *libWrapper) DevlinkSetDeviceParam(bus string, device string, param string, cmode uint8, value interface{}) error {
  return netlink.DevlinkSetDeviceParam(bus, device, param, cmode, value)
 }
+
+// RdmaLinkByName finds a link by name and returns a pointer to the object if
+// found and nil error, otherwise returns error code.
+func (w *libWrapper) RdmaLinkByName(name string) (*netlink.RdmaLink, error) {
+ return netlink.RdmaLinkByName(name)
+}
+
+// IsLinkAdminStateUp checks if the admin state of a link is up
+func (w *libWrapper) IsLinkAdminStateUp(link Link) bool {
+ return link.Attrs().Flags&net.FlagUp == 1
+}

pkg/host/internal/network/network.go

@@ -1,7 +1,9 @@
 package network
 
 import (
+ "errors"
  "fmt"
+ "io/fs"
  "os"
  "path/filepath"
  "strconv"
@@ -187,6 +189,35 @@ func (n *network) GetNetDevMac(ifaceName string) string {
  return link.Attrs().HardwareAddr.String()
 }
 
+// GetNetDevNodeGUID returns the network interface node GUID if device is RDMA capable otherwise returns empty string
+func (n *network) GetNetDevNodeGUID(pciAddr string) string {
+ if len(pciAddr) == 0 {
+ return ""
+ }
+
+ rdmaDevicesPath := filepath.Join(vars.FilesystemRoot, consts.SysBusPciDevices, pciAddr, "infiniband")
+ rdmaDevices, err := os.ReadDir(rdmaDevicesPath)
+ if err != nil {
+ if !errors.Is(err, fs.ErrNotExist) {
+ log.Log.Error(err, "GetNetDevNodeGUID(): failed to read RDMA related directory", "pciAddr", pciAddr)
+ }
+ return ""
+ }
+
+ if len(rdmaDevices) != 1 {
+ log.Log.Error(err, "GetNetDevNodeGUID(): expected just one RDMA device", "pciAddr", pciAddr, "numOfDevices", len(rdmaDevices))
+ return ""
+ }
+
+ rdmaLink, err := n.netlinkLib.RdmaLinkByName(rdmaDevices[0].Name())
+ if err != nil {
+ log.Log.Error(err, "GetNetDevNodeGUID(): failed to get RDMA link", "pciAddr", pciAddr)
+ return ""
+ }
+
+ return rdmaLink.Attrs.NodeGuid
+}
+
 func (n *network) GetNetDevLinkSpeed(ifaceName string) string {
  log.Log.V(2).Info("GetNetDevLinkSpeed(): get LinkSpeed", "device", ifaceName)
  speedFilePath := filepath.Join(vars.FilesystemRoot, consts.SysClassNet, ifaceName, "speed")
@@ -329,3 +360,23 @@ func (n *network) EnableHwTcOffload(ifaceName string) error {
  log.Log.V(0).Info("EnableHwTcOffload(): feature is still disabled, not supported by device", "device", ifaceName)
  return nil
 }
+
+// GetNetDevLinkAdminState returns the admin state of the interface.
+func (n *network) GetNetDevLinkAdminState(ifaceName string) string {
+ log.Log.V(2).Info("GetNetDevLinkAdminState(): get LinkAdminState", "device", ifaceName)
+ if len(ifaceName) == 0 {
+ return ""
+ }
+
+ link, err := n.netlinkLib.LinkByName(ifaceName)
+ if err != nil {
+ log.Log.Error(err, "GetNetDevLinkAdminState(): failed to get link", "device", ifaceName)
+ return ""
+ }
+
+ if n.netlinkLib.IsLinkAdminStateUp(link) {
+ return consts.LinkAdminStateUp
+ }
+
+ return consts.LinkAdminStateDown
+}

pkg/host/internal/network/network_test.go

@@ -15,6 +15,8 @@ import (
  ethtoolMockPkg "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/host/internal/lib/ethtool/mock"
  netlinkMockPkg "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/host/internal/lib/netlink/mock"
  "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/host/types"
+ "github.com/k8snetworkplumbingwg/sriov-network-operator/test/util/fakefilesystem"
+ "github.com/k8snetworkplumbingwg/sriov-network-operator/test/util/helpers"
 )
 
 func getDevlinkParam(t uint8, value interface{}) *netlink.DevlinkParam {
@@ -202,4 +204,38 @@ var _ = Describe("Network", func() {
  Expect(n.EnableHwTcOffload("enp216s0f0np0")).To(MatchError(testErr))
  })
  })
+ Context("GetNetDevNodeGUID", func() {
+ It("Returns empty when pciAddr is empty", func() {
+ Expect(n.GetNetDevNodeGUID("")).To(Equal(""))
+ })
+ It("Returns empty when infiniband directory can't be read", func() {
+ helpers.GinkgoConfigureFakeFS(&fakefilesystem.FS{
+ Dirs: []string{"/sys/bus/pci/devices/0000:4b:00.3/"},
+ })
+ Expect(n.GetNetDevNodeGUID("0000:4b:00.3")).To(Equal(""))
+ })
+ It("Returns empty when more than one RDMA devices are detected for pciAddr", func() {
+ helpers.GinkgoConfigureFakeFS(&fakefilesystem.FS{
+ Dirs: []string{
+ "/sys/bus/pci/devices/0000:4b:00.3/infiniband/mlx5_2",
+ "/sys/bus/pci/devices/0000:4b:00.3/infiniband/mlx5_3",
+ },
+ })
+ Expect(n.GetNetDevNodeGUID("0000:4b:00.3")).To(Equal(""))
+ })
+ It("Returns empty when it fails to read RDMA link", func() {
+ helpers.GinkgoConfigureFakeFS(&fakefilesystem.FS{
+ Dirs: []string{"/sys/bus/pci/devices/0000:4b:00.3/infiniband/mlx5_2"},
+ })
+ netlinkLibMock.EXPECT().RdmaLinkByName("mlx5_2").Return(nil, fmt.Errorf("some-error"))
+ Expect(n.GetNetDevNodeGUID("0000:4b:00.3")).To(Equal(""))
+ })
+ It("Returns populated node GUID on correct setup", func() {
+ helpers.GinkgoConfigureFakeFS(&fakefilesystem.FS{
+ Dirs: []string{"/sys/bus/pci/devices/0000:4b:00.3/infiniband/mlx5_2"},
+ })
+ netlinkLibMock.EXPECT().RdmaLinkByName("mlx5_2").Return(&netlink.RdmaLink{Attrs: netlink.RdmaLinkAttrs{NodeGuid: "1122:3344:5566:7788"}}, nil)
+ Expect(n.GetNetDevNodeGUID("0000:4b:00.3")).To(Equal("1122:3344:5566:7788"))
+ })
+ })
 })

pkg/host/internal/sriov/sriov.go

@@ -134,6 +134,7 @@ func (s *sriov) GetVfInfo(pciAddr string, devices []*ghw.PCIDevice) sriovnetwork
  vf.Mac = link.Attrs().HardwareAddr.String()
  }
  }
+ vf.GUID = s.networkHelper.GetNetDevNodeGUID(pciAddr)
 
  for _, device := range devices {
  if pciAddr == device.Address {
@@ -259,15 +260,16 @@ func (s *sriov) DiscoverSriovDevices(storeManager store.ManagerInterface) ([]sri
  }
 
  iface := sriovnetworkv1.InterfaceExt{
- Name: pfNetName,
- PciAddress: device.Address,
- Driver: driver,
- Vendor: device.Vendor.ID,
- DeviceID: device.Product.ID,
- Mtu: link.Attrs().MTU,
- Mac: link.Attrs().HardwareAddr.String(),
- LinkType: s.encapTypeToLinkType(link.Attrs().EncapType),
- LinkSpeed: s.networkHelper.GetNetDevLinkSpeed(pfNetName),
+ Name: pfNetName,
+ PciAddress: device.Address,
+ Driver: driver,
+ Vendor: device.Vendor.ID,
+ DeviceID: device.Product.ID,
+ Mtu: link.Attrs().MTU,
+ Mac: link.Attrs().HardwareAddr.String(),
+ LinkType: s.encapTypeToLinkType(link.Attrs().EncapType),
+ LinkSpeed: s.networkHelper.GetNetDevLinkSpeed(pfNetName),
+ LinkAdminState: s.networkHelper.GetNetDevLinkAdminState(pfNetName),
  }
 
  pfStatus, exist, err := storeManager.LoadPfsStatus(iface.PciAddress)
@@ -571,7 +573,7 @@ func (s *sriov) configSriovDevice(iface *sriovnetworkv1.Interface, skipVFConfigu
  if err != nil {
  return err
  }
- if pfLink.Attrs().OperState != netlink.OperUp {
+ if !s.netlinkLib.IsLinkAdminStateUp(pfLink) {
  err = s.netlinkLib.LinkSetUp(pfLink)
  if err != nil {
  return err

pkg/host/internal/sriov/sriov_test.go

@@ -120,7 +120,8 @@ var _ = Describe("SRIOV", func() {
  dputilsLibMock.EXPECT().GetVFList("0000:d8:00.0").Return([]string{"0000:d8:00.2", "0000:d8:00.3"}, nil)
  pfLinkMock := netlinkMockPkg.NewMockLink(testCtrl)
  netlinkLibMock.EXPECT().LinkByName("enp216s0f0np0").Return(pfLinkMock, nil).Times(3)
- pfLinkMock.EXPECT().Attrs().Return(&netlink.LinkAttrs{OperState: netlink.OperDown, EncapType: "ether"}).Times(2)
+ pfLinkMock.EXPECT().Attrs().Return(&netlink.LinkAttrs{Flags: 0, EncapType: "ether"})
+ netlinkLibMock.EXPECT().IsLinkAdminStateUp(pfLinkMock).Return(false)
  netlinkLibMock.EXPECT().LinkSetUp(pfLinkMock).Return(nil)
 
  dputilsLibMock.EXPECT().GetVFID("0000:d8:00.2").Return(0, nil).Times(2)
@@ -187,7 +188,7 @@ var _ = Describe("SRIOV", func() {
  dputilsLibMock.EXPECT().GetVFList("0000:d8:00.0").Return([]string{"0000:d8:00.2"}, nil)
  pfLinkMock := netlinkMockPkg.NewMockLink(testCtrl)
  netlinkLibMock.EXPECT().LinkByName("enp216s0f0np0").Return(pfLinkMock, nil).Times(2)
- pfLinkMock.EXPECT().Attrs().Return(&netlink.LinkAttrs{OperState: netlink.OperDown})
+ netlinkLibMock.EXPECT().IsLinkAdminStateUp(pfLinkMock).Return(false)
  netlinkLibMock.EXPECT().LinkSetUp(pfLinkMock).Return(nil)
 
  dputilsLibMock.EXPECT().GetVFID("0000:d8:00.2").Return(0, nil).Times(2)
@@ -240,7 +241,7 @@ var _ = Describe("SRIOV", func() {
  dputilsLibMock.EXPECT().GetVFList("0000:d8:00.0").Return([]string{"0000:d8:00.2"}, nil).Times(2)
  pfLinkMock := netlinkMockPkg.NewMockLink(testCtrl)
  netlinkLibMock.EXPECT().LinkByName("enp216s0f0np0").Return(pfLinkMock, nil).Times(2)
- pfLinkMock.EXPECT().Attrs().Return(&netlink.LinkAttrs{OperState: netlink.OperDown})
+ netlinkLibMock.EXPECT().IsLinkAdminStateUp(pfLinkMock).Return(false)
  netlinkLibMock.EXPECT().LinkSetUp(pfLinkMock).Return(nil)
  netlinkLibMock.EXPECT().DevLinkGetDeviceByName("pci", "0000:d8:00.0").Return(&netlink.DevlinkDevice{
  Attrs: netlink.DevlinkDevAttrs{Eswitch: netlink.DevlinkDevEswitchAttr{Mode: "legacy"}}}, nil).Times(2)