Networking: Add support for handling macvtap interfaces #1510

mcastelino · 2017-10-05T00:45:53Z

Add support for macvtap interfaces. This also brings in support
for generic multiqueue support in virt containers.

Signed-off-by: Manohar Castelino [email protected]

mcastelino · 2017-10-10T00:07:43Z

@markdryan please can you review this

markdryan

I think there might be some issues with leaky file descriptors. Could you check?

markdryan · 2017-10-10T08:46:03Z

qemu/qemu.go

+		// Does not make sense to have more Queues that CPUs
+		// Specify the maximum number of queues possible for the
+		// current CPU configuration
+		netdev.FDs, _ = createMacvtapFds(netdev.ID, int(config.SMP.CPUs))


Are these file descriptors are closed anywhere? In launcher we close them after the QEMU process has been launched or has failed to launch. I can see that the FDs aren't leaked but I can't see where they are closed. We can't expect the user to close them as he doesn't know about them.

Also, any FDs that he assigns to netdev.FDs will be overwritten by this line and will be leaked.

Yes. I plan to remove this logic from QEMU. I initially put it here to handle the case where the caller did not do setup properly and we have no means to return error from this API call (which is a bit unfortunate). But now I feel it is not worth doing. Let the qemu launch fail and we can leave it to the user to recover.

@markdryan @sameo it may be worthwhile to provide a way to return error from a lot of these APIs. Yes, this will require changes to the consumers of these packages, but will have benefits in the future.

markdryan · 2017-10-10T08:47:28Z

There are also some unit test failures.

mcastelino · 2017-10-10T23:04:25Z

@markdryan this fd leak should be addressed now. Also fixed the test cases.
@amshinde can you review this PR and make sure it will also work for VFIO support.

coveralls · 2017-10-10T23:16:21Z

Coverage decreased (-0.04%) to 65.789% when pulling 3469bf2 on mcastelino:topic/macvtap into cbd9f90 on ciao-project:master.

coveralls · 2017-10-10T23:46:38Z

Coverage decreased (-0.04%) to 65.789% when pulling 0fabbbd on mcastelino:topic/macvtap into cbd9f90 on ciao-project:master.

markdryan

There are still some travis failures.

qemu/qemu.go:358:1:warning: comment on exported method NetDeviceType.QemuNetdevParam should be of the form "QemuNetdevParam ..." (golint)
qemu/qemu.go:379:1:warning: comment on exported method NetDeviceType.QemuDeviceParam should be of the form "QemuDeviceParam ..." (golint)
qemu/qemu.go:457::warning: cyclomatic complexity 16 of function (NetDevice).QemuParams() is high (> 15) (gocyclo)
The command "if [ $LINT_ONLY ]; then gometalinter.v1 --deadline=10m --tests --vendor --disable-all --enable=misspell --enable=vet --enable=ineffassign --enable=gofmt --enable=gocyclo --cyclo-over=15 --enable=golint --enable=deadcode --enable=varcheck --enable=structcheck --enable=unused ./...; fi" exited with 1.
0.01s$

markdryan · 2017-10-11T09:54:31Z

qemu/qemu_test.go

+var deviceNetworkStringMq = "-netdev tap,id=tap0,vhost=on,fds=3:4 -device driver=virtio-net-pci,netdev=tap0,mac=01:02:de:ad:be:ef,disable-modern=true,mq=on,vectors=6"
+
+func TestAppendDeviceNetworkMq(t *testing.T) {
+	foo, _ := ioutil.TempFile(os.TempDir(), "qemu-ciao-test")


If I understand correctly, the QEMU package doesn't close any file descriptors. This is the responsibility of the caller. Is this correct? If so we should close the FDs in this test ( and others ). Currently, they're leaked, i.e., we're missing a foo.Close() and a bar.Close()

@markdryan good catch. This was existing code which I missed. Let me fix this.

coveralls · 2017-10-11T17:43:33Z

Coverage decreased (-0.03%) to 65.844% when pulling b6c2894 on mcastelino:topic/macvtap into 66772b7 on ciao-project:master.

coveralls · 2017-10-11T20:43:47Z

Coverage decreased (-0.06%) to 65.813% when pulling fc6d2a3 on mcastelino:topic/macvtap into 66772b7 on ciao-project:master.

coveralls · 2017-10-11T20:43:47Z

Coverage decreased (-0.02%) to 65.849% when pulling fc6d2a3 on mcastelino:topic/macvtap into 66772b7 on ciao-project:master.

mcastelino · 2017-10-11T21:04:05Z

@markdryan hopefully at the issues are fixed now.

jodh-intel · 2017-10-12T08:22:06Z

qemu/qemu.go

-		deviceParams = append(deviceParams, ",disable-modern=true")
-	}
+	deviceParams = append(deviceParams, "driver=")
+	deviceParams = append(deviceParams, netdev.Type.QemuDeviceParam())


Won't this generate an invalid cli in the case of VHOSTUSER (where QemuDeviceParam() returns "")?

@jodh-intel great catch.. I have yet to add/test that path. But this a generic issue. I could always check if we have params to append to as this code will get even more complex when more use cases come in.

@markdryan @jodh-intel fixed.

jodh-intel · 2017-10-12T08:23:52Z

qemu/qemu.go

