Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

macvlan: support chaining for master interface #903

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions plugins/main/macvlan/macvlan.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,12 +107,35 @@ func loadConf(args *skel.CmdArgs, envArgs string) (*NetConf, string, error) {
if err := json.Unmarshal(args.StdinData, n); err != nil {
return nil, "", fmt.Errorf("failed to load netconf: %v", err)
}
if n.Master == "" {
defaultRouteInterface, err := getNamespacedDefaultRouteInterfaceName(args.Netns, n.LinkContNs)

var result *current.Result
var err error
// Parse previous result
if n.NetConf.RawPrevResult != nil {
if err = version.ParsePrevResult(&n.NetConf); err != nil {
return nil, "", fmt.Errorf("could not parse prevResult: %v", err)
}

result, err = current.NewResultFromResult(n.PrevResult)
if err != nil {
return nil, "", err
return nil, "", fmt.Errorf("could not convert result to current version: %v", err)
}
}
if n.Master == "" {
if result == nil {
var defaultRouteInterface string
defaultRouteInterface, err = getNamespacedDefaultRouteInterfaceName(args.Netns, n.LinkContNs)
if err != nil {
return nil, "", err
}
n.Master = defaultRouteInterface
} else {
if len(result.Interfaces) == 1 && result.Interfaces[0].Name != "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than just picking the first interface, what if we were a bit cleverer?

Right now, the macvlan plugin has the option to pick from the host namespace or container namespace. Let's preserve that when it is chained. Specifically, if n.LinkContNS is true, then pick the first value in prevResult where sandbox == true. Even better would be to pick the first result with routes or an IP address. Likewise, if n.LinkContNS is false, then do the same but for host-side interfaces.

Makes sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, this brings up the interesting idea that, if n.LinkContNS is true, we should just use whatever interface name matches CNI_IFNAME, then we should rename that out and create our interface to CNI_IFNAME. Make sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmirecki any thoughts here?

Copy link
Author

@firemiles firemiles Jul 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than just picking the first interface, what if we were a bit cleverer?

Right now, the macvlan plugin has the option to pick from the host namespace or container namespace. Let's preserve that when it is chained. Specifically, if n.LinkContNS is true, then pick the first value in prevResult where sandbox == true. Even better would be to pick the first result with routes or an IP address. Likewise, if n.LinkContNS is false, then do the same but for host-side interfaces.

Makes sense?

We have a scenario where we need to dynamically select the host master to create macvlan, and I don't know much about the scenario where we need to select master in the container.

n.Master = result.Interfaces[0].Name
} else {
return nil, "", fmt.Errorf("chained master failure. PrevResult lacks a single named interface")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking that we could provide a way to pick interface from prevResult by 'master' option.
I'm just wondering how about to pick interface by 'master' option name if prevResult != nil and multiple interfaces in result.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

master option is a static option in cni config. how to dynamic config for different master? Give a special master option value to tell maclvan plugin to get master from prevResult ?

}
}
n.Master = defaultRouteInterface
}

// check existing and MTU of master interface
Expand Down
92 changes: 92 additions & 0 deletions plugins/main/macvlan/macvlan_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -880,6 +880,98 @@
})
Expect(err).NotTo(HaveOccurred())
})
if testutils.SpecVersionHasChaining(ver) {
It(fmt.Sprintf("[%s] configures and deconfigures an l2 macvlan link with ADD/DEL when chained", ver), func() {
const IFNAME = "macvl0"
conf := fmt.Sprintf(`{
"cniVersion": "%s",
"name": "mynet",
"type": "macvlan",
%s
"prevResult": {
"interfaces": [
{
"name": "%s"
}
],
"ips": [
{
"version": "4",
"address": "10.1.2.2/24",
"gateway": "10.1.2.1",
"interface": 0
}
],
"routes": []
}
}`, ver, linkInContainer, masterInterface)
args := &skel.CmdArgs{
ContainerID: "dummy",
Netns: targetNS.Path(),
IfName: IFNAME,
StdinData: []byte(conf),
}

var macAddress string
err := originalNS.Do(func(ns.NetNS) error {
defer GinkgoRecover()

result, _, err := testutils.CmdAddWithArgs(args, func() error {
return cmdAdd(args)
})

t := newTesterByVersion(ver)
macAddress = t.verifyResult(result, err, IFNAME, 0)
return nil
})
Expect(err).NotTo(HaveOccurred())

// Make sure macvlan link exists in the target namespace
err = targetNS.Do(func(ns.NetNS) error {
defer GinkgoRecover()

link, err := netlink.LinkByName(IFNAME)
Expect(err).NotTo(HaveOccurred())
Expect(link.Attrs().Name).To(Equal(IFNAME))

if macAddress != "" {
hwaddr, err := net.ParseMAC(macAddress)
Expect(err).NotTo(HaveOccurred())
Expect(link.Attrs().HardwareAddr).To(Equal(hwaddr))
}

addrs, err := netlink.AddrList(link, syscall.AF_INET)
Expect(err).NotTo(HaveOccurred())
Expect(addrs).To(BeEmpty())
return nil
})
Expect(err).NotTo(HaveOccurred())

err = originalNS.Do(func(ns.NetNS) error {
defer GinkgoRecover()

err := testutils.CmdDelWithArgs(args, func() error {
return cmdDel(args)
})
Expect(err).NotTo(HaveOccurred())
return nil
})
Expect(err).NotTo(HaveOccurred())

// Make sure macvlan link has been deleted
err = targetNS.Do(func(ns.NetNS) error {
defer GinkgoRecover()

link, err := netlink.LinkByName(IFNAME)
Expect(err).To(HaveOccurred())
Expect(link).To(BeNil())
return nil
})
Expect(err).NotTo(HaveOccurred())

Check failure on line 971 in plugins/main/macvlan/macvlan_test.go

View workflow job for this annotation

GitHub Actions / Lint

File is not `gofumpt`-ed (gofumpt)
})
}

}
}
})
Loading