Skip to content

Commit 5ee9839

Browse files
AkihiroSudaAkihiroSuda
authored
Merge pull request #9003 from fuweid/cp-17-8954
[releases/1.7] *: fix leaked shim caused by high IO pressure
2 parents d06fd93 + 537d752 commit 5ee9839

7 files changed

Lines changed: 459 additions & 1 deletion

File tree

‎Vagrantfile‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ Vagrant.configure("2") do |config|
7878
libselinux-devel \
7979
lsof \
8080
make \
81+
strace \
8182
${INSTALL_PACKAGES}
8283
SHELL
8384
end

‎integration/issue7496_linux_test.go‎

Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
/*
2+
Copyright The containerd Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package integration
18+
19+
import (
20+
"bufio"
21+
"context"
22+
"io"
23+
"net"
24+
"os"
25+
"strconv"
26+
"strings"
27+
"syscall"
28+
"testing"
29+
"time"
30+
31+
apitask "github.com/containerd/containerd/api/runtime/task/v2"
32+
"github.com/containerd/containerd/integration/images"
33+
"github.com/containerd/containerd/namespaces"
34+
"github.com/containerd/containerd/runtime/v2/shim"
35+
"github.com/containerd/ttrpc"
36+
"github.com/stretchr/testify/assert"
37+
"github.com/stretchr/testify/require"
38+
exec "golang.org/x/sys/execabs"
39+
)
40+
41+
// TestIssue7496 is used to reproduce https://github.com/containerd/containerd/issues/7496
42+
//
43+
// NOTE: https://github.com/containerd/containerd/issues/8931 is the same issue.
44+
func TestIssue7496(t *testing.T) {
45+
t.Logf("Checking CRI config's default runtime")
46+
criCfg, err := CRIConfig()
47+
require.NoError(t, err)
48+
49+
typ := criCfg.ContainerdConfig.Runtimes[criCfg.ContainerdConfig.DefaultRuntimeName].Type
50+
if !strings.HasSuffix(typ, "runc.v2") {
51+
t.Skipf("default runtime should be runc.v2, but it's not: %s", typ)
52+
}
53+
54+
ctx := namespaces.WithNamespace(context.Background(), "k8s.io")
55+
56+
t.Logf("Create a pod config and run sandbox container")
57+
sbConfig := PodSandboxConfig("sandbox", "issue7496")
58+
sbID, err := runtimeService.RunPodSandbox(sbConfig, *runtimeHandler)
59+
require.NoError(t, err)
60+
61+
shimCli := connectToShim(ctx, t, sbID)
62+
63+
delayInSec := 12
64+
t.Logf("[shim pid: %d]: Injecting %d seconds delay to umount2 syscall",
65+
shimPid(ctx, t, shimCli),
66+
delayInSec)
67+
68+
doneCh := injectDelayToUmount2(ctx, t, shimCli, delayInSec /* CRI plugin uses 10 seconds to delete task */)
69+
70+
t.Logf("Create a container config and run container in a pod")
71+
pauseImage := images.Get(images.Pause)
72+
EnsureImageExists(t, pauseImage)
73+
74+
containerConfig := ContainerConfig("pausecontainer", pauseImage)
75+
cnID, err := runtimeService.CreateContainer(sbID, containerConfig, sbConfig)
76+
require.NoError(t, err)
77+
require.NoError(t, runtimeService.StartContainer(cnID))
78+
79+
t.Logf("Start to StopPodSandbox and RemovePodSandbox")
80+
ctx, cancelFn := context.WithTimeout(ctx, 3*time.Minute)
81+
defer cancelFn()
82+
for {
83+
select {
84+
case <-ctx.Done():
85+
require.NoError(t, ctx.Err(), "The StopPodSandbox should be done in time")
86+
default:
87+
}
88+
89+
err := runtimeService.StopPodSandbox(sbID)
90+
if err != nil {
91+
t.Logf("Failed to StopPodSandbox: %v", err)
92+
continue
93+
}
94+
95+
err = runtimeService.RemovePodSandbox(sbID)
96+
if err == nil {
97+
break
98+
}
99+
t.Logf("Failed to RemovePodSandbox: %v", err)
100+
time.Sleep(1 * time.Second)
101+
}
102+
103+
t.Logf("PodSandbox %s has been deleted and start to wait for strace exit", sbID)
104+
select {
105+
case <-time.After(15 * time.Second):
106+
resp, err := shimCli.Connect(ctx, &apitask.ConnectRequest{})
107+
assert.Error(t, err, "should failed to call shim connect API")
108+
109+
t.Errorf("Strace doesn't exit in time")
110+
111+
t.Logf("Cleanup the shim (pid: %d)", resp.GetShimPid())
112+
syscall.Kill(int(resp.GetShimPid()), syscall.SIGKILL)
113+
<-doneCh
114+
case <-doneCh:
115+
}
116+
}
117+
118+
// injectDelayToUmount2 uses strace(1) to inject delay on umount2 syscall to
119+
// simulate IO pressure because umount2 might force kernel to syncfs, for
120+
// example, umount overlayfs rootfs which doesn't with volatile.
121+
//
122+
// REF: https://man7.org/linux/man-pages/man1/strace.1.html
123+
func injectDelayToUmount2(ctx context.Context, t *testing.T, shimCli apitask.TaskService, delayInSec int) chan struct{} {
124+
pid := shimPid(ctx, t, shimCli)
125+
126+
doneCh := make(chan struct{})
127+
128+
cmd := exec.CommandContext(ctx, "strace",
129+
"-p", strconv.Itoa(int(pid)), "-f", // attach to all the threads
130+
"--detach-on=execve", // stop to attach runc child-processes
131+
"--trace=umount2", // only trace umount2 syscall
132+
"-e", "inject=umount2:delay_enter="+strconv.Itoa(delayInSec)+"s",
133+
)
134+
cmd.SysProcAttr = &syscall.SysProcAttr{Pdeathsig: syscall.SIGKILL}
135+
136+
pipeR, pipeW := io.Pipe()
137+
cmd.Stdout = pipeW
138+
cmd.Stderr = pipeW
139+
140+
require.NoError(t, cmd.Start())
141+
142+
// ensure that strace has attached to the shim
143+
readyCh := make(chan struct{})
144+
go func() {
145+
defer close(doneCh)
146+
147+
bufReader := bufio.NewReader(pipeR)
148+
_, err := bufReader.Peek(1)
149+
assert.NoError(t, err, "failed to ensure that strace has attached to shim")
150+
151+
close(readyCh)
152+
io.Copy(os.Stdout, bufReader)
153+
t.Logf("Strace has exited")
154+
}()
155+
156+
go func() {
157+
defer pipeW.Close()
158+
assert.NoError(t, cmd.Wait(), "strace should exit with zero code")
159+
}()
160+
161+
<-readyCh
162+
return doneCh
163+
}
164+
165+
func connectToShim(ctx context.Context, t *testing.T, id string) apitask.TaskService {
166+
addr, err := shim.SocketAddress(ctx, containerdEndpoint, id)
167+
require.NoError(t, err)
168+
addr = strings.TrimPrefix(addr, "unix://")
169+
170+
conn, err := net.Dial("unix", addr)
171+
require.NoError(t, err)
172+
173+
client := ttrpc.NewClient(conn)
174+
return apitask.NewTaskClient(client)
175+
}
176+
177+
func shimPid(ctx context.Context, t *testing.T, shimCli apitask.TaskService) uint32 {
178+
resp, err := shimCli.Connect(ctx, &apitask.ConnectRequest{})
179+
require.NoError(t, err)
180+
return resp.GetShimPid()
181+
}

