network: Always bind back physical interfaces #385

amshinde · 2018-06-11T19:54:06Z

In case of physical network interfaces, we explicitly
pass through them to the VM. We need to bind them back to
the host driver when the sandbox is stopped, irrespective if
the network namespace has been created by virtcontainers or not.

Fixes #384

Signed-off-by: Archana Shinde archana.m.shinde@intel.com

egernst

looks good - couple queries on comments...

egernst · 2018-06-11T20:07:23Z

virtcontainers/cni.go

@@ -155,14 +155,18 @@ func (n *cni) add(sandbox *Sandbox, config NetworkConfig, netNsPath string, netN

 // remove unbridges and deletes TAP interfaces. It also removes virtual network


While we are in here, should we update the function description to be more accurate (ie, handle the non-virtual case as well).

egernst · 2018-06-11T20:07:39Z

virtcontainers/cnm.go

@@ -49,10 +49,14 @@ func (n *cnm) add(sandbox *Sandbox, config NetworkConfig, netNsPath string, netN

 // remove unbridges and deletes TAP interfaces. It also removes virtual network


same comment as CNI case...

katabuilder · 2018-06-11T20:20:58Z

PSS Measurement:
Qemu: 163786 KB
Proxy: 4747 KB
Shim: 10936 KB

Memory inside container:
Total Memory: 2045972 KB
Free Memory: 1996268 KB

katabuilder · 2018-06-11T21:21:20Z

PSS Measurement:
Qemu: 158018 KB
Proxy: 6685 KB
Shim: 11009 KB

Memory inside container:
Total Memory: 2045972 KB
Free Memory: 1996872 KB

jodh-intel

As @egernst noted, good catch! Can we create some tests here to assert the expected behaviour?

lgtm

jodh-intel · 2018-06-12T07:56:15Z

virtcontainers/cni.go

 		return err
 	}

 	if err := n.invokePluginsDelete(sandbox, networkNS); err != nil {
 		return err
 	}

-	return deleteNetNS(networkNS.NetNsPath, true)
+	if netNsCreated {


Aside: It looks like all calls to deleteNetNS() now call it with mounted=true so this param (and the logic) can be dropped I think? Maybe you could do that cleanup on a separate commit on this PR?

sboeuf · 2018-06-12T14:32:20Z

@amshinde thanks for the PR, looks very good, and I agree the logic related to the fact that we have created or not the network namespace should be only affecting the removal of this network namespace.

katacontainersbot · 2018-06-14T21:37:42Z

PSS Measurement:
Qemu: 142735 KB
Proxy: 4738 KB
Shim: 8732 KB

Memory inside container:
Total Memory: 2045972 KB
Free Memory: 2007588 KB

sboeuf · 2018-06-15T19:36:02Z

@amshinde could you try to look into those CI issues ? They don't look like random issues, the patch might be the cause here.

katacontainersbot · 2018-06-15T20:47:34Z

PSS Measurement:
Qemu: 146895 KB
Proxy: 4623 KB
Shim: 8875 KB

Memory inside container:
Total Memory: 2045972 KB
Free Memory: 2007564 KB

katacontainersbot · 2018-06-17T05:05:37Z

PSS Measurement:
Qemu: 144720 KB
Proxy: 4643 KB
Shim: 8767 KB

Memory inside container:
Total Memory: 2045972 KB
Free Memory: 2007432 KB

In case of physical network interfaces, we explicitly pass through them to the VM. We need to bind them back to the host driver when the sandbox is stopped, irrespective if the network namespace has been created by virtcontainers or not. Fixes kata-containers#384 Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

All calls to deleteNetNS were passing the "mounted" parameter as true. So drop this parameter. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

Out CI is failing because of a recent change introduced in the CNI plugins repo(github.com/containernetworking/plugins) that vendors in CNI v0.7.0-alpha0. Refer to commit #e4fdb6cd1883b7b. However, it looks like the the plugins themselves have not been updated yet, causing failures in CI. This was verified by vendoring in the latest CNI and CNI plugins in our repo. Till the plugin binaries our fixed, use older version of CNI plugins for testing virtcontainers. See this: containernetworking/plugins@68b4efb4056c In any case we should keep this version in sync with what we vendor in, in our runtime and not use the latest commit. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

katacontainersbot · 2018-06-17T06:11:56Z

PSS Measurement:
Qemu: 144606 KB
Proxy: 4651 KB
Shim: 8934 KB

Memory inside container:
Total Memory: 2045972 KB
Free Memory: 2007440 KB

codecov · 2018-06-18T00:40:27Z

Codecov Report

Merging #385 into master will decrease coverage by <.01%.
The diff coverage is 51.72%.

@@            Coverage Diff             @@
##           master     #385      +/-   ##
==========================================
- Coverage    63.8%   63.79%   -0.01%     
==========================================
  Files          87       87              
  Lines        8807     8811       +4     
==========================================
+ Hits         5619     5621       +2     
- Misses       2586     2587       +1     
- Partials      602      603       +1

Impacted Files	Coverage Δ
virtcontainers/sandbox.go	`67.06% <ø> (-0.12%)`	⬇️
virtcontainers/noop_network.go	`100% <100%> (+25%)`	⬆️
virtcontainers/network.go	`48.41% <44.44%> (-0.15%)`	⬇️
virtcontainers/cni.go	`61.53% <60%> (-0.31%)`	⬇️
virtcontainers/cnm.go	`60.86% <60%> (-1.04%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42821b7...a31dd49. Read the comment docs.

amshinde · 2018-06-18T01:46:03Z

@sboeuf I was seeing these unit tests failing in the last CI build for this PR:

=== RUN   TestStartStopSandboxHyperstartAgentSuccessfulWithCNINetwork
--- FAIL: TestStartStopSandboxHyperstartAgentSuccessfulWithCNINetwork (0.01s)
	cc_proxy_mock.go:80: [CCProxyMock] listening on /tmp/cc-proxy-test869276064/cc-proxy-test.sock
	api_test.go:1425: missing network name
	cc_proxy_mock.go:80: [CCProxyMock] Client connection already closed
	cc_proxy_mock.go:80: [CCProxyMock] Stopped
=== RUN   TestStartStopSandboxHyperstartAgentSuccessfulWithCNMNetwork
--- FAIL: TestStartStopSandboxHyperstartAgentSuccessfulWithCNMNetwork (0.00s)
	cc_proxy_mock.go:80: [CCProxyMock] listening on /tmp/cc-proxy-test430518911/cc-proxy-test.sock
	api_test.go:1463: sandbox 7f49d00d-1995-4156-8c79-5f5ab24ce138 exists
	cc_proxy_mock.go:80: [CCProxyMock] Client connection already closed
	cc_proxy_mock.go:80: [CCProxyMock] Stopped

This was introduced by this change in the CNI plugins repository:
containernetworking/plugins@1f6e6ef

This introduces a change requiring network name to be provided by the plugins, but the loopback plugin that we use in our tests has been been updated yet to include this as seen here :
containernetworking/plugins@1f6e6ef#diff-5a9f4821dfcce9f8ad9b7f8e616eeb7b

Since we we use the latest master in our virtcontainers setup script, our CI is failing with the error "missing network name". So I introduced a change to use plugin binaries with the same version that we vendor in our runtime. Once this has been fixed upstream, we can update the vendoring and the setup script.

jodh-intel · 2018-06-18T08:10:25Z

Tests issue raised for the CNI version issue: kata-containers/tests#42.

amshinde · 2018-06-18T16:08:40Z

@sboeuf @jodh-intel I have opened an issue upstream to address the failure on latest master for CNI plugins:
containernetworking/plugins#167

We can pull in the latest CNI plugins once the above issue is solved.

Meanwhile, can this PR be merged?

# Kata Containers 1.3.0

egernst reviewed Jun 11, 2018

View reviewed changes

amshinde force-pushed the always-bind-back-physical-interfaces branch from 42d84fa to 3f6baad Compare June 11, 2018 20:53

jodh-intel approved these changes Jun 12, 2018

View reviewed changes

sboeuf approved these changes Jun 12, 2018

View reviewed changes

amshinde force-pushed the always-bind-back-physical-interfaces branch from 3f6baad to db30b61 Compare June 14, 2018 21:25

amshinde added 3 commits June 16, 2018 22:55

network: Drop mounted parameter in call to deleteNetNS

0806dcc

All calls to deleteNetNS were passing the "mounted" parameter as true. So drop this parameter. Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

amshinde force-pushed the always-bind-back-physical-interfaces branch from bcb1036 to a31dd49 Compare June 17, 2018 06:00

jodh-intel mentioned this pull request Jun 18, 2018

pin CNI plugins versions to avoid CI build failures kata-containers/tests#427

Closed

jodh-intel mentioned this pull request Jun 18, 2018

virtcontainers : fix shared dir resource remaining #292

Merged

jshachm mentioned this pull request Jun 18, 2018

vendor: introduce libcontainer/cgroups package #405

Closed

amshinde mentioned this pull request Jun 18, 2018

Loopback plugin gives an error "missing network name" containernetworking/plugins#167

Closed

sboeuf merged commit 593bd44 into kata-containers:master Jun 18, 2018

zklei pushed a commit to zklei/runtime that referenced this pull request Jun 13, 2019

Merge pull request kata-containers#385 from bergwolf/1.3.0-branch-bump

042c3eb

# Kata Containers 1.3.0

amshinde deleted the always-bind-back-physical-interfaces branch July 11, 2019 22:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

network: Always bind back physical interfaces #385

network: Always bind back physical interfaces #385

amshinde commented Jun 11, 2018

egernst left a comment

egernst Jun 11, 2018

amshinde Jun 11, 2018

egernst Jun 11, 2018

amshinde Jun 11, 2018

katabuilder commented Jun 11, 2018

katabuilder commented Jun 11, 2018

jodh-intel left a comment

jodh-intel Jun 12, 2018

sboeuf commented Jun 12, 2018

katacontainersbot commented Jun 14, 2018

sboeuf commented Jun 15, 2018

katacontainersbot commented Jun 15, 2018

katacontainersbot commented Jun 17, 2018

katacontainersbot commented Jun 17, 2018

codecov bot commented Jun 18, 2018

amshinde commented Jun 18, 2018

jodh-intel commented Jun 18, 2018

amshinde commented Jun 18, 2018

		@@ -155,14 +155,18 @@ func (n cni) add(sandbox Sandbox, config NetworkConfig, netNsPath string, netN

		// remove unbridges and deletes TAP interfaces. It also removes virtual network

		@@ -49,10 +49,14 @@ func (n cnm) add(sandbox Sandbox, config NetworkConfig, netNsPath string, netN

		// remove unbridges and deletes TAP interfaces. It also removes virtual network

network: Always bind back physical interfaces #385

network: Always bind back physical interfaces #385

Conversation

amshinde commented Jun 11, 2018

egernst left a comment

Choose a reason for hiding this comment

egernst Jun 11, 2018

Choose a reason for hiding this comment

amshinde Jun 11, 2018

Choose a reason for hiding this comment

egernst Jun 11, 2018

Choose a reason for hiding this comment

amshinde Jun 11, 2018

Choose a reason for hiding this comment

katabuilder commented Jun 11, 2018

katabuilder commented Jun 11, 2018

jodh-intel left a comment

Choose a reason for hiding this comment

jodh-intel Jun 12, 2018

Choose a reason for hiding this comment

sboeuf commented Jun 12, 2018

katacontainersbot commented Jun 14, 2018

sboeuf commented Jun 15, 2018

katacontainersbot commented Jun 15, 2018

katacontainersbot commented Jun 17, 2018

katacontainersbot commented Jun 17, 2018

codecov bot commented Jun 18, 2018

Codecov Report

amshinde commented Jun 18, 2018

jodh-intel commented Jun 18, 2018

amshinde commented Jun 18, 2018