Merge pull request #7891 from rhatdan/rm

This PR allows users to remove external containers directly

10 files changed, 40 insertions, 35 deletions

diff --git a/cmd/podman/containers/rm.go b/cmd/podman/containers/rm.go
index f8f12234d..a7739b3ba 100644
--- a/cmd/podman/containers/rm.go
+++ b/cmd/podman/containers/rm.go
@@ -57,13 +57,12 @@ func rmFlags(flags *pflag.FlagSet) {
 	flags.BoolVarP(&rmOptions.All, "all", "a", false, "Remove all containers")
 	flags.BoolVarP(&rmOptions.Ignore, "ignore", "i", false, "Ignore errors when a specified container is missing")
 	flags.BoolVarP(&rmOptions.Force, "force", "f", false, "Force removal of a running or unusable container.  The default is false")
-	flags.BoolVar(&rmOptions.Storage, "storage", false, "Remove container from storage library")
 	flags.BoolVarP(&rmOptions.Volumes, "volumes", "v", false, "Remove anonymous volumes associated with the container")
 	flags.StringArrayVarP(&rmOptions.CIDFiles, "cidfile", "", nil, "Read the container ID from the file")
 
-	if registry.IsRemote() {
-		_ = flags.MarkHidden("ignore")
-		_ = flags.MarkHidden("cidfile")
+	if !registry.IsRemote() {
+		// This option is deprecated, but needs to still exists for backwards compatibility
+		flags.Bool("storage", false, "Remove container from storage library")
 		_ = flags.MarkHidden("storage")
 	}
 }
@@ -97,12 +96,6 @@ func removeContainers(namesOrIDs []string, rmOptions entities.RmOptions, setExit
 	var (
 		errs utils.OutputErrors
 	)
-	// Storage conflicts with --all/--latest/--volumes/--cidfile/--ignore
-	if rmOptions.Storage {
-		if rmOptions.All || rmOptions.Ignore || rmOptions.Latest || rmOptions.Volumes || rmOptions.CIDFiles != nil {
-			return errors.Errorf("--storage conflicts with --volumes, --all, --latest, --ignore and --cidfile")
-		}
-	}
 	responses, err := registry.ContainerEngine().ContainerRm(context.Background(), namesOrIDs, rmOptions)
 	if err != nil {
 		if setExit {
diff --git a/docs/source/markdown/podman-rm.1.md b/docs/source/markdown/podman-rm.1.md
index e3e6740df..36904a128 100644
--- a/docs/source/markdown/podman-rm.1.md
+++ b/docs/source/markdown/podman-rm.1.md
@@ -43,13 +43,6 @@ to run containers such as CRI-O, the last started container could be from either
 
 The latest option is not supported on the remote client.
 
-**--storage**
-
-Remove external containers from the storage library.
-This is only possible with containers that are not present in libpod can be seen by **podman ps --all --storage**).
-It is used to remove external containers from **podman build** and **buildah**, and orphan containers which were only partially removed by **podman rm**.
-The storage option conflicts with the **--all**, **--latest**, and **--volumes** options.
-
 **--volumes**, **-v**
 
 Remove anonymous volumes associated with the container. This does not include named volumes
@@ -96,7 +89,7 @@ $ podman rm -f --latest
   **125** The command fails for any other reason
 
 ## SEE ALSO
-podman(1), podman-image-rm(1), podman-ps(1), podman-build(1)
+podman(1), podman-image-rm(1), podman-ps(1), podman-build(1), buildah(1), cri-o(1)
 
 ## HISTORY
 August 2017, Originally compiled by Ryan Cole <rycole@redhat.com>
diff --git a/pkg/domain/entities/containers.go b/pkg/domain/entities/containers.go
index 7b272f01e..3b6dd106f 100644
--- a/pkg/domain/entities/containers.go
+++ b/pkg/domain/entities/containers.go
@@ -131,7 +131,6 @@ type RmOptions struct {
 	Force    bool
 	Ignore   bool
 	Latest   bool
-	Storage  bool
 	Volumes  bool
 }
 
diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go
index 0107e18c4..ac7523094 100644
--- a/pkg/domain/infra/abi/containers.go
+++ b/pkg/domain/infra/abi/containers.go
@@ -273,16 +273,6 @@ func (ic *ContainerEngine) ContainerRestart(ctx context.Context, namesOrIds []st
 
 func (ic *ContainerEngine) ContainerRm(ctx context.Context, namesOrIds []string, options entities.RmOptions) ([]*entities.RmReport, error) {
 	reports := []*entities.RmReport{}
-	if options.Storage {
-		for _, ctr := range namesOrIds {
-			report := entities.RmReport{Id: ctr}
-			if err := ic.Libpod.RemoveStorageContainer(ctr, options.Force); err != nil {
-				report.Err = err
-			}
-			reports = append(reports, &report)
-		}
-		return reports, nil
-	}
 
 	names := namesOrIds
 	for _, cidFile := range options.CIDFiles {
@@ -294,6 +284,22 @@ func (ic *ContainerEngine) ContainerRm(ctx context.Context, namesOrIds []string,
 		names = append(names, id)
 	}
 
+	// Attempt to remove named containers directly from storage, if container is defined in libpod
+	// this will fail and code will fall through to removing the container from libpod.`
+	tmpNames := []string{}
+	for _, ctr := range names {
+		report := entities.RmReport{Id: ctr}
+		if err := ic.Libpod.RemoveStorageContainer(ctr, options.Force); err != nil {
+			// remove container names that we successfully deleted
+			tmpNames = append(tmpNames, ctr)
+		} else {
+			reports = append(reports, &report)
+		}
+	}
+	if len(tmpNames) < len(names) {
+		names = tmpNames
+	}
+
 	ctrs, err := getContainersByContext(options.All, options.Latest, names, ic.Libpod)
 	if err != nil && !(options.Ignore && errors.Cause(err) == define.ErrNoSuchCtr) {
 		// Failed to get containers. If force is specified, get the containers ID
@@ -302,7 +308,7 @@ func (ic *ContainerEngine) ContainerRm(ctx context.Context, namesOrIds []string,
 			return nil, err
 		}
 
-		for _, ctr := range namesOrIds {
+		for _, ctr := range names {
 			logrus.Debugf("Evicting container %q", ctr)
 			report := entities.RmReport{Id: ctr}
 			id, err := ic.Libpod.EvictContainer(ctx, ctr, options.Volumes)
diff --git a/test/apiv2/20-containers.at b/test/apiv2/20-containers.at
index 28289955a..d7e5bfee8 100644
--- a/test/apiv2/20-containers.at
+++ b/test/apiv2/20-containers.at
@@ -206,7 +206,7 @@ t POST containers/${cid_top}/stop "" 204
 t DELETE containers/$cid 204
 t DELETE containers/$cid_top 204
 
-# test the apiv2 create, should't ignore the ENV and WORKDIR from the image
+# test the apiv2 create, shouldn't ignore the ENV and WORKDIR from the image
 t POST containers/create '"Image":"'$ENV_WORKDIR_IMG'","Env":["testKey1"]' 201 \
   .Id~[0-9a-f]\\{64\\}
 cid=$(jq -r '.Id' <<<"$output")
diff --git a/test/e2e/network_test.go b/test/e2e/network_test.go
index a15359ea3..cbfd72da6 100644
--- a/test/e2e/network_test.go
+++ b/test/e2e/network_test.go
@@ -28,7 +28,7 @@ func removeConf(confPath string) {
 // generateNetworkConfig generates a cni config with a random name
 // it returns the network name and the filepath
 func generateNetworkConfig(p *PodmanTestIntegration) (string, string) {
-	// generate a random name to preven conflicts with other tests
+	// generate a random name to prevent conflicts with other tests
 	name := "net" + stringid.GenerateNonCryptoID()
 	path := filepath.Join(p.CNIConfigDir, fmt.Sprintf("%s.conflist", name))
 	conf := fmt.Sprintf(`{
diff --git a/test/e2e/play_kube_test.go b/test/e2e/play_kube_test.go
index b0831c823..b6a390950 100644
--- a/test/e2e/play_kube_test.go
+++ b/test/e2e/play_kube_test.go
@@ -769,7 +769,7 @@ var _ = Describe("Podman generate kube", func() {
 	It("podman play kube test restartPolicy", func() {
 		// podName,  set,  expect
 		testSli := [][]string{
-			{"testPod1", "", "always"}, // Default eqaul to always
+			{"testPod1", "", "always"}, // Default equal to always
 			{"testPod2", "Always", "always"},
 			{"testPod3", "OnFailure", "on-failure"},
 			{"testPod4", "Never", "no"},
diff --git a/test/e2e/rm_test.go b/test/e2e/rm_test.go
index 7eff8c6ed..524c07cc6 100644
--- a/test/e2e/rm_test.go
+++ b/test/e2e/rm_test.go
@@ -236,7 +236,6 @@ var _ = Describe("Podman rm", func() {
 	})
 
 	It("podman rm --ignore bogus container and a running container", func() {
-
 		session := podmanTest.RunTopContainer("test1")
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).To(Equal(0))
diff --git a/test/e2e/run_test.go b/test/e2e/run_test.go
index cd32e5a77..e6bba9f67 100644
--- a/test/e2e/run_test.go
+++ b/test/e2e/run_test.go
@@ -394,7 +394,7 @@ USER bin`
 	})
 
 	It("podman run sysctl test", func() {
-		SkipIfRootless("Network sysctls are not avalable root rootless")
+		SkipIfRootless("Network sysctls are not available root rootless")
 		session := podmanTest.Podman([]string{"run", "--rm", "--sysctl", "net.core.somaxconn=65535", ALPINE, "sysctl", "net.core.somaxconn"})
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).To(Equal(0))
diff --git a/test/system/055-rm.bats b/test/system/055-rm.bats
index c8475c3e9..7176ae4b8 100644
--- a/test/system/055-rm.bats
+++ b/test/system/055-rm.bats
@@ -33,6 +33,21 @@ load helpers
     run_podman rm -f $cid
 }
 
+@test "podman rm container from storage" {
+    if is_remote; then
+        skip "only applicable for local podman"
+    fi
+    rand=$(random_string 30)
+    run_podman create --name $rand $IMAGE /bin/true
+
+    # Create a container that podman does not know about
+    run buildah from $IMAGE
+    cid="$output"
+
+    # rm should succeed
+    run_podman rm $rand $cid
+}
+
 # I'm sorry! This test takes 13 seconds. There's not much I can do about it,
 # please know that I think it's justified: podman 1.5.0 had a strange bug
 # in with exit status was not preserved on some code paths with 'rm -f'