Merge pull request #25946 from ninja-quokka/docker_compat_force_image_remove

bug: Correct Docker compat REST API image delete endpoint

Author: openshift-merge-bot[bot]

libpod/reset.go

@@ -178,7 +178,7 @@ func (r *Runtime) Reset(ctx context.Context) error {
 	rmiOptions := &libimage.RemoveImagesOptions{
 		Force:               true,
 		Ignore:              true,
-		RemoveContainerFunc: r.RemoveContainersForImageCallback(ctx),
+		RemoveContainerFunc: r.RemoveContainersForImageCallback(ctx, true),
 		Filters:             []string{"readonly=false"},
 	}
 	if _, rmiErrors := r.LibimageRuntime().RemoveImages(ctx, nil, rmiOptions); rmiErrors != nil {

libpod/runtime_img.go

@@ -23,9 +23,10 @@ import (
 
 // RemoveContainersForImageCallback returns a callback that can be used in
 // `libimage`.  When forcefully removing images, containers using the image
-// should be removed as well.  The callback allows for more graceful removal as
-// we can use the libpod-internal removal logic.
-func (r *Runtime) RemoveContainersForImageCallback(ctx context.Context) libimage.RemoveContainerFunc {
+// should be removed as well unless the request comes from the Docker compat API.
+// The callback allows for more graceful removal as we can use the libpod-internal
+// removal logic.
+func (r *Runtime) RemoveContainersForImageCallback(ctx context.Context, force bool) libimage.RemoveContainerFunc {
 	return func(imageID string) error {
 		if !r.valid {
 			return define.ErrRuntimeStopped
@@ -44,12 +45,12 @@ func (r *Runtime) RemoveContainersForImageCallback(ctx context.Context) libimage
 				if err != nil {
 					return fmt.Errorf("container %s is in pod %s, but pod cannot be retrieved: %w", ctr.ID(), ctr.config.Pod, err)
 				}
-				if _, err := r.removePod(ctx, pod, true, true, timeout); err != nil {
+				if _, err := r.removePod(ctx, pod, true, force, timeout); err != nil {
 					return fmt.Errorf("removing image %s: container %s using image could not be removed: %w", imageID, ctr.ID(), err)
 				}
 			} else {
 				opts := ctrRmOpts{
-					Force:   true,
+					Force:   force,
 					Timeout: timeout,
 				}
 				if _, _, err := r.removeContainer(ctx, ctr, opts); err != nil {

pkg/api/handlers/compat/images_remove.go

@@ -8,6 +8,7 @@ import (
 	"net/http"
 
 	"github.com/containers/podman/v5/libpod"
+	"github.com/containers/podman/v5/libpod/define"
 	"github.com/containers/podman/v5/pkg/api/handlers/utils"
 	api "github.com/containers/podman/v5/pkg/api/types"
 	"github.com/containers/podman/v5/pkg/domain/entities"
@@ -41,9 +42,10 @@ func RemoveImage(w http.ResponseWriter, r *http.Request) {
 	imageEngine := abi.ImageEngine{Libpod: runtime}
 
 	options := entities.ImageRemoveOptions{
-		Force:   query.Force,
-		NoPrune: query.NoPrune,
-		Ignore:  query.Ignore,
+		Force:                        query.Force,
+		NoPrune:                      query.NoPrune,
+		Ignore:                       query.Ignore,
+		DisableForceRemoveContainers: true,
 	}
 	report, rmerrors := imageEngine.Remove(r.Context(), []string{possiblyNormalizedName}, options)
 	if len(rmerrors) > 0 && rmerrors[0] != nil {
@@ -56,6 +58,10 @@ func RemoveImage(w http.ResponseWriter, r *http.Request) {
 			utils.Error(w, http.StatusConflict, fmt.Errorf("image %s is in use: %w", name, err))
 			return
 		}
+		if errors.Is(err, define.ErrCtrStateInvalid) {
+			utils.Error(w, http.StatusConflict, fmt.Errorf("image %s is in an invalid state: %w", name, err))
+			return
+		}
 		utils.Error(w, http.StatusInternalServerError, err)
 		return
 	}

pkg/api/server/register_images.go

@@ -227,7 +227,7 @@ func (s *APIServer) registerImagesHandlers(r *mux.Router) error {
 	//  - in: query
 	//    name: force
 	//    type: boolean
-	//    description: remove the image even if used by containers or has other tags
+	//    description: Remove the image even if it is being used by stopped containers or has other tags
 	//  - in: query
 	//    name: noprune
 	//    type: boolean

pkg/domain/entities/images.go

@@ -64,7 +64,8 @@ type ImageRemoveOptions struct {
 	// Confirms if given name is a manifest list and removes it, otherwise returns error.
 	LookupManifest bool
 	// NoPrune will not remove dangling images
-	NoPrune bool
+	NoPrune                      bool
+	DisableForceRemoveContainers bool
 }
 
 // ImageRemoveReport is the response for removing one or more image(s) from storage
@@ -73,8 +74,10 @@ type ImageRemoveReport = entitiesTypes.ImageRemoveReport
 
 type ImageHistoryOptions struct{}
 
-type ImageHistoryLayer = entitiesTypes.ImageHistoryLayer
-type ImageHistoryReport = entitiesTypes.ImageHistoryReport
+type (
+	ImageHistoryLayer  = entitiesTypes.ImageHistoryLayer
+	ImageHistoryReport = entitiesTypes.ImageHistoryReport
+)
 
 // ImagePullOptions are the arguments for pulling images.
 type ImagePullOptions struct {
@@ -255,8 +258,10 @@ type ImagePruneOptions struct {
 	Filter     []string `json:"filter" schema:"filter"`
 }
 
-type ImageTagOptions struct{}
-type ImageUntagOptions struct{}
+type (
+	ImageTagOptions   struct{}
+	ImageUntagOptions struct{}
+)
 
 // ImageInspectReport is the data when inspecting an image.
 type ImageInspectReport = entitiesTypes.ImageInspectReport

pkg/domain/infra/abi/images.go

@@ -58,7 +58,7 @@ func (ir *ImageEngine) Exists(_ context.Context, nameOrID string) (*entities.Boo
 
 func (ir *ImageEngine) Prune(ctx context.Context, opts entities.ImagePruneOptions) ([]*reports.PruneReport, error) {
 	pruneOptions := &libimage.RemoveImagesOptions{
-		RemoveContainerFunc:     ir.Libpod.RemoveContainersForImageCallback(ctx),
+		RemoveContainerFunc:     ir.Libpod.RemoveContainersForImageCallback(ctx, true),
 		IsExternalContainerFunc: ir.Libpod.IsExternalContainerCallback(ctx),
 		ExternalContainers:      opts.External,
 		Filters:                 append(opts.Filter, "readonly=false"),
@@ -666,7 +666,7 @@ func (ir *ImageEngine) Remove(ctx context.Context, images []string, opts entitie
 	if !opts.All {
 		libimageOptions.Filters = append(libimageOptions.Filters, "intermediate=false")
 	}
-	libimageOptions.RemoveContainerFunc = ir.Libpod.RemoveContainersForImageCallback(ctx)
+	libimageOptions.RemoveContainerFunc = ir.Libpod.RemoveContainersForImageCallback(ctx, !opts.DisableForceRemoveContainers)
 
 	libimageReport, libimageErrors := ir.Libpod.LibimageRuntime().RemoveImages(ctx, images, libimageOptions)
 

test/apiv2/10-images.at

@@ -231,6 +231,47 @@ t DELETE images/test1:latest 200
 
 t GET "images/get?names=alpine" 200 '[POSIX tar archive]'
 
+# START: Testing variance between Docker API and Podman API
+# regarding force deleting images.
+# Podman: Force deleting an image will force remove any
+#         container using the image.
+# Docker: Force deleting an image will only remove non
+#         running containers using the image.
+
+# Create new image
+podman image build -t docker.io/library/test1:latest - <<EOF
+from alpine
+RUN >file4
+EOF
+
+# Create running container
+podman run --rm -d --name test_container docker.io/library/test1:latest top
+
+# When using the Docker Compat API, force deleting an image
+# shouldn't force delete any container using the image, only
+# containers in a non running state should be removed.
+# https://github.com/containers/podman/issues/25871
+t DELETE images/test1:latest?force=true 409
+
+# When using the Podman Libpod API, deleting an image
+# with a running container will fail.
+t DELETE libpod/images/test1:latest 409
+
+# When using the Podman Libpod API, force deleting an
+# image will also force delete all containers using the image.
+
+# Verify container exists.
+t GET libpod/containers/test_container/exists 204
+
+# Delete image with force.
+t DELETE libpod/images/test1:latest?force=true 200
+
+# Verify container also removed.
+t GET libpod/containers/test_container/exists 404
+
+# END: Testing variance between Docker API and Podman API
+# regarding force deleting images.
+
 podman pull busybox
 t GET "images/get?names=alpine&names=busybox" 200 '[POSIX tar archive]'
 img_cnt=$(tar xf "$WORKDIR/curl.result.out" manifest.json -O | jq "length")

test/apiv2/python/rest_api/test_v2_0_0_image.py

@@ -66,9 +66,8 @@ def test_inspect(self):
         self.assertIn("sha256:",image['Id'])
 
     def test_delete(self):
-        r = requests.delete(self.podman_url + "/v1.40/images/alpine?force=true")
-        self.assertEqual(r.status_code, 200, r.text)
-        self.assertIsInstance(r.json(), list)
+        r = requests.delete(self.compat_uri("images/alpine?force=true"))
+        self.assertEqual(r.status_code, 409, r.text)
 
     def test_pull(self):
         r = requests.post(self.uri("/images/pull?reference=alpine"), timeout=15)