Add macvtap device plugin #2

jcaamano · 2020-01-31T14:48:22Z

This commit adds the initial implementation of the macvtap device
plugin. Includes manifests & templates for deployment, as well as
some basic tests.

Further changes are expected as the macvtap cni is integrated.
RBAC and more thorough testing pending for a later patch.

What this PR does / why we need it:

Special notes for your reviewer:

pkg/deviceplugin/plugin.go

maiqueb

Let me try this locally before merging, but, right now can't nit-pick.

maiqueb · 2020-02-04T13:37:33Z

Let me try this locally before merging, but, right now can't nit-pick.

Actually, I'm missing CI to run :)

That could possibly be done on a PR to be merged before this one; I can work on that later this week.

jcaamano · 2020-02-04T13:55:59Z

Let me try this locally before merging, but, right now can't nit-pick.

Actually, I'm missing CI to run :)

That could possibly be done on a PR to be merged before this one; I can work on that later this week.

You can still do cluster-up and a manual test with your code on top if you like but I actually think that it is better to get this PR and the CNI (with ut) in first and then we can both work on the e2e tests.

maiqueb · 2020-02-04T13:58:24Z

Let me try this locally before merging, but, right now can't nit-pick.

Actually, I'm missing CI to run :)
That could possibly be done on a PR to be merged before this one; I can work on that later this week.

You can still do cluster-up and a manual test with your code on top if you like but I actually think that it is better to get this PR and the CNI (with ut) in first and then we can both work on the e2e tests.

Of course; I just mean triggering the unit tests you've added as a result of this PR.

phoracek · 2020-02-04T16:05:44Z

WIP CI PR: kubevirt/project-infra#308

phoracek · 2020-02-04T16:06:06Z

cmd/Dockerfile

+
+FROM registry.access.redhat.com/ubi8/ubi-minimal
+COPY --from=builder /macvtap /macvtap
+ENTRYPOINT [ "./macvtap", "-v", "3", "-logtostderr"]


Please insert a new line at the end of the file.

phoracek · 2020-02-04T16:22:00Z

pkg/util/netlink.go

