From 659d7b190f967c2e7c95ab97929c3550e32fed6f Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Tue, 9 Jan 2024 13:32:31 -0500 Subject: [PATCH] libcontainerd: create unstarted tasks Split task creation and start into two separate method calls in the libcontainerd API. Clients now have the opportunity to inspect the freshly-created task and customize its runtime environment before starting execution of the user-specified binary. Signed-off-by: Cory Snider --- daemon/start.go | 25 ++++++++++++++++++++---- libcontainerd/local/local_windows.go | 7 ++++++- libcontainerd/remote/client.go | 19 +++++++----------- libcontainerd/types/types.go | 4 +++- plugin/executor/containerd/containerd.go | 6 +++++- 5 files changed, 42 insertions(+), 19 deletions(-) diff --git a/daemon/start.go b/daemon/start.go index 948f458c0e..24e72e2248 100644 --- a/daemon/start.go +++ b/daemon/start.go @@ -11,6 +11,7 @@ import ( "github.com/docker/docker/api/types/events" "github.com/docker/docker/container" "github.com/docker/docker/errdefs" + "github.com/docker/docker/internal/compatcontext" "github.com/docker/docker/libcontainerd" "github.com/pkg/errors" ) @@ -198,16 +199,32 @@ func (daemon *Daemon) containerStart(ctx context.Context, daemonCfg *configStore if err != nil { return setExitCodeFromError(container.SetExitCode, err) } + defer func() { + if retErr != nil { + if err := ctr.Delete(compatcontext.WithoutCancel(ctx)); err != nil { + log.G(ctx).WithError(err).WithField("container", container.ID). + Error("failed to delete failed start container") + } + } + }() // TODO(mlaventure): we need to specify checkpoint options here - tsk, err := ctr.Start(context.TODO(), // Passing ctx to ctr.Start caused integration tests to be stuck in the cleanup phase + tsk, err := ctr.NewTask(context.TODO(), // Passing ctx caused integration tests to be stuck in the cleanup phase checkpointDir, container.StreamConfig.Stdin() != nil || container.Config.Tty, container.InitializeStdio) if err != nil { - if err := ctr.Delete(context.Background()); err != nil { - log.G(ctx).WithError(err).WithField("container", container.ID). - Error("failed to delete failed start container") + return setExitCodeFromError(container.SetExitCode, err) + } + defer func() { + if retErr != nil { + if err := tsk.ForceDelete(compatcontext.WithoutCancel(ctx)); err != nil { + log.G(ctx).WithError(err).WithField("container", container.ID). + Error("failed to delete task after fail start") + } } + }() + + if err := tsk.Start(context.TODO()); err != nil { // passing ctx caused integration tests to be stuck in the cleanup phase return setExitCodeFromError(container.SetExitCode, err) } diff --git a/libcontainerd/local/local_windows.go b/libcontainerd/local/local_windows.go index 08125e131e..82ec0d71af 100644 --- a/libcontainerd/local/local_windows.go +++ b/libcontainerd/local/local_windows.go @@ -387,7 +387,7 @@ func (c *client) extractResourcesFromSpec(spec *specs.Spec, configuration *hcssh } } -func (ctr *container) Start(_ context.Context, _ string, withStdin bool, attachStdio libcontainerdtypes.StdioCallback) (_ libcontainerdtypes.Task, retErr error) { +func (ctr *container) NewTask(_ context.Context, _ string, withStdin bool, attachStdio libcontainerdtypes.StdioCallback) (_ libcontainerdtypes.Task, retErr error) { ctr.mu.Lock() defer ctr.mu.Unlock() @@ -514,6 +514,11 @@ func (ctr *container) Start(_ context.Context, _ string, withStdin bool, attachS return t, nil } +func (*task) Start(context.Context) error { + // No-op on Windows. + return nil +} + func (ctr *container) Task(context.Context) (libcontainerdtypes.Task, error) { ctr.mu.Lock() defer ctr.mu.Unlock() diff --git a/libcontainerd/remote/client.go b/libcontainerd/remote/client.go index 498f91ecfd..670c2d089d 100644 --- a/libcontainerd/remote/client.go +++ b/libcontainerd/remote/client.go @@ -145,8 +145,8 @@ func (c *client) NewContainer(ctx context.Context, id string, ociSpec *specs.Spe return &created, nil } -// Start create and start a task for the specified containerd id -func (c *container) Start(ctx context.Context, checkpointDir string, withStdin bool, attachStdio libcontainerdtypes.StdioCallback) (libcontainerdtypes.Task, error) { +// NewTask creates a task for the specified containerd id +func (c *container) NewTask(ctx context.Context, checkpointDir string, withStdin bool, attachStdio libcontainerdtypes.StdioCallback) (libcontainerdtypes.Task, error) { var ( checkpoint *types.Descriptor t containerd.Task @@ -236,19 +236,14 @@ func (c *container) Start(ctx context.Context, checkpointDir string, withStdin b // Signal c.createIO that it can call CloseIO stdinCloseSync <- t - if err := t.Start(ctx); err != nil { - // Only Stopped tasks can be deleted. Created tasks have to be - // killed first, to transition them to Stopped. - if _, err := t.Delete(ctx, containerd.WithProcessKill); err != nil { - c.client.logger.WithError(err).WithField("container", c.c8dCtr.ID()). - Error("failed to delete task after fail start") - } - return nil, wrapError(err) - } - return c.newTask(t), nil } +func (t *task) Start(ctx context.Context) error { + return wrapError(t.Task.Start(ctx)) + +} + // Exec creates exec process. // // The containerd client calls Exec to register the exec config in the shim side. diff --git a/libcontainerd/types/types.go b/libcontainerd/types/types.go index 0b484e23f0..f05eb4fc90 100644 --- a/libcontainerd/types/types.go +++ b/libcontainerd/types/types.go @@ -64,7 +64,7 @@ type Client interface { // Container provides access to a containerd container. type Container interface { - Start(ctx context.Context, checkpointDir string, withStdin bool, attachStdio StdioCallback) (Task, error) + NewTask(ctx context.Context, checkpointDir string, withStdin bool, attachStdio StdioCallback) (Task, error) Task(ctx context.Context) (Task, error) // AttachTask returns the current task for the container and reattaches // to the IO for the running task. If no task exists for the container @@ -79,6 +79,8 @@ type Container interface { // Task provides access to a running containerd container. type Task interface { Process + // Start begins execution of the task + Start(context.Context) error // Pause suspends the execution of the task Pause(context.Context) error // Resume the execution of the task diff --git a/plugin/executor/containerd/containerd.go b/plugin/executor/containerd/containerd.go index 7ea01b7985..4c7c03bee0 100644 --- a/plugin/executor/containerd/containerd.go +++ b/plugin/executor/containerd/containerd.go @@ -81,11 +81,15 @@ func (e *Executor) Create(id string, spec specs.Spec, stdout, stderr io.WriteClo } p := c8dPlugin{log: log.G(ctx).WithField("plugin", id), ctr: ctr} - p.tsk, err = ctr.Start(ctx, "", false, attachStreamsFunc(stdout, stderr)) + p.tsk, err = ctr.NewTask(ctx, "", false, attachStreamsFunc(stdout, stderr)) if err != nil { p.deleteTaskAndContainer(ctx) return err } + if err := p.tsk.Start(ctx); err != nil { + p.deleteTaskAndContainer(ctx) + return err + } e.mu.Lock() defer e.mu.Unlock() e.plugins[id] = &p