+func (netdev NetDevice) QemuNetdevParams(config *Config) []string {
+	var netdevParams []string
+
+	netdevParams = append(netdevParams, netdev.Type.QemuNetdevParam())


Add support for macvtap interfaces. This also brings in support for generic multiqueue support in virt containers. Signed-off-by: Manohar Castelino <[email protected]>

coveralls · 2017-10-12T17:17:37Z

Coverage decreased (-0.02%) to 65.847% when pulling f6469f3 on mcastelino:topic/macvtap into 60525fb on ciao-project:master.

coveralls · 2017-10-12T17:17:37Z

Coverage decreased (-0.02%) to 65.847% when pulling f6469f3 on mcastelino:topic/macvtap into 60525fb on ciao-project:master.

coveralls · 2017-10-12T17:17:37Z

Coverage decreased (-0.02%) to 65.852% when pulling f6469f3 on mcastelino:topic/macvtap into 60525fb on ciao-project:master.

coveralls · 2017-10-12T17:28:37Z

Coverage decreased (-0.03%) to 65.843% when pulling 113bcaf on mcastelino:topic/macvtap into 60525fb on ciao-project:master.

mcastelino mentioned this pull request Oct 5, 2017

Use macvlan to connect the VM to the container network containers/virtcontainers#393

Merged

mcastelino changed the title ~~WIP: Networking: Add support for handling macvtap interfaces~~ Networking: Add support for handling macvtap interfaces Oct 10, 2017

mcastelino force-pushed the topic/macvtap branch from cc4f0fc to eea6389 Compare October 10, 2017 00:23

markdryan suggested changes Oct 10, 2017

View reviewed changes

mcastelino force-pushed the topic/macvtap branch from eea6389 to 3469bf2 Compare October 10, 2017 23:01

mcastelino force-pushed the topic/macvtap branch from 3469bf2 to 0fabbbd Compare October 10, 2017 23:36

markdryan suggested changes Oct 11, 2017

View reviewed changes

mcastelino force-pushed the topic/macvtap branch from 0fabbbd to b6c2894 Compare October 11, 2017 17:24

mcastelino force-pushed the topic/macvtap branch from b6c2894 to fc6d2a3 Compare October 11, 2017 20:33

jodh-intel reviewed Oct 12, 2017

View reviewed changes

Networking: Add support for handling macvtap interfaces

113bcaf

Add support for macvtap interfaces. This also brings in support for generic multiqueue support in virt containers. Signed-off-by: Manohar Castelino <[email protected]>

mcastelino force-pushed the topic/macvtap branch from fc6d2a3 to f6469f3 Compare October 12, 2017 17:07

mcastelino force-pushed the topic/macvtap branch from f6469f3 to 113bcaf Compare October 12, 2017 17:18

markdryan approved these changes Oct 13, 2017

View reviewed changes

markdryan merged commit 0fa9ee6 into ciao-project:master Oct 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Networking: Add support for handling macvtap interfaces #1510

Networking: Add support for handling macvtap interfaces #1510

mcastelino commented Oct 5, 2017

mcastelino commented Oct 10, 2017

markdryan left a comment

markdryan Oct 10, 2017

mcastelino Oct 10, 2017

mcastelino Oct 10, 2017

markdryan commented Oct 10, 2017

mcastelino commented Oct 10, 2017

coveralls commented Oct 10, 2017 •

edited

Loading

coveralls commented Oct 10, 2017 •

edited

Loading

markdryan left a comment

markdryan Oct 11, 2017

mcastelino Oct 11, 2017

coveralls commented Oct 11, 2017 •

edited

Loading

coveralls commented Oct 11, 2017

coveralls commented Oct 11, 2017 •

edited

Loading

mcastelino commented Oct 11, 2017

jodh-intel Oct 12, 2017

mcastelino Oct 12, 2017

mcastelino Oct 12, 2017

jodh-intel Oct 12, 2017

coveralls commented Oct 12, 2017

coveralls commented Oct 12, 2017

coveralls commented Oct 12, 2017 •

edited

Loading

coveralls commented Oct 12, 2017 •

edited

Loading

Networking: Add support for handling macvtap interfaces #1510

Networking: Add support for handling macvtap interfaces #1510

Conversation

mcastelino commented Oct 5, 2017

mcastelino commented Oct 10, 2017

markdryan left a 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

markdryan commented Oct 10, 2017

mcastelino commented Oct 10, 2017

coveralls commented Oct 10, 2017 • edited Loading

coveralls commented Oct 10, 2017 • edited Loading

markdryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 11, 2017 • edited Loading

coveralls commented Oct 11, 2017

coveralls commented Oct 11, 2017 • edited Loading

mcastelino commented Oct 11, 2017

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

coveralls commented Oct 12, 2017

coveralls commented Oct 12, 2017

coveralls commented Oct 12, 2017 • edited Loading

coveralls commented Oct 12, 2017 • edited Loading

coveralls commented Oct 10, 2017 •

edited

Loading

coveralls commented Oct 10, 2017 •

edited

Loading

coveralls commented Oct 11, 2017 •

edited

Loading

coveralls commented Oct 11, 2017 •

edited

Loading

coveralls commented Oct 12, 2017 •

edited

Loading

coveralls commented Oct 12, 2017 •

edited

Loading