From a10ffb173a746ee08a735e8f8f58ad745ecc0ace Mon Sep 17 00:00:00 2001 From: Marcelo Guerrero Date: Thu, 15 May 2025 13:15:16 +0200 Subject: [PATCH] Avoid handling mac duplication during deletion The bond-cni should not attempt to modify the mac addresses of slaves during deletion of the bond interface. The bond driver takes care of reverting any modifications. Only balance-tlb and balance-alb do not support slaves with same mac addresses. The plugin now handles mac duplicates only for these modes. Signed-off-by: Marcelo Guerrero --- bond/bond.go | 32 +++++++++++--------------- bond/bond_test.go | 51 ----------------------------------------- bond/util/validation.go | 1 + 3 files changed, 14 insertions(+), 70 deletions(-) diff --git a/bond/bond.go b/bond/bond.go index 618f9378..bf231188 100644 --- a/bond/bond.go +++ b/bond/bond.go @@ -29,9 +29,10 @@ import ( "github.com/containernetworking/cni/pkg/version" "github.com/containernetworking/plugins/pkg/ipam" "github.com/containernetworking/plugins/pkg/ns" - "github.com/intel/bond-cni/bond/util" "github.com/vishvananda/netlink" "github.com/vishvananda/netns" + + "github.com/intel/bond-cni/bond/util" ) type bondingConfig struct { @@ -143,10 +144,15 @@ func createBondedLink(bondName string, bondConf *bondingConfig, netNsHandle *net // loop over the linkObjectsToBond, set each DOWN, update the interface MASTER & set it UP again. // again we use the netNsHandle to interfact with these links in the namespace provided. return error -func attachLinksToBond(bondLinkObj *netlink.Bond, linkObjectsToBond []netlink.Link, netNsHandle *netlink.Handle) error { - err := util.HandleMacDuplicates(linkObjectsToBond, netNsHandle) - if err != nil { - return fmt.Errorf("failed to handle duplicated macs on link slaves, error: %+v", err) +func attachLinksToBond(bondLinkObj *netlink.Bond, linkObjectsToBond []netlink.Link, mode string, netNsHandle *netlink.Handle) error { + var err error + + bondMode := netlink.StringToBondMode(mode) + if bondMode == netlink.BOND_MODE_BALANCE_TLB || bondMode == netlink.BOND_MODE_BALANCE_ALB { + err = util.HandleMacDuplicates(linkObjectsToBond, netNsHandle) + if err != nil { + return fmt.Errorf("failed to handle duplicated macs on link slaves, error: %+v", err) + } } bondLinkIndex := bondLinkObj.Index @@ -283,12 +289,13 @@ func createBond(bondName string, bondConf *bondingConfig, nspath string, ns ns.N if bondConf.FailOverMac < 0 || bondConf.FailOverMac > 2 { return nil, fmt.Errorf("FailOverMac mode should be 0, 1 or 2 actual: %+v", bondConf.FailOverMac) } + bondLinkObj, err := createBondedLink(bondName, bondConf, netNsHandle) if err != nil { return nil, fmt.Errorf("failed to create bonded link (%+v), error: %+v", bondName, err) } - err = attachLinksToBond(bondLinkObj, linkObjectsToBond, netNsHandle) + err = attachLinksToBond(bondLinkObj, linkObjectsToBond, bondConf.Mode, netNsHandle) if err != nil { return nil, fmt.Errorf("failed to attached links to bond, error: %+v", err) } @@ -433,19 +440,6 @@ func cmdDel(args *skel.CmdArgs) error { return fmt.Errorf("failed to deattached links from bond, error: %+v", err) } - // Fetch slave links again to have the latest state - // For instance, Active-backup mode with fail_over_mac=0 reverts the mac address of backup slaves - for i := range linkObjectsToDeattach { - linkObjectsToDeattach[i], err = netNsHandle.LinkByName(linkObjectsToDeattach[i].Attrs().Name) - if err != nil { - return fmt.Errorf("failed to find link (%+v), error: %+v", linkObjectsToDeattach[i].Attrs().Name, err) - } - } - - if err = util.HandleMacDuplicates(linkObjectsToDeattach, netNsHandle); err != nil { - return fmt.Errorf("failed to validate deattached links macs, error: %+v", err) - } - err = netNsHandle.LinkDel(linkObjToDel) if err != nil { return fmt.Errorf("failed to delete bonded link (%+v), error: %+v", linkObjToDel.Attrs().Name, err) diff --git a/bond/bond_test.go b/bond/bond_test.go index ec253d57..9d1bac40 100644 --- a/bond/bond_test.go +++ b/bond/bond_test.go @@ -228,57 +228,6 @@ var _ = Describe("bond plugin", func() { Expect(err).NotTo(HaveOccurred()) }) - It("verifies the plugin handles duplicated macs on delete", func() { - var slave1, slave2 netlink.Link - var err error - - err = podNS.Do(func(ns.NetNS) error { - defer GinkgoRecover() - slave1, err = netlink.LinkByName(Slave1) - Expect(err).NotTo(HaveOccurred()) - - slave2, err = netlink.LinkByName(Slave2) - Expect(err).NotTo(HaveOccurred()) - - err = netlink.LinkSetHardwareAddr(slave2, slave1.Attrs().HardwareAddr) - Expect(err).NotTo(HaveOccurred()) - return nil - }) - - By("creating the plugin") - r, _, err := testutils.CmdAddWithArgs(args, func() error { - return cmdAdd(args) - }) - Expect(err).NotTo(HaveOccurred()) - - By("checking the bond was created") - checkAddReturnResult(&r, IfName) - - err = podNS.Do(func(ns.NetNS) error { - defer GinkgoRecover() - By("duplicating the macs on the slaves") - err = netlink.LinkSetHardwareAddr(slave2, slave1.Attrs().HardwareAddr) - Expect(err).NotTo(HaveOccurred()) - return nil - }) - By("deleting the plugin") - err = testutils.CmdDel(podNS.Path(), - args.ContainerID, "", func() error { return cmdDel(args) }) - Expect(err).NotTo(HaveOccurred()) - - err = podNS.Do(func(ns.NetNS) error { - defer GinkgoRecover() - By("validating the macs are not duplicated") - slave1, err = netlink.LinkByName(Slave1) - Expect(err).NotTo(HaveOccurred()) - slave2, err = netlink.LinkByName(Slave2) - Expect(err).NotTo(HaveOccurred()) - Expect(slave1.Attrs().HardwareAddr.String()).NotTo(Equal(slave2.Attrs().HardwareAddr.String())) - return nil - }) - Expect(err).NotTo(HaveOccurred()) - }) - It("verifies that mac addresses are restored correctly in active-backup with fail_over_mac 0", func() { var bond netlink.Link var slave1 netlink.Link diff --git a/bond/util/validation.go b/bond/util/validation.go index d2971a63..16c2269b 100644 --- a/bond/util/validation.go +++ b/bond/util/validation.go @@ -4,6 +4,7 @@ import ( "bytes" "crypto/rand" "fmt" + "github.com/vishvananda/netlink" )