Refactor network.go #811

amshinde · 2018-10-09T05:32:49Z

Split network.go into separate endpoint implementations.

amshinde · 2018-10-09T05:33:41Z

/test

jodh-intel · 2018-10-09T10:54:45Z

virtcontainers/bridgedmacvlan_endpoint.go

+// Attach for virtual endpoint bridges the network pair and adds the
+// tap interface of the network pair to the hypervisor.
+func (endpoint *BridgedMacvlanEndpoint) Attach(h hypervisor) error {
+	networkLogger().Info("Attaching macvlan endpoint")


For consistency, this should be:

networkLogger().WithField("endpoint-type", "bridged-macvlan").Info("Attaching endpoint")

But as mentioned recently, it would be good if we could factor out all these log calls for Attach() and Detach() and maybe get the caller to call Type() on the endpoint and log that in a single location (maybe in add() and remove() in virtcontainers/default_network.go?)

@jodh-intel Missed that from our previous conversation! I have fixed this now, by getting rid of all those calls and logging at a single location.

jodh-intel · 2018-10-09T15:23:23Z

Hi @amshinde - our friendly neighbourhood CI has found a minor issue:

INFO: Checking for SPDX license headers
ERROR: Required license identifier ('SPDX-License-Identifier: Apache-2.0') missing from following files:

virtcontainers/endpoint.go

amshinde · 2018-10-09T17:55:56Z

/retest

amshinde · 2018-10-09T17:56:42Z

@sboeuf PTAL

codecov · 2018-10-09T18:21:10Z

Codecov Report

Merging #811 into master will increase coverage by 0.44%.
The diff coverage is 14.28%.

@@            Coverage Diff             @@
##           master     #811      +/-   ##
==========================================
+ Coverage   65.64%   66.09%   +0.44%     
==========================================
  Files          87       87              
  Lines       10802    10505     -297     
==========================================
- Hits         7091     6943     -148     
+ Misses       3004     2863     -141     
+ Partials      707      699       -8

amshinde · 2018-10-09T18:30:05Z

/retest

caoruidong · 2018-10-10T06:45:09Z

I think macvtap hotplug is supported now. So what is relationship between XconnectNetModel and type of endpoint? like this NetXConnectBridgedModel->VethEndpoint NetXConnectMacVtapModel->MacvtapEndpoint?

jodh-intel · 2018-10-10T08:48:57Z

Thanks @amshinde.

lgtm

caoruidong · 2018-10-10T08:56:14Z

virtcontainers/qemu.go

@@ -833,7 +833,7 @@ func (q *qemu) hotplugVFIODevice(device *config.VFIODev, op operation) error {
 	return nil
 }

-func (q *qemu) hotplugMacvtap(drive *VirtualEndpoint) error {
+func (q *qemu) hotplugMacvtap(drive *VethEndpoint) error {


This function is for macvtap

@caoruidong Yes, but the underlying interface is the Veth interface. Macvtap is used as the bridging mechanism. Answering your previous question, the difference is when you get a macvtap interface from the network plugin that you can directly pass to the VM vs an interface such as a veth that you get from the plugin wherein you use a macvtap or a bridge+tap for bridging the interface to the VM.

Oh, so the new MacvtapEndpoint is without veth pair? Got it.

caoruidong · 2018-10-11T09:28:02Z

Why not create a sub-directory under virtcontainers?

Split endpoint implementations into their own file. Fixes kata-containers#799 Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

Split network_test.go into separate test files. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

As this really represents a veth pair rather than a generic virtual interface, rename VirtualEndpoint to VethEndpoint. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

Explicitly check for "veth" intergace type while creating a veth endpoint. Error out for unsupported network interfaces. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

Log Attach, Detach, HotAttach and HotDetach at a single location. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

amshinde · 2018-10-11T21:46:48Z

/retest

sboeuf

Thanks @amshinde ! I bet this was not fun but it helps clarifying the code base a lot :)

amshinde · 2018-10-12T00:01:07Z

/retest

jodh-intel · 2018-10-12T08:57:30Z

/me grinds his teeth at CI failures - 2 nodes missing VT-x and a network timeout. Re-spinnning...

/cc @chavafg, @mnaser.

egernst

Thanks @amshinde

bergwolf · 2018-10-15T03:03:17Z

Thanks @amshinde ! It makes the code a lot more clear.
LGTM!

devimc · 2018-10-15T14:41:56Z

lgtm

jodh-intel reviewed Oct 9, 2018

View reviewed changes

amshinde force-pushed the network_refactor branch 2 times, most recently from 676b792 to f68d964 Compare October 9, 2018 17:55

amshinde requested a review from sboeuf October 9, 2018 17:56

caoruidong reviewed Oct 10, 2018

View reviewed changes

amshinde added 5 commits October 11, 2018 14:45

network: Refactor network.go

adcd910

Split endpoint implementations into their own file. Fixes kata-containers#799 Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

network: Refactor network tests.

df8f21d

Split network_test.go into separate test files. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

network: Rename VirtualEndpoint to VethEndpoint

3c590b0

As this really represents a veth pair rather than a generic virtual interface, rename VirtualEndpoint to VethEndpoint. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

network: Explicitly check for veth type

ab15498

Explicitly check for "veth" intergace type while creating a veth endpoint. Error out for unsupported network interfaces. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

network: Collapse log calls for endpoint Attach and Detach

b04691e

Log Attach, Detach, HotAttach and HotDetach at a single location. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

amshinde force-pushed the network_refactor branch from b6c6378 to b04691e Compare October 11, 2018 21:46

sboeuf approved these changes Oct 11, 2018

View reviewed changes

egernst approved these changes Oct 15, 2018

View reviewed changes

amshinde merged commit d00742f into kata-containers:master Oct 15, 2018

amshinde deleted the network_refactor branch July 11, 2019 22:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor network.go #811

Refactor network.go #811

amshinde commented Oct 9, 2018

amshinde commented Oct 9, 2018

jodh-intel Oct 9, 2018

amshinde Oct 9, 2018

jodh-intel commented Oct 9, 2018

amshinde commented Oct 9, 2018

amshinde commented Oct 9, 2018

codecov bot commented Oct 9, 2018 •

edited

Loading

amshinde commented Oct 9, 2018

caoruidong commented Oct 10, 2018

jodh-intel commented Oct 10, 2018 •

edited by amshinde

Loading

caoruidong Oct 10, 2018

amshinde Oct 10, 2018

caoruidong Oct 11, 2018

caoruidong commented Oct 11, 2018

amshinde commented Oct 11, 2018

sboeuf left a comment

amshinde commented Oct 12, 2018

jodh-intel commented Oct 12, 2018

egernst left a comment

bergwolf commented Oct 15, 2018 •

edited by amshinde

Loading

devimc commented Oct 15, 2018 •

edited by amshinde

Loading

Refactor network.go #811

Refactor network.go #811

Conversation

amshinde commented Oct 9, 2018

amshinde commented Oct 9, 2018

jodh-intel Oct 9, 2018

Choose a reason for hiding this comment

amshinde Oct 9, 2018

Choose a reason for hiding this comment

jodh-intel commented Oct 9, 2018

amshinde commented Oct 9, 2018

amshinde commented Oct 9, 2018

codecov bot commented Oct 9, 2018 • edited Loading

Codecov Report

amshinde commented Oct 9, 2018

caoruidong commented Oct 10, 2018

jodh-intel commented Oct 10, 2018 • edited by amshinde Loading

caoruidong Oct 10, 2018

Choose a reason for hiding this comment

amshinde Oct 10, 2018

Choose a reason for hiding this comment

caoruidong Oct 11, 2018

Choose a reason for hiding this comment

caoruidong commented Oct 11, 2018

amshinde commented Oct 11, 2018

sboeuf left a comment

Choose a reason for hiding this comment

amshinde commented Oct 12, 2018

jodh-intel commented Oct 12, 2018

egernst left a comment

Choose a reason for hiding this comment

bergwolf commented Oct 15, 2018 • edited by amshinde Loading

devimc commented Oct 15, 2018 • edited by amshinde Loading

codecov bot commented Oct 9, 2018 •

edited

Loading

jodh-intel commented Oct 10, 2018 •

edited by amshinde

Loading

bergwolf commented Oct 15, 2018 •

edited by amshinde

Loading

devimc commented Oct 15, 2018 •

edited by amshinde

Loading