From 57650a1e5b5de0ee474f5fe0ff7d3be25c19c56a Mon Sep 17 00:00:00 2001 From: Bruce Ma Date: Fri, 19 Jul 2019 21:26:07 +0800 Subject: [PATCH 1/3] host-device: revert name setting to make retries idempotent Signed-off-by: Bruce Ma --- plugins/main/host-device/host-device.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/plugins/main/host-device/host-device.go b/plugins/main/host-device/host-device.go index 71c7da3e..799fafe0 100644 --- a/plugins/main/host-device/host-device.go +++ b/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 From 4b68f5682081f3c7f7156300b3d69409ac0eb1e4 Mon Sep 17 00:00:00 2001 From: Bruce Ma Date: Mon, 29 Jul 2019 20:47:28 +0800 Subject: [PATCH 2/3] host-device: add testcases for imdempotence of CmdDel Signed-off-by: Bruce Ma --- plugins/main/host-device/host-device_test.go | 276 +++++++++++++++++++ 1 file changed, 276 insertions(+) diff --git a/plugins/main/host-device/host-device_test.go b/plugins/main/host-device/host-device_test.go index f7f83d63..178b3b78 100644 --- a/plugins/main/host-device/host-device_test.go +++ b/plugins/main/host-device/host-device_test.go @@ -328,6 +328,144 @@ var _ = Describe("base functionality", func() { }) + It("Test idempotence of CmdDel", 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.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 @@ -705,4 +843,142 @@ var _ = Describe("base functionality", func() { }) + 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 + }) + }) }) From 5e2e3652912a71dec04c3256e722caed912d1929 Mon Sep 17 00:00:00 2001 From: Bruce Ma Date: Mon, 29 Jul 2019 21:09:03 +0800 Subject: [PATCH 3/3] host-device: remove useless Expects in testcases Signed-off-by: Bruce Ma --- plugins/main/host-device/host-device_test.go | 52 ++++++-------------- 1 file changed, 16 insertions(+), 36 deletions(-) diff --git a/plugins/main/host-device/host-device_test.go b/plugins/main/host-device/host-device_test.go index 178b3b78..b6b46b1c 100644 --- a/plugins/main/host-device/host-device_test.go +++ b/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,8 +321,6 @@ var _ = Describe("base functionality", func() { Expect(err).NotTo(HaveOccurred()) return nil }) - Expect(err).NotTo(HaveOccurred()) - }) It("Test idempotence of CmdDel", func() { @@ -470,7 +465,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{ @@ -484,7 +479,6 @@ var _ = Describe("base functionality", func() { Expect(err).NotTo(HaveOccurred()) return nil }) - Expect(err).NotTo(HaveOccurred()) // call CmdAdd targetNS, err := testutils.NewNS() @@ -532,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()) @@ -547,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) @@ -570,8 +562,6 @@ var _ = Describe("base functionality", func() { Expect(err).NotTo(HaveOccurred()) return nil }) - Expect(err).NotTo(HaveOccurred()) - }) It("fails an invalid config", func() { @@ -596,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{ @@ -610,7 +600,6 @@ var _ = Describe("base functionality", func() { Expect(err).NotTo(HaveOccurred()) return nil }) - Expect(err).NotTo(HaveOccurred()) // call CmdAdd targetNS, err := testutils.NewNS() @@ -650,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{} @@ -693,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) @@ -704,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{ @@ -726,7 +711,6 @@ var _ = Describe("base functionality", func() { Expect(err).NotTo(HaveOccurred()) return nil }) - Expect(err).NotTo(HaveOccurred()) // call CmdAdd targetNS, err := testutils.NewNS() @@ -774,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()) @@ -789,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{} @@ -828,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) @@ -839,8 +821,6 @@ var _ = Describe("base functionality", func() { Expect(err).NotTo(HaveOccurred()) return nil }) - Expect(err).NotTo(HaveOccurred()) - }) It("Test idempotence of CmdDel with 0.4.0 config", func() {