Use the allocator interface to prevent allocation of still in used pc…

…i addresses

Fixes: #219

Signed-off-by: Sebastian Sch <sebassch@gmail.com>

cmd/sriov/main.go

@@ -169,6 +169,12 @@ func cmdAdd(args *skel.CmdArgs) error {
 		return fmt.Errorf("error saving NetConf %q", err)
 	}
 
+	allocator := utils.NewPCIAllocator(config.DefaultCNIDir)
+	// Mark the pci address as in used
+	if err = allocator.SaveAllocatedPCI(netConf.DeviceID, args.Netns); err != nil {
+		return fmt.Errorf("error saving the pci allocation for vf pci address %s: %v", netConf.DeviceID, err)
+	}
+
 	return types.PrintResult(result, netConf.CNIVersion)
 }
 
@@ -231,6 +237,12 @@ func cmdDel(args *skel.CmdArgs) error {
 		return fmt.Errorf("cmdDel() error reseting VF: %q", err)
 	}
 
+	// Mark the pci address as released
+	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)
+	}
+
 	return nil
 }
 

pkg/config/config.go

@@ -36,6 +36,20 @@ func LoadConf(bytes []byte) (*sriovtypes.NetConf, error) {
 		return nil, fmt.Errorf("LoadConf(): VF pci addr is required")
 	}
 
