Merge pull request #313 from zeeke/us/ocpbugs-45028

Avoid resetting the VF if the netns does not exist

Author: SchSeba

.github/workflows/buildtest.yml

@@ -80,7 +80,7 @@ jobs:
         env:
           LOCAL_SRIOV_CNI_IMAGE: ghaction-sriov-cni:pr-${{github.event.pull_request.number}}
 
-      - uses: actions/upload-artifact@v3
+      - uses: actions/upload-artifact@v4
         if: always()
         with:
           name: ${{ env.TEST_REPORT_PATH }}

cmd/sriov/main.go

@@ -245,6 +245,17 @@ func cmdDel(args *skel.CmdArgs) error {
 		return nil
 	}
 
+	allocator := utils.NewPCIAllocator(config.DefaultCNIDir)
+
+	err = allocator.Lock(netConf.DeviceID)
+	if err != nil {
+		return fmt.Errorf("cmdDel() error obtaining lock for device [%s]: %w", netConf.DeviceID, err)
+	}
+
+	logging.Debug("Acquired device lock",
+		"func", "cmdDel",
+		"DeviceID", netConf.DeviceID)
+
 	defer func() {
 		if err == nil && cRefPath != "" {
 			_ = utils.CleanCachedNetConf(cRefPath)
@@ -270,6 +281,9 @@ func cmdDel(args *skel.CmdArgs) error {
 
 	sm := sriov.NewSriovManager()
 
+	logging.Debug("Reset VF configuration",
+		"func", "cmdDel",
+		"netConf.DeviceID", netConf.DeviceID)
 	/* ResetVFConfig resets a VF administratively. We must run ResetVFConfig
 	   before ReleaseVF because some drivers will error out if we try to
 	   reset netdev VF with trust off. So, reset VF MAC address via PF first.
@@ -288,13 +302,22 @@ func cmdDel(args *skel.CmdArgs) error {
 			// IPAM resources
 			_, ok := err.(ns.NSPathNotExistErr)
 			if ok {
+				logging.Debug("Exiting as the network namespace does not exists anymore",
+					"func", "cmdDel",
+					"netConf.DeviceID", netConf.DeviceID,
+					"args.Netns", args.Netns)
 				return nil
 			}
 
 			return fmt.Errorf("failed to open netns %s: %q", netns, err)
 		}
 		defer netns.Close()
 
+		logging.Debug("Release the VF",
+			"func", "cmdDel",
+			"netConf.DeviceID", netConf.DeviceID,
+			"args.Netns", args.Netns,
+			"args.IfName", args.IfName)
 		if err = sm.ReleaseVF(netConf, args.IfName, netns); err != nil {
 			return err
 		}
@@ -305,7 +328,6 @@ func cmdDel(args *skel.CmdArgs) error {
 		"func", "cmdDel",
 		"config.DefaultCNIDir", config.DefaultCNIDir,
 		"netConf.DeviceID", netConf.DeviceID)
-	allocator := utils.NewPCIAllocator(config.DefaultCNIDir)
 	if err = allocator.DeleteAllocatedPCI(netConf.DeviceID); err != nil {
 		return fmt.Errorf("error cleaning the pci allocation for vf pci address %s: %v", netConf.DeviceID, err)
 	}

pkg/config/config.go

@@ -48,6 +48,15 @@ func LoadConf(bytes []byte) (*sriovtypes.NetConf, error) {
 		return nil, fmt.Errorf("LoadConf(): VF pci addr is required")
 	}
 
+	allocator := utils.NewPCIAllocator(DefaultCNIDir)
+	err := allocator.Lock(n.DeviceID)
+	if err != nil {
+		return nil, err
+	}
+	logging.Debug("Acquired device lock",
+		"func", "LoadConf",
+		"DeviceID", n.DeviceID)
+	
 	// Check if the device is already allocated.
 	// This is to prevent issues where kubelet request to delete a pod and in the same time a new pod using the same
 	// vf is started. we can have an issue where the cmdDel of the old pod is called AFTER the cmdAdd of the new one
@@ -56,7 +65,6 @@ func LoadConf(bytes []byte) (*sriovtypes.NetConf, error) {
 		"func", "LoadConf",
 		"DefaultCNIDir", DefaultCNIDir,
 		"n.DeviceID", n.DeviceID)
-	allocator := utils.NewPCIAllocator(DefaultCNIDir)
 	isAllocated, err := allocator.IsAllocated(n.DeviceID)
 	if err != nil {
 		return n, err

pkg/config/config_test.go

@@ -13,6 +13,11 @@ import (
 )
 
 var _ = Describe("Config", func() {
+	BeforeEach(func() {
+		DeferCleanup(func(x string) { DefaultCNIDir = x }, DefaultCNIDir)
+		DefaultCNIDir = GinkgoT().TempDir()
+	})
+
 	Context("Checking LoadConf function", func() {
 		It("Assuming correct config file - existing DeviceID", func() {
 			conf := []byte(`{
@@ -80,6 +85,7 @@ var _ = Describe("Config", func() {
 		invalidProto := "802"
 		DescribeTable("Vlan ID, QoS and Proto",
 			func(vlanID *int, vlanQoS *int, vlanProto *string, failure bool) {
+
 				s := `{
         "name": "mynet",
         "type": "sriov",

pkg/utils/pci_allocator.go

@@ -3,12 +3,18 @@ package utils
 import (
 	"fmt"
 	"os"
+	"path"
 	"path/filepath"
+	"time"
 
 	"github.com/containernetworking/plugins/pkg/ns"
 	"github.com/k8snetworkplumbingwg/sriov-cni/pkg/logging"
+
+	"golang.org/x/sys/unix"
 )
 
+const pciLockAcquireTimeout = 60 * time.Second
+
 type PCIAllocation interface {
 	SaveAllocatedPCI(string, string) error
 	DeleteAllocatedPCI(string) error
@@ -25,6 +31,41 @@ func NewPCIAllocator(dataDir string) *PCIAllocator {
 	return &PCIAllocator{dataDir: filepath.Join(dataDir, "pci")}
 }
 
+// Lock gets an exclusive lock on the given PCI address, ensuring there is no other process configuring / or de-configuring the same device.
+func (p *PCIAllocator) Lock(pciAddress string) error {
+	lockDir := path.Join(p.dataDir, "vf_lock")
+	if err := os.MkdirAll(lockDir, 0600); err != nil {
+		return fmt.Errorf("failed to create the sriov lock directory(%q): %v", lockDir, err)
+	}
+
+	path := filepath.Join(lockDir, fmt.Sprintf("%s.lock", pciAddress))
+
+	// unix.O_CREAT - Create the file if it doesn't exist
+	// unix.O_RDONLY - Open the file for read
+	// unix.O_CLOEXEC - Automatically close the file on exit. This is useful to keep the flock until the process exits
+	fd, err := unix.Open(path, unix.O_CREAT|unix.O_RDONLY|unix.O_CLOEXEC, 0600)
+	if err != nil {
+		return fmt.Errorf("failed to open PCI file [%s] for locking: %w", path, err)
+	}
+
+	errCh := make(chan error)
+	go func() {
+		// unix.LOCK_EX - Exclusive lock
+		errCh <- unix.Flock(fd, unix.LOCK_EX)
+	}()
+
+	select {
+	case err = <-errCh:
+		if err != nil {
+			return fmt.Errorf("failed to flock PCI file [%s]: %w", path, err)
+		}
+		return nil
+
+	case <-time.After(pciLockAcquireTimeout):
+		return fmt.Errorf("time out while waiting to acquire exclusive lock on [%s]", path)
+	}
+}
+
 // SaveAllocatedPCI creates a file with the pci address as a name and the network namespace as the content
 // return error if the file was not created
 func (p *PCIAllocator) SaveAllocatedPCI(pciAddress, ns string) error {