Topic/network simplify #1214

mcastelino · 2019-02-05T19:25:54Z

Eliminate legacy networking models

Prior to the addition of tcMirroring support kata-runtime had compatibility issues with some CNI plugins some of which were addressed by the bridged model. With the addition of tc mode there are no gaps in networking that can be filled by the bridged mode or enlightened mode (which was never implemented).

Eliminate both of these option to simplify the setup.

Also addresses some of the simplification needed for #1113
Fixes #1213

mcastelino · 2019-02-05T19:45:39Z

@grahamwhaley @devimc @egernst any idea

The CI keeps failing

The command "sudo apt update -y -qq" failed and exited with 100 during .

devimc · 2019-02-05T20:09:09Z

restarting travis

devimc · 2019-02-05T20:10:06Z

@mcastelino btw

# github.com/kata-containers/runtime/virtcontainers
../virtcontainers/network.go:93:35: undefined: NetXConnectTcFilterModel
Makefile:445: recipe for target '/tmp/jenkins/workspace/kata-containers-runtime-ubuntu-16-04-PR-firecracker/go/src/github.com/kata-containers/runtime/kata-runtime' failed
make: *** [/tmp/jenkins/workspace/kata-containers-runtime-ubuntu-16-04-PR-firecracker/go/src/github.com/kata-containers/runtime/kata-runtime] Error 2
Build step 'Execute shell' marked build as failure

amshinde

lgtm

sameo · 2019-02-06T00:31:08Z

@mcastelino In terms of performance, we know that tc mirroring is superior to the bridged mode?

sameo

LGTM

mcastelino · 2019-02-06T00:35:50Z

@mcastelino In terms of performance, we know that tc mirroring is superior to the bridged mode?

@sameo tc mirroring is slightly less performant than macvtap. We did not measure tc mirror vs bridged mode. However as macvtap mode should support all the modes that bridged mode supports, even if tcmirror mode is lower performance that bridged mode, the user should not loose when choosing an opinionated configuration.

/cc @amshinde

mcastelino · 2019-02-06T00:41:20Z

/cc @amshinde @sboeuf

We will also need to update https://github.com/clearcontainers/runtime/blob/master/docs/architecture/architecture.md#networking with the updated networking setup once this PR goes through

egernst · 2019-02-06T01:08:18Z

/test

sameo · 2019-02-06T08:58:01Z

@mcastelino In terms of performance, we know that tc mirroring is superior to the bridged mode?

@sameo tc mirroring is slightly less performant than macvtap. We did not measure tc mirror vs bridged mode. However as macvtap mode should support all the modes that bridged mode supports, even if tcmirror mode is lower performance that bridged mode, the user should not loose when choosing an opinionated configuration.

So having macvtap and tc mirrroring make bridged redundant. Sounds good to me.

grahamwhaley · 2019-02-06T09:42:34Z

Got this on the 16.04 CI - looks like there might be some debug/rework to do here @mcastelino before we re-fire the CIs?

=== RUN   TestCreateMacVtap
--- FAIL: TestCreateMacVtap (0.00s)
    <autogenerated>:1: 
                          
	Error Trace:	network_test.go:212
        
	Error:      	Received unexpected error:
        
	            	Unsupported link type bridge
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x10052c6]

goroutine 1072 [running]:
testing.tRunner.func1(0xc000312100)
	/usr/local/go/src/testing/testing.go:792 +0x6a7
panic(0x1168460, 0x1d224e0)
	/usr/local/go/src/runtime/panic.go:513 +0x1b9
github.com/kata-containers/runtime/virtcontainers.TestCreateMacVtap(0xc000312100)
	/tmp/jenkins/workspace/kata-containers-runtime-ubuntu-16-04-PR/go/src/github.com/kata-containers/runtime/virtcontainers/network_test.go:214 +0x206
testing.tRunner(0xc000312100, 0x12a3818)
	/usr/local/go/src/testing/testing.go:827 +0x163
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:878 +0x651
FAIL	github.com/kata-containers/runtime/virtcontainers	4.557s
Makefile:524: recipe for target 'go-test' failed

jodh-intel

Nice! Slash and burn! ;)

sboeuf

@mcastelino please split this into 2 distincts PRs. The idea of the first patch is to simplify Kata network implementation by removing unused models, and that's fine. But the second patch, which is very simple and obvious, relates to modifying the default model used. I'm not sure this falls under the same "simplification" category, and I would prefer if this was in its own PR to make sure nobody is missing this.

virtcontainers/network.go

amshinde · 2019-02-06T16:53:36Z

We will also need to update https://github.com/clearcontainers/runtime/blob/master/docs/architecture/architecture.md#networking with the updated networking setup once this PR goes through

