Add cni plugin #4

maiqueb · 2020-02-13T16:29:27Z

What this PR does / why we need it:
This PR adds a CNI plugin that moves the macvtap interface created by the macvtap device plugin into the new pod's netns.

The macvtap CNI is built in the same image as the macvtap device plugin, and mounted in the corresponding host using the 'initContainers' functionality.

Special notes for your reviewer:

cmd/Dockerfile

templates/macvtap.yaml.in

pkg/cni/plugin.go

templates/macvtap.yaml.in

pkg/cni/plugin.go

pkg/cni/plugin_test.go

pkg/cni/plugin.go

jcaamano · 2020-02-17T12:13:02Z

pkg/cni/plugin.go

+	return macvtap, err
+}
+
+func renameMacvtapIface(macvtapLink netlink.Link, macvtapIface *current.Interface, ifaceName string, netns ns.NetNS) error {


I would argue it could also be left out of configureMacvtap. Just fetch the mac of the interface if none has been provided directly in CmdAdd.

pkg/cni/plugin.go

maiqueb · 2020-02-20T12:39:16Z

/release-note-none

pkg/cni/plugin.go

maiqueb · 2020-02-21T11:44:03Z

pkg/cni/plugin_test.go

@@ -115,7 +115,6 @@ var _ = Describe("Macvtap CNI", func() {
 					Expect(err).NotTo(HaveOccurred())

 					Expect(link.Attrs().HardwareAddr.String()).NotTo(BeNil())
-					Expect(link.Attrs().OperState.String()).To(Equal("up"))


I got no clue as to why, but this works locally.

phoracek

Just a nit, other than that it looks great. Thanks.

templates/macvtap.yaml.in

pkg/cni/plugin.go

maiqueb · 2020-02-24T16:35:45Z

/hold

Allow me to re-write the PR's history (squash some stuff), to make it cleaner.

phoracek · 2020-02-24T16:53:37Z

/lgtm
/approve

kubevirt-bot · 2020-02-24T16:53:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maiqueb, phoracek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [maiqueb,phoracek]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

The ConfigureMacvtap interface code is located in the `netlink` util pkg. The following helper functions are also made available on the `netlink` pkg. - configureArp - renameMacvtapIface All the configuration is performed within a single netns.Do instruction. Interface renaming is performed in the target netns, thus reducing the risk of interface name collision. Signed-off-by: Miguel Duarte Barroso <[email protected]>

The init-container installs the macvtap-cni plugin into the host, and afterwards, the macvtap container listens to device plugin requests, and operates on them. This way, we can achieve macvtap cni & dp co-existence within the same container. Signed-off-by: Miguel Duarte Barroso <[email protected]>

Signed-off-by: Miguel Duarte Barroso <[email protected]>

The -logtostderr flag is somehow preventing the cni unit tests from running. Can't figure this one out; this is just to show I'm not insane. Signed-off-by: Miguel Duarte Barroso <[email protected]>

maiqueb · 2020-02-24T19:51:03Z

/hold cancel

phoracek · 2020-02-25T09:40:01Z

/lgtm

jcaamano

/lgtm
I only have cosmetic comments with no strong opinion. Feel free to go by them or not.

jcaamano · 2020-02-25T09:50:27Z

pkg/cni/plugin.go

+		if err := ip.DelLinkByName(args.IfName); err != nil {
+			if err != ip.ErrLinkNotFound {
+				return err
+			}


I wondered if there was any difference between ip.DelLinkByName and what we have in util/netlink.go @ LinkDelete. The only difference is that the latter would not return ErrLinkNotFound which is not meaningful here. Since we are also using the latter elsewhere, If I were me I would probably change it to that for consistency. Non binding comment.

jcaamano · 2020-02-25T09:59:03Z

pkg/util/netlink.go

+	macvtapIface, err := netlink.LinkByName(currentIfaceName)
+	if err != nil {
+		return nil, fmt.Errorf("failed to lookup device %q: %v", currentIfaceName, err)
+	}
+
+	// move the macvtap interface to the pod's netns
+	if err = netlink.LinkSetNsFd(macvtapIface, int(netns.Fd())); err != nil {
+		return nil, fmt.Errorf("failed to move iface %s to the netns %d because: %v", macvtapIface, netns.Fd(), err)
+	}
+
+	var macvtap *current.Interface = nil
+
+	// configure the macvtap iface
+	err = netns.Do(func(_ ns.NetNS) error {
+		defer func() {
+			if err != nil {
+				LinkDelete(currentIfaceName)
+				LinkDelete(newIfaceName)
+			}
+		}()


Another non binding cosmetic comment on my side. I would prefer the namespace business outside of the utility method. For me the namespace is too closely related to the main purpose of the cni that I would like it visible from the main CmdAdd method.

jcaamano · 2020-02-25T10:06:28Z

pkg/util/netlink.go

+		macvtap = &current.Interface{
+			Name:    newIfaceName,
+			Mac:     macvtapIface.Attrs().HardwareAddr.String(),
+			Sandbox: netns.Path(),
+		}


Ditto, as my previous cosmetic comment, with this section. The current.Interface is directly related to the cni api and I would prefer it to be built in the main CmdAdd method than buried in an utility method. ConfigureInterface could return nothing, and we could have a specific utility method to re-fetch the mac address (only if none was specified).

jcaamano · 2020-02-25T10:16:20Z

pkg/util/netlink.go

+	return renamedMacvtapIface, nil
+}
+
+func configureArp(ifaceName string) error {


Ditto again with this method. I would probably call this from CmdAdd to give it more visibility/awareness as it would be diffcult to guess it from ConfigureInterface name or parameters and wends up pretty well hidden.

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 13, 2020

kubevirt-bot requested review from jcaamano and phoracek February 13, 2020 16:29

kubevirt-bot added size/XXL approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 13, 2020

phoracek requested changes Feb 14, 2020

View reviewed changes

maiqueb force-pushed the add_cni_plugin branch from 4c6b0c2 to 732b635 Compare February 14, 2020 15:25

maiqueb requested a review from phoracek February 14, 2020 16:16

phoracek requested changes Feb 15, 2020

View reviewed changes

jcaamano reviewed Feb 17, 2020

View reviewed changes

maiqueb requested review from phoracek and jcaamano February 20, 2020 12:38

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 20, 2020

jcaamano reviewed Feb 21, 2020

View reviewed changes

pkg/cni/plugin.go Outdated Show resolved Hide resolved

maiqueb requested a review from jcaamano February 21, 2020 11:21

maiqueb commented Feb 21, 2020

View reviewed changes

phoracek requested changes Feb 24, 2020

View reviewed changes

templates/macvtap.yaml.in Outdated Show resolved Hide resolved

pkg/cni/plugin.go Show resolved Hide resolved

maiqueb requested a review from phoracek February 24, 2020 16:33

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 24, 2020

kubevirt-bot assigned phoracek Feb 24, 2020

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 24, 2020

maiqueb added 3 commits February 24, 2020 20:43

Add unit test to check the correct behavior of the CNI plugin

15d608f

Signed-off-by: Miguel Duarte Barroso <[email protected]>

workaround: remove flag breaking unit tests

5f9d7a7

The -logtostderr flag is somehow preventing the cni unit tests from running. Can't figure this one out; this is just to show I'm not insane. Signed-off-by: Miguel Duarte Barroso <[email protected]>

maiqueb force-pushed the add_cni_plugin branch from ce36296 to 5f9d7a7 Compare February 24, 2020 19:50

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 24, 2020

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 24, 2020

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 25, 2020

kubevirt-bot merged commit fde0a6f into master Feb 25, 2020

maiqueb deleted the add_cni_plugin branch February 25, 2020 09:41

jcaamano reviewed Feb 25, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cni plugin #4

Add cni plugin #4

maiqueb commented Feb 13, 2020

jcaamano Feb 17, 2020

maiqueb commented Feb 20, 2020

maiqueb Feb 21, 2020

phoracek left a comment

maiqueb commented Feb 24, 2020

phoracek commented Feb 24, 2020

kubevirt-bot commented Feb 24, 2020

maiqueb commented Feb 24, 2020

phoracek commented Feb 25, 2020

jcaamano left a comment

jcaamano Feb 25, 2020

jcaamano Feb 25, 2020

jcaamano Feb 25, 2020

jcaamano Feb 25, 2020

Add cni plugin #4

Add cni plugin #4

Conversation

maiqueb commented Feb 13, 2020

jcaamano Feb 17, 2020

Choose a reason for hiding this comment

maiqueb commented Feb 20, 2020

maiqueb Feb 21, 2020

Choose a reason for hiding this comment

phoracek left a comment

Choose a reason for hiding this comment

maiqueb commented Feb 24, 2020

phoracek commented Feb 24, 2020

kubevirt-bot commented Feb 24, 2020

maiqueb commented Feb 24, 2020

phoracek commented Feb 25, 2020

jcaamano left a comment

Choose a reason for hiding this comment

jcaamano Feb 25, 2020

Choose a reason for hiding this comment

jcaamano Feb 25, 2020

Choose a reason for hiding this comment

jcaamano Feb 25, 2020

Choose a reason for hiding this comment

jcaamano Feb 25, 2020

Choose a reason for hiding this comment