Codecov Report

Merging #2151 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2151   +/-   ##
=======================================
  Coverage   45.41%   45.41%           
=======================================
  Files          82       82           
  Lines        8749     8749           
=======================================
  Hits         3973     3973           
  Misses       4130     4130           
  Partials      646      646

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 3b4fcf7...afb3f39. Read the comment docs.

What about calling close after client.TaskService().Delete() instead?

What about calling close after client.TaskService().Delete() instead?

That is an option, too. It depends on how we define the order.
If we think that a deleted task means all associated resources are released, then we should put close in front.
If we don't care about the fifo directory. I think we can do that after. :)

If we don't care about the fifo directory. I think we can do that after. :)

Well, it's a temp directory, a new one is created everytime. I'm on the position to delete the resources only once the task is actually gone. Especially since we assume those resources are there on attach.

I don't think we should have the mkdir in two places. I think I agree with @mlaventure. How about something like this?

diff --git a/task.go b/task.go
index 68308b62..5ee36337 100644
--- a/task.go
+++ b/task.go
@@ -267,7 +267,6 @@ func (t *task) Delete(ctx context.Context, opts ...ProcessDeleteOpts) (*ExitStat
 	if t.io != nil {
 		t.io.Cancel()
 		t.io.Wait()
-		t.io.Close()
 	}
 	r, err := t.client.TaskService().Delete(ctx, &tasks.DeleteTaskRequest{
 		ContainerID: t.id,
@@ -275,6 +274,10 @@ func (t *task) Delete(ctx context.Context, opts ...ProcessDeleteOpts) (*ExitStat
 	if err != nil {
 		return nil, errdefs.FromGRPC(err)
 	}
+	// Only close the IO after a successful Delete
+	if t.io != nil {
+		t.io.Close()
+	}
 	return &ExitStatus{code: r.ExitStatus, exitedAt: r.ExitedAt}, nil
 }

@dnephin OK. Will do so. :)

Done

The test failure seems like something wrong with golang 1.10 to me:

    The stdout of exec should be hello
    Expected
        <string>: hello
        
    to equal
        <string>: hello
        

It does equal, even the tailing \n is right...

Hm, it constantly fails with golang 1.10...
Let me take a look.

golang 1.10 has a bug... I'll file an issue and send a PR.

This is a golang 1.10 bug, see:

• bytes: Buffer.ReadFrom is wrong golang/go#24091
• bytes: set buffer size correctly in ReadFrom golang/go#24092

Let's not use golang 1.10 for now... This bug is really easy to hit...

This turns out to be a race condition exposed in golang 1.10. Send out a fix for cri-tools kubernetes-sigs/cri-tools#257.

Will update cri-tools as soon as that is fixed to unblock the submit queue.

Rebase now that the other PR is merged?

Will do now.

@crosbymichael Done.

Seems like another race condition for concurrent image pull.

E0226 22:26:15.401313    5139 remote_runtime.go:92] RunPodSandbox from runtime service failed: rpc error: code = Unknown desc = failed to get sandbox image "gcr.io/google_containers/pause:3.0": failed to pull image "gcr.io/google_containers/pause:3.0": failed to pull image "gcr.io/google_containers/pause:3.0": failed to write config: ref k8s.io/11/config-sha256:5fbdc14c118c31ddd5c553f66892b846469341287a0d356485b477467cae6dd0 locked: unavailable
Feb 26 22:26:15.401: INFO: Unexpected error occurred: rpc error: code = Unknown desc = failed to get sandbox image "gcr.io/google_containers/pause:3.0": failed to pull image "gcr.io/google_containers/pause:3.0": failed to pull image "gcr.io/google_containers/pause:3.0": failed to write config: ref k8s.io/11/config-sha256:5fbdc14c118c31ddd5c553f66892b846469341287a0d356485b477467cae6dd0 locked: unavailable

LGTM