Make sure plugin container is removed on failure#36715

cpuguy83 · 2018-03-27T20:26:59Z

No description provided.

anusha-ragunathan · 2018-03-27T23:03:58Z

@cpuguy83 : Janky failure

anusha-ragunathan · 2018-03-28T00:28:13Z

plugin/executor/containerd/containerd.go

@@ -52,10 +66,23 @@ func (e *Executor) Create(id string, spec specs.Spec, stdout, stderr io.WriteClo
 	ctx := context.Background()
 	err := e.client.Create(ctx, id, &spec, &opts)
 	if err != nil {
-		return err
+		status, _ := e.client.Status(ctx, id)


Why ignore error from Status? We could be dealing with a corrupt status in L70

anusha-ragunathan · 2018-03-28T00:35:32Z

plugin/executor/containerd/containerd.go

+			if err2 := e.client.Delete(ctx, id); err2 != nil && !errdefs.IsNotFound(err2) {
+				logrus.WithError(err2).WithField("plugin", id).Error("Error cleaning up containerd container")
+			}
+			err = e.client.Create(ctx, id, &spec, &opts)


I dont understand this additional call to Create.

It's a recovery from left-over containerd containers. The bug leaves stuff behind on error, this cleans them up and tries again.

The bug being fixed by this PR, introduced by the containerd 1.0 integration.

haha! I expected a bug number :)

anusha-ragunathan · 2018-03-28T00:36:28Z

plugin/executor/containerd/containerd.go

+			}
+			err = e.client.Create(ctx, id, &spec, &opts)
+		}
+		if err != nil {


err will be nil, given this block is about a failed creation.

err is set again by the 2nd create above.

anusha-ragunathan · 2018-03-28T00:38:51Z

plugin/executor/containerd/containerd.go

 	}

 	_, err = e.client.Start(ctx, id, "", false, attachStreamsFunc(stdout, stderr))