+	allocator := utils.NewPCIAllocator(DefaultCNIDir)
+	// 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
+	// This will block the new pod creation until the cmdDel is done.
+	isAllocated, err := allocator.IsAllocated(n.DeviceID)
+	if err != nil {
+		return n, err
+	}
+
+	if isAllocated {
+		return n, fmt.Errorf("pci address %s is already allocated", n.DeviceID)
+	}
+
 	// Assuming VF is netdev interface; Get interface name(s)
 	hostIFNames, err := utils.GetVFLinkNames(n.DeviceID)
 	if err != nil || hostIFNames == "" {

pkg/config/config_test.go

@@ -1,8 +1,11 @@
 package config
 
 import (
+	"github.com/containernetworking/plugins/pkg/testutils"
+	"github.com/k8snetworkplumbingwg/sriov-cni/pkg/utils"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	"os"
 )
 
 var _ = Describe("Config", func() {
@@ -61,6 +64,49 @@ var _ = Describe("Config", func() {
 			_, err := LoadConf(conf)
 			Expect(err).To(HaveOccurred())
 		})
+
+		It("Assuming device is allocated", func() {
+			conf := []byte(`{
+        "name": "mynet",
+        "type": "sriov",
+        "deviceID": "0000:af:06.1",
+        "vf": 0,
+        "ipam": {
+            "type": "host-local",
+            "subnet": "10.55.206.0/26",
+            "routes": [
+                { "dst": "0.0.0.0/0" }
+            ],
+            "gateway": "10.55.206.1"
+        }
+                        }`)
+
+			tmpdir, err := os.MkdirTemp("/tmp", "sriovplugin-testfiles-")
+			Expect(err).ToNot(HaveOccurred())
+			originCNIDir := DefaultCNIDir
+			DefaultCNIDir = tmpdir
+			defer func() {
+				DefaultCNIDir = originCNIDir
+			}()
+
+			targetNetNS, err := testutils.NewNS()
+			Expect(err).NotTo(HaveOccurred())
+			defer func() {
+				if targetNetNS != nil {
+					targetNetNS.Close()
+					testutils.UnmountNS(targetNetNS)
+				}
+			}()
+
+			allocator := utils.NewPCIAllocator(tmpdir)
+			err = allocator.SaveAllocatedPCI("0000:af:06.1", targetNetNS.Path())
+			Expect(err).ToNot(HaveOccurred())
+
+			_, err = LoadConf(conf)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("pci address 0000:af:06.1 is already allocated"))
+		})
+
 	})
 	Context("Checking getVfInfo function", func() {
 		It("Assuming existing PF", func() {

pkg/utils/pci_allocator.go

@@ -9,15 +9,17 @@ import (
 
 type PCIAllocation interface {
 	SaveAllocatedPCI(string, string) error
-	CleanAllocatedPCI(string) error
+	DeleteAllocatedPCI(string) error
 	IsAllocated(string) error
 }
 
 type PCIAllocator struct {
 	dataDir string
 }
 
-func NewPCIAllocator(dataDir string) PCIAllocation {
+// NewPCIAllocator returns a new PCI allocator
+// it will use the <dataDir>/pci folder to store the information about allocated PCI addresses
+func NewPCIAllocator(dataDir string) *PCIAllocator {
 	return &PCIAllocator{dataDir: filepath.Join(dataDir, "pci")}
 }
 
@@ -37,9 +39,9 @@ func (p *PCIAllocator) SaveAllocatedPCI(pciAddress, ns string) error {
 	return err
 }
 
-// CleanAllocatedPCI Remove the allocated PCI file
+// DeleteAllocatedPCI Remove the allocated PCI file
 // return error if the file doesn't exist
-func (p *PCIAllocator) CleanAllocatedPCI(pciAddress string) error {
+func (p *PCIAllocator) DeleteAllocatedPCI(pciAddress string) error {
 	path := filepath.Join(p.dataDir, pciAddress)
 	if err := os.Remove(path); err != nil {
 		return fmt.Errorf("error removing PCI address lock file %s: %v", path, err)
@@ -48,36 +50,38 @@ func (p *PCIAllocator) CleanAllocatedPCI(pciAddress string) error {
 }
 
 // IsAllocated checks if the PCI address file exist
-// if it exists we also check the network namespace still exist if not we clean the allocation
+// if it exists we also check the network namespace still exist if not we delete the allocation
 // The function will return an error if the pci is still allocated to a running pod
-func (p *PCIAllocator) IsAllocated(pciAddress string) error {
+func (p *PCIAllocator) IsAllocated(pciAddress string) (bool, error) {
 	path := filepath.Join(p.dataDir, pciAddress)
 	_, err := os.Stat(path)
 	if err != nil {
 		if os.IsNotExist(err) {
-			return nil
+			return false, nil
 		}
 
-		return fmt.Errorf("failed to check for pci address file for %s: %v", path, err)
+		return false, fmt.Errorf("failed to check for pci address file for %s: %v", path, err)
 	}
 
 	dat, err := os.ReadFile(path)
 	if err != nil {
-		return fmt.Errorf("failed to read for pci address file for %s: %v", path, err)
+		return false, fmt.Errorf("failed to read for pci address file for %s: %v", path, err)
 	}
 
 	// To prevent a locking of a PCI address for every pciAddress file we also add the netns path where it's been used
 	// This way if for some reason the cmdDel command was not called but the pod namespace doesn't exist anymore
 	// we release the PCI address
-	_, err = ns.GetNS(string(dat))
+	networkNamespace, err := ns.GetNS(string(dat))
 	if err != nil {
-		err = p.CleanAllocatedPCI(pciAddress)
+		err = p.DeleteAllocatedPCI(pciAddress)
 		if err != nil {
-			return fmt.Errorf("error cleaning the pci allocation for vf pci address %s: %v", pciAddress, err)
+			return false, fmt.Errorf("error deleting the pci allocation for vf pci address %s: %v", pciAddress, err)
 		}
+
+		return false, nil
 	} else {
-		return fmt.Errorf("error the deivce is already allocated for pci address %s", pciAddress)
+		// Close the network namespace
+		networkNamespace.Close()
+		return true, nil
 	}
-
-	return nil
 }

pkg/utils/pci_allocator_test.go

@@ -0,0 +1,60 @@
+package utils
+
+import (
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+
+	"github.com/containernetworking/plugins/pkg/ns"
+	"github.com/containernetworking/plugins/pkg/testutils"
+)
+
+var _ = Describe("PCIAllocator", func() {
+	var targetNetNS ns.NetNS
+	var err error
+
+	AfterEach(func() {
+		if targetNetNS != nil {
+			targetNetNS.Close()
+			testutils.UnmountNS(targetNetNS)
+		}
+	})
+
+	Context("IsAllocated", func() {
+		It("Assuming is not allocated", func() {
+			allocator := NewPCIAllocator(ts.dirRoot)
+			isAllocated, err := allocator.IsAllocated("0000:af:00.1")
+			Expect(err).ToNot(HaveOccurred())
+			Expect(isAllocated).To(BeFalse())
+		})
+
+		It("Assuming is allocated and namespace exist", func() {
+			targetNetNS, err = testutils.NewNS()
+			Expect(err).NotTo(HaveOccurred())
+			allocator := NewPCIAllocator(ts.dirRoot)
+
+			err = allocator.SaveAllocatedPCI("0000:af:00.1", targetNetNS.Path())
+			Expect(err).ToNot(HaveOccurred())
+
+			isAllocated, err := allocator.IsAllocated("0000:af:00.1")
+			Expect(err).ToNot(HaveOccurred())
+			Expect(isAllocated).To(BeTrue())
+		})
+
+		It("Assuming is allocated and namespace doesn't exist", func() {
+			targetNetNS, err = testutils.NewNS()
+			Expect(err).NotTo(HaveOccurred())
+
+			allocator := NewPCIAllocator(ts.dirRoot)
+			err = allocator.SaveAllocatedPCI("0000:af:00.1", targetNetNS.Path())
+			Expect(err).ToNot(HaveOccurred())
+			err = targetNetNS.Close()
+			Expect(err).ToNot(HaveOccurred())
+			err = testutils.UnmountNS(targetNetNS)
+			Expect(err).ToNot(HaveOccurred())
+
+			isAllocated, err := allocator.IsAllocated("0000:af:00.1")
+			Expect(err).ToNot(HaveOccurred())
+			Expect(isAllocated).To(BeFalse())
+		})
+	})
+})

pkg/utils/utils.go

@@ -120,7 +120,7 @@ func GetPfName(vf string) (string, error) {
 	return strings.TrimSpace(files[0].Name()), nil
 }
 
-// GetPciAddress takes in a interface(ifName) and VF id and returns returns its pci addr as string
+// GetPciAddress takes in a interface(ifName) and VF id and returns its pci addr as string
 func GetPciAddress(ifName string, vf int) (string, error) {
 	var pciaddr string
 	vfDir := filepath.Join(NetDirectory, ifName, "device", fmt.Sprintf("virtfn%d", vf))
@@ -282,7 +282,7 @@ func ReadScratchNetConf(cRefPath string) ([]byte, error) {
 // CleanCachedNetConf removed cached NetConf from disk
 func CleanCachedNetConf(cRefPath string) error {
 	if err := os.Remove(cRefPath); err != nil {
-		return fmt.Errorf("error removing NetConf file %s: %q", cRefPath, err)
+		return fmt.Errorf("error removing NetConf file %s: %v", cRefPath, err)
 	}
 	return nil
 }