network: Use tc filtering rules in bridge mode by amshinde · Pull Request #827 · kata-containers/runtime

amshinde · 2018-10-17T01:16:47Z

Instead of creating a bridge, use tc filter rules to redirect
traffic between veth and tap interface.

Signed-off-by: Archana Shinde [email protected]

amshinde · 2018-10-17T01:17:04Z

/test

sboeuf

@amshinde maybe a dumb question, but why did you replace the bridge mode, instead of creating a new internetworking model?

jodh-intel · 2018-10-17T13:53:56Z

virtcontainers/network.go

+	return nil
+}
+
+func addQdiscIngress(index int) error {


What happens if index < 0? Does netlink.QdiscAdd() handle that? Is it a valid index? (might be given QdiscAttrs.Index is an int wheres all other members are uint32?!?)

@jodh-intel netlink.QdiscAdd will handle the case where the network index is negative.
It will error out if it cannot find an interface with the specified index.

jodh-intel · 2018-10-17T13:54:41Z

virtcontainers/network.go

+	return nil
+}
+
+func addRedirectTCFilter(sourceIndex, destIndex int) error {


Same question - what if either of these two params are negative?

Same as above.

amshinde · 2018-10-17T22:27:01Z

@sboeuf I was planning to replace the bridge mode initially. I pushed the changes for testing.
I have now introduced a new mode. I plan to do some testing for different plugins out there, plan is to then deprecate bridged mode in a future PR.

amshinde · 2018-10-17T22:27:14Z

cc @mcastelino PTAL.

mcastelino

Minor comment. We need to undo the qdisk we added on ingress on the CNI/CNM created interface.

mcastelino · 2018-10-17T22:49:40Z

virtcontainers/network.go

 	return nil
 }

+func tcRemoveNetwork(endpoint Endpoint) error {


@amshinde you need to remove the ingress qdisc you added to the veth/incoming interface. As part of the cleanup you should remove that too.

So you need to undo what do you did before

if err := addQdiscIngress(attrs.Index); err != nil { return err }

@mcastelino Have addressed this. PTAL.

I would rename it removeTCFiltering()

mcastelino · 2018-10-17T22:53:13Z

Some more context for this review
https://gist.github.com/mcastelino/7d85f4164ffdaf48242f9281bb1d0f9b

amshinde · 2018-10-18T16:54:30Z

@bergwolf Can you take a look as well, I think you guys are using tc in runv?

amshinde · 2018-10-19T00:08:31Z

/retest

mcastelino · 2018-10-19T00:10:30Z

virtcontainers/network.go

+		return err
+	}
+
+	if err := removeRedirectTCFilter(link); err != nil {


@amshinde you should not need to do this. Removing the qdisk removes the filters. But does not hurt.

mcastelino

LGTM

amshinde · 2018-10-19T00:41:08Z

/retest

codecov · 2018-10-19T01:51:35Z

Codecov Report

❗ No coverage uploaded for pull request base (master@5a8b738). Click here to learn what that means.
The diff coverage is 51.2%.

@@            Coverage Diff            @@
##             master     #827   +/-   ##
=========================================
  Coverage          ?   66.09%           
=========================================
  Files             ?       87           
  Lines             ?    10705           
  Branches          ?        0           
=========================================
  Hits              ?     7076           
  Misses            ?     2897           
  Partials          ?      732

amshinde · 2018-10-19T16:11:11Z

@grahamwhaley Is the metrics failure here a valid failure?

grahamwhaley · 2018-10-19T16:21:19Z

@amshinde probably not - we had a bunch of fails for the last 36h - I have tweaked the settings as per XXX to avoid false failures whilst I look at how to get things more stable.
The logs for your fail look like:

02:47:48 Report Summary:
02:47:48 +-----+----------------------+-------+--------+--------+-------+--------+--------+-------+------+-----+
02:47:48 | P/F |         NAME         |  FLR  |  MEAN  |  CEIL  |  GAP  |  MIN   |  MAX   |  RNG  | COV  | ITS |
02:47:48 +-----+----------------------+-------+--------+--------+-------+--------+--------+-------+------+-----+
02:47:48 | P   | boot-times           | 90.0% | 91.5%  | 110.0% | 20.0% | 88.7%  | 100.6% | 13.4% | 2.7% |  20 |
02:47:48 | *F* | memory-footprint     | 92.3% | 113.8% | 107.7% | 15.4% | 113.8% | 113.8% | 0.0%  | 0.0% |   1 |
02:47:48 | P   | memory-footprint-ksm | 89.3% | 92.4%  | 110.7% | 21.4% | 92.4%  | 92.4%  | 0.0%  | 0.0% |   1 |
02:47:48 +-----+----------------------+-------+--------+--------+-------+--------+--------+-------+------+-----+

but, as we discussed, you are better off running the network cpu test and/or other network tests locally to check if you have made any network impact. I'm half way through adding that test to the report gen btw.

grahamwhaley · 2018-10-19T16:21:50Z

s/XXX/ kata-containers/ci#72 /

sboeuf

@amshinde This PR looks very good to me, just a few comments, but mostly related to the fact that you should add more comments to make easy the understanding of this code ;)

