Merge pull request #13790 from halaney/ahalaney/shim-leak-error

shim_load: Consider shim leaked only if we can't find pids

core/runtime/v2/shim_load.go

@@ -28,6 +28,7 @@ import (
 	"github.com/containerd/log"
 
 	"github.com/containerd/containerd/v2/core/mount"
+	runtimeapi "github.com/containerd/containerd/v2/core/runtime"
 	"github.com/containerd/containerd/v2/pkg/namespaces"
 	"github.com/containerd/containerd/v2/pkg/timeout"
 	"golang.org/x/sync/errgroup"
@@ -188,18 +189,31 @@ func (m *ShimManager) loadShim(ctx context.Context, bundle *Bundle) error {
 
 	_, sgetErr := m.sandboxStore.Get(ctx, id)
 	pInfo, pidErr := shim.Pids(ctx)
-	if sgetErr != nil && errors.Is(sgetErr, errdefs.ErrNotFound) && (len(pInfo) == 0 || errors.Is(pidErr, errdefs.ErrNotFound)) {
-		log.G(ctx).WithField("id", id).Info("cleaning leaked shim process")
-		// We are unable to get Pids from the shim and it's not a sandbox
-		// shim. We should clean it up her.
-		// No need to do anything for removeTask since we never added this shim.
+	if shouldCleanupShim(sgetErr, pidErr, pInfo) {
+		logEntry := log.G(ctx).WithField("id", id)
+		if pidErr != nil {
+			logEntry = logEntry.WithError(pidErr)
+		}
+		logEntry.Info("cleaning leaked shim process")
 		shim.delete(ctx, false, func(ctx context.Context, id string) {})
 	} else {
+		if pidErr != nil {
+			log.G(ctx).WithField("id", id).WithError(pidErr).Warn("failed to query shim pids, keeping shim registered")
+		}
 		m.shims.Add(ctx, shim.ShimInstance)
 	}
 	return nil
 }
 
+// shouldCleanupShim determines whether or not a shim is in such a state that
+// we should reap it. To be reapable we confirm that it is not a sandbox shim
+// and it has no pids running
+func shouldCleanupShim(sgetErr, pidErr error, pInfo []runtimeapi.ProcessInfo) bool {
+	return errors.Is(sgetErr, errdefs.ErrNotFound) &&
+		(errors.Is(pidErr, errdefs.ErrNotFound) ||
+			(pidErr == nil && len(pInfo) == 0))
+}
+
 func loadShimTask(ctx context.Context, bundle *Bundle, onClose func()) (_ *shimTask, retErr error) {
 	shim, err := loadShim(ctx, bundle, onClose)
 	if err != nil {

core/runtime/v2/shim_load_test.go

@@ -0,0 +1,88 @@
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package v2
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/containerd/errdefs"
+	"github.com/stretchr/testify/require"
+
+	runtimeapi "github.com/containerd/containerd/v2/core/runtime"
+)
+
+func TestShouldCleanupShim(t *testing.T) {
+	otherErr := errors.New("some other error")
+
+	testCases := []struct {
+		Name     string
+		SgetErr  error
+		PidErr   error
+		PInfo    []runtimeapi.ProcessInfo
+		Expected bool
+	}{
+		{
+			Name:     "sandbox found",
+			SgetErr:  nil,
+			PidErr:   nil,
+			PInfo:    nil,
+			Expected: false,
+		},
+		{
+			Name:     "sandbox lookup fails with unrelated error",
+			SgetErr:  otherErr,
+			PidErr:   nil,
+			PInfo:    nil,
+			Expected: false,
+		},
+		{
+			Name:     "not a sandbox, no pids running",
+			SgetErr:  errdefs.ErrNotFound,
+			PidErr:   nil,
+			PInfo:    []runtimeapi.ProcessInfo{},
+			Expected: true,
+		},
+		{
+			Name:     "not a sandbox, pids still running",
+			SgetErr:  errdefs.ErrNotFound,
+			PidErr:   nil,
+			PInfo:    []runtimeapi.ProcessInfo{{Pid: 1234}},
+			Expected: false,
+		},
+		{
+			Name:     "not a sandbox, pids lookup returns not found",
+			SgetErr:  errdefs.ErrNotFound,
+			PidErr:   errdefs.ErrNotFound,
+			PInfo:    nil,
+			Expected: true,
+		},
+		{
+			Name:     "not a sandbox, pids lookup fails with other error",
+			SgetErr:  errdefs.ErrNotFound,
+			PidErr:   otherErr,
+			PInfo:    nil,
+			Expected: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.Name, func(t *testing.T) {
+			require.Equal(t, tc.Expected, shouldCleanupShim(tc.SgetErr, tc.PidErr, tc.PInfo))
+		})
+	}
+}