+	if err != nil {
+		e.client.DeleteTask(ctx, id)
+		e.client.Delete(ctx, id)


Check return similar to L71

anusha-ragunathan · 2018-03-28T00:53:14Z

plugin/executor/containerd/containerd.go

@@ -40,7 +54,7 @@ func New(rootDir string, remote libcontainerd.Remote, exitHandler ExitHandler) (
 // Executor is the containerd client implementation of a plugin executor
 type Executor struct {
 	rootDir     string
-	client      libcontainerd.Client
+	client      Client


Is this mainly so that we can write the unit test with mockClient? If so, I think we should continue using libcontainerd.Client and think of other ways to rewrite the test.

This limits the scope of libcontainerd.Client to just the functions that are called here which makes the mock implementation simpler.

I would indeed like to use the real libcontainerd client implementation, but this is much harder and would either require actually spinning up containerd or building a custom containerd service. The latter sounds nice but is not a simple change.

anusha-ragunathan · 2018-03-28T01:01:15Z

plugin/executor/containerd/containerd_test.go

+}
+
+func (c *mockClient) Status(ctx context.Context, id string) (libcontainerd.Status, error) {
+	c.mu.Lock()


c.mu.RLock

There is no need to use a read lock here, these are fast reads and not high concurrency. The only real reason I added locking was to make sure if someone tried to use it concurrently in another test it would be fine.

Even so, a read lock would be sufficient, given the method only reads the map and doesnt update it.

An RW mutex is slower and more complex than a normal one... I can update it if you think it should use one I think it is unnecessarily complex to do so.

That's fair. Lets keep it AS-IS

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

stevvooe · 2018-03-29T23:35:29Z

LGTM

anusha-ragunathan · 2018-03-30T05:25:23Z

Janky and ppc failures are due to known flaky tests.

Experimental failure is new though. And might be related to this PR.

14:12:06 FAIL: docker_api_swarm_service_test.go:614: DockerSwarmSuite.TestAPISwarmServicesPlugin
14:12:06 
14:12:06 [d699a76c39512] waiting for daemon to start
14:12:06 [d699a76c39512] daemon started
14:12:06 
14:12:06 [d0241c324309d] waiting for daemon to start
14:12:06 [d0241c324309d] daemon started
14:12:06 
14:12:06 [dbbfcc658cf89] waiting for daemon to start
14:12:06 [dbbfcc658cf89] daemon started
14:12:06 
14:12:06 docker_api_swarm_service_test.go:683:
14:12:06     waitAndAssert(c, defaultReconciliationTimeout, d1.CheckPluginRunning(repo), checker.False)
14:12:06 docker_utils_test.go:452:
14:12:06     c.Assert(v, checker, args...)
14:12:06 ... obtained bool = true
14:12:06 ... &{Config:{Args:{Description: Name: Settable:[] Value:[]} Description: DockerVersion:dev Documentation: Entrypoint:[/basic] Env:[] Interface:{Socket:basic.sock Types:[.docker.dummy/1.0/]} IpcHost:false Linux:{AllowAllDevices:false Capabilities:[] Devices:[]} Mounts:[] Network:{Type:} PidHost:false PropagatedMount: User:{GID:0 UID:0} WorkDir: Rootfs:0xc423748c00} Enabled:true ID:67342d0b4379499e319163d3f9b98b1626de91d6be8909b56555a4321e5a9733 Name:127.0.0.1:5000/swarm/test:v1 PluginReference:127.0.0.1:5000/swarm/test:v1 Settings:{Args:[] Devices:[] Env:[] Mounts:[]}}

codecov · 2018-03-30T11:08:28Z

Codecov Report

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

@@            Coverage Diff            @@
##             master   #36715   +/-   ##
=========================================
  Coverage          ?   35.14%           
=========================================
  Files             ?      614           
  Lines             ?    46245           
  Branches          ?        0           
=========================================
  Hits              ?    16252           
  Misses            ?    27860           
  Partials          ?     2133

vdemeester

LGTM 🐅

GordonTheTurtle added the status/0-triage label Mar 27, 2018

cpuguy83 requested review from anusha-ragunathan and stevvooe March 27, 2018 20:27

cpuguy83 force-pushed the plugin_exec_fixes branch from e2f95a4 to 086203b Compare March 27, 2018 21:17

cpuguy83 added status/2-code-review and removed status/0-triage labels Mar 27, 2018

anusha-ragunathan reviewed Mar 28, 2018

View reviewed changes

Make sure plugin container is removed on failure

f81172b

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 force-pushed the plugin_exec_fixes branch from 086203b to f81172b Compare March 28, 2018 13:07

vdemeester added rebuild/janky labels Mar 30, 2018

GordonTheTurtle removed rebuild/experimental labels Mar 30, 2018

vdemeester approved these changes Mar 30, 2018

View reviewed changes

vdemeester merged commit 859e43e into moby:master Mar 30, 2018

cpuguy83 deleted the plugin_exec_fixes branch March 30, 2018 15:29

thaJeztah added the area/plugins label Apr 2, 2018

andrewhsu mentioned this pull request Apr 3, 2018

[18.04] Make sure plugin container is removed on failure docker-archive/docker-ce#485

Merged

thaJeztah mentioned this pull request Apr 11, 2018

[18.03] Make sure plugin container is removed on failure docker-archive/docker-ce#507

Merged

anusha-ragunathan mentioned this pull request Apr 16, 2018

Migrate TestAPISwarmServicesPlugin to integration #36865

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure plugin container is removed on failure#36715

Make sure plugin container is removed on failure #36715

cpuguy83 commented Mar 27, 2018

anusha-ragunathan commented Mar 27, 2018

anusha-ragunathan Mar 28, 2018 •
edited

Loading

anusha-ragunathan Mar 28, 2018

cpuguy83 Mar 28, 2018

anusha-ragunathan Mar 28, 2018

cpuguy83 Mar 29, 2018

anusha-ragunathan Mar 29, 2018 •
edited

Loading

anusha-ragunathan Mar 28, 2018

cpuguy83 Mar 28, 2018

anusha-ragunathan Mar 28, 2018

anusha-ragunathan Mar 28, 2018

cpuguy83 Mar 28, 2018

anusha-ragunathan Mar 28, 2018

cpuguy83 Mar 28, 2018

anusha-ragunathan Mar 28, 2018

cpuguy83 Mar 29, 2018

anusha-ragunathan Mar 29, 2018

stevvooe commented Mar 29, 2018

anusha-ragunathan commented Mar 30, 2018

codecov bot commented Mar 30, 2018 •
edited

Loading

vdemeester left a comment

Make sure plugin container is removed on failure#36715

Make sure plugin container is removed on failure #36715

Conversation

cpuguy83 commented Mar 27, 2018

anusha-ragunathan commented Mar 27, 2018

anusha-ragunathan Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anusha-ragunathan Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevvooe commented Mar 29, 2018

anusha-ragunathan commented Mar 30, 2018

codecov bot commented Mar 30, 2018 • edited Loading

Codecov Report

vdemeester left a comment

Choose a reason for hiding this comment

anusha-ragunathan Mar 28, 2018 •
edited

Loading

anusha-ragunathan Mar 29, 2018 •
edited

Loading

codecov bot commented Mar 30, 2018 •
edited

Loading