diff --git a/core/runtime/v2/shim_load.go b/core/runtime/v2/shim_load.go index 4d8e6217d9..3401596c81 100644 --- a/core/runtime/v2/shim_load.go +++ b/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 { diff --git a/core/runtime/v2/shim_load_test.go b/core/runtime/v2/shim_load_test.go new file mode 100644 index 0000000000..7be1b0b4f6 --- /dev/null +++ b/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)) + }) + } +}