From 093a69136f037c76d33f07795104ebadf535b25a Mon Sep 17 00:00:00 2001 From: Djordje Lukic Date: Tue, 11 Aug 2020 15:46:29 +0200 Subject: [PATCH] Add --force to rm on ACI If a container is running the user must force the removal of the container. --- aci/backend.go | 44 ++++++++++++++++++++++++++++------- aci/backend_test.go | 2 +- cli/cmd/rm.go | 15 ++++++++++-- cli/main.go | 14 ++++++----- containers/api.go | 7 +++++- example/backend.go | 10 ++++---- local/backend.go | 4 ++-- server/proxy/containers.go | 4 +++- tests/aci-e2e/e2e-aci_test.go | 18 +++++++++++--- 9 files changed, 87 insertions(+), 31 deletions(-) diff --git a/aci/backend.go b/aci/backend.go index 1a28e9550..7de513568 100644 --- a/aci/backend.go +++ b/aci/backend.go @@ -49,11 +49,9 @@ const ( composeContainerTag = "docker-compose-application" composeContainerSeparator = "_" statusUnknown = "Unknown" + statusRunning = "Running" ) -// ErrNoSuchContainer is returned when the mentioned container does not exist -var ErrNoSuchContainer = errors.New("no such container") - // ContextParams options for creating ACI context type ContextParams struct { Description string @@ -280,18 +278,46 @@ func (cs *aciContainerService) Logs(ctx context.Context, containerName string, r return err } -func (cs *aciContainerService) Delete(ctx context.Context, containerID string, _ bool) error { +func (cs *aciContainerService) Delete(ctx context.Context, containerID string, request containers.DeleteRequest) error { groupName, containerName := getGroupAndContainerName(containerID) if groupName != containerID { msg := "cannot delete service %q from compose application %q, you can delete the entire compose app with docker compose down --project-name %s" return errors.New(fmt.Sprintf(msg, containerName, groupName, groupName)) } - cg, err := deleteACIContainerGroup(ctx, cs.ctx, groupName) + + containerGroupsClient, err := getContainerGroupsClient(cs.ctx.SubscriptionID) if err != nil { return err } + + if !request.Force { + cg, err := containerGroupsClient.Get(ctx, cs.ctx.ResourceGroup, groupName) + if err != nil { + if cg.StatusCode == http.StatusNotFound { + return errdefs.ErrNotFound + } + return err + } + + for _, container := range *cg.Containers { + status := statusUnknown + if container.InstanceView != nil && container.InstanceView.CurrentState != nil { + status = *container.InstanceView.CurrentState.State + } + + if status == statusRunning { + return errdefs.ErrForbidden + } + } + } + + cg, err := deleteACIContainerGroup(ctx, cs.ctx, groupName) + // Delete returns `StatusNoContent` if the group is not found if cg.StatusCode == http.StatusNoContent { - return ErrNoSuchContainer + return errdefs.ErrNotFound + } + if err != nil { + return err } return err @@ -305,7 +331,7 @@ func (cs *aciContainerService) Inspect(ctx context.Context, containerID string) return containers.Container{}, err } if cg.StatusCode == http.StatusNoContent { - return containers.Container{}, ErrNoSuchContainer + return containers.Container{}, errdefs.ErrNotFound } var cc containerinstance.Container @@ -318,7 +344,7 @@ func (cs *aciContainerService) Inspect(ctx context.Context, containerID string) } } if !found { - return containers.Container{}, ErrNoSuchContainer + return containers.Container{}, errdefs.ErrNotFound } return convert.ContainerGroupToContainer(containerID, cg, cc) @@ -362,7 +388,7 @@ func (cs *aciComposeService) Down(ctx context.Context, opts cli.ProjectOptions) return err } if cg.StatusCode == http.StatusNoContent { - return ErrNoSuchContainer + return errdefs.ErrNotFound } return err diff --git a/aci/backend_test.go b/aci/backend_test.go index e6b72765c..9f109678e 100644 --- a/aci/backend_test.go +++ b/aci/backend_test.go @@ -41,7 +41,7 @@ func TestGetContainerName(t *testing.T) { func TestErrorMessageDeletingContainerFromComposeApplication(t *testing.T) { service := aciContainerService{} - err := service.Delete(context.TODO(), "compose-app_service1", false) + err := service.Delete(context.TODO(), "compose-app_service1", containers.DeleteRequest{Force: false}) assert.Error(t, err, "cannot delete service \"service1\" from compose application \"compose-app\", you can delete the entire compose app with docker compose down --project-name compose-app") } diff --git a/cli/cmd/rm.go b/cli/cmd/rm.go index ec8b91c81..06ad878a4 100644 --- a/cli/cmd/rm.go +++ b/cli/cmd/rm.go @@ -24,6 +24,8 @@ import ( "github.com/spf13/cobra" "github.com/docker/api/client" + "github.com/docker/api/containers" + "github.com/docker/api/errdefs" "github.com/docker/api/multierror" ) @@ -57,11 +59,20 @@ func runRm(ctx context.Context, args []string, opts rmOpts) error { var errs *multierror.Error for _, id := range args { - err := c.ContainerService().Delete(ctx, id, opts.force) + err := c.ContainerService().Delete(ctx, id, containers.DeleteRequest{ + Force: opts.force, + }) if err != nil { - errs = multierror.Append(errs, err) + if errdefs.IsForbiddenError(err) { + errs = multierror.Append(errs, fmt.Errorf("you cannot remove a running container %s. Stop the container before attempting removal or force remove", id)) + } else if errdefs.IsNotFoundError(err) { + errs = multierror.Append(errs, fmt.Errorf("container %s not found", id)) + } else { + errs = multierror.Append(errs, err) + } continue } + fmt.Println(id) } diff --git a/cli/main.go b/cli/main.go index 9ef5fd258..24e5f70cc 100644 --- a/cli/main.go +++ b/cli/main.go @@ -179,10 +179,11 @@ func main() { ctx = store.WithContextStore(ctx, s) if err = root.ExecuteContext(ctx); err != nil { - //if user canceled request, simply exit without any error message + // if user canceled request, simply exit without any error message if errors.Is(ctx.Err(), context.Canceled) { os.Exit(130) } + // Context should always be handled by new CLI requiredCmd, _, _ := root.Find(os.Args[1:]) if requiredCmd != nil && isOwnCommand(requiredCmd) { @@ -191,6 +192,7 @@ func main() { mobycli.ExecIfDefaultCtxType(ctx) checkIfUnknownCommandExistInDefaultContext(err, currentContext) + exit(err) } } @@ -203,6 +205,11 @@ func exit(err error) { fatal(err) } +func fatal(err error) { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) +} + func checkIfUnknownCommandExistInDefaultContext(err error, currentContext string) { submatch := unknownCommandRegexp.FindSubmatch([]byte(err.Error())) if len(submatch) == 2 { @@ -241,8 +248,3 @@ func determineCurrentContext(flag string, configDir string) string { } return res } - -func fatal(err error) { - fmt.Fprintln(os.Stderr, err) - os.Exit(1) -} diff --git a/containers/api.go b/containers/api.go index b2a77bf74..eeb01409c 100644 --- a/containers/api.go +++ b/containers/api.go @@ -105,6 +105,11 @@ type LogsRequest struct { Writer io.Writer } +// DeleteRequest contains configuration about a delete request +type DeleteRequest struct { + Force bool +} + // Service interacts with the underlying container backend type Service interface { // List returns all the containers @@ -118,7 +123,7 @@ type Service interface { // Logs returns all the logs of a container Logs(ctx context.Context, containerName string, request LogsRequest) error // Delete removes containers - Delete(ctx context.Context, id string, force bool) error + Delete(ctx context.Context, containerID string, request DeleteRequest) error // Inspect get a specific container Inspect(ctx context.Context, id string) (Container, error) } diff --git a/example/backend.go b/example/backend.go index cf0240404..0102d76c6 100644 --- a/example/backend.go +++ b/example/backend.go @@ -24,15 +24,13 @@ import ( "fmt" "github.com/compose-spec/compose-go/cli" - - "github.com/docker/api/context/cloud" - "github.com/docker/api/errdefs" - ecstypes "github.com/docker/ecs-plugin/pkg/compose" "github.com/docker/api/backend" "github.com/docker/api/compose" "github.com/docker/api/containers" + "github.com/docker/api/context/cloud" + "github.com/docker/api/errdefs" ) type apiService struct { @@ -108,8 +106,8 @@ func (cs *containerService) Logs(ctx context.Context, containerName string, requ return nil } -func (cs *containerService) Delete(ctx context.Context, id string, force bool) error { - fmt.Printf("Deleting container %q with force = %t\n", id, force) +func (cs *containerService) Delete(ctx context.Context, id string, request containers.DeleteRequest) error { + fmt.Printf("Deleting container %q with force = %t\n", id, request.Force) return nil } diff --git a/local/backend.go b/local/backend.go index dd41111fa..0a807d133 100644 --- a/local/backend.go +++ b/local/backend.go @@ -249,9 +249,9 @@ func (ms *local) Logs(ctx context.Context, containerName string, request contain return err } -func (ms *local) Delete(ctx context.Context, containerID string, force bool) error { +func (ms *local) Delete(ctx context.Context, containerID string, request containers.DeleteRequest) error { err := ms.apiClient.ContainerRemove(ctx, containerID, types.ContainerRemoveOptions{ - Force: force, + Force: request.Force, }) if client.IsErrNotFound(err) { return errors.Wrapf(errdefs.ErrNotFound, "container %q", containerID) diff --git a/server/proxy/containers.go b/server/proxy/containers.go index 66c6108b5..84ed7c8ac 100644 --- a/server/proxy/containers.go +++ b/server/proxy/containers.go @@ -77,7 +77,9 @@ func (p *proxy) Inspect(ctx context.Context, request *containersv1.InspectReques } func (p *proxy) Delete(ctx context.Context, request *containersv1.DeleteRequest) (*containersv1.DeleteResponse, error) { - return &containersv1.DeleteResponse{}, Client(ctx).ContainerService().Delete(ctx, request.Id, request.Force) + return &containersv1.DeleteResponse{}, Client(ctx).ContainerService().Delete(ctx, request.Id, containers.DeleteRequest{ + Force: request.Force, + }) } func (p *proxy) Exec(ctx context.Context, request *containersv1.ExecRequest) (*containersv1.ExecResponse, error) { diff --git a/tests/aci-e2e/e2e-aci_test.go b/tests/aci-e2e/e2e-aci_test.go index 19831228a..ccab641df 100644 --- a/tests/aci-e2e/e2e-aci_test.go +++ b/tests/aci-e2e/e2e-aci_test.go @@ -265,9 +265,21 @@ func TestContainerRun(t *testing.T) { } }) - t.Run("rm", func(t *testing.T) { + t.Run("rm a running container", func(t *testing.T) { res := c.RunDockerCmd("rm", container) - res.Assert(t, icmd.Expected{Out: container}) + res.Assert(t, icmd.Expected{ + Err: fmt.Sprintf("Error: You cannot remove a running container %s. Stop the container before attempting removal or force remove\n", container), + ExitCode: 1, + }) + }) + + t.Run("force rm", func(t *testing.T) { + res := c.RunDockerCmd("rm", "-f", container) + res.Assert(t, icmd.Expected{ + Out: container, + ExitCode: 0, + }) + checkStopped := func(t poll.LogT) poll.Result { res := c.RunDockerCmd("inspect", container) if res.ExitCode == 1 { @@ -349,7 +361,7 @@ func TestContainerRunAttached(t *testing.T) { }) t.Run("rm attached", func(t *testing.T) { - res := c.RunDockerCmd("rm", container) + res := c.RunDockerCmd("rm", "-f", container) res.Assert(t, icmd.Expected{Out: container}) }) }