From 7bca70c0c31749596f6c9376fd871318dcb44264 Mon Sep 17 00:00:00 2001
From: Abel Feng <fshb1988@gmail.com>
Date: Mon, 5 Jun 2023 21:02:19 +0800
Subject: [PATCH] sandbox: do not call Connect when loadShim

The ShimManager.Start() will call loadShim() to get the existing shim if SandboxID
is specified for a container, but shimTask.PID() is called in loadShim,
which will call Connect() of Task API with the ID of a task that is not
created yet(containerd is getting the shim and Task API address to call
Create, so the task is not created yet).
In this commit we change the logic of loadShim() to get the shim without calling
Connect() of the not created container ID.

Signed-off-by: Abel Feng <fshb1988@gmail.com>
---
 runtime/v2/shim.go      | 13 -------------
 runtime/v2/shim_load.go | 27 ++++++++++++++++++++++-----
 2 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/runtime/v2/shim.go b/runtime/v2/shim.go
index 46452b55b..4b6172602 100644
--- a/runtime/v2/shim.go
+++ b/runtime/v2/shim.go
@@ -129,19 +129,6 @@ func loadShim(ctx context.Context, bundle *Bundle, onClose func()) (_ ShimInstan
 		client: conn,
 	}
 
-	ctx, cancel := timeout.WithContext(ctx, loadTimeout)
-	defer cancel()
-
-	// Check connectivity, TaskService is the only required service, so create a temp one to check connection.
-	s, err := newShimTask(shim)
-	if err != nil {
-		return nil, err
-	}
-
-	if _, err := s.PID(ctx); err != nil {
-		return nil, err
-	}
-
 	return shim, nil
 }
 
diff --git a/runtime/v2/shim_load.go b/runtime/v2/shim_load.go
index 7b0b4c8b7..5556e6c4d 100644
--- a/runtime/v2/shim_load.go
+++ b/runtime/v2/shim_load.go
@@ -26,6 +26,7 @@ import (
 	"github.com/containerd/containerd/mount"
 	"github.com/containerd/containerd/namespaces"
 	"github.com/containerd/containerd/pkg/cleanup"
+	"github.com/containerd/containerd/pkg/timeout"
 	"github.com/containerd/log"
 )
 
@@ -141,7 +142,7 @@ func (m *ShimManager) loadShims(ctx context.Context) error {
 				ttrpcAddress: m.containerdTTRPCAddress,
 				schedCore:    m.schedCore,
 			})
-		instance, err := loadShim(ctx, bundle, func() {
+		shim, err := loadShimTask(ctx, bundle, func() {
 			log.G(ctx).WithField("id", id).Info("shim disconnected")
 
 			cleanupAfterDeadShim(cleanup.Background(ctx), id, m.shims, m.events, binaryCall)
@@ -152,10 +153,6 @@ func (m *ShimManager) loadShims(ctx context.Context) error {
 			cleanupAfterDeadShim(ctx, id, m.shims, m.events, binaryCall)
 			continue
 		}
-		shim, err := newShimTask(instance)
-		if err != nil {
-			return err
-		}
 
 		// There are 3 possibilities for the loaded shim here:
 		// 1. It could be a shim that is running a task.
@@ -180,6 +177,26 @@ func (m *ShimManager) loadShims(ctx context.Context) error {
 	return nil
 }
 
+func loadShimTask(ctx context.Context, bundle *Bundle, onClose func()) (_ *shimTask, retErr error) {
+	shim, err := loadShim(ctx, bundle, onClose)
+	if err != nil {
+		return nil, err
+	}
+	// Check connectivity, TaskService is the only required service, so create a temp one to check connection.
+	s, err := newShimTask(shim)
+	if err != nil {
+		return nil, err
+	}
+
+	ctx, cancel := timeout.WithContext(ctx, loadTimeout)
+	defer cancel()
+
+	if _, err := s.PID(ctx); err != nil {
+		return nil, err
+	}
+	return s, nil
+}
+
 func (m *ShimManager) cleanupWorkDirs(ctx context.Context) error {
 	ns, err := namespaces.NamespaceRequired(ctx)
 	if err != nil {