Merge pull request #222 from Nordix/vf-stuck-issue/adil

Reset the VF if failure occurs before the netconf is cached
k8snetworkplumbingwg · Nov 22, 2022 · 7eb235b · 7eb235b
2 parents 2aea1bb + e92d077
commit 7eb235b
Show file tree

Hide file tree

Showing 3 changed files with 80 additions and 23 deletions.
diff --git a/cmd/sriov/main.go b/cmd/sriov/main.go
@@ -74,6 +74,23 @@ func cmdAdd(args *skel.CmdArgs) error {
 	defer netns.Close()
 
 	sm := sriov.NewSriovManager()
+	err = sm.FillOriginalVfInfo(netConf)
+	if err != nil {
+		return fmt.Errorf("failed to get original vf information: %v", err)
+	}
+	defer func() {
+		if err != nil {
+			err := netns.Do(func(_ ns.NetNS) error {
+				_, err := netlink.LinkByName(args.IfName)
+				return err
+			})
+			if err == nil {
+				_ = sm.ReleaseVF(netConf, args.IfName, args.ContainerID, netns)
+			}
+			// Reset the VF if failure occurs before the netconf is cached
+			_ = sm.ResetVFConfig(netConf)
+		}
+	}()
 	if err := sm.ApplyVFConfig(netConf); err != nil {
 		return fmt.Errorf("SRIOV-CNI failed to configure VF %q", err)
 	}
@@ -86,17 +103,7 @@ func cmdAdd(args *skel.CmdArgs) error {
 
 	if !netConf.DPDKMode {
 		macAddr, err = sm.SetupVF(netConf, args.IfName, args.ContainerID, netns)
-		defer func() {
-			if err != nil {
-				err := netns.Do(func(_ ns.NetNS) error {
-					_, err := netlink.LinkByName(args.IfName)
-					return err
-				})
-				if err == nil {
-					_ = sm.ReleaseVF(netConf, args.IfName, args.ContainerID, netns)
-				}
-			}
-		}()
+
 		if err != nil {
 			return fmt.Errorf("failed to set up pod interface %q from the device %q: %v", args.IfName, netConf.Master, err)
 		}
@@ -105,7 +112,8 @@ func cmdAdd(args *skel.CmdArgs) error {
 
 	// run the IPAM plugin
 	if netConf.IPAM.Type != "" {
-		r, err := ipam.ExecAdd(netConf.IPAM.Type, args.StdinData)
+		var r types.Result
+		r, err = ipam.ExecAdd(netConf.IPAM.Type, args.StdinData)
 		if err != nil {
 			return fmt.Errorf("failed to set up IPAM plugin type %q from the device %q: %v", netConf.IPAM.Type, netConf.Master, err)
 		}
@@ -117,13 +125,15 @@ func cmdAdd(args *skel.CmdArgs) error {
 		}()
 
 		// Convert the IPAM result into the current Result type
-		newResult, err := current.NewResultFromResult(r)
+		var newResult *current.Result
+		newResult, err = current.NewResultFromResult(r)
 		if err != nil {
 			return err
 		}
 
 		if len(newResult.IPs) == 0 {
-			return errors.New("IPAM plugin returned missing IP config")
+			err = errors.New("IPAM plugin returned missing IP config")
+			return err
 		}
 
 		newResult.Interfaces = result.Interfaces

diff --git a/pkg/sriov/sriov.go b/pkg/sriov/sriov.go
@@ -42,6 +42,7 @@ type Manager interface {
 	ReleaseVF(conf *sriovtypes.NetConf, podifName string, cid string, netns ns.NetNS) error
 	ResetVFConfig(conf *sriovtypes.NetConf) error
 	ApplyVFConfig(conf *sriovtypes.NetConf) error
+	FillOriginalVfInfo(conf *sriovtypes.NetConf) error
 }
 
 type sriovManager struct {
@@ -191,14 +192,6 @@ func (s *sriovManager) ApplyVFConfig(conf *sriovtypes.NetConf) error {
 	if err != nil {
 		return fmt.Errorf("failed to lookup master %q: %v", conf.Master, err)
 	}
-
-	// Save current the VF state before modifying it
-	vfState := getVfInfo(pfLink, conf.VFID)
-	if vfState == nil {
-		return fmt.Errorf("failed to find vf %d", conf.VFID)
-	}
-	conf.OrigVfState.FillFromVfInfo(vfState)
-
 	// 1. Set vlan
 	if conf.Vlan == nil {
 		vlan := new(int)
@@ -293,6 +286,21 @@ func (s *sriovManager) ApplyVFConfig(conf *sriovtypes.NetConf) error {
 	return nil
 }
 
+// FillOriginalVfInfo fills the original vf info
+func (s *sriovManager) FillOriginalVfInfo(conf *sriovtypes.NetConf) error {
+	pfLink, err := s.nLink.LinkByName(conf.Master)
+	if err != nil {
+		return fmt.Errorf("failed to lookup master %q: %v", conf.Master, err)
+	}
+	// Save current the VF state before modifying it
+	vfState := getVfInfo(pfLink, conf.VFID)
+	if vfState == nil {
+		return fmt.Errorf("failed to find vf %d", conf.VFID)
+	}
+	conf.OrigVfState.FillFromVfInfo(vfState)
+	return err
+}
+
 // ResetVFConfig reset a VF to its original state
 func (s *sriovManager) ResetVFConfig(conf *sriovtypes.NetConf) error {
 	pfLink, err := s.nLink.LinkByName(conf.Master)

diff --git a/pkg/sriov/sriov_test.go b/pkg/sriov/sriov_test.go
@@ -223,6 +223,46 @@ var _ = Describe("Sriov", func() {
 			mocked.AssertExpectations(t)
 		})
 	})
+	Context("Checking FillOriginalVfInfo function", func() {
+		var (
+			netconf *sriovtypes.NetConf
+		)
+
+		BeforeEach(func() {
+			netconf = &sriovtypes.NetConf{
+				Master:      "enp175s0f1",
+				DeviceID:    "0000:af:06.0",
+				VFID:        0,
+				ContIFNames: "net1",
+				OrigVfState: sriovtypes.VfState{
+					HostIFName: "enp175s6",
+				},
+			}
+		})
+		It("Saves the current VF state", func() {
+			mocked := &mocks_utils.NetlinkManager{}
+			//fakeLink := &FakeLink{netlink.LinkAttrs{Index: 1000, Name: "dummylink"}}
+			fakeMac, err := net.ParseMAC("6e:16:06:0e:b7:e9")
+			Expect(err).NotTo(HaveOccurred())
+
+			fakeLink := &FakeLink{netlink.LinkAttrs{
+				Index:        1000,
+				Name:         "dummylink",
+				HardwareAddr: fakeMac,
+				Vfs: []netlink.VfInfo{
+					{
+						ID:  0,
+						Mac: net.HardwareAddr(fakeMac),
+					},
+				},
+			}}
+			mocked.On("LinkByName", netconf.Master).Return(fakeLink, nil)
+			sm := sriovManager{nLink: mocked}
+			err = sm.FillOriginalVfInfo(netconf)
+			Expect(err).NotTo(HaveOccurred())
+			mocked.AssertExpectations(t)
+		})
+	})
 	Context("Checking ResetVFConfig function - restore config no user params", func() {
 		var (
 			netconf *sriovtypes.NetConf
@@ -250,7 +290,6 @@ var _ = Describe("Sriov", func() {
 			mocked.AssertExpectations(t)
 		})
 	})
-
 	Context("Checking ResetVFConfig function - restore config with user params", func() {
 		var (
 			netconf *sriovtypes.NetConf