Merge pull request #4755 from thaJeztah/1.3_backport_cancel_shim_log_ctx_by_onclose

mxpv · web-flow · commit 1e683ff2250a · 2020-12-11T11:08:31.000-08:00
[release/1.3 backport] v2: Cancel shim log ctx when ttrpc is closed
diff --git a/runtime/v2/binary.go b/runtime/v2/binary.go
@@ -26,6 +26,7 @@ import (
 
 	"github.com/containerd/containerd/events/exchange"
 	"github.com/containerd/containerd/log"
+	"github.com/containerd/containerd/namespaces"
 	"github.com/containerd/containerd/runtime"
 	client "github.com/containerd/containerd/runtime/v2/shim"
 	"github.com/containerd/containerd/runtime/v2/task"
@@ -74,7 +75,15 @@ 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)
+	// Windows needs a namespace when openShimLog
+	ns, _ := namespaces.Namespace(ctx)
+	shimCtx, cancelShimLog := context.WithCancel(namespaces.WithNamespace(context.Background(), ns))
+	defer func() {
+		if err != nil {
+			cancelShimLog()
+		}
+	}()
+	f, err := openShimLog(shimCtx, b.bundle, client.AnonDialer)
 	if err != nil {
 		return nil, errors.Wrap(err, "open shim log pipe")
 	}
@@ -103,7 +112,11 @@ func (b *binary) Start(ctx context.Context, opts *types.Any, onClose func()) (_
 	if err != nil {
 		return nil, err
 	}
-	client := ttrpc.NewClient(conn, ttrpc.WithOnClose(onClose))
+	onCloseWithShimLog := func() {
+		onClose()
+		cancelShimLog()
+	}
+	client := ttrpc.NewClient(conn, ttrpc.WithOnClose(onCloseWithShimLog))
 	return &shim{
 		bundle:  b.bundle,
 		client:  client,
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
`@@ -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`