@mcastelino Yes the docs need to be updated with all the changes we have done recently. Let me know if you are going to take a look at that, I can take a shot at it as well.

amshinde · 2019-02-06T16:56:24Z

So having macvtap and tc mirrroring make bridged redundant. Sounds good to me.

@sameo @mcastelino Yes, bridged mode has shown the least performance when I measured the performance of each of the modes and all networking drivers are supported between macvtap and tc, making bridged redundant.

mcastelino · 2019-02-06T17:25:57Z

Got this on the 16.04 CI - looks like there might be some debug/rework to do here @mcastelino before we re-fire the CIs?

@grahamwhaley I love our CI. Let me retest this with macvtap any my end.

mcastelino · 2019-02-06T20:35:48Z

@amshinde looks like our unit test case exercises path that our code will no longer take. i.e. we create the macvtap on top of bridge that serves as the parent link. I eliminated the use of bridge interface here.

mcastelino · 2019-02-06T20:39:20Z

virtcontainers/qemu_arch_base.go

 	case NetXConnectMacVtapModel:
 		return govmmQemu.MACVTAP
-	//case ModelEnlightened:


Note: This is already implemented implicitly today. The interconnect model is only used today when the interface cannot be directly connected to the VM. The vhost-user interface is also handed using a special case and works with only a certain class of vhost-user plugins.

So our default model is actually ModelEnlightened when running in tcFilter or macvtap mode. The none mode is the only one that does not behave this way.

/cc @amshinde

jodh-intel · 2019-07-29T10:48:41Z

@amshinde - could you possibly take over this PR maybe?

amshinde · 2019-07-31T18:14:28Z

@jodh-intel Will do.

amshinde · 2019-08-02T21:44:24Z

/test

amshinde · 2019-08-02T22:01:17Z

/test

amshinde · 2019-08-02T22:20:39Z

/test

codecov · 2019-08-02T23:20:57Z

Codecov Report

❗ No coverage uploaded for pull request base (master@00e0aaa). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             master   #1214   +/-   ##
========================================
  Coverage          ?   52.5%           
========================================
  Files             ?     108           
  Lines             ?   14085           
  Branches          ?       0           
========================================
  Hits              ?    7396           
  Misses            ?    5810           
  Partials          ?     879

codecov · 2019-08-02T23:20:58Z

Codecov Report

❗ No coverage uploaded for pull request base (master@8c7a83b). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1214   +/-   ##
=========================================
  Coverage          ?   51.25%           
=========================================
  Files             ?      110           
  Lines             ?    15114           
  Branches          ?        0           
=========================================
  Hits              ?     7747           
  Misses            ?     6410           
  Partials          ?      957

amshinde · 2019-08-02T23:29:47Z

@jodh-intel PR updated.

amshinde · 2019-08-03T00:12:31Z

@egernst @jodh-intel PTAL
Lets try to merge this, before this goes stale again.

grahamwhaley · 2019-08-05T09:16:24Z

Fedora CI fail looked like a net/repo issue (failed to update) - re-triggered...

amshinde · 2019-08-05T18:03:10Z

Thanks @grahamwhaley. All tests have passed now.

grahamwhaley · 2019-08-06T08:13:19Z

@amshinde - need one more ack - given it's heavy network nature, and the original author is mcastelino, I'll be happy if you ack this PR, and then we can merge :-)

amshinde · 2019-08-06T16:03:28Z

ping @mcastelino @egernst

grahamwhaley · 2019-08-08T10:55:56Z

re-ping @mcastelino @egernst - CI is happy here, just needs knowledgeable reviewers...

amshinde · 2019-08-09T20:05:09Z

Adding "do-not-merge" until we decide to remove "bridged". This feature has been marked as deprecated in a separate PR.

Prior to the addition of tcMirroring support kata-runtime had compatibility issues with some CNI plugins some of which were addressed by the bridged model. With the addition of tc mode there are no gaps in networking that can be filled by the bridged mode or enlightened mode (which was never implemented). Eliminate both of these options to simplify the setup. Fixes: kata-containers#1213 Signed-off-by: Manohar Castelino <[email protected]>

Since we have dropped support for bridged model, remove it from the configuration as well. Signed-off-by: Archana Shinde <[email protected]>

If the configuration for networking is missing, tcfilter will be chosen. Signed-off-by: Archana Shinde <[email protected]>

amshinde · 2019-11-11T17:17:46Z

/test

amshinde · 2019-11-11T17:19:02Z

@kata-containers/runtime @egernst This PR is ready for review now, since we plan to remove bridged mode in 1.10. PTAL.

