feature: add "volume" flag when remove container

rudyfly · rudyfly · commit 954cd17c4d15 · 2018-04-28T14:40:03.000+08:00
Add "volume" flag when remove container, it is used to remove these
volumes that is created by container.

Signed-off-by: Rudy Zhang &lt;rudyflyzhang@gmail.com&gt;
diff --git a/apis/server/container_bridge.go b/apis/server/container_bridge.go
@@ -23,11 +23,11 @@ import (
 func (s *Server) removeContainers(ctx context.Context, rw http.ResponseWriter, req *http.Request) error {
 	name := mux.Vars(req)["name"]
 
-	option := &mgr.ContainerRemoveOption{
-		Force: httputils.BoolValue(req, "force"),
-		// TODO Volume and Link will be supported in the future.
-		Volume: httputils.BoolValue(req, "v"),
-		Link:   httputils.BoolValue(req, "link"),
+	option := &types.ContainerRemoveOptions{
+		Force:   httputils.BoolValue(req, "force"),
+		Volumes: httputils.BoolValue(req, "v"),
+		// TODO: Link will be supported in the future.
+		Link: httputils.BoolValue(req, "link"),
 	}
 
 	if err := s.ContainerMgr.Remove(ctx, name, option); err != nil {
diff --git a/apis/server/volume_bridge.go b/apis/server/volume_bridge.go
@@ -3,7 +3,6 @@ package server
 import (
 	"context"
 	"encoding/json"
-	"fmt"
 	"net/http"
 
 	"github.com/alibaba/pouch/apis/types"
@@ -100,16 +99,6 @@ func (s *Server) getVolume(ctx context.Context, rw http.ResponseWriter, req *htt
 func (s *Server) removeVolume(ctx context.Context, rw http.ResponseWriter, req *http.Request) error {
 	name := mux.Vars(req)["name"]
 
-	volume, err := s.VolumeMgr.Get(ctx, name)
-	if err != nil {
-		return err
-	}
-
-	ref := volume.Option("ref")
-	if ref != "" {
-		return fmt.Errorf("failed to remove volume: %s, using by: %s", name, ref)
-	}
-
 	if err := s.VolumeMgr.Remove(ctx, name); err != nil {
 		return err
 	}
diff --git a/apis/swagger.yml b/apis/swagger.yml
@@ -3376,6 +3376,17 @@ definitions:
       Width:
         type: "integer"
 
+  ContainerRemoveOptions:
+    description: "options of remove container"
+    type: "object"
+    properties:
+      Force:
+        type: "boolean"
+      Volumes:
+        type: "boolean"
+      Link:
+        type: "boolean"
+
 parameters:
   id:
     name: id
diff --git a/apis/types/container_remove_options.go b/apis/types/container_remove_options.go
diff --git a/cli/rm.go b/cli/rm.go
@@ -4,6 +4,8 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/alibaba/pouch/apis/types"
+
 	"github.com/spf13/cobra"
 )
 
@@ -18,7 +20,8 @@ be released.
 // RmCommand is used to implement 'rm' command.
 type RmCommand struct {
 	baseCommand
-	force bool
+	force        bool
+	removeVolume bool
 }
 
 // Init initializes RmCommand command.
@@ -39,16 +42,24 @@ func (r *RmCommand) Init(c *Cli) {
 
 // addFlags adds flags for specific command.
 func (r *RmCommand) addFlags() {
-	r.cmd.Flags().BoolVarP(&r.force, "force", "f", false, "if the container is running, force to remove it")
+	flagSet := r.cmd.Flags()
+
+	flagSet.BoolVarP(&r.force, "force", "f", false, "if the container is running, force to remove it")
+	flagSet.BoolVarP(&r.removeVolume, "volumes", "v", false, "remove container's volumes that create by the container")
 }
 
 // runRm is the entry of RmCommand command.
 func (r *RmCommand) runRm(args []string) error {
 	ctx := context.Background()
 	apiClient := r.cli.Client()
 
+	options := &types.ContainerRemoveOptions{
+		Force:   r.force,
+		Volumes: r.removeVolume,
+	}
+
 	for _, name := range args {
-		if err := apiClient.ContainerRemove(ctx, name, r.force); err != nil {
+		if err := apiClient.ContainerRemove(ctx, name, options); err != nil {
 			return fmt.Errorf("failed to remove container: %v", err)
 		}
 		fmt.Printf("%s\n", name)
diff --git a/cli/run.go b/cli/run.go
@@ -7,6 +7,7 @@ import (
 	"os"
 	"strings"
 
+	"github.com/alibaba/pouch/apis/types"
 	"github.com/spf13/cobra"
 )
 
@@ -145,7 +146,7 @@ func (rc *RunCommand) runRun(args []string) error {
 	}
 
 	if rc.rm {
-		if err := apiClient.ContainerRemove(ctx, containerName, true); err != nil {
+		if err := apiClient.ContainerRemove(ctx, containerName, &types.ContainerRemoveOptions{Force: true}); err != nil {
 			return fmt.Errorf("failed to remove container %s: %v", containerName, err)
 		}
 	}
diff --git a/client/container_remove.go b/client/container_remove.go
@@ -3,14 +3,19 @@ package client
 import (
 	"context"
 	"net/url"
+
+	"github.com/alibaba/pouch/apis/types"
 )
 
 // ContainerRemove removes a container.
-func (client *APIClient) ContainerRemove(ctx context.Context, name string, force bool) error {
+func (client *APIClient) ContainerRemove(ctx context.Context, name string, options *types.ContainerRemoveOptions) error {
 	q := url.Values{}
-	if force {
+	if options.Force {
 		q.Set("force", "true")
 	}
+	if options.Volumes {
+		q.Set("v", "true")
+	}
 
 	resp, err := client.delete(ctx, "/containers/"+name, q, nil)
 	if err != nil {
diff --git a/client/interface.go b/client/interface.go
@@ -23,7 +23,7 @@ type ContainerAPIClient interface {
 	ContainerCreate(ctx context.Context, config types.ContainerConfig, hostConfig *types.HostConfig, networkConfig *types.NetworkingConfig, containerName string) (*types.ContainerCreateResp, error)
 	ContainerStart(ctx context.Context, name, detachKeys string) error
 	ContainerStop(ctx context.Context, name, timeout string) error
-	ContainerRemove(ctx context.Context, name string, force bool) error
+	ContainerRemove(ctx context.Context, name string, options *types.ContainerRemoveOptions) error
 	ContainerList(ctx context.Context, all bool) ([]*types.Container, error)
 	ContainerAttach(ctx context.Context, name string, stdin bool) (net.Conn, *bufio.Reader, error)
 	ContainerCreateExec(ctx context.Context, name string, config *types.ExecCreateConfig) (*types.ExecCreateResp, error)
diff --git a/daemon/mgr/container.go b/daemon/mgr/container.go
@@ -77,7 +77,7 @@ type ContainerMgr interface {
 	GetExecConfig(ctx context.Context, execid string) (*ContainerExecConfig, error)
 
 	// Remove removes a container, it may be running or stopped and so on.
-	Remove(ctx context.Context, name string, option *ContainerRemoveOption) error
+	Remove(ctx context.Context, name string, option *types.ContainerRemoveOptions) error
 
 	// Rename renames a container.
 	Rename(ctx context.Context, oldName string, newName string) error
@@ -209,20 +209,20 @@ func (mgr *ContainerManager) Restore(ctx context.Context) error {
 }
 
 // Remove removes a container, it may be running or stopped and so on.
-func (mgr *ContainerManager) Remove(ctx context.Context, name string, option *ContainerRemoveOption) error {
+func (mgr *ContainerManager) Remove(ctx context.Context, name string, options *types.ContainerRemoveOptions) error {
 	c, err := mgr.container(name)
 	if err != nil {
 		return err
 	}
 	c.Lock()
 	defer c.Unlock()
 
-	if !c.IsStopped() && !c.IsExited() && !c.IsCreated() && !option.Force {
+	if !c.IsStopped() && !c.IsExited() && !c.IsCreated() && !options.Force {
 		return fmt.Errorf("container: %s is not stopped, can't remove it without flag force", c.ID())
 	}
 
 	// if the container is running, force to stop it.
-	if c.IsRunning() && option.Force {
+	if c.IsRunning() && options.Force {
 		msg, err := mgr.Client.DestroyContainer(ctx, c.ID(), c.StopTimeout())
 		if err != nil && !errtypes.IsNotfound(err) {
 			return errors.Wrapf(err, "failed to destroy container: %s", c.ID())
@@ -232,7 +232,7 @@ func (mgr *ContainerManager) Remove(ctx context.Context, name string, option *Co
 		}
 	}
 
-	if err := mgr.detachVolumes(ctx, c.meta); err != nil {
+	if err := mgr.detachVolumes(ctx, c.meta, options.Volumes); err != nil {
 		logrus.Errorf("failed to detach volume: %v", err)
 	}
 
@@ -1854,7 +1854,7 @@ func (mgr *ContainerManager) generateMountPoints(ctx context.Context, meta *Cont
 
 	defer func() {
 		if err != nil {
-			if err := mgr.detachVolumes(ctx, meta); err != nil {
+			if err := mgr.detachVolumes(ctx, meta, false); err != nil {
 				logrus.Errorf("failed to detach volume, err: %v", err)
 			}
 		}
@@ -2235,7 +2235,7 @@ func (mgr *ContainerManager) setMountPointDiskQuota(ctx context.Context, c *Cont
 	return nil
 }
 
-func (mgr *ContainerManager) detachVolumes(ctx context.Context, c *ContainerMeta) error {
+func (mgr *ContainerManager) detachVolumes(ctx context.Context, c *ContainerMeta, remove bool) error {
 	for _, mount := range c.Mounts {
 		name := mount.Name
 		if name == "" {
@@ -2271,6 +2271,12 @@ func (mgr *ContainerManager) detachVolumes(ctx context.Context, c *ContainerMeta
 		}
 
 		mgr.VolumeMgr.Detach(ctx, name, option)
+
+		if remove {
+			if err := mgr.VolumeMgr.Remove(ctx, name); err != nil && !errtypes.IsUsing(err) {
+				logrus.Warnf("failed to remove volume: %s when remove container", name)
+			}
+		}
 	}
 
 	return nil
diff --git a/daemon/mgr/container_types.go b/daemon/mgr/container_types.go
@@ -75,13 +75,6 @@ type AttachConfig struct {
 	CriLogFile *os.File
 }
 
-// ContainerRemoveOption wraps the container remove interface params.
-type ContainerRemoveOption struct {
-	Force  bool
-	Volume bool
-	Link   bool
-}
-
 // ContainerListOption wraps the container list interface params.
 type ContainerListOption struct {
 	All bool
diff --git a/daemon/mgr/cri.go b/daemon/mgr/cri.go
@@ -309,14 +309,14 @@ func (c *CriManager) RemovePodSandbox(ctx context.Context, r *runtime.RemovePodS
 
 	// Remove all containers in the sandbox.
 	for _, container := range containers {
-		err = c.ContainerMgr.Remove(ctx, container.ID, &ContainerRemoveOption{Volume: true, Force: true})
+		err = c.ContainerMgr.Remove(ctx, container.ID, &apitypes.ContainerRemoveOptions{Volumes: true, Force: true})
 		if err != nil {
 			// TODO: log an error message or break?
 		}
 	}
 
 	// Remove the sandbox container.
-	err = c.ContainerMgr.Remove(ctx, podSandboxID, &ContainerRemoveOption{Volume: true, Force: true})
+	err = c.ContainerMgr.Remove(ctx, podSandboxID, &apitypes.ContainerRemoveOptions{Volumes: true, Force: true})
 	if err != nil {
 		return nil, fmt.Errorf("failed to remove sandbox %q: %v", podSandboxID, err)
 	}
@@ -528,7 +528,7 @@ func (c *CriManager) StopContainer(ctx context.Context, r *runtime.StopContainer
 func (c *CriManager) RemoveContainer(ctx context.Context, r *runtime.RemoveContainerRequest) (*runtime.RemoveContainerResponse, error) {
 	containerID := r.GetContainerId()
 
-	err := c.ContainerMgr.Remove(ctx, containerID, &ContainerRemoveOption{Volume: true, Force: true})
+	err := c.ContainerMgr.Remove(ctx, containerID, &apitypes.ContainerRemoveOptions{Volumes: true, Force: true})
 	if err != nil {
 		return nil, fmt.Errorf("failed to remove container %q: %v", containerID, err)
 	}
diff --git a/daemon/mgr/volume.go b/daemon/mgr/volume.go
@@ -93,7 +93,16 @@ func (vm *VolumeManager) Create(ctx context.Context, name, driver string, option
 
 // Remove is used to delete an existing volume.
 func (vm *VolumeManager) Remove(ctx context.Context, name string) error {
-	// TODO: check container use.
+	vol, err := vm.Get(ctx, name)
+	if err != nil {
+		return errors.Wrapf(err, "failed to get volume: %s", name)
+	}
+
+	ref := vol.Option("ref")
+	if ref != "" {
+		return errors.Wrapf(errtypes.ErrUsingbyContainers, "failed to remove volume: %s", name)
+	}
+
 	id := types.VolumeID{
 		Name: name,
 	}
@@ -102,6 +111,7 @@ func (vm *VolumeManager) Remove(ctx context.Context, name string) error {
 			return errors.Wrap(errtypes.ErrNotfound, err.Error())
 		}
 	}
+
 	return nil
 }
 
diff --git a/pkg/errtypes/errors.go b/pkg/errtypes/errors.go
@@ -28,6 +28,9 @@ var (
 
 	// ErrNotImplemented represents that the function is not implemented.
 	ErrNotImplemented = errorType{codeNotImplemented, "not implemented"}
+
+	// Useing by Containers
+	ErrUsingbyContainers = errorType{codeUsingByContainers, "using by containers"}
 )
 
 const (
@@ -39,6 +42,7 @@ const (
 	codeTimeout
 	codeLockfailed
 	codeNotImplemented
+	codeUsingByContainers
 )
 
 type errorType struct {
@@ -70,6 +74,11 @@ func IsTimeout(err error) bool {
 	return checkError(err, codeTimeout)
 }
 
+// IsUsing checks the error is using by containers or not.
+func IsUsing(err error) bool {
+	return checkError(err, codeUsingByContainers)
+}
+
 func checkError(err error, code int) bool {
 	err = causeError(err)
 
diff --git a/test/cli_rm_test.go b/test/cli_rm_test.go
diff --git a/test/environment/cleanup.go b/test/environment/cleanup.go

Original file line number	Diff line number	Diff line change
`@@ -7,6 +7,7 @@ import (`
`7`	`7`	`"os"`
`8`	`8`	`"strings"`
`9`	`9`
	`10`	`+ "github.com/alibaba/pouch/apis/types"`
`10`	`11`	`"github.com/spf13/cobra"`
`11`	`12`	`)`
`12`	`13`
`@@ -145,7 +146,7 @@ func (rc *RunCommand) runRun(args []string) error {`
`145`	`146`	`}`
`146`	`147`
`147`	`148`	`if rc.rm {`
`148`		`- if err := apiClient.ContainerRemove(ctx, containerName, true); err != nil {`
	`149`	`+ if err := apiClient.ContainerRemove(ctx, containerName, &types.ContainerRemoveOptions{Force: true}); err != nil {`
`149`	`150`	`return fmt.Errorf("failed to remove container %s: %v", containerName, err)`
`150`	`151`	`}`
`151`	`152`	`}`
Original file line number	Diff line number	Diff line change
`@@ -77,7 +77,7 @@ type ContainerMgr interface {`
`77`	`77`	`GetExecConfig(ctx context.Context, execid string) (*ContainerExecConfig, error)`
`78`	`78`
`79`	`79`	`// Remove removes a container, it may be running or stopped and so on.`
`80`		`- Remove(ctx context.Context, name string, option *ContainerRemoveOption) error`
	`80`	`+ Remove(ctx context.Context, name string, option *types.ContainerRemoveOptions) error`
`81`	`81`
`82`	`82`	`// Rename renames a container.`
`83`	`83`	`Rename(ctx context.Context, oldName string, newName string) error`
`@@ -209,20 +209,20 @@ func (mgr *ContainerManager) Restore(ctx context.Context) error {`
`209`	`209`	`}`
`210`	`210`
`211`	`211`	`// Remove removes a container, it may be running or stopped and so on.`
`212`		`-func (mgr ContainerManager) Remove(ctx context.Context, name string, option ContainerRemoveOption) error {`
	`212`	`+func (mgr ContainerManager) Remove(ctx context.Context, name string, options types.ContainerRemoveOptions) error {`
`213`	`213`	`c, err := mgr.container(name)`
`214`	`214`	`if err != nil {`
`215`	`215`	`return err`
`216`	`216`	`}`
`217`	`217`	`c.Lock()`
`218`	`218`	`defer c.Unlock()`
`219`	`219`
`220`		`- if !c.IsStopped() && !c.IsExited() && !c.IsCreated() && !option.Force {`
	`220`	`+ if !c.IsStopped() && !c.IsExited() && !c.IsCreated() && !options.Force {`
`221`	`221`	`return fmt.Errorf("container: %s is not stopped, can't remove it without flag force", c.ID())`
`222`	`222`	`}`
`223`	`223`
`224`	`224`	`// if the container is running, force to stop it.`
`225`		`- if c.IsRunning() && option.Force {`
	`225`	`+ if c.IsRunning() && options.Force {`
`226`	`226`	`msg, err := mgr.Client.DestroyContainer(ctx, c.ID(), c.StopTimeout())`
`227`	`227`	`if err != nil && !errtypes.IsNotfound(err) {`
`228`	`228`	`return errors.Wrapf(err, "failed to destroy container: %s", c.ID())`
`@@ -232,7 +232,7 @@ func (mgr ContainerManager) Remove(ctx context.Context, name string, option Co`
`232`	`232`	`}`
`233`	`233`	`}`
`234`	`234`
`235`		`- if err := mgr.detachVolumes(ctx, c.meta); err != nil {`
	`235`	`+ if err := mgr.detachVolumes(ctx, c.meta, options.Volumes); err != nil {`
`236`	`236`	`logrus.Errorf("failed to detach volume: %v", err)`
`237`	`237`	`}`
`238`	`238`
`@@ -1854,7 +1854,7 @@ func (mgr ContainerManager) generateMountPoints(ctx context.Context, meta Cont`
`1854`	`1854`
`1855`	`1855`	`defer func() {`
`1856`	`1856`	`if err != nil {`
`1857`		`- if err := mgr.detachVolumes(ctx, meta); err != nil {`
	`1857`	`+ if err := mgr.detachVolumes(ctx, meta, false); err != nil {`
`1858`	`1858`	`logrus.Errorf("failed to detach volume, err: %v", err)`
`1859`	`1859`	`}`
`1860`	`1860`	`}`
`@@ -2235,7 +2235,7 @@ func (mgr ContainerManager) setMountPointDiskQuota(ctx context.Context, c Cont`
`2235`	`2235`	`return nil`
`2236`	`2236`	`}`
`2237`	`2237`
`2238`		`-func (mgr ContainerManager) detachVolumes(ctx context.Context, c ContainerMeta) error {`
	`2238`	`+func (mgr ContainerManager) detachVolumes(ctx context.Context, c ContainerMeta, remove bool) error {`
`2239`	`2239`	`for _, mount := range c.Mounts {`
`2240`	`2240`	`name := mount.Name`
`2241`	`2241`	`if name == "" {`
`@@ -2271,6 +2271,12 @@ func (mgr ContainerManager) detachVolumes(ctx context.Context, c ContainerMeta`
`2271`	`2271`	`}`
`2272`	`2272`
`2273`	`2273`	`mgr.VolumeMgr.Detach(ctx, name, option)`
	`2274`	`+`
	`2275`	`+ if remove {`
	`2276`	`+ if err := mgr.VolumeMgr.Remove(ctx, name); err != nil && !errtypes.IsUsing(err) {`
	`2277`	`+ logrus.Warnf("failed to remove volume: %s when remove container", name)`
	`2278`	`+ }`
	`2279`	`+ }`
`2274`	`2280`	`}`
`2275`	`2281`
`2276`	`2282`	`return nil`