v2: Call shim.Delete at first when create is failed

darfux · darfux · commit d82fa431937f · 2019-12-28T00:02:11.000+08:00
If the context is cancelled during `shim.Create()`, such as the client
disconnects unexpectedly. The created shim will never be deleted.
What's more, if the context is cancelled during `openShimLog()`, the
fifo will be closed and block the shim output.

Signed-off-by: Li Yuxuan &lt;liyuxuan04@baidu.com&gt;
diff --git a/runtime/v2/binary.go b/runtime/v2/binary.go
@@ -74,7 +74,7 @@ func (b *binary) Start(ctx context.Context, opts *types.Any, onClose func()) (_
 	if err != nil {
 		return nil, err
 	}
-	f, err := openShimLog(ctx, b.bundle, client.AnonDialer)
+	f, err := openShimLog(context.Background(), b.bundle, client.AnonDialer)
 	if err != nil {
 		return nil, errors.Wrap(err, "open shim log pipe")
 	}
diff --git a/runtime/v2/manager.go b/runtime/v2/manager.go
@@ -29,6 +29,7 @@ import (
 	"github.com/containerd/containerd/metadata"
 	"github.com/containerd/containerd/mount"
 	"github.com/containerd/containerd/namespaces"
+	"github.com/containerd/containerd/pkg/timeout"
 	"github.com/containerd/containerd/platforms"
 	"github.com/containerd/containerd/plugin"
 	"github.com/containerd/containerd/runtime"
@@ -154,8 +155,13 @@ func (m *TaskManager) Create(ctx context.Context, id string, opts runtime.Create
 	}
 	defer func() {
 		if err != nil {
-			shim.Shutdown(ctx)
-			shim.Close()
+			dctx, cancel := timeout.WithContext(context.Background(), cleanupTimeout)
+			defer cancel()
+			_, errShim := shim.Delete(dctx)
+			if errShim != nil {
+				shim.Shutdown(ctx)
+				shim.Close()
+			}
 		}
 	}()
 	t, err := shim.Create(ctx, opts)
diff --git a/runtime/v2/shim.go b/runtime/v2/shim.go
@@ -235,11 +235,11 @@ func (s *shim) Delete(ctx context.Context) (*runtime.Exit, error) {
 	// this seems dirty but it cleans up the API across runtimes, tasks, and the service
 	s.rtTasks.Delete(ctx, s.ID())
 	if err := s.waitShutdown(ctx); err != nil {
-		log.G(ctx).WithError(err).Error("failed to shutdown shim")
+		log.G(ctx).WithField("id", s.ID()).WithError(err).Error("failed to shutdown shim")
 	}
 	s.Close()
 	if err := s.bundle.Delete(); err != nil {
-		log.G(ctx).WithError(err).Error("failed to delete bundle")
+		log.G(ctx).WithField("id", s.ID()).WithError(err).Error("failed to delete bundle")
 	}
 	if shimErr != nil {
 		return nil, shimErr

Original file line number	Diff line number	Diff line change
`@@ -74,7 +74,7 @@ func (b binary) Start(ctx context.Context, opts types.Any, onClose func()) (_`
`74`	`74`	`if err != nil {`
`75`	`75`	`return nil, err`
`76`	`76`	`}`
`77`		`- f, err := openShimLog(ctx, b.bundle, client.AnonDialer)`
	`77`	`+ f, err := openShimLog(context.Background(), b.bundle, client.AnonDialer)`
`78`	`78`	`if err != nil {`
`79`	`79`	`return nil, errors.Wrap(err, "open shim log pipe")`
`80`	`80`	`}`
Original file line number	Diff line number	Diff line change
`@@ -235,11 +235,11 @@ func (s shim) Delete(ctx context.Context) (runtime.Exit, error) {`
`235`	`235`	`// this seems dirty but it cleans up the API across runtimes, tasks, and the service`
`236`	`236`	`s.rtTasks.Delete(ctx, s.ID())`
`237`	`237`	`if err := s.waitShutdown(ctx); err != nil {`
`238`		`- log.G(ctx).WithError(err).Error("failed to shutdown shim")`
	`238`	`+ log.G(ctx).WithField("id", s.ID()).WithError(err).Error("failed to shutdown shim")`
`239`	`239`	`}`
`240`	`240`	`s.Close()`
`241`	`241`	`if err := s.bundle.Delete(); err != nil {`
`242`		`- log.G(ctx).WithError(err).Error("failed to delete bundle")`
	`242`	`+ log.G(ctx).WithField("id", s.ID()).WithError(err).Error("failed to delete bundle")`
`243`	`243`	`}`
`244`	`244`	`if shimErr != nil {`
`245`	`245`	`return nil, shimErr`