From 7f97896c59d23d9dda704b19203a9ceb49b57237 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 4 May 2020 14:01:39 +0200 Subject: image removal: refactor part 2 Continue the refactoring of image removal. I didn't manage to break all the following changes into smaller and easier to digest commits due to time constraints: * Return an error slice instead of a single error. Use multierror only in the client/frontend. Reflect that in the types. * Use the batch image removal in the client while preserving the more rest-idiomatic single-image removal endpoint. * Add a new handler for the single-image removal endpoint to make it share the same code as the batch endpoint. * Expose bindings for the single and batch endpoints, so we can properly test them. * Add several convenience functions for error handling to pkg/errorhandling. * Set the correct error type in libpod to set the exit code to 2 when one or more containers are using an image. * Massage the bindings tests a bit and tackle compilation errors. Signed-off-by: Valentin Rothberg --- cmd/podman/containers/run.go | 6 +- cmd/podman/images/rm.go | 7 ++- libpod/runtime_img.go | 2 +- pkg/api/handlers/libpod/images.go | 51 +++++++++++++++-- pkg/api/handlers/types.go | 2 +- pkg/api/server/register_images.go | 64 ++++++++++----------- pkg/bindings/images/images.go | 30 ---------- pkg/bindings/images/rm.go | 65 +++++++++++++++++++++ pkg/bindings/test/images_test.go | 40 +++++++------ pkg/bindings/test/manifests_test.go | 2 +- pkg/domain/entities/engine_image.go | 2 +- pkg/domain/infra/abi/images.go | 109 +++++++++++++++--------------------- pkg/domain/infra/tunnel/images.go | 4 +- pkg/errorhandling/errorhandling.go | 36 ++++++++++++ 14 files changed, 262 insertions(+), 158 deletions(-) create mode 100644 pkg/bindings/images/rm.go diff --git a/cmd/podman/containers/run.go b/cmd/podman/containers/run.go index b13983e37..f72446cb6 100644 --- a/cmd/podman/containers/run.go +++ b/cmd/podman/containers/run.go @@ -170,9 +170,9 @@ func run(cmd *cobra.Command, args []string) error { return nil } if runRmi { - _, err := registry.ImageEngine().Remove(registry.GetContext(), []string{args[0]}, entities.ImageRemoveOptions{}) - if err != nil { - logrus.Errorf("%s", errors.Wrapf(err, "failed removing image")) + _, rmErrors := registry.ImageEngine().Remove(registry.GetContext(), []string{args[0]}, entities.ImageRemoveOptions{}) + if len(rmErrors) > 0 { + logrus.Errorf("%s", errors.Wrapf(errorhandling.JoinErrors(rmErrors), "failed removing image")) } } return nil diff --git a/cmd/podman/images/rm.go b/cmd/podman/images/rm.go index 1cf5fa365..4b9920532 100644 --- a/cmd/podman/images/rm.go +++ b/cmd/podman/images/rm.go @@ -5,6 +5,7 @@ import ( "github.com/containers/libpod/cmd/podman/registry" "github.com/containers/libpod/pkg/domain/entities" + "github.com/containers/libpod/pkg/errorhandling" "github.com/pkg/errors" "github.com/spf13/cobra" "github.com/spf13/pflag" @@ -48,7 +49,9 @@ func rm(cmd *cobra.Command, args []string) error { return errors.Errorf("when using the --all switch, you may not pass any images names or IDs") } - report, err := registry.ImageEngine().Remove(registry.GetContext(), args, imageOpts) + // Note: certain image-removal errors are non-fatal. Hence, the report + // might be set even if err != nil. + report, rmErrors := registry.ImageEngine().Remove(registry.GetContext(), args, imageOpts) if report != nil { for _, u := range report.Untagged { fmt.Println("Untagged: " + u) @@ -62,5 +65,5 @@ func rm(cmd *cobra.Command, args []string) error { registry.SetExitCode(report.ExitCode) } - return err + return errorhandling.JoinErrors(rmErrors) } diff --git a/libpod/runtime_img.go b/libpod/runtime_img.go index 919080c42..cd7f54799 100644 --- a/libpod/runtime_img.go +++ b/libpod/runtime_img.go @@ -56,7 +56,7 @@ func (r *Runtime) RemoveImage(ctx context.Context, img *image.Image, force bool) } } } else { - return nil, fmt.Errorf("could not remove image %s as it is being used by %d containers", img.ID(), len(imageCtrs)) + return nil, errors.Wrapf(define.ErrImageInUse, "could not remove image %s as it is being used by %d containers", img.ID(), len(imageCtrs)) } } diff --git a/pkg/api/handlers/libpod/images.go b/pkg/api/handlers/libpod/images.go index f7be5ce9a..93b4564a1 100644 --- a/pkg/api/handlers/libpod/images.go +++ b/pkg/api/handlers/libpod/images.go @@ -23,6 +23,7 @@ import ( "github.com/containers/libpod/pkg/api/handlers/utils" "github.com/containers/libpod/pkg/domain/entities" "github.com/containers/libpod/pkg/domain/infra/abi" + "github.com/containers/libpod/pkg/errorhandling" "github.com/containers/libpod/pkg/util" utils2 "github.com/containers/libpod/utils" "github.com/gorilla/schema" @@ -700,8 +701,8 @@ func SearchImages(w http.ResponseWriter, r *http.Request) { utils.WriteResponse(w, http.StatusOK, reports) } -// ImagesRemove is the endpoint for image removal. -func ImagesRemove(w http.ResponseWriter, r *http.Request) { +// ImagesBatchRemove is the endpoint for batch image removal. +func ImagesBatchRemove(w http.ResponseWriter, r *http.Request) { runtime := r.Context().Value("runtime").(*libpod.Runtime) decoder := r.Context().Value("decoder").(*schema.Decoder) query := struct { @@ -722,7 +723,49 @@ func ImagesRemove(w http.ResponseWriter, r *http.Request) { opts := entities.ImageRemoveOptions{All: query.All, Force: query.Force} imageEngine := abi.ImageEngine{Libpod: runtime} - rmReport, rmError := imageEngine.Remove(r.Context(), query.Images, opts) - report := handlers.LibpodImagesRemoveReport{ImageRemoveReport: *rmReport, Error: rmError.Error()} + rmReport, rmErrors := imageEngine.Remove(r.Context(), query.Images, opts) + + strErrs := errorhandling.ErrorsToStrings(rmErrors) + report := handlers.LibpodImagesRemoveReport{ImageRemoveReport: *rmReport, Errors: strErrs} utils.WriteResponse(w, http.StatusOK, report) } + +// ImagesRemove is the endpoint for removing one image. +func ImagesRemove(w http.ResponseWriter, r *http.Request) { + runtime := r.Context().Value("runtime").(*libpod.Runtime) + decoder := r.Context().Value("decoder").(*schema.Decoder) + query := struct { + Force bool `schema:"force"` + }{ + Force: false, + } + + if err := decoder.Decode(&query, r.URL.Query()); err != nil { + utils.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest, + errors.Wrapf(err, "failed to parse parameters for %s", r.URL.String())) + return + } + + opts := entities.ImageRemoveOptions{Force: query.Force} + imageEngine := abi.ImageEngine{Libpod: runtime} + rmReport, rmErrors := imageEngine.Remove(r.Context(), []string{utils.GetName(r)}, opts) + + // In contrast to batch-removal, where we're only setting the exit + // code, we need to have another closer look at the errors here and set + // the appropriate http status code. + + switch rmReport.ExitCode { + case 0: + report := handlers.LibpodImagesRemoveReport{ImageRemoveReport: *rmReport, Errors: []string{}} + utils.WriteResponse(w, http.StatusOK, report) + case 1: + // 404 - no such image + utils.Error(w, "error removing image", http.StatusNotFound, errorhandling.JoinErrors(rmErrors)) + case 2: + // 409 - conflict error (in use by containers) + utils.Error(w, "error removing image", http.StatusConflict, errorhandling.JoinErrors(rmErrors)) + default: + // 500 - internal error + utils.Error(w, "failed to remove image", http.StatusInternalServerError, errorhandling.JoinErrors(rmErrors)) + } +} diff --git a/pkg/api/handlers/types.go b/pkg/api/handlers/types.go index 58a12ea6a..a7abf59c0 100644 --- a/pkg/api/handlers/types.go +++ b/pkg/api/handlers/types.go @@ -41,7 +41,7 @@ type LibpodImagesPullReport struct { type LibpodImagesRemoveReport struct { entities.ImageRemoveReport // Image removal requires is to return data and an error. - Error string + Errors []string } type ContainersPruneReport struct { diff --git a/pkg/api/server/register_images.go b/pkg/api/server/register_images.go index f59dca6f5..0e8d68b7e 100644 --- a/pkg/api/server/register_images.go +++ b/pkg/api/server/register_images.go @@ -822,7 +822,7 @@ func (s *APIServer) registerImagesHandlers(r *mux.Router) error { // 500: // $ref: '#/responses/InternalError' r.Handle(VersionedPath("/libpod/images/import"), s.APIHandler(libpod.ImagesImport)).Methods(http.MethodPost) - // swagger:operation GET /libpod/images/remove libpod libpodImagesRemove + // swagger:operation DELETE /libpod/images/remove libpod libpodImagesRemove // --- // tags: // - images @@ -853,7 +853,37 @@ func (s *APIServer) registerImagesHandlers(r *mux.Router) error { // $ref: "#/responses/BadParamError" // 500: // $ref: '#/responses/InternalError' - r.Handle(VersionedPath("/libpod/images/remove"), s.APIHandler(libpod.ImagesRemove)).Methods(http.MethodGet) + r.Handle(VersionedPath("/libpod/images/remove"), s.APIHandler(libpod.ImagesBatchRemove)).Methods(http.MethodDelete) + // swagger:operation DELETE /libpod/images/{name:.*}/remove libpod libpodRemoveImage + // --- + // tags: + // - images + // summary: Remove an image from the local storage. + // description: Remove an image from the local storage. + // parameters: + // - in: path + // name: name:.* + // type: string + // required: true + // description: name or ID of image to remove + // - in: query + // name: force + // type: boolean + // description: remove the image even if used by containers or has other tags + // produces: + // - application/json + // responses: + // 200: + // $ref: "#/responses/DocsImageDeleteResponse" + // 400: + // $ref: "#/responses/BadParamError" + // 404: + // $ref: '#/responses/NoSuchImage' + // 409: + // $ref: '#/responses/ConflictError' + // 500: + // $ref: '#/responses/InternalError' + r.Handle(VersionedPath("/libpod/images/{name:.*}/remove"), s.APIHandler(libpod.ImagesRemove)).Methods(http.MethodDelete) // swagger:operation POST /libpod/images/pull libpod libpodImagesPull // --- // tags: @@ -952,36 +982,6 @@ func (s *APIServer) registerImagesHandlers(r *mux.Router) error { // 500: // $ref: '#/responses/InternalError' r.Handle(VersionedPath("/libpod/images/search"), s.APIHandler(libpod.SearchImages)).Methods(http.MethodGet) - // swagger:operation DELETE /libpod/images/{name:.*} libpod libpodRemoveImage - // --- - // tags: - // - images - // summary: Remove Image - // description: Delete an image from local store - // parameters: - // - in: path - // name: name:.* - // type: string - // required: true - // description: name or ID of image to delete - // - in: query - // name: force - // type: boolean - // description: remove the image even if used by containers or has other tags - // produces: - // - application/json - // responses: - // 200: - // $ref: "#/responses/DocsImageDeleteResponse" - // 400: - // $ref: "#/responses/BadParamError" - // 404: - // $ref: '#/responses/NoSuchImage' - // 409: - // $ref: '#/responses/ConflictError' - // 500: - // $ref: '#/responses/InternalError' - r.Handle(VersionedPath("/libpod/images/{name:.*}"), s.APIHandler(compat.RemoveImage)).Methods(http.MethodDelete) // swagger:operation GET /libpod/images/{name:.*}/get libpod libpodExportImage // --- // tags: diff --git a/pkg/bindings/images/images.go b/pkg/bindings/images/images.go index 4d8ae6a6e..034ade618 100644 --- a/pkg/bindings/images/images.go +++ b/pkg/bindings/images/images.go @@ -109,36 +109,6 @@ func Load(ctx context.Context, r io.Reader, name *string) (*entities.ImageLoadRe return &report, response.Process(&report) } -// Remove deletes an image from local storage. The optional force parameter -// will forcibly remove the image by removing all all containers, including -// those that are Running, first. -func Remove(ctx context.Context, images []string, opts entities.ImageRemoveOptions) (*entities.ImageRemoveReport, error) { - var report handlers.LibpodImagesRemoveReport - conn, err := bindings.GetClient(ctx) - if err != nil { - return nil, err - } - params := url.Values{} - params.Set("all", strconv.FormatBool(opts.All)) - params.Set("force", strconv.FormatBool(opts.Force)) - for _, i := range images { - params.Add("images", i) - } - - response, err := conn.DoRequest(nil, http.MethodGet, "/images/remove", params) - if err != nil { - return nil, err - } - if err := response.Process(&report); err != nil { - return nil, err - } - var rmError error - if report.Error != "" { - rmError = errors.New(report.Error) - } - return &report.ImageRemoveReport, rmError -} - // Export saves an image from local storage as a tarball or image archive. The optional format // parameter is used to change the format of the output. func Export(ctx context.Context, nameOrID string, w io.Writer, format *string, compress *bool) error { diff --git a/pkg/bindings/images/rm.go b/pkg/bindings/images/rm.go new file mode 100644 index 000000000..e3b5590df --- /dev/null +++ b/pkg/bindings/images/rm.go @@ -0,0 +1,65 @@ +package images + +import ( + "context" + "net/http" + "net/url" + "strconv" + + "github.com/containers/libpod/pkg/api/handlers" + "github.com/containers/libpod/pkg/bindings" + "github.com/containers/libpod/pkg/domain/entities" + "github.com/containers/libpod/pkg/errorhandling" +) + +// BachtRemove removes a batch of images from the local storage. +func BatchRemove(ctx context.Context, images []string, opts entities.ImageRemoveOptions) (*entities.ImageRemoveReport, []error) { + // FIXME - bindings tests are missing for this endpoint. Once the CI is + // re-enabled for bindings, we need to add them. At the time of writing, + // the tests don't compile. + var report handlers.LibpodImagesRemoveReport + conn, err := bindings.GetClient(ctx) + if err != nil { + return nil, []error{err} + } + + params := url.Values{} + params.Set("all", strconv.FormatBool(opts.All)) + params.Set("force", strconv.FormatBool(opts.Force)) + for _, i := range images { + params.Add("images", i) + } + + response, err := conn.DoRequest(nil, http.MethodDelete, "/images/remove", params) + if err != nil { + return nil, []error{err} + } + if err := response.Process(&report); err != nil { + return nil, []error{err} + } + + return &report.ImageRemoveReport, errorhandling.StringsToErrors(report.Errors) +} + +// Remove removes an image from the local storage. Use force to remove an +// image, even if it's used by containers. +func Remove(ctx context.Context, nameOrID string, force bool) (*entities.ImageRemoveReport, error) { + var report handlers.LibpodImagesRemoveReport + conn, err := bindings.GetClient(ctx) + if err != nil { + return nil, err + } + + params := url.Values{} + params.Set("force", strconv.FormatBool(force)) + response, err := conn.DoRequest(nil, http.MethodDelete, "/images/%s/remove", params, nameOrID) + if err != nil { + return nil, err + } + if err := response.Process(&report); err != nil { + return nil, err + } + + errs := errorhandling.StringsToErrors(report.Errors) + return &report.ImageRemoveReport, errorhandling.JoinErrors(errs) +} diff --git a/pkg/bindings/test/images_test.go b/pkg/bindings/test/images_test.go index 58210efd0..9c8e82149 100644 --- a/pkg/bindings/test/images_test.go +++ b/pkg/bindings/test/images_test.go @@ -84,17 +84,20 @@ var _ = Describe("Podman images", func() { // Test to validate the remove image api It("remove image", func() { // Remove invalid image should be a 404 - _, err = images.Remove(bt.conn, "foobar5000", &bindings.PFalse) + response, err := images.Remove(bt.conn, "foobar5000", false) Expect(err).ToNot(BeNil()) + Expect(response).To(BeNil()) code, _ := bindings.CheckResponseCode(err) Expect(code).To(BeNumerically("==", http.StatusNotFound)) // Remove an image by name, validate image is removed and error is nil inspectData, err := images.GetImage(bt.conn, busybox.shortName, nil) Expect(err).To(BeNil()) - response, err := images.Remove(bt.conn, busybox.shortName, nil) + response, err = images.Remove(bt.conn, busybox.shortName, false) Expect(err).To(BeNil()) - Expect(inspectData.ID).To(Equal(response[0]["Deleted"])) + code, _ = bindings.CheckResponseCode(err) + + Expect(inspectData.ID).To(Equal(response.Deleted[0])) inspectData, err = images.GetImage(bt.conn, busybox.shortName, nil) code, _ = bindings.CheckResponseCode(err) Expect(code).To(BeNumerically("==", http.StatusNotFound)) @@ -104,30 +107,31 @@ var _ = Describe("Podman images", func() { _, err = bt.RunTopContainer(&top, &bindings.PFalse, nil) Expect(err).To(BeNil()) // we should now have a container called "top" running - containerResponse, err := containers.Inspect(bt.conn, "top", &bindings.PFalse) + containerResponse, err := containers.Inspect(bt.conn, "top", nil) Expect(err).To(BeNil()) Expect(containerResponse.Name).To(Equal("top")) // try to remove the image "alpine". This should fail since we are not force // deleting hence image cannot be deleted until the container is deleted. - response, err = images.Remove(bt.conn, alpine.shortName, &bindings.PFalse) + response, err = images.Remove(bt.conn, alpine.shortName, false) code, _ = bindings.CheckResponseCode(err) - Expect(code).To(BeNumerically("==", http.StatusInternalServerError)) + Expect(code).To(BeNumerically("==", http.StatusConflict)) // Removing the image "alpine" where force = true - response, err = images.Remove(bt.conn, alpine.shortName, &bindings.PTrue) + response, err = images.Remove(bt.conn, alpine.shortName, true) Expect(err).To(BeNil()) - - // Checking if both the images are gone as well as the container is deleted - inspectData, err = images.GetImage(bt.conn, busybox.shortName, nil) + // To be extra sure, check if the previously created container + // is gone as well. + _, err = containers.Inspect(bt.conn, "top", &bindings.PFalse) code, _ = bindings.CheckResponseCode(err) Expect(code).To(BeNumerically("==", http.StatusNotFound)) - inspectData, err = images.GetImage(bt.conn, alpine.shortName, nil) + // Now make sure both images are gone. + inspectData, err = images.GetImage(bt.conn, busybox.shortName, nil) code, _ = bindings.CheckResponseCode(err) Expect(code).To(BeNumerically("==", http.StatusNotFound)) - _, err = containers.Inspect(bt.conn, "top", &bindings.PFalse) + inspectData, err = images.GetImage(bt.conn, alpine.shortName, nil) code, _ = bindings.CheckResponseCode(err) Expect(code).To(BeNumerically("==", http.StatusNotFound)) }) @@ -209,7 +213,7 @@ var _ = Describe("Podman images", func() { It("Load|Import Image", func() { // load an image - _, err := images.Remove(bt.conn, alpine.name, nil) + _, err := images.Remove(bt.conn, alpine.name, false) Expect(err).To(BeNil()) exists, err := images.Exists(bt.conn, alpine.name) Expect(err).To(BeNil()) @@ -219,7 +223,7 @@ var _ = Describe("Podman images", func() { Expect(err).To(BeNil()) names, err := images.Load(bt.conn, f, nil) Expect(err).To(BeNil()) - Expect(names.Name).To(Equal(alpine.name)) + Expect(names.Names[0]).To(Equal(alpine.name)) exists, err = images.Exists(bt.conn, alpine.name) Expect(err).To(BeNil()) Expect(exists).To(BeTrue()) @@ -227,7 +231,7 @@ var _ = Describe("Podman images", func() { // load with a repo name f, err = os.Open(filepath.Join(ImageCacheDir, alpine.tarballName)) Expect(err).To(BeNil()) - _, err = images.Remove(bt.conn, alpine.name, nil) + _, err = images.Remove(bt.conn, alpine.name, false) Expect(err).To(BeNil()) exists, err = images.Exists(bt.conn, alpine.name) Expect(err).To(BeNil()) @@ -235,7 +239,7 @@ var _ = Describe("Podman images", func() { newName := "quay.io/newname:fizzle" names, err = images.Load(bt.conn, f, &newName) Expect(err).To(BeNil()) - Expect(names.Name).To(Equal(alpine.name)) + Expect(names.Names[0]).To(Equal(alpine.name)) exists, err = images.Exists(bt.conn, newName) Expect(err).To(BeNil()) Expect(exists).To(BeTrue()) @@ -243,7 +247,7 @@ var _ = Describe("Podman images", func() { // load with a bad repo name should trigger a 500 f, err = os.Open(filepath.Join(ImageCacheDir, alpine.tarballName)) Expect(err).To(BeNil()) - _, err = images.Remove(bt.conn, alpine.name, nil) + _, err = images.Remove(bt.conn, alpine.name, false) Expect(err).To(BeNil()) exists, err = images.Exists(bt.conn, alpine.name) Expect(err).To(BeNil()) @@ -271,7 +275,7 @@ var _ = Describe("Podman images", func() { It("Import Image", func() { // load an image - _, err = images.Remove(bt.conn, alpine.name, nil) + _, err = images.Remove(bt.conn, alpine.name, false) Expect(err).To(BeNil()) exists, err := images.Exists(bt.conn, alpine.name) Expect(err).To(BeNil()) diff --git a/pkg/bindings/test/manifests_test.go b/pkg/bindings/test/manifests_test.go index 23c3d8194..4987dfe5b 100644 --- a/pkg/bindings/test/manifests_test.go +++ b/pkg/bindings/test/manifests_test.go @@ -47,7 +47,7 @@ var _ = Describe("Podman containers ", func() { code, _ := bindings.CheckResponseCode(err) Expect(code).To(BeNumerically("==", http.StatusInternalServerError)) - _, err = images.Remove(bt.conn, id, nil) + _, err = images.Remove(bt.conn, id, false) Expect(err).To(BeNil()) // create manifest list with images diff --git a/pkg/domain/entities/engine_image.go b/pkg/domain/entities/engine_image.go index 46a96ca20..45686ec79 100644 --- a/pkg/domain/entities/engine_image.go +++ b/pkg/domain/entities/engine_image.go @@ -19,7 +19,7 @@ type ImageEngine interface { Prune(ctx context.Context, opts ImagePruneOptions) (*ImagePruneReport, error) Pull(ctx context.Context, rawImage string, opts ImagePullOptions) (*ImagePullReport, error) Push(ctx context.Context, source string, destination string, opts ImagePushOptions) error - Remove(ctx context.Context, images []string, opts ImageRemoveOptions) (*ImageRemoveReport, error) + Remove(ctx context.Context, images []string, opts ImageRemoveOptions) (*ImageRemoveReport, []error) Save(ctx context.Context, nameOrId string, tags []string, options ImageSaveOptions) error Search(ctx context.Context, term string, opts ImageSearchOptions) ([]ImageSearchReport, error) Shutdown(ctx context.Context) diff --git a/pkg/domain/infra/abi/images.go b/pkg/domain/infra/abi/images.go index 0af06fb89..7ab5131f0 100644 --- a/pkg/domain/infra/abi/images.go +++ b/pkg/domain/infra/abi/images.go @@ -21,7 +21,6 @@ import ( domainUtils "github.com/containers/libpod/pkg/domain/utils" "github.com/containers/libpod/pkg/util" "github.com/containers/storage" - "github.com/hashicorp/go-multierror" imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -423,8 +422,10 @@ func (ir *ImageEngine) Tree(ctx context.Context, nameOrId string, opts entities. return &entities.ImageTreeReport{Tree: results}, nil } -// Remove removes one or more images from local storage. -func (ir *ImageEngine) Remove(ctx context.Context, images []string, opts entities.ImageRemoveOptions) (report *entities.ImageRemoveReport, finalError error) { +// removeErrorsToExitCode returns an exit code for the specified slice of +// image-removal errors. The error codes are set according to the documented +// behaviour in the Podman man pages. +func removeErrorsToExitCode(rmErrors []error) int { var ( // noSuchImageErrors indicates that at least one image was not found. noSuchImageErrors bool @@ -434,59 +435,53 @@ func (ir *ImageEngine) Remove(ctx context.Context, images []string, opts entitie // otherErrors indicates that at least one error other than the two // above occured. otherErrors bool - // deleteError is a multierror to conveniently collect errors during - // removal. We really want to delete as many images as possible and not - // error out immediately. - deleteError *multierror.Error ) - report = &entities.ImageRemoveReport{} + if len(rmErrors) == 0 { + return 0 + } - // Set the removalCode and the error after all work is done. - defer func() { - switch { - // 2 - case inUseErrors: - // One of the specified images has child images or is - // being used by a container. - report.ExitCode = 2 - // 1 - case noSuchImageErrors && !(otherErrors || inUseErrors): - // One of the specified images did not exist, and no other - // failures. - report.ExitCode = 1 - // 0 + for _, e := range rmErrors { + switch errors.Cause(e) { + case define.ErrNoSuchImage: + noSuchImageErrors = true + case define.ErrImageInUse, storage.ErrImageUsedByContainer: + inUseErrors = true default: - // Nothing to do. - } - if deleteError != nil { - // go-multierror has a trailing new line which we need to remove to normalize the string. - finalError = deleteError.ErrorOrNil() - finalError = errors.New(strings.TrimSpace(finalError.Error())) + otherErrors = true } + } + + switch { + case inUseErrors: + // One of the specified images has child images or is + // being used by a container. + return 2 + case noSuchImageErrors && !(otherErrors || inUseErrors): + // One of the specified images did not exist, and no other + // failures. + return 1 + default: + return 125 + } +} + +// Remove removes one or more images from local storage. +func (ir *ImageEngine) Remove(ctx context.Context, images []string, opts entities.ImageRemoveOptions) (report *entities.ImageRemoveReport, rmErrors []error) { + report = &entities.ImageRemoveReport{} + + // Set the exit code at very end. + defer func() { + report.ExitCode = removeErrorsToExitCode(rmErrors) }() // deleteImage is an anonymous function to conveniently delete an image // without having to pass all local data around. deleteImage := func(img *image.Image) error { results, err := ir.Libpod.RemoveImage(ctx, img, opts.Force) - switch errors.Cause(err) { - case nil: - break - case define.ErrNoSuchImage: - inUseErrors = true // ExitCode is expected - case storage.ErrImageUsedByContainer: - inUseErrors = true // Important for exit codes in Podman. - return errors.New( - fmt.Sprintf("A container associated with containers/storage, i.e. via Buildah, CRI-O, etc., may be associated with this image: %-12.12s\n", img.ID())) - case define.ErrImageInUse: - inUseErrors = true - return err - default: - otherErrors = true // Important for exit codes in Podman. + if err != nil { return err } - report.Deleted = append(report.Deleted, results.Deleted) report.Untagged = append(report.Untagged, results.Untagged...) return nil @@ -499,9 +494,7 @@ func (ir *ImageEngine) Remove(ctx context.Context, images []string, opts entitie for { storageImages, err := ir.Libpod.ImageRuntime().GetRWImages() if err != nil { - deleteError = multierror.Append(deleteError, - errors.Wrapf(err, "unable to query local images")) - otherErrors = true // Important for exit codes in Podman. + rmErrors = append(rmErrors, err) return } // No images (left) to remove, so we're done. @@ -510,9 +503,7 @@ func (ir *ImageEngine) Remove(ctx context.Context, images []string, opts entitie } // Prevent infinity loops by making a delete-progress check. if previousImages == len(storageImages) { - otherErrors = true // Important for exit codes in Podman. - deleteError = multierror.Append(deleteError, - errors.New("unable to delete all images, check errors and re-run image removal if needed")) + rmErrors = append(rmErrors, errors.New("unable to delete all images, check errors and re-run image removal if needed")) break } previousImages = len(storageImages) @@ -520,15 +511,15 @@ func (ir *ImageEngine) Remove(ctx context.Context, images []string, opts entitie for _, img := range storageImages { isParent, err := img.IsParent(ctx) if err != nil { - otherErrors = true // Important for exit codes in Podman. - deleteError = multierror.Append(deleteError, err) + rmErrors = append(rmErrors, err) + continue } // Skip parent images. if isParent { continue } if err := deleteImage(img); err != nil { - deleteError = multierror.Append(deleteError, err) + rmErrors = append(rmErrors, err) } } } @@ -539,21 +530,13 @@ func (ir *ImageEngine) Remove(ctx context.Context, images []string, opts entitie // Delete only the specified images. for _, id := range images { img, err := ir.Libpod.ImageRuntime().NewFromLocal(id) - switch errors.Cause(err) { - case nil: - break - case image.ErrNoSuchImage: - noSuchImageErrors = true // Important for exit codes in Podman. - fallthrough - default: - deleteError = multierror.Append(deleteError, errors.Wrapf(err, "failed to remove image '%s'", id)) + if err != nil { + rmErrors = append(rmErrors, err) continue } - err = deleteImage(img) if err != nil { - otherErrors = true // Important for exit codes in Podman. - deleteError = multierror.Append(deleteError, err) + rmErrors = append(rmErrors, err) } } diff --git a/pkg/domain/infra/tunnel/images.go b/pkg/domain/infra/tunnel/images.go index dcc5fc3e7..00893194c 100644 --- a/pkg/domain/infra/tunnel/images.go +++ b/pkg/domain/infra/tunnel/images.go @@ -20,8 +20,8 @@ func (ir *ImageEngine) Exists(_ context.Context, nameOrId string) (*entities.Boo return &entities.BoolReport{Value: found}, err } -func (ir *ImageEngine) Remove(ctx context.Context, imagesArg []string, opts entities.ImageRemoveOptions) (*entities.ImageRemoveReport, error) { - return images.Remove(ir.ClientCxt, imagesArg, opts) +func (ir *ImageEngine) Remove(ctx context.Context, imagesArg []string, opts entities.ImageRemoveOptions) (*entities.ImageRemoveReport, []error) { + return images.BatchRemove(ir.ClientCxt, imagesArg, opts) } func (ir *ImageEngine) List(ctx context.Context, opts entities.ImageListOptions) ([]*entities.ImageSummary, error) { diff --git a/pkg/errorhandling/errorhandling.go b/pkg/errorhandling/errorhandling.go index 970d47636..3117b0ca4 100644 --- a/pkg/errorhandling/errorhandling.go +++ b/pkg/errorhandling/errorhandling.go @@ -2,10 +2,46 @@ package errorhandling import ( "os" + "strings" + "github.com/hashicorp/go-multierror" + "github.com/pkg/errors" "github.com/sirupsen/logrus" ) +// JoinErrors converts the error slice into a single human-readable error. +func JoinErrors(errs []error) error { + if len(errs) == 0 { + return nil + } + + // `multierror` appends new lines which we need to remove to prevent + // blank lines when printing the error. + var multiE *multierror.Error + multiE = multierror.Append(multiE, errs...) + return errors.New(strings.TrimSpace(multiE.ErrorOrNil().Error())) +} + +// ErrorsToString converts the slice of errors into a slice of corresponding +// error messages. +func ErrorsToStrings(errs []error) []string { + strErrs := make([]string, len(errs)) + for i := range errs { + strErrs[i] = errs[i].Error() + } + return strErrs +} + +// StringsToErrors converts a slice of error messages into a slice of +// corresponding errors. +func StringsToErrors(strErrs []string) []error { + errs := make([]error, len(strErrs)) + for i := range strErrs { + errs[i] = errors.New(strErrs[i]) + } + return errs +} + // SyncQuiet syncs a file and logs any error. Should only be used within // a defer. func SyncQuiet(f *os.File) { -- cgit v1.2.3-54-g00ecf