Skip to content

Commit 859e43e

Browse files
author
Vincent Demeester
authored
Merge pull request moby#36715 from cpuguy83/plugin_exec_fixes
Make sure plugin container is removed on failure
2 parents 1d59c66 + f81172b commit 859e43e

3 files changed

Lines changed: 200 additions & 16 deletions

File tree

‎libcontainerd/client_daemon.go‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,7 @@ func (c *client) Create(ctx context.Context, id string, ociSpec *specs.Spec, run
205205
// TODO(mlaventure): when containerd support lcow, revisit runtime value
206206
containerd.WithRuntime(fmt.Sprintf("io.containerd.runtime.v1.%s", runtime.GOOS), runtimeOptions))
207207
if err != nil {
208-
return err
208+
return wrapError(err)
209209
}
210210

211211
c.Lock()
@@ -286,7 +286,7 @@ func (c *client) Start(ctx context.Context, id, checkpointDir string, withStdin
286286
rio.Cancel()
287287
rio.Close()
288288
}
289-
return -1, err
289+
return -1, wrapError(err)
290290
}
291291

292292
ctr.setTask(t)
@@ -300,7 +300,7 @@ func (c *client) Start(ctx context.Context, id, checkpointDir string, withStdin
300300
Error("failed to delete task after fail start")
301301
}
302302
ctr.setTask(nil)
303-
return -1, err
303+
return -1, wrapError(err)
304304
}
305305

306306
return int(t.Pid()), nil
@@ -344,7 +344,7 @@ func (c *client) Exec(ctx context.Context, containerID, processID string, spec *
344344
})
345345
if err != nil {
346346
close(stdinCloseSync)
347-
return -1, err
347+
return -1, wrapError(err)
348348
}
349349

350350
ctr.addProcess(processID, p)
@@ -355,7 +355,7 @@ func (c *client) Exec(ctx context.Context, containerID, processID string, spec *
355355
if err = p.Start(ctx); err != nil {
356356
p.Delete(context.Background())
357357
ctr.deleteProcess(processID)
358-
return -1, err
358+
return -1, wrapError(err)
359359
}
360360

361361
return int(p.Pid()), nil
@@ -393,7 +393,7 @@ func (c *client) Pause(ctx context.Context, containerID string) error {
393393
return err
394394
}
395395

396-
return p.(containerd.Task).Pause(ctx)
396+
return wrapError(p.(containerd.Task).Pause(ctx))
397397
}
398398

