Add more checks on generic plugin

Today, we are missing 2 checks on settings that sriov netop is
configuring:
* PF is up
* GUID is set

Without checking for these 2, we risk that changes made by the user
directly to the system are not reconciled by the netop leaving the
system in a bad state.

This commit is adding those checks.

Signed-off-by: Vasilis Remmas <vremmas@nvidia.com>

api/v1/helper.go

@@ -271,6 +271,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 == "down" {
+		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
@@ -300,6 +306,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

@@ -63,6 +63,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"`
@@ -82,6 +83,7 @@ type VirtualFunction struct {
 	Mtu        int    `json:"mtu,omitempty"`
 	VfID       int    `json:"vfID"`
 	VdpaType   string `json:"vdpaType,omitempty"`
+	GUID       string `json:"guid,omitempty"`
 }
 
 // SriovNetworkNodeStateStatus defines the observed state of SriovNetworkNodeState

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

@@ -108,6 +108,8 @@ spec:
                             type: string
                           driver:
                             type: string
+                          guid:
+                            type: string
                           mac:
                             type: string
                           mtu:
@@ -135,6 +137,8 @@ spec:
                       type: string
                     externallyManaged:
                       type: boolean
+                    linkAdminState:
+                      type: string
                     linkSpeed:
                       type: string
                     linkType:

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

@@ -108,6 +108,8 @@ spec:
                             type: string
                           driver:
                             type: string
+                          guid:
+                            type: string
                           mac:
                             type: string
                           mtu:
@@ -135,6 +137,8 @@ spec:
                       type: string
                     externallyManaged:
                       type: boolean
+                    linkAdminState:
+                      type: string
                     linkSpeed:
                       type: string
                     linkType:

pkg/consts/constants.go

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

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

@@ -58,6 +58,9 @@ 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)
 }
 
 type libWrapper struct{}
@@ -142,3 +145,9 @@ 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)
+}

pkg/host/internal/network/network.go

@@ -1,7 +1,10 @@
 package network
 
 import (
+	"errors"
 	"fmt"
+	"io/fs"
+	"net"
 	"os"
 	"path/filepath"
 	"strconv"
@@ -187,6 +190,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 +361,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 link.Attrs().Flags&net.FlagUp == 0 {
+		return "down"
+	}
+
+	return "up"
+}

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

@@ -3,6 +3,7 @@ package sriov
 import (
 	"errors"
 	"fmt"
+	"net"
 	"os"
 	"path/filepath"
 	"strconv"
@@ -134,6 +135,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 +261,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 +574,7 @@ func (s *sriov) configSriovDevice(iface *sriovnetworkv1.Interface, skipVFConfigu
 	if err != nil {
 		return err
 	}
-	if pfLink.Attrs().OperState != netlink.OperUp {
+	if pfLink.Attrs().Flags&net.FlagUp == 0 {
 		err = s.netlinkLib.LinkSetUp(pfLink)
 		if err != nil {
 			return err