+func CreateMacvtap(name string, master string, mode string) (int, error) {
+	// attempt to delete any previously existing link
+	if l, _ := netlink.LinkByName(name); l != nil {
+		_ = netlink.LinkDel(l)


You can drop _ = if you don't care about the returned value

phoracek · 2020-02-04T16:22:54Z

pkg/util/netlink.go

+}
+
+func CreateMacvtap(name string, master string, mode string) (int, error) {
+	// attempt to delete any previously existing link


To make it clear what is the int you are returning representing, would you please initialize the variable here with a proper name and default value 0?

phoracek · 2020-02-04T16:24:49Z

pkg/util/netlink.go

+	return CreateMacvtap(name, master, mode)
+}
+
+func LinkExists(link string) error {


This should return bool. If it fails, it does not necessarily mean the link does not exist. Could you check the returned value? As you do in the following function.

phoracek · 2020-02-04T16:31:31Z

pkg/deviceplugin/lister.go

+}
+
+func (ml MacvtapLister) Discover(pluginListCh chan dpm.PluginNameList) {
+	var plugins = make(dpm.PluginNameList, 0)


So to apply new config, all DaemonSets need to be restarted? Could you document it?

Yes, ideally deleting the config map and creating a new one would take care of it. I was waiting for the cni to wire an encompassing readme to include this information.

phoracek · 2020-02-04T16:33:01Z

pkg/deviceplugin/plugin.go

+
+const (
+	tapPath         = "/dev/tap"
+	suffix          = "Mvp"


what's that?

phoracek · 2020-02-04T16:35:11Z

pkg/deviceplugin/plugin.go

+}
+
+func (mdp *MacvtapDevicePlugin) ListAndWatch(e *pluginapi.Empty, s pluginapi.DevicePlugin_ListAndWatchServer) error {
+	masterDevs := mdp.generateMacvtapDevices()


These are not master devs though, these are slaves connected to the master.

@jcaamano I understand what you're doing, it's easier to follow now. Despite that, the name 'masterDevs' is extremely misleading.

Do you think allocatedMacvtaps & emptyMacvtapList is an acceptable compromise ?

I see. I tried to improve these variable names on the latest push.

Thanks for listening; it's now OK for me.

phoracek · 2020-02-04T16:36:31Z

pkg/deviceplugin/plugin.go

+		}
+	}
+
+	didMasterExist := false


Could you please add an inline comment documenting what are you doing in the following block?

phoracek · 2020-02-04T16:37:23Z

pkg/deviceplugin/plugin.go

+	return true
+}
+
+func (mdp *MacvtapDevicePlugin) ListAndWatch(e *pluginapi.Empty, s pluginapi.DevicePlugin_ListAndWatchServer) error {


Please add a comment describing which interfaces are offered here. I don't get why it requires so much code, you just need to create bunch of fake Devices, no?

Not only that but also keeping up with the master device. If there is no master, there is no point in offering devices for allocation as it will fail.

phoracek · 2020-02-04T16:39:36Z

pkg/deviceplugin/plugin.go

+		var devices []*pluginapi.DeviceSpec
+		for _, name := range req.DevicesIDs {
+			dev := new(pluginapi.DeviceSpec)
+			index, err := util.RecreateMacvtap(name, mdp.Master, mdp.Mode)


Please add a comment explaining why do you need to recreate

jcaamano · 2020-02-04T18:01:12Z

@phoracek hopefully addressed your requests. Thanks!

This commit adds the initial implementation of the macvtap device plugin. Includes manifests & templates for deployment, as well as some basic tests. Further changes are expected as the macvtap cni is integrated. RBAC and more thorough testing pending for a later patch. Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

maiqueb · 2020-02-05T12:07:43Z

LGTM

kubevirt-bot added the dco-signoff: no Indicates the PR's author has not DCO signed all their commits. label Jan 31, 2020

kubevirt-bot requested review from maiqueb and phoracek January 31, 2020 14:48

kubevirt-bot added the size/XXL label Jan 31, 2020

jcaamano force-pushed the master branch from cfe4647 to 7942e2c Compare January 31, 2020 14:49

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Jan 31, 2020

maiqueb reviewed Feb 3, 2020

View reviewed changes

pkg/deviceplugin/plugin.go Show resolved Hide resolved

maiqueb reviewed Feb 4, 2020

View reviewed changes

phoracek requested changes Feb 4, 2020

View reviewed changes

jcaamano force-pushed the master branch from 7942e2c to 4092673 Compare February 4, 2020 17:53

jcaamano force-pushed the master branch from 4092673 to caada1c Compare February 5, 2020 11:53

jcaamano requested a review from phoracek February 5, 2020 11:54

phoracek approved these changes Feb 7, 2020

View reviewed changes

phoracek merged commit 7601bd6 into kubevirt:master Feb 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add macvtap device plugin #2

Add macvtap device plugin #2

jcaamano commented Jan 31, 2020

maiqueb left a comment

maiqueb commented Feb 4, 2020

jcaamano commented Feb 4, 2020

maiqueb commented Feb 4, 2020

phoracek commented Feb 4, 2020

phoracek Feb 4, 2020

phoracek Feb 4, 2020

phoracek Feb 4, 2020

phoracek Feb 4, 2020

phoracek Feb 4, 2020

jcaamano Feb 4, 2020

phoracek Feb 4, 2020

phoracek Feb 4, 2020

maiqueb Feb 5, 2020

jcaamano Feb 5, 2020

maiqueb Feb 5, 2020

phoracek Feb 4, 2020

jcaamano Feb 4, 2020

phoracek Feb 4, 2020

jcaamano Feb 4, 2020

phoracek Feb 4, 2020

jcaamano commented Feb 4, 2020

maiqueb commented Feb 5, 2020

Add macvtap device plugin #2

Add macvtap device plugin #2

Conversation

jcaamano commented Jan 31, 2020

maiqueb left a comment

Choose a reason for hiding this comment

maiqueb commented Feb 4, 2020

jcaamano commented Feb 4, 2020

maiqueb commented Feb 4, 2020

phoracek commented Feb 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcaamano commented Feb 4, 2020

maiqueb commented Feb 5, 2020