399399
func (c *client) Resume(ctx context.Context, containerID string) error {
@@ -493,7 +493,7 @@ func (c *client) Delete(ctx context.Context, containerID string) error {
493493
}
494494

495495
if err := ctr.ctr.Delete(ctx); err != nil {
496-
return err
496+
return wrapError(err)
497497
}
498498

499499
if os.Getenv("LIBCONTAINERD_NOCLEAN") != "1" {
@@ -523,7 +523,7 @@ func (c *client) Status(ctx context.Context, containerID string) (Status, error)
523523

524524
s, err := t.Status(ctx)
525525
if err != nil {
526-
return StatusUnknown, err
526+
return StatusUnknown, wrapError(err)
527527
}
528528

529529
return Status(s.Status), nil
@@ -537,7 +537,7 @@ func (c *client) CreateCheckpoint(ctx context.Context, containerID, checkpointDi
537537

538538
img, err := p.(containerd.Task).Checkpoint(ctx)
539539
if err != nil {
540-
return err
540+
return wrapError(err)
541541
}
542542
// Whatever happens, delete the checkpoint from containerd
543543
defer func() {

‎plugin/executor/containerd/containerd.go‎

Lines changed: 43 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"io"
66
"path/filepath"
77
"sync"
8+
"time"
89

910
"github.com/containerd/containerd/cio"
1011
"github.com/containerd/containerd/linux/runctypes"
@@ -15,21 +16,34 @@ import (
1516
"github.com/sirupsen/logrus"
1617
)
1718

18-
// PluginNamespace is the name used for the plugins namespace
19-
var PluginNamespace = "plugins.moby"
19+
// pluginNamespace is the name used for the plugins namespace
20+
const pluginNamespace = "plugins.moby"
2021

2122
// ExitHandler represents an object that is called when the exit event is received from containerd
2223
type ExitHandler interface {
2324
HandleExitEvent(id string) error
2425
}
2526

27+
// Client is used by the exector to perform operations.
28+
// TODO(@cpuguy83): This should really just be based off the containerd client interface.
29+
// However right now this whole package is tied to github.com/docker/docker/libcontainerd
30+
type Client interface {
31+
Create(ctx context.Context, containerID string, spec *specs.Spec, runtimeOptions interface{}) error
32+
Restore(ctx context.Context, containerID string, attachStdio libcontainerd.StdioCallback) (alive bool, pid int, err error)
33+
Status(ctx context.Context, containerID string) (libcontainerd.Status, error)
34+
Delete(ctx context.Context, containerID string) error
35+
DeleteTask(ctx context.Context, containerID string) (uint32, time.Time, error)
36+
Start(ctx context.Context, containerID, checkpointDir string, withStdin bool, attachStdio libcontainerd.StdioCallback) (pid int, err error)
37+
SignalProcess(ctx context.Context, containerID, processID string, signal int) error
38+
}
39+
2640
// New creates a new containerd plugin executor
2741
func New(rootDir string, remote libcontainerd.Remote, exitHandler ExitHandler) (*Executor, error) {
2842
e := &Executor{
2943
rootDir: rootDir,
3044
exitHandler: exitHandler,
3145
}
32-
client, err := remote.NewClient(PluginNamespace, e)
46+
client, err := remote.NewClient(pluginNamespace, e)
3347
if err != nil {
3448
return nil, errors.Wrap(err, "error creating containerd exec client")
3549
}
@@ -40,7 +54,7 @@ func New(rootDir string, remote libcontainerd.Remote, exitHandler ExitHandler) (
4054
// Executor is the containerd client implementation of a plugin executor
4155
type Executor struct {
4256
rootDir string
43-
client libcontainerd.Client
57+
client Client
4458
exitHandler ExitHandler
4559
}
4660

@@ -52,10 +66,34 @@ func (e *Executor) Create(id string, spec specs.Spec, stdout, stderr io.WriteClo
5266
ctx := context.Background()
5367
err := e.client.Create(ctx, id, &spec, &opts)
5468
if err != nil {
55-
return err
69+
status, err2 := e.client.Status(ctx, id)
70+
if err2 != nil {
71+
if !errdefs.IsNotFound(err2) {
72+
logrus.WithError(err2).WithField("id", id).Warn("Received an error while attempting to read plugin status")
73+
}
74+
} else {
75+
if status != libcontainerd.StatusRunning && status != libcontainerd.StatusUnknown {
76+
if err2 := e.client.Delete(ctx, id); err2 != nil && !errdefs.IsNotFound(err2) {
77+
logrus.WithError(err2).WithField("plugin", id).Error("Error cleaning up containerd container")
78+
}
79+
err = e.client.Create(ctx, id, &spec, &opts)
80+
}
81+
}
82+
83+
if err != nil {
84+
return errors.Wrap(err, "error creating containerd container")
85+
}
5686
}
5787

5888
_, err = e.client.Start(ctx, id, "", false, attachStreamsFunc(stdout, stderr))
89+
if err != nil {
90+
if _, _, err2 := e.client.DeleteTask(ctx, id); err2 != nil && !errdefs.IsNotFound(err2) {
91+
logrus.WithError(err2).WithField("id", id).Warn("Received an error while attempting to clean up containerd plugin task after failed start")
92+
}
93+
if err2 := e.client.Delete(ctx, id); err2 != nil && !errdefs.IsNotFound(err2) {
94+
logrus.WithError(err2).WithField("id", id).Warn("Received an error while attempting to clean up containerd plugin container after failed start")
95+
}
96+
}
5997
return err
6098
}
6199

@@ -69,13 +107,11 @@ func (e *Executor) Restore(id string, stdout, stderr io.WriteCloser) error {
69107
_, _, err = e.client.DeleteTask(context.Background(), id)
70108
if err != nil && !errdefs.IsNotFound(err) {
71109
logrus.WithError(err).Errorf("failed to delete container plugin %s task from containerd", id)
72-
return err
73110
}
74111

75112
err = e.client.Delete(context.Background(), id)
76113
if err != nil && !errdefs.IsNotFound(err) {
77114
logrus.WithError(err).Errorf("failed to delete container plugin %s from containerd", id)
78-
return err
79115
}
80116
}
81117
return nil

‎plugin/executor/containerd/containerd_test.go‎

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
package containerd
2+
3+
import (
4+
"context"
5+
"io/ioutil"
6+
"os"
7+
"sync"
8+
"testing"
9+
"time"
10+
11+
"github.com/docker/docker/libcontainerd"
12+
"github.com/gotestyourself/gotestyourself/assert"
13+
specs "github.com/opencontainers/runtime-spec/specs-go"
14+
"github.com/pkg/errors"
15+
)
16+
17+
func TestLifeCycle(t *testing.T) {
18+
t.Parallel()
19+
20+
mock := newMockClient()
21+
exec, cleanup := setupTest(t, mock, mock)
22+
defer cleanup()
23+
24+
id := "test-create"
25+
mock.simulateStartError(true, id)
26+
err := exec.Create(id, specs.Spec{}, nil, nil)
27+
assert.Assert(t, err != nil)
28+
mock.simulateStartError(false, id)
29+
30+
err = exec.Create(id, specs.Spec{}, nil, nil)
31+
assert.Assert(t, err)
32+
running, _ := exec.IsRunning(id)
33+
assert.Assert(t, running)
34+
35+
// create with the same ID
36+
err = exec.Create(id, specs.Spec{}, nil, nil)
37+
assert.Assert(t, err != nil)
38+
39+
mock.HandleExitEvent(id) // simulate a plugin that exits
40+
41+
err = exec.Create(id, specs.Spec{}, nil, nil)
42+
assert.Assert(t, err)
43+
}
44+
45+
func setupTest(t *testing.T, client Client, eh ExitHandler) (*Executor, func()) {
46+
rootDir, err := ioutil.TempDir("", "test-daemon")
47+
assert.Assert(t, err)
48+
assert.Assert(t, client != nil)
49+
assert.Assert(t, eh != nil)
50+
51+
return &Executor{
52+
rootDir: rootDir,
53+
client: client,
54+
exitHandler: eh,
55+
}, func() {
56+
assert.Assert(t, os.RemoveAll(rootDir))
57+
}
58+
}
59+
60+
type mockClient struct {
61+
mu sync.Mutex
62+
containers map[string]bool
63+
errorOnStart map[string]bool
64+
}
65+
66+
func newMockClient() *mockClient {
67+
return &mockClient{
68+
containers: make(map[string]bool),
69+
errorOnStart: make(map[string]bool),
70+
}
71+
}
72+
73+
func (c *mockClient) Create(ctx context.Context, id string, _ *specs.Spec, _ interface{}) error {
74+
c.mu.Lock()
75+
defer c.mu.Unlock()
76+
77+
if _, ok := c.containers[id]; ok {
78+
return errors.New("exists")
79+
}
80+
81+
c.containers[id] = false
82+
return nil
83+
}
84+
85+
func (c *mockClient) Restore(ctx context.Context, id string, attachStdio libcontainerd.StdioCallback) (alive bool, pid int, err error) {
86+
return false, 0, nil
87+
}
88+
89+
func (c *mockClient) Status(ctx context.Context, id string) (libcontainerd.Status, error) {
90+
c.mu.Lock()
91+
defer c.mu.Unlock()
92+
93+
running, ok := c.containers[id]
94+
if !ok {
95+
return libcontainerd.StatusUnknown, errors.New("not found")
96+
}
97+
if running {
98+
return libcontainerd.StatusRunning, nil
99+
}
100+
return libcontainerd.StatusStopped, nil
101+
}
102+
103+
func (c *mockClient) Delete(ctx context.Context, id string) error {
104+
c.mu.Lock()
105+
defer c.mu.Unlock()
106+
delete(c.containers, id)
107+
return nil
108+
}
109+
110+
func (c *mockClient) DeleteTask(ctx context.Context, id string) (uint32, time.Time, error) {
111+
return 0, time.Time{}, nil
112+
}
113+
114+
func (c *mockClient) Start(ctx context.Context, id, checkpointDir string, withStdin bool, attachStdio libcontainerd.StdioCallback) (pid int, err error) {
115+
c.mu.Lock()
116+
defer c.mu.Unlock()
117+
118+
if _, ok := c.containers[id]; !ok {
119+
return 0, errors.New("not found")
120+
}
121+
122+
if c.errorOnStart[id] {
123+
return 0, errors.New("some startup error")
124+
}
125+
c.containers[id] = true
126+
return 1, nil
127+
}
128+
129+
func (c *mockClient) SignalProcess(ctx context.Context, containerID, processID string, signal int) error {
130+
return nil
131+
}
132+
133+
func (c *mockClient) simulateStartError(sim bool, id string) {
134+
c.mu.Lock()
135+
defer c.mu.Unlock()
136+
if sim {
137+
c.errorOnStart[id] = sim
138+
return
139+
}
140+
delete(c.errorOnStart, id)
141+
}
142+
143+
func (c *mockClient) HandleExitEvent(id string) error {
144+
c.mu.Lock()
145+
defer c.mu.Unlock()
146+
delete(c.containers, id)
147+
return nil
148+
}

0 commit comments

Comments
 (0)