Merge pull request #638 from adrianchiris/optimize-device-discovery

refactor: DiscoverSriovDevices imporvements

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

@@ -30,6 +30,9 @@ type NetlinkLib interface {
 	// LinkSetUp enables the link device.
 	// Equivalent to: `ip link set $link up`
 	LinkSetUp(link Link) error
+	// LinkSetMTU sets the mtu of the link device.
+	// Equivalent to: `ip link set $link mtu $mtu`
+	LinkSetMTU(link Link, mtu int) error
 	// DevlinkGetDeviceByName provides a pointer to devlink device and nil error,
 	// otherwise returns an error code.
 	DevLinkGetDeviceByName(bus string, device string) (*netlink.DevlinkDevice, error)
@@ -71,6 +74,12 @@ func (w *libWrapper) LinkSetUp(link Link) error {
 	return netlink.LinkSetUp(link)
 }
 
+// LinkSetMTU sets the mtu of the link device.
+// Equivalent to: `ip link set $link mtu $mtu`
+func (w *libWrapper) LinkSetMTU(link Link, mtu int) error {
+	return netlink.LinkSetMTU(link, mtu)
+}
+
 // DevlinkGetDeviceByName provides a pointer to devlink device and nil error,
 // otherwise returns an error code.
 func (w *libWrapper) DevLinkGetDeviceByName(bus string, device string) (*netlink.DevlinkDevice, error) {

pkg/host/internal/network/network.go

@@ -4,7 +4,6 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"strconv"
 	"strings"
 	"time"
 
@@ -13,6 +12,7 @@ import (
 
 	"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/consts"
 	dputilsPkg "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/host/internal/lib/dputils"
+	netlinkPkg "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/host/internal/lib/netlink"
 	"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/host/types"
 	"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/utils"
 	"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/vars"
@@ -21,12 +21,14 @@ import (
 type network struct {
 	utilsHelper utils.CmdInterface
 	dputilsLib  dputilsPkg.DPUtilsLib
+	netlinkLib  netlinkPkg.NetlinkLib
 }
 
-func New(utilsHelper utils.CmdInterface, dputilsLib dputilsPkg.DPUtilsLib) types.NetworkInterface {
+func New(utilsHelper utils.CmdInterface, dputilsLib dputilsPkg.DPUtilsLib, netlinkLib netlinkPkg.NetlinkLib) types.NetworkInterface {
 	return &network{
 		utilsHelper: utilsHelper,
 		dputilsLib:  dputilsLib,
+		netlinkLib:  netlinkLib,
 	}
 }
 
@@ -123,28 +125,20 @@ func (n *network) IsSwitchdev(name string) bool {
 	return true
 }
 
-func mtuFilePath(ifaceName string, pciAddr string) string {
-	mtuFile := "net/" + ifaceName + "/mtu"
-	return filepath.Join(vars.FilesystemRoot, consts.SysBusPciDevices, pciAddr, mtuFile)
-}
-
 func (n *network) GetNetdevMTU(pciAddr string) int {
 	log.Log.V(2).Info("GetNetdevMTU(): get MTU", "device", pciAddr)
 	ifaceName := n.TryGetInterfaceName(pciAddr)
 	if ifaceName == "" {
 		return 0
 	}
-	data, err := os.ReadFile(mtuFilePath(ifaceName, pciAddr))
-	if err != nil {
-		log.Log.Error(err, "GetNetdevMTU(): fail to read mtu file", "path", mtuFilePath)
-		return 0
-	}
-	mtu, err := strconv.Atoi(strings.TrimSpace(string(data)))
+
+	link, err := n.netlinkLib.LinkByName(ifaceName)
 	if err != nil {
-		log.Log.Error(err, "GetNetdevMTU(): fail to convert mtu to int", "raw-mtu", strings.TrimSpace(string(data)))
+		log.Log.Error(err, "GetNetdevMTU(): fail to get Link ", "device", ifaceName)
 		return 0
 	}
-	return mtu
+
+	return link.Attrs().MTU
 }
 
 func (n *network) SetNetdevMTU(pciAddr string, mtu int) error {
@@ -155,34 +149,37 @@ func (n *network) SetNetdevMTU(pciAddr string, mtu int) error {
 	}
 	b := backoff.NewConstantBackOff(1 * time.Second)
 	err := backoff.Retry(func() error {
-		ifaceName, err := n.dputilsLib.GetNetNames(pciAddr)
+		ifaceName := n.TryGetInterfaceName(pciAddr)
+		if ifaceName == "" {
+			log.Log.Error(nil, "SetNetdevMTU(): fail to get interface name", "device", pciAddr)
+			return fmt.Errorf("failed to get netdevice for device %s", pciAddr)
+		}
+
+		link, err := n.netlinkLib.LinkByName(ifaceName)
 		if err != nil {
-			log.Log.Error(err, "SetNetdevMTU(): fail to get interface name", "device", pciAddr)
+			log.Log.Error(err, "SetNetdevMTU(): fail to get Link ", "device", ifaceName)
 			return err
 		}
-		if len(ifaceName) < 1 {
-			return fmt.Errorf("SetNetdevMTU(): interface name is empty")
-		}
-		mtuFilePath := mtuFilePath(ifaceName[0], pciAddr)
-		return os.WriteFile(mtuFilePath, []byte(strconv.Itoa(mtu)), os.ModeAppend)
+		return n.netlinkLib.LinkSetMTU(link, mtu)
 	}, backoff.WithMaxRetries(b, 10))
+
 	if err != nil {
-		log.Log.Error(err, "SetNetdevMTU(): fail to write mtu file after retrying")
+		log.Log.Error(err, "SetNetdevMTU(): fail to set mtu after retrying")
 		return err
 	}
 	return nil
 }
 
+// GetNetDevMac returns network device MAC address or empty string if address cannot be
+// retrieved.
 func (n *network) GetNetDevMac(ifaceName string) string {
 	log.Log.V(2).Info("GetNetDevMac(): get Mac", "device", ifaceName)
-	macFilePath := filepath.Join(vars.FilesystemRoot, consts.SysClassNet, ifaceName, "address")
-	data, err := os.ReadFile(macFilePath)
+	link, err := n.netlinkLib.LinkByName(ifaceName)
 	if err != nil {
-		log.Log.Error(err, "GetNetDevMac(): fail to read Mac file", "path", macFilePath)
+		log.Log.Error(err, "GetNetDevMac(): failed to get Link", "device", ifaceName)
 		return ""
 	}
-
-	return strings.TrimSpace(string(data))
+	return link.Attrs().HardwareAddr.String()
 }
 
 func (n *network) GetNetDevLinkSpeed(ifaceName string) string {

pkg/host/internal/sriov/sriov.go

@@ -115,12 +115,15 @@ func (s *sriov) GetVfInfo(pciAddr string, devices []*ghw.PCIDevice) sriovnetwork
 		VfID:       id,
 	}
 
-	if mtu := s.networkHelper.GetNetdevMTU(pciAddr); mtu > 0 {
-		vf.Mtu = mtu
-	}
 	if name := s.networkHelper.TryGetInterfaceName(pciAddr); name != "" {
-		vf.Name = name
-		vf.Mac = s.networkHelper.GetNetDevMac(name)
+		link, err := s.netlinkLib.LinkByName(name)
+		if err != nil {
+			log.Log.Error(err, "getVfInfo(): unable to get VF Link Object", "name", name, "device", pciAddr)
+		} else {
+			vf.Name = name
+			vf.Mtu = link.Attrs().MTU
+			vf.Mac = link.Attrs().HardwareAddr.String()
+		}
 	}
 
 	for _, device := range devices {
@@ -129,7 +132,6 @@ func (s *sriov) GetVfInfo(pciAddr string, devices []*ghw.PCIDevice) sriovnetwork
 			vf.DeviceID = device.Product.ID
 			break
 		}
-		continue
 	}
 	return vf
 }
@@ -221,44 +223,42 @@ func (s *sriov) DiscoverSriovDevices(storeManager store.ManagerInterface) ([]sri
 			continue
 		}
 
+		if !vars.DevMode {
+			if !sriovnetworkv1.IsSupportedModel(device.Vendor.ID, device.Product.ID) {
+				log.Log.Info("DiscoverSriovDevices(): unsupported device", "device", device)
+				continue
+			}
+		}
+
 		driver, err := s.dputilsLib.GetDriverName(device.Address)
 		if err != nil {
 			log.Log.Error(err, "DiscoverSriovDevices(): unable to parse device driver for device, skipping", "device", device)
 			continue
 		}
 
-		deviceNames, err := s.dputilsLib.GetNetNames(device.Address)
-		if err != nil {
-			log.Log.Error(err, "DiscoverSriovDevices(): unable to get device names for device, skipping", "device", device)
-			continue
-		}
+		pfNetName := s.networkHelper.TryGetInterfaceName(device.Address)
 
-		if len(deviceNames) == 0 {
-			// no network devices found, skipping device
+		if pfNetName == "" {
+			log.Log.Error(err, "DiscoverSriovDevices(): unable to get device name for device, skipping", "device", device.Address)
 			continue
 		}
 
-		if !vars.DevMode {
-			if !sriovnetworkv1.IsSupportedModel(device.Vendor.ID, device.Product.ID) {
-				log.Log.Info("DiscoverSriovDevices(): unsupported device", "device", device)
-				continue
-			}
+		link, err := s.netlinkLib.LinkByName(pfNetName)
+		if err != nil {
+			log.Log.Error(err, "DiscoverSriovDevices(): unable to get Link for device, skipping", "device", device.Address)
+			continue
 		}
 
 		iface := sriovnetworkv1.InterfaceExt{
+			Name:       pfNetName,
 			PciAddress: device.Address,
 			Driver:     driver,
 			Vendor:     device.Vendor.ID,
 			DeviceID:   device.Product.ID,
-		}
-		if mtu := s.networkHelper.GetNetdevMTU(device.Address); mtu > 0 {
-			iface.Mtu = mtu
-		}
-		if name := s.networkHelper.TryGetInterfaceName(device.Address); name != "" {
-			iface.Name = name
-			iface.Mac = s.networkHelper.GetNetDevMac(name)
-			iface.LinkSpeed = s.networkHelper.GetNetDevLinkSpeed(name)
-			iface.LinkType = s.GetLinkType(name)
+			Mtu:        link.Attrs().MTU,
+			Mac:        link.Attrs().HardwareAddr.String(),
+			LinkType:   s.encapTypeToLinkType(link.Attrs().EncapType),
+			LinkSpeed:  s.networkHelper.GetNetDevLinkSpeed(pfNetName),
 		}
 
 		pfStatus, exist, err := storeManager.LoadPfsStatus(iface.PciAddress)
@@ -809,10 +809,13 @@ func (s *sriov) GetLinkType(name string) string {
 		log.Log.Error(err, "GetLinkType(): failed to get link", "device", name)
 		return ""
 	}
-	linkType := link.Attrs().EncapType
-	if linkType == "ether" {
+	return s.encapTypeToLinkType(link.Attrs().EncapType)
+}
+
+func (s *sriov) encapTypeToLinkType(encapType string) string {
+	if encapType == "ether" {
 		return consts.LinkTypeETH
-	} else if linkType == "infiniband" {
+	} else if encapType == "infiniband" {
 		return consts.LinkTypeIB
 	}
 	return ""

pkg/host/manager.go

@@ -38,11 +38,12 @@ type hostManager struct {
 
 func NewHostManager(utilsInterface utils.CmdInterface) HostManagerInterface {
 	dpUtils := dputils.New()
+	nl := netlink.New()
 	k := kernel.New(utilsInterface)
-	n := network.New(utilsInterface, dpUtils)
+	n := network.New(utilsInterface, dpUtils, nl)
 	sv := service.New(utilsInterface)
 	u := udev.New(utilsInterface)
-	sr := sriov.New(utilsInterface, k, n, u, netlink.New(), dpUtils)
+	sr := sriov.New(utilsInterface, k, n, u, nl, dpUtils)
 	v := vdpa.New(k, govdpa.New())
 
 	return &hostManager{