amshinde · 2019-11-11T18:00:26Z

/test

amshinde · 2019-11-11T18:21:15Z

cc @devimc @jodh-intel

devimc

restarting initrd and opensuse CIs

jschintag · 2019-11-12T09:52:17Z

I don't see a problem for s390x.

mcastelino requested a review from a team as a code owner February 5, 2019 19:25

mcastelino requested review from amshinde, egernst, grahamwhaley and jodh-intel February 5, 2019 19:26

mcastelino force-pushed the topic/network-simplify branch from beca7e5 to d5814a2 Compare February 5, 2019 21:39

amshinde previously approved these changes Feb 5, 2019

View reviewed changes

sameo previously approved these changes Feb 6, 2019

View reviewed changes

jodh-intel previously approved these changes Feb 6, 2019

View reviewed changes

sboeuf reviewed Feb 6, 2019

View reviewed changes

jodh-intel reviewed Feb 6, 2019

View reviewed changes

virtcontainers/network.go Show resolved Hide resolved

mcastelino dismissed stale reviews from jodh-intel, sameo, and amshinde via a359db4 February 6, 2019 20:27

mcastelino force-pushed the topic/network-simplify branch 2 times, most recently from a359db4 to 7333a66 Compare February 6, 2019 20:35

mcastelino commented Feb 6, 2019

View reviewed changes

amshinde force-pushed the topic/network-simplify branch from 7333a66 to d1336f6 Compare August 2, 2019 21:43

amshinde force-pushed the topic/network-simplify branch from 9389435 to 64bd5c2 Compare August 2, 2019 22:15

amshinde added the do-not-merge PR has problems or depends on another label Aug 9, 2019

mcastelino and others added 3 commits November 11, 2019 09:14

config: Get rid of bridged model docs in the configuration

27433d9

Since we have dropped support for bridged model, remove it from the configuration as well. Signed-off-by: Archana Shinde <[email protected]>

network: Set the default config to tcfilter

744ccd4

If the configuration for networking is missing, tcfilter will be chosen. Signed-off-by: Archana Shinde <[email protected]>

amshinde force-pushed the topic/network-simplify branch from 64bd5c2 to 744ccd4 Compare November 11, 2019 17:17

amshinde removed the do-not-merge PR has problems or depends on another label Nov 11, 2019

devimc approved these changes Nov 11, 2019

View reviewed changes

egernst merged commit 75d149c into kata-containers:master Nov 13, 2019

Topic/network simplify #1214

Topic/network simplify #1214

Conversation

mcastelino commented Feb 5, 2019 • edited by amshinde Loading

Eliminate legacy networking models

mcastelino commented Feb 5, 2019

devimc commented Feb 5, 2019

devimc commented Feb 5, 2019

amshinde left a comment

Choose a reason for hiding this comment

sameo commented Feb 6, 2019

sameo left a comment

Choose a reason for hiding this comment

mcastelino commented Feb 6, 2019

mcastelino commented Feb 6, 2019

egernst commented Feb 6, 2019

sameo commented Feb 6, 2019

grahamwhaley commented Feb 6, 2019

jodh-intel left a comment

Choose a reason for hiding this comment

sboeuf left a comment

Choose a reason for hiding this comment

amshinde commented Feb 6, 2019

amshinde commented Feb 6, 2019

mcastelino commented Feb 6, 2019

mcastelino commented Feb 6, 2019

mcastelino Feb 6, 2019 • edited Loading

Choose a reason for hiding this comment

jodh-intel commented Jul 29, 2019

amshinde commented Jul 31, 2019

amshinde commented Aug 2, 2019

amshinde commented Aug 2, 2019

amshinde commented Aug 2, 2019

codecov bot commented Aug 2, 2019

Codecov Report

codecov bot commented Aug 2, 2019 • edited Loading

Codecov Report

amshinde commented Aug 2, 2019

amshinde commented Aug 3, 2019

grahamwhaley commented Aug 5, 2019

amshinde commented Aug 5, 2019

grahamwhaley commented Aug 6, 2019

amshinde commented Aug 6, 2019

grahamwhaley commented Aug 8, 2019

amshinde commented Aug 9, 2019

amshinde commented Nov 11, 2019

amshinde commented Nov 11, 2019

amshinde commented Nov 11, 2019

amshinde commented Nov 11, 2019

devimc left a comment

Choose a reason for hiding this comment

jschintag commented Nov 12, 2019

mcastelino commented Feb 5, 2019 •

edited by amshinde

Loading

mcastelino Feb 6, 2019 •

edited

Loading

codecov bot commented Aug 2, 2019 •

edited

Loading