Merge pull request #4538 from fuweid/update-shim-cleanup

estesp · web-flow · commit 68d97331be50 · 2020-09-21T13:32:40.000-04:00
runtime/v2: cleanup dead shim before delete bundle
diff --git a/runtime/v2/manager.go b/runtime/v2/manager.go
@@ -138,12 +138,8 @@ func (m *TaskManager) Create(ctx context.Context, id string, opts runtime.Create
 	b := shimBinary(ctx, bundle, opts.Runtime, m.containerdAddress, m.containerdTTRPCAddress, m.events, m.tasks)
 	shim, err := b.Start(ctx, topts, func() {
 		log.G(ctx).WithField("id", id).Info("shim disconnected")
-		_, err := m.tasks.Get(ctx, id)
-		if err != nil {
-			// Task was never started or was already successfully deleted
-			return
-		}
-		cleanupAfterDeadShim(context.Background(), id, ns, m.events, b)
+
+		cleanupAfterDeadShim(context.Background(), id, ns, m.tasks, m.events, b)
 		// Remove self from the runtime task list. Even though the cleanupAfterDeadShim()
 		// would publish taskExit event, but the shim.Delete() would always failed with ttrpc
 		// disconnect and there is no chance to remove this dead task from runtime task lists.
@@ -266,17 +262,13 @@ func (m *TaskManager) loadTasks(ctx context.Context) error {
 		binaryCall := shimBinary(ctx, bundle, container.Runtime.Name, m.containerdAddress, m.containerdTTRPCAddress, m.events, m.tasks)
 		shim, err := loadShim(ctx, bundle, m.events, m.tasks, func() {
 			log.G(ctx).WithField("id", id).Info("shim disconnected")
-			_, err := m.tasks.Get(ctx, id)
-			if err != nil {
-				// Task was never started or was already successfully deleted
-				return
-			}
-			cleanupAfterDeadShim(context.Background(), id, ns, m.events, binaryCall)
+
+			cleanupAfterDeadShim(context.Background(), id, ns, m.tasks, m.events, binaryCall)
 			// Remove self from the runtime task list.
 			m.tasks.Delete(ctx, id)
 		})
 		if err != nil {
-			cleanupAfterDeadShim(ctx, id, ns, m.events, binaryCall)
+			cleanupAfterDeadShim(ctx, id, ns, m.tasks, m.events, binaryCall)
 			continue
 		}
 		m.tasks.Add(ctx, shim)
diff --git a/runtime/v2/shim.go b/runtime/v2/shim.go
@@ -121,7 +121,7 @@ func loadShim(ctx context.Context, bundle *Bundle, events *exchange.Exchange, rt
 	return s, nil
 }
 
-func cleanupAfterDeadShim(ctx context.Context, id, ns string, events *exchange.Exchange, binaryCall *binary) {
+func cleanupAfterDeadShim(ctx context.Context, id, ns string, rt *runtime.TaskList, events *exchange.Exchange, binaryCall *binary) {
 	ctx = namespaces.WithNamespace(ctx, ns)
 	ctx, cancel := timeout.WithContext(ctx, cleanupTimeout)
 	defer cancel()
@@ -138,6 +138,12 @@ func cleanupAfterDeadShim(ctx context.Context, id, ns string, events *exchange.E
 		}).Warn("failed to clean up after shim disconnected")
 	}
 
+	if _, err := rt.Get(ctx, id); err != nil {
+		// Task was never started or was already successfully deleted
+		// No need to publish events
+		return
+	}
+
 	var (
 		pid        uint32
 		exitStatus uint32
@@ -234,13 +240,15 @@ func (s *shim) Delete(ctx context.Context) (*runtime.Exit, error) {
 			}
 		}
 	}
-	// remove self from the runtime task list
-	// 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).WithField("id", s.ID()).WithError(err).Error("failed to shutdown shim")
 	}
 	s.Close()
+	s.client.UserOnCloseWait(ctx)
+
+	// remove self from the runtime task list
+	// this seems dirty but it cleans up the API across runtimes, tasks, and the service
+	s.rtTasks.Delete(ctx, s.ID())
 	if err := s.bundle.Delete(); err != nil {
 		log.G(ctx).WithField("id", s.ID()).WithError(err).Error("failed to delete bundle")
 	}
diff --git a/vendor.conf b/vendor.conf
@@ -8,7 +8,7 @@ github.com/containerd/continuity                    efbc4488d8fe1bdc16bde3b2d299
 github.com/containerd/fifo                          f15a3290365b9d2627d189e619ab4008e0069caf
 github.com/containerd/go-runc                       7016d3ce2328dd2cb1192b2076ebd565c4e8df0c
 github.com/containerd/nri                           0afc7f031eaf9c7d9c1a381b7ab5462e89c998fc
-github.com/containerd/ttrpc                         v1.0.1
+github.com/containerd/ttrpc                         v1.0.2
 github.com/containerd/typeurl                       v1.0.1
 github.com/coreos/go-systemd/v22                    v22.1.0
 github.com/cpuguy83/go-md2man/v2                    v2.0.0
diff --git a/vendor/github.com/containerd/ttrpc/client.go b/vendor/github.com/containerd/ttrpc/client.go

Original file line number	Diff line number	Diff line change
`@@ -121,7 +121,7 @@ func loadShim(ctx context.Context, bundle Bundle, events exchange.Exchange, rt`
`121`	`121`	`return s, nil`
`122`	`122`	`}`
`123`	`123`
`124`		`-func cleanupAfterDeadShim(ctx context.Context, id, ns string, events exchange.Exchange, binaryCall binary) {`
	`124`	`+func cleanupAfterDeadShim(ctx context.Context, id, ns string, rt runtime.TaskList, events exchange.Exchange, binaryCall *binary) {`
`125`	`125`	`ctx = namespaces.WithNamespace(ctx, ns)`
`126`	`126`	`ctx, cancel := timeout.WithContext(ctx, cleanupTimeout)`
`127`	`127`	`defer cancel()`
`@@ -138,6 +138,12 @@ func cleanupAfterDeadShim(ctx context.Context, id, ns string, events *exchange.E`
`138`	`138`	`}).Warn("failed to clean up after shim disconnected")`
`139`	`139`	`}`
`140`	`140`
	`141`	`+ if _, err := rt.Get(ctx, id); err != nil {`
	`142`	`+ // Task was never started or was already successfully deleted`
	`143`	`+ // No need to publish events`
	`144`	`+ return`
	`145`	`+ }`
	`146`	`+`
`141`	`147`	`var (`
`142`	`148`	`pid uint32`
`143`	`149`	`exitStatus uint32`
`@@ -234,13 +240,15 @@ func (s shim) Delete(ctx context.Context) (runtime.Exit, error) {`
`234`	`240`	`}`
`235`	`241`	`}`
`236`	`242`	`}`
`237`		`- // remove self from the runtime task list`
`238`		`- // this seems dirty but it cleans up the API across runtimes, tasks, and the service`
`239`		`- s.rtTasks.Delete(ctx, s.ID())`
`240`	`243`	`if err := s.waitShutdown(ctx); err != nil {`
`241`	`244`	`log.G(ctx).WithField("id", s.ID()).WithError(err).Error("failed to shutdown shim")`
`242`	`245`	`}`
`243`	`246`	`s.Close()`
	`247`	`+ s.client.UserOnCloseWait(ctx)`
	`248`	`+`
	`249`	`+ // remove self from the runtime task list`
	`250`	`+ // this seems dirty but it cleans up the API across runtimes, tasks, and the service`
	`251`	`+ s.rtTasks.Delete(ctx, s.ID())`
`244`	`252`	`if err := s.bundle.Delete(); err != nil {`
`245`	`253`	`log.G(ctx).WithField("id", s.ID()).WithError(err).Error("failed to delete bundle")`
`246`	`254`	`}`