sboeuf · 2018-10-19T16:34:45Z

virtcontainers/network.go

 		}
 		return untapNetworkPair(endpoint)
+	case NetXConnectTCFilterModel:
+		netPair.NetInterworkingModel = NetXConnectTCFilterModel


This is not needed. The switch already checks this variable, there's no reason to reassign it.

sboeuf · 2018-10-19T16:36:33Z

virtcontainers/network.go

 	return nil
 }

+func tcRedirectNetwork(endpoint Endpoint, numCPUs uint32, disableVhostNet bool) error {


Just a comment about naming here. I think it'd be clearer to call it setupTCFiltering().

sboeuf · 2018-10-19T16:40:14Z

virtcontainers/network.go

 	return nil
 }

+func tcRemoveNetwork(endpoint Endpoint) error {


I would rename it removeTCFiltering()

sboeuf · 2018-10-19T17:22:44Z

virtcontainers/network.go

 	case "enlightened":
 		*n = NetXConnectEnlightenedModel
 		return nil
+	case "tcfilter":


I know it's not only related to this PR, but we should define a constant for this. We don't want to play around with strings like this.

sboeuf · 2018-10-19T17:42:20Z

virtcontainers/network.go

+	return nil
+}
+
+func addQdiscIngress(index int) error {


A comment about this function role would be nice (along those lines):

// addQdiscIngress creates a traffic rule for any incoming traffic to the interface // designated by its index.

sboeuf · 2018-10-19T17:45:29Z

virtcontainers/network.go

+	return nil
+}
+
+func addRedirectTCFilter(sourceIndex, destIndex int) error {


same here, a comment could be nice ;)

sboeuf · 2018-10-19T17:48:36Z

virtcontainers/network.go

+
+	for _, qdisc := range qdiscs {
+		ingress, ok := qdisc.(*netlink.Ingress)
+


you can remove this blank line

sboeuf · 2018-10-19T17:51:10Z

cli/config/configuration.toml.in

 #     Used when the Container network interface can be bridged using
 #     macvtap.
+#
+#   - tcfilter


Have we measured the perf impact before we take this decision?

@sboeuf I ran some iperf tests to compare macvtap, bridge and tcfilter solutions on an Intel nuc.
With macvtap I got around 15Gbs throughtput, tcfiltering giving 13.5 Gbs while bridge gave around 12.9 to 13.5 Gbs.
So we get slightly better performance with tcfiltering as compared to bridge mode, but less performance as compared to macvtap. But I think, given that we get added functionality in terms of support for ipvlan, we can make this as the default. In case ipvlan support is not needed, users can choose to use macvtap instead.
WDYT?

Oh that's cool, thanks for running those tests :)
I don't have any strong opinion on this, but we need the community feedback here. I think we need to decide if we want the default to be the most performant or the most functional.

/cc @bergwolf @jodh-intel @egernst @mcastelino @WeiZhang555 @grahamwhaley @kata-containers/runtime

@amshinde have you measured if this impacts launch time latency? As this is simpler, we may be a bit faster to launch a kata-container with this. If @egernst can let us know how this performs when pushing 40Gbps, that may give us a better data point.

/cc @grahamwhaley

@mcastelino I did see the latency was about the same, but did see some improvements in jitter, but this was over a tiny data set. I'll rerun the tests few more times for latency and post my results.

raravena80 · 2018-10-22T16:48:30Z

@amshinde ping :)

amshinde · 2018-10-22T16:56:07Z

@sboeuf I have addressed all your comments. PTAL.

sboeuf · 2018-10-23T15:26:46Z

/test

sboeuf · 2018-10-23T15:27:49Z

@amshinde thanks!
LGTM

amshinde · 2018-10-23T17:32:53Z

/retest

amshinde · 2018-10-23T17:33:52Z

@sboeuf As discussed, I have dropped the commit to make this mode as the default. Will open another PR to address that.

sboeuf · 2018-10-23T19:21:04Z

@amshinde I'll merge as soon as we get the CI all green!

amshinde · 2018-10-23T22:43:50Z

@sboeuf Can we merge this?

amshinde · 2018-10-23T23:00:21Z

@jodh-intel Can you take another look?

amshinde · 2018-10-24T00:57:31Z

/retest

amshinde · 2018-10-24T01:02:08Z

Retriggering CI for code coverage.

jodh-intel · 2018-10-24T10:41:41Z

lgtm

Although you have conflicts...

Introduce a new mode that uses tc filters to redirect traffic from the network interface created by the network plugin to a tap interface that we connect to the VM. This mode will help support ipvlan as well. Fixes kata-containers#144 Signed-off-by: Archana Shinde <[email protected]>

