[specific ci=3-03-Docker-Compose-Basic] Unbind container in containerRm #6209

chengwang86 · 2017-09-05T16:26:26Z

This PR tries to unbind the container in the before removing the container.

This may potentially degrade the performance of containerRm. However, since the PL will not do anything if the container is already unbound, its impact on performance may be neligible.

hmahmood · 2017-09-05T16:30:11Z

Will this actually solve the issue with kill not actually unbinding the container from the network?

chengwang86 · 2017-09-05T16:35:38Z

@hmahmood I think the original issue can be fixed by doing this. The reason why the failure occurs is that the container is removed before it is unbound, and that compose down calls container stop followed by container rm instead of container rm -f. So this PR unbinds the container in the code path where the containerRM() is called without -f.

chengwang86 · 2017-09-05T16:37:32Z

Deleting dangling containerVMs from network layer when calling deleteScope is also an option, but it requires that the network layer knows about the status of the exec cache, which violates the isolation of layers.

hmahmood · 2017-09-05T16:41:36Z

So if I call docker kill for a container, it can potentially still be connected to its network afterwards if I don't follow it with an rm?

hmahmood · 2017-09-05T16:44:16Z

Why do we not have a special case for SIGKILL in https://github.com/vmware/vic/blob/master/lib/apiservers/engine/backends/container.go#L634 as we do on the portlayer side (https://github.com/vmware/vic/blob/master/lib/portlayer/exec/container.go#L454), and unbind the container there?

chengwang86 · 2017-09-05T16:50:21Z

@hmahmood If docker kill is not followed by docker rm, the container will not be removed from cache and the containerPoweredOff event will finally be processed properly. So the container will be unbound from the network in this case.

The failure occurs because docker kill is followed by docker rm before the event is processed.

About why I didn't have a special case checking SIGKILL in containerKill(), George's opinion was that we should target at a more general set of failures that might occur due to this race condition, instead of only focusing on docker kill. @hickeng Thoughts?

hmahmood · 2017-09-05T17:14:58Z

What about port mappings? Do they also get unmapped when with a kill?

chengwang86 · 2017-09-05T17:24:32Z

@hmahmood ContainerKill() calls container_proxy.Signal(), which would unmap the ports if the state is not running. So I think the ports should've been unmapped after docker kill.

In this case, I think I can move unmapPorts() outside of UnbindContainerfFromNetwork().

anchal-agrawal · 2017-09-05T21:13:16Z

lib/apiservers/engine/backends/container_proxy.go

