Merge pull request #357 from mars1024/bugfix/host-device

host-device: revert name setting to make retries idempotent

plugins/main/host-device/host-device.go

@@ -218,14 +218,23 @@ func moveLinkOut(containerNs ns.NetNS, ifName string) error {
 		}
 
 		// Devices can be renamed only when down
-		if err := netlink.LinkSetDown(dev); err != nil {
+		if err = netlink.LinkSetDown(dev); err != nil {
 			return fmt.Errorf("failed to set %q down: %v", ifName, err)
 		}
+
 		// Rename device to it's original name
-		if err := netlink.LinkSetName(dev, dev.Attrs().Alias); err != nil {
+		if err = netlink.LinkSetName(dev, dev.Attrs().Alias); err != nil {
 			return fmt.Errorf("failed to restore %q to original name %q: %v", ifName, dev.Attrs().Alias, err)
 		}
-		if err := netlink.LinkSetNsFd(dev, int(defaultNs.Fd())); err != nil {
+		defer func() {
+			if err != nil {
+				// if moving device to host namespace fails, we should revert device name
+				// to ifName to make sure that device can be found in retries
+				_ = netlink.LinkSetName(dev, ifName)
+			}
+		}()
+
+		if err = netlink.LinkSetNsFd(dev, int(defaultNs.Fd())); err != nil {
 			return fmt.Errorf("failed to move %q to host netns: %v", dev.Attrs().Alias, err)
 		}
 		return nil

plugins/main/host-device/host-device_test.go

@@ -240,7 +240,7 @@ var _ = Describe("base functionality", func() {
 		var origLink netlink.Link
 
 		// prepare ifname in original namespace
-		err := originalNS.Do(func(ns.NetNS) error {
+		_ = originalNS.Do(func(ns.NetNS) error {
 			defer GinkgoRecover()
 			err := netlink.LinkAdd(&netlink.Dummy{
 				LinkAttrs: netlink.LinkAttrs{
@@ -254,7 +254,6 @@ var _ = Describe("base functionality", func() {
 			Expect(err).NotTo(HaveOccurred())
 			return nil
 		})
-		Expect(err).NotTo(HaveOccurred())
 
 		// call CmdAdd
 		targetNS, err := testutils.NewNS()
@@ -294,26 +293,24 @@ var _ = Describe("base functionality", func() {
 		}))
 
 		// assert that dummy0 is now in the target namespace
-		err = targetNS.Do(func(ns.NetNS) error {
+		_ = targetNS.Do(func(ns.NetNS) error {
 			defer GinkgoRecover()
 			link, err := netlink.LinkByName(cniName)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(link.Attrs().HardwareAddr).To(Equal(origLink.Attrs().HardwareAddr))
 			return nil
 		})
-		Expect(err).NotTo(HaveOccurred())
 
 		// assert that dummy0 is now NOT in the original namespace anymore
-		err = originalNS.Do(func(ns.NetNS) error {
+		_ = originalNS.Do(func(ns.NetNS) error {
 			defer GinkgoRecover()
 			_, err := netlink.LinkByName(ifname)
 			Expect(err).To(HaveOccurred())
 			return nil
 		})
-		Expect(err).NotTo(HaveOccurred())
 
 		// Check that deleting the device moves it back and restores the name
-		err = originalNS.Do(func(ns.NetNS) error {
+		_ = originalNS.Do(func(ns.NetNS) error {
 			defer GinkgoRecover()
 			err = testutils.CmdDelWithArgs(args, func() error {
 				return cmdDel(args)
@@ -324,15 +321,16 @@ var _ = Describe("base functionality", func() {
 			Expect(err).NotTo(HaveOccurred())
 			return nil
 		})
-		Expect(err).NotTo(HaveOccurred())
-
 	})
 
-	It("Works with a valid config with IPAM", func() {
+	It("Test idempotence of CmdDel", func() {
-		var origLink netlink.Link
+		var (
+			origLink     netlink.Link
+			conflictLink netlink.Link
+		)
 
-		// prepare ifname in original namespace
+		// prepare host device in original namespace
-		err := originalNS.Do(func(ns.NetNS) error {
+		_ = originalNS.Do(func(ns.NetNS) error {
 			defer GinkgoRecover()
 			err := netlink.LinkAdd(&netlink.Dummy{
 				LinkAttrs: netlink.LinkAttrs{
@@ -346,8 +344,142 @@ var _ = Describe("base functionality", func() {
 			Expect(err).NotTo(HaveOccurred())
 			return nil
 		})
+
+		// call CmdAdd
+		targetNS, err := testutils.NewNS()
 		Expect(err).NotTo(HaveOccurred())
 
+		cniName := "eth0"
+		conf := fmt.Sprintf(`{
+			"cniVersion": "0.3.0",
+			"name": "cni-plugin-host-device-test",
+			"type": "host-device",
+			"device": %q
+		}`, ifname)
+		args := &skel.CmdArgs{
+			ContainerID: "dummy",
+			Netns:       targetNS.Path(),
+			IfName:      cniName,
+			StdinData:   []byte(conf),
+		}
+		var resI types.Result
+		err = originalNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+			var err error
+			resI, _, err = testutils.CmdAddWithArgs(args, func() error { return cmdAdd(args) })
+			return err
+		})
+		Expect(err).NotTo(HaveOccurred())
+
+		// check that the result is sane
+		res, err := current.NewResultFromResult(resI)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(res.Interfaces).To(Equal([]*current.Interface{
+			{
+				Name:    cniName,
+				Mac:     origLink.Attrs().HardwareAddr.String(),
+				Sandbox: targetNS.Path(),
+			},
+		}))
+
+		// assert that dummy0 is now in the target namespace
+		_ = targetNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+			link, err := netlink.LinkByName(cniName)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(link.Attrs().HardwareAddr).To(Equal(origLink.Attrs().HardwareAddr))
+			return nil
+		})
+
+		// assert that dummy0 is now NOT in the original namespace anymore
+		_ = originalNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+			_, err := netlink.LinkByName(ifname)
+			Expect(err).To(HaveOccurred())
+			return nil
+		})
+
+		// create another conflict host device with same name "dummy0"
+		_ = originalNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+			err := netlink.LinkAdd(&netlink.Dummy{
+				LinkAttrs: netlink.LinkAttrs{
+					Name: ifname,
+				},
+			})
+			Expect(err).NotTo(HaveOccurred())
+			conflictLink, err = netlink.LinkByName(ifname)
+			Expect(err).NotTo(HaveOccurred())
+			err = netlink.LinkSetUp(conflictLink)
+			Expect(err).NotTo(HaveOccurred())
+			return nil
+		})
+
+		// try to call CmdDel and fails
+		_ = originalNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+			err = testutils.CmdDelWithArgs(args, func() error {
+				return cmdDel(args)
+			})
+			Expect(err).To(HaveOccurred())
+			return nil
+		})
+
+		// assert container interface "eth0" still exists in target namespace
+		_ = targetNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+			link, err := netlink.LinkByName(cniName)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(link.Attrs().HardwareAddr).To(Equal(origLink.Attrs().HardwareAddr))
+			return nil
+		})
+
+		// remove the conflict host device
+		_ = originalNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+			err = netlink.LinkDel(conflictLink)
+			Expect(err).NotTo(HaveOccurred())
+			return nil
+		})
+
+		// try to call CmdDel and succeed
+		_ = originalNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+			err = testutils.CmdDelWithArgs(args, func() error {
+				return cmdDel(args)
+			})
+			Expect(err).NotTo(HaveOccurred())
+			return nil
+		})
+
+		// assert that dummy0 is now back in the original namespace
+		_ = originalNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+			_, err := netlink.LinkByName(ifname)
+			Expect(err).NotTo(HaveOccurred())
+			return nil
+		})
+	})
+
+	It("Works with a valid config with IPAM", func() {
+		var origLink netlink.Link
+
+		// prepare ifname in original namespace
+		_ = originalNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+			err := netlink.LinkAdd(&netlink.Dummy{
+				LinkAttrs: netlink.LinkAttrs{
+					Name: ifname,
+				},
+			})
+			Expect(err).NotTo(HaveOccurred())
+			origLink, err = netlink.LinkByName(ifname)
+			Expect(err).NotTo(HaveOccurred())
+			err = netlink.LinkSetUp(origLink)
+			Expect(err).NotTo(HaveOccurred())
+			return nil
+		})
+
 		// call CmdAdd
 		targetNS, err := testutils.NewNS()
 		Expect(err).NotTo(HaveOccurred())
@@ -394,7 +526,7 @@ var _ = Describe("base functionality", func() {
 		}))
 
 		// assert that dummy0 is now in the target namespace
-		err = targetNS.Do(func(ns.NetNS) error {
+		_ = targetNS.Do(func(ns.NetNS) error {
 			defer GinkgoRecover()
 			link, err := netlink.LinkByName(cniName)
 			Expect(err).NotTo(HaveOccurred())
@@ -409,19 +541,17 @@ var _ = Describe("base functionality", func() {
 
 			return nil
 		})
-		Expect(err).NotTo(HaveOccurred())
 
 		// assert that dummy0 is now NOT in the original namespace anymore
-		err = originalNS.Do(func(ns.NetNS) error {
+		_ = originalNS.Do(func(ns.NetNS) error {
 			defer GinkgoRecover()
 			_, err := netlink.LinkByName(ifname)
 			Expect(err).To(HaveOccurred())
 			return nil
 		})
-		Expect(err).NotTo(HaveOccurred())
 
 		// Check that deleting the device moves it back and restores the name
-		err = originalNS.Do(func(ns.NetNS) error {
+		_ = originalNS.Do(func(ns.NetNS) error {
 			defer GinkgoRecover()
 			err = testutils.CmdDelWithArgs(args, func() error {
 				return cmdDel(args)
@@ -432,8 +562,6 @@ var _ = Describe("base functionality", func() {
 			Expect(err).NotTo(HaveOccurred())
 			return nil
 		})
-		Expect(err).NotTo(HaveOccurred())
-
 	})
 
 	It("fails an invalid config", func() {
@@ -458,7 +586,7 @@ var _ = Describe("base functionality", func() {
 		var origLink netlink.Link
 
 		// prepare ifname in original namespace
-		err := originalNS.Do(func(ns.NetNS) error {
+		_ = originalNS.Do(func(ns.NetNS) error {
 			defer GinkgoRecover()
 			err := netlink.LinkAdd(&netlink.Dummy{
 				LinkAttrs: netlink.LinkAttrs{
@@ -472,7 +600,6 @@ var _ = Describe("base functionality", func() {
 			Expect(err).NotTo(HaveOccurred())
 			return nil
 		})
-		Expect(err).NotTo(HaveOccurred())
 
 		// call CmdAdd
 		targetNS, err := testutils.NewNS()
@@ -512,23 +639,21 @@ var _ = Describe("base functionality", func() {
 		}))
 
 		// assert that dummy0 is now in the target namespace
-		err = targetNS.Do(func(ns.NetNS) error {
+		_ = targetNS.Do(func(ns.NetNS) error {
 			defer GinkgoRecover()
 			link, err := netlink.LinkByName(cniName)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(link.Attrs().HardwareAddr).To(Equal(origLink.Attrs().HardwareAddr))
 			return nil
 		})
-		Expect(err).NotTo(HaveOccurred())
 
 		// assert that dummy0 is now NOT in the original namespace anymore
-		err = originalNS.Do(func(ns.NetNS) error {
+		_ = originalNS.Do(func(ns.NetNS) error {
 			defer GinkgoRecover()
 			_, err := netlink.LinkByName(ifname)
 			Expect(err).To(HaveOccurred())
 			return nil
 		})
-		Expect(err).NotTo(HaveOccurred())
 
 		// call CmdCheck
 		n := &Net{}
@@ -555,7 +680,7 @@ var _ = Describe("base functionality", func() {
 		Expect(err).NotTo(HaveOccurred())
 
 		// Check that deleting the device moves it back and restores the name
-		err = originalNS.Do(func(ns.NetNS) error {
+		_ = originalNS.Do(func(ns.NetNS) error {
 			defer GinkgoRecover()
 			err = testutils.CmdDelWithArgs(args, func() error {
 				return cmdDel(args)
@@ -566,15 +691,13 @@ var _ = Describe("base functionality", func() {
 			Expect(err).NotTo(HaveOccurred())
 			return nil
 		})
-		Expect(err).NotTo(HaveOccurred())
-
 	})
 
 	It("Works with a valid 0.4.0 config with IPAM", func() {
 		var origLink netlink.Link
 
 		// prepare ifname in original namespace
-		err := originalNS.Do(func(ns.NetNS) error {
+		_ = originalNS.Do(func(ns.NetNS) error {
 			defer GinkgoRecover()
 			err := netlink.LinkAdd(&netlink.Dummy{
 				LinkAttrs: netlink.LinkAttrs{
@@ -588,7 +711,6 @@ var _ = Describe("base functionality", func() {
 			Expect(err).NotTo(HaveOccurred())
 			return nil
 		})
-		Expect(err).NotTo(HaveOccurred())
 
 		// call CmdAdd
 		targetNS, err := testutils.NewNS()
@@ -636,7 +758,7 @@ var _ = Describe("base functionality", func() {
 		}))
 
 		// assert that dummy0 is now in the target namespace
-		err = targetNS.Do(func(ns.NetNS) error {
+		_ = targetNS.Do(func(ns.NetNS) error {
 			defer GinkgoRecover()
 			link, err := netlink.LinkByName(cniName)
 			Expect(err).NotTo(HaveOccurred())
@@ -651,16 +773,14 @@ var _ = Describe("base functionality", func() {
 
 			return nil
 		})
-		Expect(err).NotTo(HaveOccurred())
 
 		// assert that dummy0 is now NOT in the original namespace anymore
-		err = originalNS.Do(func(ns.NetNS) error {
+		_ = originalNS.Do(func(ns.NetNS) error {
 			defer GinkgoRecover()
 			_, err := netlink.LinkByName(ifname)
 			Expect(err).To(HaveOccurred())
 			return nil
 		})
-		Expect(err).NotTo(HaveOccurred())
 
 		// call CmdCheck
 		n := &Net{}
@@ -690,7 +810,7 @@ var _ = Describe("base functionality", func() {
 		Expect(err).NotTo(HaveOccurred())
 
 		// Check that deleting the device moves it back and restores the name
-		err = originalNS.Do(func(ns.NetNS) error {
+		_ = originalNS.Do(func(ns.NetNS) error {
 			defer GinkgoRecover()
 			err = testutils.CmdDelWithArgs(args, func() error {
 				return cmdDel(args)
@@ -701,8 +821,144 @@ var _ = Describe("base functionality", func() {
 			Expect(err).NotTo(HaveOccurred())
 			return nil
 		})
+	})
+
+	It("Test idempotence of CmdDel with 0.4.0 config", func() {
+		var (
+			origLink     netlink.Link
+			conflictLink netlink.Link
+		)
+
+		// prepare host device in original namespace
+		_ = originalNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+			err := netlink.LinkAdd(&netlink.Dummy{
+				LinkAttrs: netlink.LinkAttrs{
+					Name: ifname,
+				},
+			})
+			Expect(err).NotTo(HaveOccurred())
+			origLink, err = netlink.LinkByName(ifname)
+			Expect(err).NotTo(HaveOccurred())
+			err = netlink.LinkSetUp(origLink)
+			Expect(err).NotTo(HaveOccurred())
+			return nil
+		})
+
+		// call CmdAdd
+		targetNS, err := testutils.NewNS()
 		Expect(err).NotTo(HaveOccurred())
 
+		cniName := "eth0"
+		conf := fmt.Sprintf(`{
+			"cniVersion": "0.4.0",
+			"name": "cni-plugin-host-device-test",
+			"type": "host-device",
+			"device": %q
+		}`, ifname)
+		args := &skel.CmdArgs{
+			ContainerID: "dummy",
+			Netns:       targetNS.Path(),
+			IfName:      cniName,
+			StdinData:   []byte(conf),
+		}
+		var resI types.Result
+		err = originalNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+			var err error
+			resI, _, err = testutils.CmdAddWithArgs(args, func() error { return cmdAdd(args) })
+			return err
+		})
+		Expect(err).NotTo(HaveOccurred())
+
+		// check that the result is sane
+		res, err := current.NewResultFromResult(resI)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(res.Interfaces).To(Equal([]*current.Interface{
+			{
+				Name:    cniName,
+				Mac:     origLink.Attrs().HardwareAddr.String(),
+				Sandbox: targetNS.Path(),
+			},
+		}))
+
+		// assert that dummy0 is now in the target namespace
+		_ = targetNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+			link, err := netlink.LinkByName(cniName)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(link.Attrs().HardwareAddr).To(Equal(origLink.Attrs().HardwareAddr))
+			return nil
 		})
 
+		// assert that dummy0 is now NOT in the original namespace anymore
+		_ = originalNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+			_, err := netlink.LinkByName(ifname)
+			Expect(err).To(HaveOccurred())
+			return nil
+		})
+
+		// create another conflict host device with same name "dummy0"
+		_ = originalNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+			err := netlink.LinkAdd(&netlink.Dummy{
+				LinkAttrs: netlink.LinkAttrs{
+					Name: ifname,
+				},
+			})
+			Expect(err).NotTo(HaveOccurred())
+			conflictLink, err = netlink.LinkByName(ifname)
+			Expect(err).NotTo(HaveOccurred())
+			err = netlink.LinkSetUp(conflictLink)
+			Expect(err).NotTo(HaveOccurred())
+			return nil
+		})
+
+		// try to call CmdDel and fails
+		_ = originalNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+			err = testutils.CmdDelWithArgs(args, func() error {
+				return cmdDel(args)
+			})
+			Expect(err).To(HaveOccurred())
+			return nil
+		})
+
+		// assert container interface "eth0" still exists in target namespace
+		err = targetNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+			link, err := netlink.LinkByName(cniName)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(link.Attrs().HardwareAddr).To(Equal(origLink.Attrs().HardwareAddr))
+			return nil
+		})
+		Expect(err).NotTo(HaveOccurred())
+
+		// remove the conflict host device
+		_ = originalNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+			err = netlink.LinkDel(conflictLink)
+			Expect(err).NotTo(HaveOccurred())
+			return nil
+		})
+
+		// try to call CmdDel and succeed
+		_ = originalNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+			err = testutils.CmdDelWithArgs(args, func() error {
+				return cmdDel(args)
+			})
+			Expect(err).NotTo(HaveOccurred())
+			return nil
+		})
+
+		// assert that dummy0 is now back in the original namespace
+		_ = originalNS.Do(func(ns.NetNS) error {
+			defer GinkgoRecover()
+			_, err := netlink.LinkByName(ifname)
+			Expect(err).NotTo(HaveOccurred())
+			return nil
+		})
+	})
 })