The test verifies tc filter setup by creating a test veth interface. Signed-off-by: Archana Shinde <[email protected]>

Introduce constants for the network model strings, so as to avoid using the strings directly at multiple places. Signed-off-by: Archana Shinde <[email protected]>

Document this mode for users to be able to use it. Signed-off-by: Archana Shinde <[email protected]>

amshinde · 2018-10-24T20:40:05Z

/retest

mcastelino · 2018-10-25T21:31:40Z

@amshinde a long time ago I added a mode called enlighted interworking mode. The goal of that was to use it after we added tc support.

The way I saw it implemented was

If the interface is something macvtap can handle use macvtap mode
If we know macvtap cannot handle it (or we are not familiar with) use the tc mode.

That way the user does not make the call. We do.

A PR now needs *two* labels to be applied before it can be merged. One label must be a backport label from the list below and the other a forward port label: - backport labels: `needs-backport`, `no-backport-needed`, `backport`. - forward-port labels: `needs-forward-port`, `no-forward-port-needed`, `forward-port`. This is to make the maintainer think carefully before merging a PR and hopefully maximise efficient porting. Related: kata-containers/kata-containers#634 Fixes: kata-containers#827. Signed-off-by: James O. D. Hunt <[email protected]>

amshinde added the do-not-merge label Oct 17, 2018

sboeuf reviewed Oct 17, 2018

View reviewed changes

jodh-intel reviewed Oct 17, 2018

View reviewed changes

amshinde force-pushed the tc-filtering branch 2 times, most recently from a735361 to 2e26a2b Compare October 17, 2018 22:24

amshinde changed the title ~~wip: network: Use tc filtering rules in bridge mode~~ network: Use tc filtering rules in bridge mode Oct 17, 2018

mcastelino suggested changes Oct 17, 2018

View reviewed changes

amshinde force-pushed the tc-filtering branch from 2e26a2b to 7bd8355 Compare October 19, 2018 00:07

mcastelino reviewed Oct 19, 2018

View reviewed changes

mcastelino approved these changes Oct 19, 2018

View reviewed changes

amshinde force-pushed the tc-filtering branch from 7bd8355 to 658af9a Compare October 19, 2018 00:27

sboeuf reviewed Oct 19, 2018

View reviewed changes

amshinde force-pushed the tc-filtering branch from 658af9a to 71e7fa6 Compare October 22, 2018 16:39

amshinde force-pushed the tc-filtering branch from 8152832 to 20774ea Compare October 23, 2018 17:32

sboeuf approved these changes Oct 23, 2018

View reviewed changes

amshinde removed the do-not-merge label Oct 23, 2018

amshinde force-pushed the tc-filtering branch from 20774ea to 6e13e69 Compare October 24, 2018 00:57

jodh-intel mentioned this pull request Oct 24, 2018

Add support for ipvlan network driver #852

Merged

mcastelino mentioned this pull request Oct 24, 2018

add macvlan error -- kata-runtime 1.3.1 #853

Closed

amshinde added 4 commits October 24, 2018 10:46

test: Add test to verify tc redirect

5da973d

The test verifies tc filter setup by creating a test veth interface. Signed-off-by: Archana Shinde <[email protected]>

network: Introduce constants for the network model strings

17be8e3

Introduce constants for the network model strings, so as to avoid using the strings directly at multiple places. Signed-off-by: Archana Shinde <[email protected]>

config: Add documentation for tcfilter in configuration.toml

c38792e

Document this mode for users to be able to use it. Signed-off-by: Archana Shinde <[email protected]>

amshinde force-pushed the tc-filtering branch from 6e13e69 to c38792e Compare October 24, 2018 20:39

sboeuf merged commit c7a9e45 into kata-containers:master Oct 24, 2018

amshinde deleted the tc-filtering branch July 11, 2019 22:27


		for _, qdisc := range qdiscs {
		ingress, ok := qdisc.(*netlink.Ingress)

Conversation

amshinde commented Oct 17, 2018

Uh oh!

amshinde commented Oct 17, 2018

Uh oh!

sboeuf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amshinde commented Oct 17, 2018

Uh oh!

amshinde commented Oct 17, 2018

Uh oh!

mcastelino left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcastelino commented Oct 17, 2018

Uh oh!

amshinde commented Oct 18, 2018

Uh oh!

amshinde commented Oct 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcastelino left a comment

Choose a reason for hiding this comment

Uh oh!

amshinde commented Oct 19, 2018

Uh oh!

codecov bot commented Oct 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

amshinde commented Oct 19, 2018

Uh oh!

grahamwhaley commented Oct 19, 2018

Uh oh!

grahamwhaley commented Oct 19, 2018

Uh oh!

sboeuf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sboeuf Oct 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov bot commented Oct 19, 2018 •

edited

Loading

sboeuf Oct 20, 2018 •

edited

Loading

jodh-intel commented Oct 24, 2018 •

edited by amshinde

Loading