diff --git a/checkpoint_test.go b/checkpoint_test.go index 7afcbb679..2695caa42 100644 --- a/checkpoint_test.go +++ b/checkpoint_test.go @@ -163,11 +163,11 @@ func TestCheckpointRestoreNewContainer(t *testing.T) { <-statusC - if err := container.Delete(ctx); err != nil { + if _, err := task.Delete(ctx); err != nil { t.Error(err) return } - if _, err := task.Delete(ctx); err != nil { + if err := container.Delete(ctx); err != nil { t.Error(err) return } diff --git a/container.go b/container.go index 70efc8c0f..935796255 100644 --- a/container.go +++ b/container.go @@ -17,8 +17,9 @@ import ( ) var ( - ErrNoImage = errors.New("container does not have an image") - ErrNoRunningTask = errors.New("no running task") + ErrNoImage = errors.New("container does not have an image") + ErrNoRunningTask = errors.New("no running task") + ErrDeleteRunningTask = errors.New("cannot delete container with running task") ) type Container interface { @@ -45,7 +46,6 @@ type container struct { client *Client c containers.Container - task *task } // ID returns the container's unique id @@ -69,6 +69,9 @@ func (c *container) Spec() (*specs.Spec, error) { // Delete deletes an existing container // an error is returned if the container has running tasks func (c *container) Delete(ctx context.Context) (err error) { + if _, err := c.Task(ctx, nil); err == nil { + return ErrDeleteRunningTask + } // TODO: should the client be the one removing resources attached // to the container at the moment before we have GC? if c.c.RootFS != "" { @@ -83,16 +86,7 @@ func (c *container) Delete(ctx context.Context) (err error) { } func (c *container) Task(ctx context.Context, attach IOAttach) (Task, error) { - c.mu.Lock() - defer c.mu.Unlock() - if c.task == nil { - t, err := c.loadTask(ctx, attach) - if err != nil { - return nil, err - } - c.task = t.(*task) - } - return c.task, nil + return c.loadTask(ctx, attach) } // Image returns the image that the container is based on @@ -163,7 +157,6 @@ func (c *container) NewTask(ctx context.Context, ioCreate IOCreation, opts ...Ne t.pid = response.Pid close(t.pidSync) } - c.task = t return t, nil } @@ -206,7 +199,6 @@ func (c *container) loadTask(ctx context.Context, ioAttach IOAttach) (Task, erro pid: response.Task.Pid, pidSync: ps, } - c.task = t return t, nil } diff --git a/container_test.go b/container_test.go index 499008182..09d423b96 100644 --- a/container_test.go +++ b/container_test.go @@ -597,3 +597,71 @@ func TestContainerAttach(t *testing.T) { t.Errorf("expected output %q but received %q", expected, output) } } + +func TestDeleteRunningContainer(t *testing.T) { + if testing.Short() { + t.Skip() + } + client, err := New(address) + if err != nil { + t.Fatal(err) + } + defer client.Close() + + var ( + ctx, cancel = testContext() + id = t.Name() + ) + defer cancel() + + image, err := client.GetImage(ctx, testImage) + if err != nil { + t.Error(err) + return + } + spec, err := GenerateSpec(WithImageConfig(ctx, image), WithProcessArgs("sleep", "100")) + if err != nil { + t.Error(err) + return + } + container, err := client.NewContainer(ctx, id, WithSpec(spec), WithImage(image), WithNewRootFS(id, image)) + if err != nil { + t.Error(err) + return + } + defer container.Delete(ctx) + + task, err := container.NewTask(ctx, empty()) + if err != nil { + t.Error(err) + return + } + defer task.Delete(ctx) + + statusC := make(chan uint32, 1) + go func() { + status, err := task.Wait(ctx) + if err != nil { + t.Error(err) + } + statusC <- status + }() + + if err := task.Start(ctx); err != nil { + t.Error(err) + return + } + + err = container.Delete(ctx) + if err == nil { + t.Error("delete did not error with running task") + } + if err != ErrDeleteRunningTask { + t.Errorf("expected error %q but received %q", ErrDeleteRunningTask, err) + } + if err := task.Kill(ctx, syscall.SIGKILL); err != nil { + t.Error(err) + return + } + <-statusC +}