‎integration/issue7496_shutdown_linux_test.go‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
/*
2+
Copyright The containerd Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package integration
18+
19+
import (
20+
"context"
21+
"testing"
22+
23+
"github.com/stretchr/testify/require"
24+
25+
apitask "github.com/containerd/containerd/api/runtime/task/v2"
26+
"github.com/containerd/containerd/namespaces"
27+
)
28+
29+
// TestIssue7496_ShouldRetryShutdown is based on https://github.com/containerd/containerd/issues/7496.
30+
//
31+
// Assume that the shim.Delete takes almost 10 seconds and returns successfully
32+
// and there is no container in shim. However, the context is close to be
33+
// canceled. It will fail to call Shutdown. If we ignores the Canceled error,
34+
// the shim will be leaked. In order to reproduce this, this case will use
35+
// failpoint to inject error into Shutdown API, and then check whether the shim
36+
// is leaked.
37+
func TestIssue7496_ShouldRetryShutdown(t *testing.T) {
38+
// TODO: re-enable if we can retry Shutdown API.
39+
t.Skipf("Please re-enable me if we can retry Shutdown API")
40+
41+
ctx := namespaces.WithNamespace(context.Background(), "k8s.io")
42+
43+
t.Logf("Create a pod config with shutdown failpoint")
44+
sbConfig := PodSandboxConfig("sandbox", "issue7496_shouldretryshutdown")
45+
injectShimFailpoint(t, sbConfig, map[string]string{
46+
"Shutdown": "1*error(please retry)",
47+
})
48+
49+
t.Logf("RunPodSandbox")
50+
sbID, err := runtimeService.RunPodSandbox(sbConfig, failpointRuntimeHandler)
51+
require.NoError(t, err)
52+
53+
t.Logf("Connect to the shim %s", sbID)
54+
shimCli := connectToShim(ctx, t, sbID)
55+
56+
t.Logf("Log shim %s's pid: %d", sbID, shimPid(ctx, t, shimCli))
57+
58+
t.Logf("StopPodSandbox and RemovePodSandbox")
59+
require.NoError(t, runtimeService.StopPodSandbox(sbID))
60+
require.NoError(t, runtimeService.RemovePodSandbox(sbID))
61+
62+
t.Logf("Check the shim connection")
63+
_, err = shimCli.Connect(ctx, &apitask.ConnectRequest{})
64+
require.Error(t, err, "should failed to call shim connect API")
65+
}

‎pkg/cri/sbserver/events.go‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525

2626
"github.com/containerd/containerd"
2727
eventtypes "github.com/containerd/containerd/api/events"
28+
apitasks "github.com/containerd/containerd/api/services/tasks/v1"
2829
containerdio "github.com/containerd/containerd/cio"
2930
"github.com/containerd/containerd/errdefs"
3031
"github.com/containerd/containerd/events"
@@ -404,6 +405,51 @@ func handleContainerExit(ctx context.Context, e *eventtypes.TaskExit, cntr conta
404405
// Move on to make sure container status is updated.
405406
}
406407
}
408+
409+
// NOTE: Both sb.Container.Task and task.Delete interface always ensures
410+
// that the status of target task. However, the interfaces return
411+
// ErrNotFound, which doesn't mean that the shim instance doesn't exist.
412+
//
413+
// There are two caches for task in containerd:
414+
//
415+
// 1. io.containerd.service.v1.tasks-service
416+
// 2. io.containerd.runtime.v2.task
417+
//
418+
// First one is to maintain the shim connection and shutdown the shim
419+
// in Delete API. And the second one is to maintain the lifecycle of
420+
// task in shim server.
421+
//
422+
// So, if the shim instance is running and task has been deleted in shim
423+
// server, the sb.Container.Task and task.Delete will receive the
424+
// ErrNotFound. If we don't delete the shim instance in io.containerd.service.v1.tasks-service,
425+
// shim will be leaky.
426+
//
427+
// Based on containerd/containerd#7496 issue, when host is under IO
428+
// pressure, the umount2 syscall will take more than 10 seconds so that
429+
// the CRI plugin will cancel this task.Delete call. However, the shim
430+
// server isn't aware about this. After return from umount2 syscall, the
431+
// shim server continue delete the task record. And then CRI plugin
432+
// retries to delete task and retrieves ErrNotFound and marks it as
433+
// stopped. Therefore, The shim is leaky.
434+
//
435+
// It's hard to handle the connection lost or request canceled cases in
436+
// shim server. We should call Delete API to io.containerd.service.v1.tasks-service
437+
// to ensure that shim instance is shutdown.
438+
//
439+
// REF:
440+
// 1. https://github.com/containerd/containerd/issues/7496#issuecomment-1671100968
441+
// 2. https://github.com/containerd/containerd/issues/8931
442+
if errdefs.IsNotFound(err) {
443+
_, err = c.client.TaskService().Delete(ctx, &apitasks.DeleteTaskRequest{ContainerID: cntr.Container.ID()})
444+
if err != nil {
445+
err = errdefs.FromGRPC(err)
446+
if !errdefs.IsNotFound(err) {
447+
return fmt.Errorf("failed to cleanup container %s in task-service: %w", cntr.Container.ID(), err)
448+
}
449+
}
450+
logrus.Infof("Ensure that container %s in task-service has been cleanup successfully", cntr.Container.ID())
451+
}
452+
407453
err = cntr.Status.UpdateSync(func(status containerstore.Status) (containerstore.Status, error) {
408454
if status.FinishedAt == 0 {
409455
status.Pid = 0

‎pkg/cri/sbserver/podsandbox/sandbox_delete.go‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"fmt"
2222

2323
"github.com/containerd/containerd"
24+
apitasks "github.com/containerd/containerd/api/services/tasks/v1"
2425
"github.com/containerd/containerd/errdefs"
2526
"github.com/containerd/containerd/log"
2627
)
@@ -49,6 +50,10 @@ func (c *Controller) Shutdown(ctx context.Context, sandboxID string) error {
4950

5051
// Delete sandbox container.
5152
if sandbox.Container != nil {
53+
if err := c.cleanupSandboxTask(ctx, sandbox.Container); err != nil {
54+
return fmt.Errorf("failed to delete sandbox task %q: %w", sandboxID, err)
55+
}
56+
5257
if err := sandbox.Container.Delete(ctx, containerd.WithSnapshotCleanup); err != nil {
5358
if !errdefs.IsNotFound(err) {
5459
return fmt.Errorf("failed to delete sandbox container %q: %w", sandboxID, err)
@@ -59,3 +64,63 @@ func (c *Controller) Shutdown(ctx context.Context, sandboxID string) error {
5964

6065
return nil
6166
}
67+
68+
func (c *Controller) cleanupSandboxTask(ctx context.Context, sbCntr containerd.Container) error {
69+
task, err := sbCntr.Task(ctx, nil)
70+
if err != nil {
71+
if !errdefs.IsNotFound(err) {
72+
return fmt.Errorf("failed to load task for sandbox: %w", err)
73+
}
74+
} else {
75+
if _, err = task.Delete(ctx, containerd.WithProcessKill); err != nil {
76+
if !errdefs.IsNotFound(err) {
77+
return fmt.Errorf("failed to stop sandbox: %w", err)
78+
}
79+
}
80+
}
81+
82+
// NOTE: Both sb.Container.Task and task.Delete interface always ensures
83+
// that the status of target task. However, the interfaces return
84+
// ErrNotFound, which doesn't mean that the shim instance doesn't exist.
85+
//
86+
// There are two caches for task in containerd:
87+
//
88+
// 1. io.containerd.service.v1.tasks-service
89+
// 2. io.containerd.runtime.v2.task
90+
//
91+
// First one is to maintain the shim connection and shutdown the shim
92+
// in Delete API. And the second one is to maintain the lifecycle of
93+
// task in shim server.
94+
//
95+
// So, if the shim instance is running and task has been deleted in shim
96+
// server, the sb.Container.Task and task.Delete will receive the
97+
// ErrNotFound. If we don't delete the shim instance in io.containerd.service.v1.tasks-service,
98+
// shim will be leaky.
99+
//
100+
// Based on containerd/containerd#7496 issue, when host is under IO
101+
// pressure, the umount2 syscall will take more than 10 seconds so that
102+
// the CRI plugin will cancel this task.Delete call. However, the shim
103+
// server isn't aware about this. After return from umount2 syscall, the
104+
// shim server continue delete the task record. And then CRI plugin
105+
// retries to delete task and retrieves ErrNotFound and marks it as
106+
// stopped. Therefore, The shim is leaky.
107+
//
108+
// It's hard to handle the connection lost or request canceled cases in
109+
// shim server. We should call Delete API to io.containerd.service.v1.tasks-service
110+
// to ensure that shim instance is shutdown.
111+
//
112+
// REF:
113+
// 1. https://github.com/containerd/containerd/issues/7496#issuecomment-1671100968
114+
// 2. https://github.com/containerd/containerd/issues/8931
115+
if errdefs.IsNotFound(err) {
116+
_, err = c.client.TaskService().Delete(ctx, &apitasks.DeleteTaskRequest{ContainerID: sbCntr.ID()})
117+
if err != nil {
118+
err = errdefs.FromGRPC(err)
119+
if !errdefs.IsNotFound(err) {
120+
return fmt.Errorf("failed to cleanup sandbox %s in task-service: %w", sbCntr.ID(), err)
121+
}
122+
}
123+
log.G(ctx).Infof("Ensure that sandbox %s in task-service has been cleanup successfully", sbCntr.ID())
124+
}
125+
return nil
126+
}

0 commit comments

Comments
 (0)