+		default:
+			return "", InternalServerError(err.Error())
+		}
+	} else {


Don't think the else block is needed - we can just say

if err != nil { ... } return ub.Payload.Handle, nil

anchal-agrawal · 2017-09-05T21:14:12Z

lib/apiservers/engine/backends/container_proxy.go

@@ -835,6 +826,28 @@ func (c *ContainerProxy) Stop(vc *viccontainer.VicContainer, name string, second
 	return nil
 }

+func (c *ContainerProxy) UnbindContainerFromNetwork(vc *viccontainer.VicContainer, handle string) (string, error) {


We should have function comments for exported functions.

anchal-agrawal · 2017-09-05T21:15:13Z

lib/apiservers/engine/backends/container.go

+
+		handle, err := c.Handle(id, name)
+		if err != nil {
+			return err


Is this a docker-typed error?

I think so. It returns either NotFoundError or InternalServerError.

jzt · 2017-09-05T21:52:38Z

lib/apiservers/engine/backends/container_proxy.go

@@ -835,6 +826,27 @@ func (c *ContainerProxy) Stop(vc *viccontainer.VicContainer, name string, second
 	return nil
 }

+// UnbindContainerFromNetwork unbinds a container from the networks that it connects to
+func (c *ContainerProxy) UnbindContainerFromNetwork(vc *viccontainer.VicContainer, handle string) (string, error) {
+	defer trace.End(trace.Begin(""))


Would it be worthwhile to trace on the container ID here?

rajanashok

lgtm

vmwclabot added the cla-not-required label Sep 5, 2017

chengwang86 requested review from hickeng and hmahmood September 5, 2017 16:27

hmahmood approved these changes Sep 5, 2017

View reviewed changes

chengwang86 requested a review from sflxn September 5, 2017 20:41

anchal-agrawal reviewed Sep 5, 2017

View reviewed changes

chengwang86 requested a review from jzt September 5, 2017 21:29

jzt reviewed Sep 5, 2017

View reviewed changes

jzt approved these changes Sep 5, 2017

View reviewed changes

chengwang86 added 5 commits September 5, 2017 15:10

unbind container in containerRm

48e4505

update unit test

84de1f6

do not unmap ports when unbinding container from network

13b6c97

cosmetic changes

19929ce

cosmetic change

567f9c8

chengwang86 force-pushed the cheng-containerRm branch from 464cb3e to 567f9c8 Compare September 5, 2017 22:12

chengwang86 changed the title ~~[specific ci=1-11-Docker-RM] Unbind container in containerRm~~ [specific ci=3-03-Docker-Compose-Basic] Unbind container in containerRm Sep 5, 2017

chengwang86 added 2 commits September 5, 2017 15:18

fake commit to trigger test

eb27a1c

fix goimports

2396ecf

rajanashok approved these changes Sep 7, 2017

View reviewed changes

Merge branch 'master' into cheng-containerRm

6bd48ff

chengwang86 merged commit 8091d51 into vmware:master Sep 7, 2017

mhagen-vmware pushed a commit that referenced this pull request Sep 13, 2017

Unbind container in containerRm (#6209)

c58507f

mhagen-vmware added a commit that referenced this pull request Sep 13, 2017

Cherry-pick: Unbind container in containerRm (#6209) (#6310)

ffee5f7

hickeng mentioned this pull request Sep 15, 2017

Concurrent container auto removes result in orphaned ips / network info #6340

Closed

AngieCris pushed a commit to AngieCris/vic that referenced this pull request Nov 20, 2017

Unbind container in containerRm (vmware#6209)

4525df6

sflxn mentioned this pull request Nov 28, 2017

Performance regression in remove containers operation #6627

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[specific ci=3-03-Docker-Compose-Basic] Unbind container in containerRm #6209

[specific ci=3-03-Docker-Compose-Basic] Unbind container in containerRm #6209

chengwang86 commented Sep 5, 2017

hmahmood commented Sep 5, 2017

chengwang86 commented Sep 5, 2017

chengwang86 commented Sep 5, 2017

hmahmood commented Sep 5, 2017 •

edited

Loading

hmahmood commented Sep 5, 2017

chengwang86 commented Sep 5, 2017

hmahmood commented Sep 5, 2017

chengwang86 commented Sep 5, 2017

anchal-agrawal Sep 5, 2017 •

edited

Loading

anchal-agrawal Sep 5, 2017

anchal-agrawal Sep 5, 2017

chengwang86 Sep 5, 2017

jzt Sep 5, 2017

rajanashok left a comment

[specific ci=3-03-Docker-Compose-Basic] Unbind container in containerRm #6209

[specific ci=3-03-Docker-Compose-Basic] Unbind container in containerRm #6209

Conversation

chengwang86 commented Sep 5, 2017

hmahmood commented Sep 5, 2017

chengwang86 commented Sep 5, 2017

chengwang86 commented Sep 5, 2017

hmahmood commented Sep 5, 2017 • edited Loading

hmahmood commented Sep 5, 2017

chengwang86 commented Sep 5, 2017

hmahmood commented Sep 5, 2017

chengwang86 commented Sep 5, 2017

anchal-agrawal Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

anchal-agrawal Sep 5, 2017

Choose a reason for hiding this comment

anchal-agrawal Sep 5, 2017

Choose a reason for hiding this comment

chengwang86 Sep 5, 2017

Choose a reason for hiding this comment

jzt Sep 5, 2017

Choose a reason for hiding this comment

rajanashok left a comment

Choose a reason for hiding this comment

hmahmood commented Sep 5, 2017 •

edited

Loading

anchal-agrawal Sep 5, 2017 •

edited

Loading