*: properly shutdown non-groupable shims to prevent resource leaks

fuweid · k8s-infra-cherrypick-robot · commit cff1feb28c79 · 2025-06-10T18:28:14.000Z
Previously, to address issue #11708, PR #11793 changed containerd to always invoke the shim binary to establish shim connections, rather than reusing the sandbox shim. However, this change did not ensure that the Shutdown API was called to stop the shim process. Starting with containerd v2.0.0, the Shutdown API is only invoked for sandbox containers (when container.SandboxID is empty). This approach works for groupable shims, where multiple containers share a single socket address and only require a single Shutdown call. However, for non-groupable shims, each container requires its own Shutdown call during cleanup to avoid leaking shim processes. Additionally, PR #11793 introduced a corner case during upgrades: - T1: An old container-shim-runc-v2 (<=v1.7.X) is running for pod A. - T2: containerd is upgraded to v2.X.Y. - T3: A new container A-C1 is created in pod A using the new shim-runc-v2 binary. - T4: bootstrap.json indicates version:3 protocol, but it is downgraded to version:2 in memory. - T5: containerd is restarted. - T6: containerd fails to connect to A-C1. - T7: The A-C1 container is left in EXITED status in the CRI plugin. To address this, ensure that loadShimTask downgrades to version:2 if necessary, and always invoke the Shutdown API for each non-groupable shim during cleanup to prevent resource leaks and handle upgrade scenarios correctly. (Introduced by #11793) Signed-off-by: Wei Fu <fuweid89@gmail.com>
diff --git a/core/runtime/v2/shim.go b/core/runtime/v2/shim.go
@@ -556,6 +556,11 @@ func (s *shimTask) delete(ctx context.Context, sandboxed bool, removeTask func(c
 		removeTask(ctx, s.ID())
 	}
 
+	const supportSandboxAPIVersion = 3
+	if _, apiVer := s.ShimInstance.Endpoint(); apiVer < supportSandboxAPIVersion {
+		sandboxed = false
+	}
+
 	// Don't shutdown sandbox as there may be other containers running.
 	// Let controller decide when to shutdown.
 	if !sandboxed {
diff --git a/core/runtime/v2/shim_load.go b/core/runtime/v2/shim_load.go
@@ -206,7 +206,26 @@ func loadShimTask(ctx context.Context, bundle *Bundle, onClose func()) (_ *shimT
 	defer cancel()
 
 	if _, err := s.PID(ctx); err != nil {
-		return nil, err
+		if !errdefs.IsNotImplemented(err) {
+			return nil, err
+		}
+
+		downgrader, ok := shim.(clientVersionDowngrader)
+		if ok {
+			if derr := downgrader.Downgrade(); derr == nil {
+				log.G(ctx).WithError(err).WithField("id", shim.ID()).
+					Warning("failed to call task.PID, downgrading client API version to try again")
+
+				s, err = newShimTask(shim)
+				if err != nil {
+					return nil, fmt.Errorf("failed to create shim task after downgrading: %w", err)
+				}
+				_, err = s.PID(ctx)
+			}
+		}
+		if err != nil {
+			return nil, err
+		}
 	}
 	return s, nil
 }
diff --git a/integration/issue10467_linux_test.go b/integration/issue10467_linux_test.go
@@ -66,7 +66,8 @@ func TestIssue10467(t *testing.T) {
 	})
 
 	t.Log("Prepare pods for current release")
-	upgradeCaseFunc, hookFunc := shouldManipulateContainersInPodAfterUpgrade("")(t, 2, previousProc.criRuntimeService(t), previousProc.criImageService(t))
+	upgradeCaseFuncs, hookFunc := shouldManipulateContainersInPodAfterUpgrade("")(t, 2, previousProc.criRuntimeService(t), previousProc.criImageService(t))
+	upgradeCaseFunc := upgradeCaseFuncs[0]
 	needToCleanup = false
 	require.Nil(t, hookFunc)
 
diff --git a/integration/release_upgrade_linux_test.go b/integration/release_upgrade_linux_test.go
