From bc806da71258b11832bbcffddcd8a3b219bff904 Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Wed, 7 Sep 2022 16:24:47 -0400 Subject: [PATCH 1/4] build: label built images for reliable cleanup on `down` When running `compose down`, the `--rmi` flag can be passed, which currently supports two values: * `local`: remove any _implicitly-named_ images that Compose built * `all` : remove any named images (locally-built or fetched from a remote repo) Removing images in the `local` case can be problematic, as it's historically been done via a fair amount of inference over the Compose model. Additionally, when using the "project-model" (by passing `--project-name` instead of using a Compose file), we're even more limited: if no containers for the project are running, there's nothing to derive state from to perform the inference on. As a first pass, we started labeling _containers_ with the name of the locally-built image associated with it (if any) in #9715. Unfortunately, this still suffers from the aforementioned problems around using actual state (i.e. the containers might no longer exist) and meant that when operating in file mode (the default), things did not behave as expected: the label is not available in the project since it only exists at runtime. Now, with these changes, Compose will label any images it builds with project metadata. Upon cleanup during `down`, the engine image API is queried for related images and matched up with the services for the project. As a fallback for images built with prior versions of Compose, the previous approach is still taken. See also: * https://github.com/docker/compose/issues/9655 * https://github.com/docker/compose/pull/9715 Signed-off-by: Milas Bowman --- pkg/api/labels.go | 6 +- pkg/compose/build.go | 21 ++- pkg/compose/build_buildkit.go | 11 ++ pkg/compose/build_classic.go | 5 + pkg/compose/compose.go | 9 +- pkg/compose/down.go | 128 +++++++++++++-- pkg/compose/down_test.go | 152 ++++++++++++------ pkg/compose/kill_test.go | 1 - .../fixtures/build-dependencies/compose.yaml | 2 + 9 files changed, 262 insertions(+), 73 deletions(-) diff --git a/pkg/api/labels.go b/pkg/api/labels.go index 8e0a8604a7..4352d9c03b 100644 --- a/pkg/api/labels.go +++ b/pkg/api/labels.go @@ -47,14 +47,14 @@ const ( OneoffLabel = "com.docker.compose.oneoff" // SlugLabel stores unique slug used for one-off container identity SlugLabel = "com.docker.compose.slug" - // ImageNameLabel stores the content of the image section in the compose file - ImageNameLabel = "com.docker.compose.image_name" // ImageDigestLabel stores digest of the container image used to run service ImageDigestLabel = "com.docker.compose.image" // DependenciesLabel stores service dependencies DependenciesLabel = "com.docker.compose.depends_on" - // VersionLabel stores the compose tool version used to run application + // VersionLabel stores the compose tool version used to build/run application VersionLabel = "com.docker.compose.version" + // ImageBuilderLabel stores the builder (classic or BuildKit) used to produce the image. + ImageBuilderLabel = "com.docker.compose.image.builder" ) // ComposeVersion is the compose tool version as declared by label VersionLabel diff --git a/pkg/compose/build.go b/pkg/compose/build.go index 5ab257dd2c..1b9b1aaa2a 100644 --- a/pkg/compose/build.go +++ b/pkg/compose/build.go @@ -139,7 +139,6 @@ func (s *composeService) ensureImagesExists(ctx context.Context, project *types. project.Services[i].Labels = types.Labels{} } project.Services[i].CustomLabels.Add(api.ImageDigestLabel, digest) - project.Services[i].CustomLabels.Add(api.ImageNameLabel, service.Image) } } return nil @@ -192,7 +191,6 @@ func (s *composeService) getLocalImagesDigests(ctx context.Context, project *typ digest, ok := images[imgName] if ok { project.Services[i].CustomLabels.Add(api.ImageDigestLabel, digest) - project.Services[i].CustomLabels.Add(api.ImageNameLabel, project.Services[i].Image) } } @@ -263,6 +261,8 @@ func (s *composeService) toBuildOptions(project *types.Project, service types.Se tags = append(tags, service.Build.Tags...) } + imageLabels := getImageBuildLabels(project, service) + return build.Options{ Inputs: build.Inputs{ ContextPath: service.Build.Context, @@ -277,7 +277,7 @@ func (s *composeService) toBuildOptions(project *types.Project, service types.Se Target: service.Build.Target, Exports: []bclient.ExportEntry{{Type: "image", Attrs: map[string]string{}}}, Platforms: plats, - Labels: service.Build.Labels, + Labels: imageLabels, NetworkMode: service.Build.Network, ExtraHosts: service.Build.ExtraHosts.AsList(), Session: sessionConfig, @@ -327,7 +327,6 @@ func sshAgentProvider(sshKeys types.SSHConfig) (session.Attachable, error) { } func addSecretsConfig(project *types.Project, service types.ServiceConfig) (session.Attachable, error) { - var sources []secretsprovider.Source for _, secret := range service.Build.Secrets { config := project.Secrets[secret.Source] @@ -352,3 +351,17 @@ func addSecretsConfig(project *types.Project, service types.ServiceConfig) (sess } return secretsprovider.NewSecretProvider(store), nil } + +func getImageBuildLabels(project *types.Project, service types.ServiceConfig) types.Labels { + ret := make(types.Labels) + if service.Build != nil { + for k, v := range service.Build.Labels { + ret.Add(k, v) + } + } + + ret.Add(api.VersionLabel, api.ComposeVersion) + ret.Add(api.ProjectLabel, project.Name) + ret.Add(api.ServiceLabel, service.Name) + return ret +} diff --git a/pkg/compose/build_buildkit.go b/pkg/compose/build_buildkit.go index d4120ced3d..6ed66a217c 100644 --- a/pkg/compose/build_buildkit.go +++ b/pkg/compose/build_buildkit.go @@ -25,6 +25,8 @@ import ( "github.com/docker/buildx/build" "github.com/docker/buildx/driver" xprogress "github.com/docker/buildx/util/progress" + + "github.com/docker/compose/v2/pkg/api" ) func (s *composeService) doBuildBuildkit(ctx context.Context, project *types.Project, opts map[string]build.Options, mode string) (map[string]string, error) { @@ -47,6 +49,15 @@ func (s *composeService) doBuildBuildkit(ctx context.Context, project *types.Pro defer cancel() w := xprogress.NewPrinter(progressCtx, s.stdout(), os.Stdout, mode) + for k := range opts { + if opts[k].Labels == nil { + opt := opts[k] + opt.Labels = make(map[string]string) + opts[k] = opt + } + opts[k].Labels[api.ImageBuilderLabel] = "buildkit" + } + // We rely on buildx "docker" builder integrated in docker engine, so don't need a DockerAPI here response, err := build.Build(ctx, driverInfo, opts, nil, filepath.Dir(s.configFile().Filename), w) errW := w.Wait() diff --git a/pkg/compose/build_classic.go b/pkg/compose/build_classic.go index e362ae247d..156401db6c 100644 --- a/pkg/compose/build_classic.go +++ b/pkg/compose/build_classic.go @@ -89,6 +89,11 @@ func (s *composeService) doBuildClassicSimpleImage(ctx context.Context, options } } + if options.Labels == nil { + options.Labels = make(map[string]string) + } + options.Labels[api.ImageBuilderLabel] = "classic" + switch { case isLocalDir(specifiedContext): contextDir, relDockerfile, err = build.GetContextFromLocalDir(specifiedContext, dockerfileName) diff --git a/pkg/compose/compose.go b/pkg/compose/compose.go index 603e7057a5..bf9ec3feed 100644 --- a/pkg/compose/compose.go +++ b/pkg/compose/compose.go @@ -30,11 +30,12 @@ import ( "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/config/configfile" "github.com/docker/cli/cli/streams" - "github.com/docker/compose/v2/pkg/api" moby "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/filters" "github.com/docker/docker/client" "github.com/pkg/errors" + + "github.com/docker/compose/v2/pkg/api" ) // NewComposeService create a local implementation of the compose.Service API @@ -130,13 +131,9 @@ func (s *composeService) projectFromName(containers Containers, projectName stri serviceLabel := c.Labels[api.ServiceLabel] _, ok := set[serviceLabel] if !ok { - serviceImage := c.Image - if serviceNameFromLabel, ok := c.Labels[api.ImageNameLabel]; ok { - serviceImage = serviceNameFromLabel - } set[serviceLabel] = &types.ServiceConfig{ Name: serviceLabel, - Image: serviceImage, + Image: c.Image, Labels: c.Labels, } } diff --git a/pkg/compose/down.go b/pkg/compose/down.go index 091480402e..e5aa45a518 100644 --- a/pkg/compose/down.go +++ b/pkg/compose/down.go @@ -23,6 +23,7 @@ import ( "time" "github.com/compose-spec/compose-go/types" + "github.com/distribution/distribution/v3/reference" moby "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/filters" "github.com/docker/docker/errdefs" @@ -31,6 +32,7 @@ import ( "github.com/docker/compose/v2/pkg/api" "github.com/docker/compose/v2/pkg/progress" + "github.com/docker/compose/v2/pkg/utils" ) type downOp func() error @@ -86,7 +88,11 @@ func (s *composeService) down(ctx context.Context, projectName string, options a ops := s.ensureNetworksDown(ctx, project, w) if options.Images != "" { - ops = append(ops, s.ensureImagesDown(ctx, project, options, w)...) + imgOps, err := s.ensureImagesDown(ctx, project, options, w) + if err != nil { + return err + } + ops = append(ops, imgOps...) } if options.Volumes { @@ -118,15 +124,20 @@ func (s *composeService) ensureVolumesDown(ctx context.Context, project *types.P return ops } -func (s *composeService) ensureImagesDown(ctx context.Context, project *types.Project, options api.DownOptions, w progress.Writer) []downOp { +func (s *composeService) ensureImagesDown(ctx context.Context, project *types.Project, options api.DownOptions, w progress.Writer) ([]downOp, error) { + images, err := s.getServiceImagesToRemove(ctx, options, project) + if err != nil { + return nil, err + } + var ops []downOp - for image := range s.getServiceImagesToRemove(options, project) { - image := image + for i := range images { + img := images[i] ops = append(ops, func() error { - return s.removeImage(ctx, image, w) + return s.removeImage(ctx, img, w) }) } - return ops + return ops, nil } func (s *composeService) ensureNetworksDown(ctx context.Context, project *types.Project, w progress.Writer) []downOp { @@ -190,17 +201,108 @@ func (s *composeService) removeNetwork(ctx context.Context, name string, w progr return nil } -func (s *composeService) getServiceImagesToRemove(options api.DownOptions, project *types.Project) map[string]struct{} { - images := map[string]struct{}{} +//nolint:gocyclo +func (s *composeService) getServiceImagesToRemove(ctx context.Context, options api.DownOptions, project *types.Project) ([]string, error) { + if options.Images == "" { + return nil, nil + } + + var localServiceImages []string + var imagesToRemove []string + addImageToRemove := func(img string, checkExistence bool) { + // since some references come from user input (service.image) and some + // come from the engine API, we standardize them, opting for the + // familiar name format since they'll also be displayed in the CLI + ref, err := reference.ParseNormalizedNamed(img) + if err != nil { + return + } + ref = reference.TagNameOnly(ref) + img = reference.FamiliarString(ref) + if utils.StringContains(imagesToRemove, img) { + return + } + + if checkExistence { + _, _, err := s.apiClient().ImageInspectWithRaw(ctx, img) + if errdefs.IsNotFound(err) { + // err on the side of caution: only skip if we successfully + // queried the API and got back a definitive "not exists" + return + } + } + + imagesToRemove = append(imagesToRemove, img) + } + + imageListOpts := moby.ImageListOptions{ + Filters: filters.NewArgs( + projectFilter(project.Name), + // TODO(milas): we should really clean up the dangling images as + // well (historically we have NOT); need to refactor this to handle + // it gracefully without producing confusing CLI output, i.e. we + // do not want to print out a bunch of untagged/dangling image IDs, + // they should be grouped into a logical operation for the relevant + // service + filters.Arg("dangling", "false"), + ), + } + projectImages, err := s.apiClient().ImageList(ctx, imageListOpts) + if err != nil { + return nil, err + } + + // 1. Remote / custom-named images - only deleted on `--rmi="all"` for _, service := range project.Services { - image, ok := service.Labels[api.ImageNameLabel] // Information on the compose file at the creation of the container - if !ok || (options.Images == "local" && image != "") { + if service.Image == "" { + localServiceImages = append(localServiceImages, service.Name) + continue + } + + if options.Images == "all" { + addImageToRemove(service.Image, true) + } + } + + // 2. *LABELED* Locally-built images with implicit image names + // + // If `--remove-orphans` is being used, then ALL images for the project + // will be selected for removal. Otherwise, only those that match a known + // service based on the loaded project will be included. + for _, img := range projectImages { + if len(img.RepoTags) == 0 { + // currently, we're only removing the tagged references, but + // if we start removing the dangling images and grouping by + // service, we can remove this (and should rely on `Image::ID`) + continue + } + + shouldRemove := options.RemoveOrphans + for _, service := range localServiceImages { + if img.Labels[api.ServiceLabel] == service { + shouldRemove = true + break + } + } + + if shouldRemove { + addImageToRemove(img.RepoTags[0], false) + } + } + + // 3. *UNLABELED* Locally-built images with implicit image names + // + // This is a fallback for (2) to handle images built by previous + // versions of Compose, which did not label their built images. + for _, serviceName := range localServiceImages { + service, err := project.GetService(serviceName) + if err != nil || service.Image != "" { continue } - image = api.GetImageNameOrDefault(service, project.Name) - images[image] = struct{}{} + imgName := api.GetImageNameOrDefault(service, project.Name) + addImageToRemove(imgName, true) } - return images + return imagesToRemove, nil } func (s *composeService) removeImage(ctx context.Context, image string, w progress.Writer) error { diff --git a/pkg/compose/down_test.go b/pkg/compose/down_test.go index e5d527fdab..6bbae48113 100644 --- a/pkg/compose/down_test.go +++ b/pkg/compose/down_test.go @@ -18,12 +18,15 @@ package compose import ( "context" + "fmt" "strings" "testing" + "github.com/compose-spec/compose-go/types" moby "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/filters" "github.com/docker/docker/api/types/volume" + "github.com/docker/docker/errdefs" "github.com/golang/mock/gomock" "gotest.tools/v3/assert" @@ -143,38 +146,109 @@ func TestDownRemoveVolumes(t *testing.T) { assert.NilError(t, err) } -func TestDownRemoveImageLocal(t *testing.T) { +func TestDownRemoveImages(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() + opts := compose.DownOptions{ + Project: &types.Project{ + Name: strings.ToLower(testProject), + Services: types.Services{ + {Name: "local-anonymous"}, + {Name: "local-named", Image: "local-named-image"}, + {Name: "remote", Image: "remote-image"}, + {Name: "remote-tagged", Image: "registry.example.com/remote-image-tagged:v1.0"}, + {Name: "no-images-anonymous"}, + {Name: "no-images-named", Image: "missing-named-image"}, + }, + }, + } + api := mocks.NewMockAPIClient(mockCtrl) cli := mocks.NewMockCli(mockCtrl) tested.dockerCli = cli cli.EXPECT().Client().Return(api).AnyTimes() - container := testContainer("service1", "123", false) - container.Labels[compose.ImageNameLabel] = "" - - api.EXPECT().ContainerList(gomock.Any(), projectFilterListOpt(false)).Return( - []moby.Container{container}, nil) - - api.EXPECT().VolumeList(gomock.Any(), filters.NewArgs(projectFilter(strings.ToLower(testProject)))). - Return(volume.VolumeListOKBody{ - Volumes: []*moby.Volume{{Name: "myProject_volume"}}, - }, nil) - api.EXPECT().NetworkList(gomock.Any(), moby.NetworkListOptions{Filters: filters.NewArgs(projectFilter(strings.ToLower(testProject)))}). - Return(nil, nil) - - api.EXPECT().ContainerStop(gomock.Any(), "123", nil).Return(nil) - api.EXPECT().ContainerRemove(gomock.Any(), "123", moby.ContainerRemoveOptions{Force: true}).Return(nil) - - api.EXPECT().ImageRemove(gomock.Any(), "testproject-service1", moby.ImageRemoveOptions{}).Return(nil, nil) + api.EXPECT().ContainerList(gomock.Any(), projectFilterListOpt(false)). + Return([]moby.Container{ + testContainer("service1", "123", false), + }, nil). + AnyTimes() + + api.EXPECT().ImageList(gomock.Any(), moby.ImageListOptions{ + Filters: filters.NewArgs( + projectFilter(strings.ToLower(testProject)), + filters.Arg("dangling", "false"), + ), + }).Return([]moby.ImageSummary{ + { + Labels: types.Labels{compose.ServiceLabel: "local-anonymous"}, + RepoTags: []string{"testproject-local-anonymous:latest"}, + }, + { + Labels: types.Labels{compose.ServiceLabel: "local-named"}, + RepoTags: []string{"local-named-image:latest"}, + }, + }, nil).AnyTimes() + + imagesToBeInspected := map[string]bool{ + "local-named-image:latest": true, + "remote-image:latest": true, + "testproject-no-images-anonymous:latest": false, + "missing-named-image:latest": false, + } + for img, exists := range imagesToBeInspected { + var resp moby.ImageInspect + var err error + if exists { + resp.RepoTags = []string{img} + } else { + err = errdefs.NotFound(fmt.Errorf("test specified that image %q should not exist", img)) + } + + api.EXPECT().ImageInspectWithRaw(gomock.Any(), img). + Return(resp, nil, err). + AnyTimes() + } + + api.EXPECT().ImageInspectWithRaw(gomock.Any(), "registry.example.com/remote-image-tagged:v1.0"). + Return(moby.ImageInspect{RepoTags: []string{"registry.example.com/remote-image-tagged:v1.0"}}, nil, nil). + AnyTimes() + + localImagesToBeRemoved := []string{ + "testproject-local-anonymous:latest", + } + for _, img := range localImagesToBeRemoved { + // test calls down --rmi=local then down --rmi=all, so local images + // get "removed" 2x, while other images are only 1x + api.EXPECT().ImageRemove(gomock.Any(), img, moby.ImageRemoveOptions{}). + Return(nil, nil). + Times(2) + } + + t.Log("-> docker compose down --rmi=local") + opts.Images = "local" + err := tested.Down(context.Background(), strings.ToLower(testProject), opts) + assert.NilError(t, err) - err := tested.Down(context.Background(), strings.ToLower(testProject), compose.DownOptions{Images: "local"}) + otherImagesToBeRemoved := []string{ + "local-named-image:latest", + "remote-image:latest", + "registry.example.com/remote-image-tagged:v1.0", + } + for _, img := range otherImagesToBeRemoved { + api.EXPECT().ImageRemove(gomock.Any(), img, moby.ImageRemoveOptions{}). + Return(nil, nil). + Times(1) + } + + t.Log("-> docker compose down --rmi=all") + opts.Images = "all" + err = tested.Down(context.Background(), strings.ToLower(testProject), opts) assert.NilError(t, err) } -func TestDownRemoveImageLocalNoLabel(t *testing.T) { +func TestDownRemoveImages_NoLabel(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() @@ -195,37 +269,23 @@ func TestDownRemoveImageLocalNoLabel(t *testing.T) { api.EXPECT().NetworkList(gomock.Any(), moby.NetworkListOptions{Filters: filters.NewArgs(projectFilter(strings.ToLower(testProject)))}). Return(nil, nil) - api.EXPECT().ContainerStop(gomock.Any(), "123", nil).Return(nil) - api.EXPECT().ContainerRemove(gomock.Any(), "123", moby.ContainerRemoveOptions{Force: true}).Return(nil) + // ImageList returns no images for the project since they were unlabeled + // (created by an older version of Compose) + api.EXPECT().ImageList(gomock.Any(), moby.ImageListOptions{ + Filters: filters.NewArgs( + projectFilter(strings.ToLower(testProject)), + filters.Arg("dangling", "false"), + ), + }).Return(nil, nil) - err := tested.Down(context.Background(), strings.ToLower(testProject), compose.DownOptions{Images: "local"}) - assert.NilError(t, err) -} - -func TestDownRemoveImageAll(t *testing.T) { - mockCtrl := gomock.NewController(t) - defer mockCtrl.Finish() - - api := mocks.NewMockAPIClient(mockCtrl) - cli := mocks.NewMockCli(mockCtrl) - tested.dockerCli = cli - cli.EXPECT().Client().Return(api).AnyTimes() - - api.EXPECT().ContainerList(gomock.Any(), projectFilterListOpt(false)).Return( - []moby.Container{testContainer("service1", "123", false)}, nil) - - api.EXPECT().VolumeList(gomock.Any(), filters.NewArgs(projectFilter(strings.ToLower(testProject)))). - Return(volume.VolumeListOKBody{ - Volumes: []*moby.Volume{{Name: "myProject_volume"}}, - }, nil) - api.EXPECT().NetworkList(gomock.Any(), moby.NetworkListOptions{Filters: filters.NewArgs(projectFilter(strings.ToLower(testProject)))}). - Return(nil, nil) + api.EXPECT().ImageInspectWithRaw(gomock.Any(), "testproject-service1:latest"). + Return(moby.ImageInspect{}, nil, nil) api.EXPECT().ContainerStop(gomock.Any(), "123", nil).Return(nil) api.EXPECT().ContainerRemove(gomock.Any(), "123", moby.ContainerRemoveOptions{Force: true}).Return(nil) - api.EXPECT().ImageRemove(gomock.Any(), "service1-img", moby.ImageRemoveOptions{}).Return(nil, nil) + api.EXPECT().ImageRemove(gomock.Any(), "testproject-service1:latest", moby.ImageRemoveOptions{}).Return(nil, nil) - err := tested.Down(context.Background(), strings.ToLower(testProject), compose.DownOptions{Images: "all"}) + err := tested.Down(context.Background(), strings.ToLower(testProject), compose.DownOptions{Images: "local"}) assert.NilError(t, err) } diff --git a/pkg/compose/kill_test.go b/pkg/compose/kill_test.go index b5cc8176f8..e5dc27aae5 100644 --- a/pkg/compose/kill_test.go +++ b/pkg/compose/kill_test.go @@ -109,7 +109,6 @@ func containerLabels(service string, oneOff bool) map[string]string { composefile := filepath.Join(workingdir, "compose.yaml") labels := map[string]string{ compose.ServiceLabel: service, - compose.ImageNameLabel: service + "-img", compose.ConfigFilesLabel: composefile, compose.WorkingDirLabel: workingdir, compose.ProjectLabel: strings.ToLower(testProject)} diff --git a/pkg/e2e/fixtures/build-dependencies/compose.yaml b/pkg/e2e/fixtures/build-dependencies/compose.yaml index 7de1960b4e..c974b72413 100644 --- a/pkg/e2e/fixtures/build-dependencies/compose.yaml +++ b/pkg/e2e/fixtures/build-dependencies/compose.yaml @@ -10,3 +10,5 @@ services: build: context: . dockerfile: service.dockerfile + nginx: + image: nginx From 680763f8b7d7ff9794052a4a297108693c7c50dd Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Tue, 13 Sep 2022 14:39:51 +0100 Subject: [PATCH 2/4] down: refactor image pruning Signed-off-by: Milas Bowman --- pkg/compose/down.go | 113 ++------------------ pkg/compose/down_test.go | 11 +- pkg/compose/image_pruner.go | 202 ++++++++++++++++++++++++++++++++++++ pkg/e2e/build_test.go | 14 +++ 4 files changed, 228 insertions(+), 112 deletions(-) create mode 100644 pkg/compose/image_pruner.go diff --git a/pkg/compose/down.go b/pkg/compose/down.go index e5aa45a518..bdcc4ca91a 100644 --- a/pkg/compose/down.go +++ b/pkg/compose/down.go @@ -23,7 +23,6 @@ import ( "time" "github.com/compose-spec/compose-go/types" - "github.com/distribution/distribution/v3/reference" moby "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/filters" "github.com/docker/docker/errdefs" @@ -32,7 +31,6 @@ import ( "github.com/docker/compose/v2/pkg/api" "github.com/docker/compose/v2/pkg/progress" - "github.com/docker/compose/v2/pkg/utils" ) type downOp func() error @@ -125,7 +123,12 @@ func (s *composeService) ensureVolumesDown(ctx context.Context, project *types.P } func (s *composeService) ensureImagesDown(ctx context.Context, project *types.Project, options api.DownOptions, w progress.Writer) ([]downOp, error) { - images, err := s.getServiceImagesToRemove(ctx, options, project) + imagePruner := NewImagePruner(s.apiClient(), project) + pruneOpts := ImagePruneOptions{ + Mode: ImagePruneMode(options.Images), + RemoveOrphans: options.RemoveOrphans, + } + images, err := imagePruner.ImagesToPrune(ctx, pruneOpts) if err != nil { return nil, err } @@ -201,110 +204,6 @@ func (s *composeService) removeNetwork(ctx context.Context, name string, w progr return nil } -//nolint:gocyclo -func (s *composeService) getServiceImagesToRemove(ctx context.Context, options api.DownOptions, project *types.Project) ([]string, error) { - if options.Images == "" { - return nil, nil - } - - var localServiceImages []string - var imagesToRemove []string - addImageToRemove := func(img string, checkExistence bool) { - // since some references come from user input (service.image) and some - // come from the engine API, we standardize them, opting for the - // familiar name format since they'll also be displayed in the CLI - ref, err := reference.ParseNormalizedNamed(img) - if err != nil { - return - } - ref = reference.TagNameOnly(ref) - img = reference.FamiliarString(ref) - if utils.StringContains(imagesToRemove, img) { - return - } - - if checkExistence { - _, _, err := s.apiClient().ImageInspectWithRaw(ctx, img) - if errdefs.IsNotFound(err) { - // err on the side of caution: only skip if we successfully - // queried the API and got back a definitive "not exists" - return - } - } - - imagesToRemove = append(imagesToRemove, img) - } - - imageListOpts := moby.ImageListOptions{ - Filters: filters.NewArgs( - projectFilter(project.Name), - // TODO(milas): we should really clean up the dangling images as - // well (historically we have NOT); need to refactor this to handle - // it gracefully without producing confusing CLI output, i.e. we - // do not want to print out a bunch of untagged/dangling image IDs, - // they should be grouped into a logical operation for the relevant - // service - filters.Arg("dangling", "false"), - ), - } - projectImages, err := s.apiClient().ImageList(ctx, imageListOpts) - if err != nil { - return nil, err - } - - // 1. Remote / custom-named images - only deleted on `--rmi="all"` - for _, service := range project.Services { - if service.Image == "" { - localServiceImages = append(localServiceImages, service.Name) - continue - } - - if options.Images == "all" { - addImageToRemove(service.Image, true) - } - } - - // 2. *LABELED* Locally-built images with implicit image names - // - // If `--remove-orphans` is being used, then ALL images for the project - // will be selected for removal. Otherwise, only those that match a known - // service based on the loaded project will be included. - for _, img := range projectImages { - if len(img.RepoTags) == 0 { - // currently, we're only removing the tagged references, but - // if we start removing the dangling images and grouping by - // service, we can remove this (and should rely on `Image::ID`) - continue - } - - shouldRemove := options.RemoveOrphans - for _, service := range localServiceImages { - if img.Labels[api.ServiceLabel] == service { - shouldRemove = true - break - } - } - - if shouldRemove { - addImageToRemove(img.RepoTags[0], false) - } - } - - // 3. *UNLABELED* Locally-built images with implicit image names - // - // This is a fallback for (2) to handle images built by previous - // versions of Compose, which did not label their built images. - for _, serviceName := range localServiceImages { - service, err := project.GetService(serviceName) - if err != nil || service.Image != "" { - continue - } - imgName := api.GetImageNameOrDefault(service, project.Name) - addImageToRemove(imgName, true) - } - return imagesToRemove, nil -} - func (s *composeService) removeImage(ctx context.Context, image string, w progress.Writer) error { id := fmt.Sprintf("Image %s", image) w.Event(progress.NewEvent(id, progress.Working, "Removing")) diff --git a/pkg/compose/down_test.go b/pkg/compose/down_test.go index 6bbae48113..08e4548cc4 100644 --- a/pkg/compose/down_test.go +++ b/pkg/compose/down_test.go @@ -192,10 +192,11 @@ func TestDownRemoveImages(t *testing.T) { }, nil).AnyTimes() imagesToBeInspected := map[string]bool{ - "local-named-image:latest": true, - "remote-image:latest": true, - "testproject-no-images-anonymous:latest": false, - "missing-named-image:latest": false, + "testproject-local-anonymous": true, + "local-named-image": true, + "remote-image": true, + "testproject-no-images-anonymous": false, + "missing-named-image": false, } for img, exists := range imagesToBeInspected { var resp moby.ImageInspect @@ -278,7 +279,7 @@ func TestDownRemoveImages_NoLabel(t *testing.T) { ), }).Return(nil, nil) - api.EXPECT().ImageInspectWithRaw(gomock.Any(), "testproject-service1:latest"). + api.EXPECT().ImageInspectWithRaw(gomock.Any(), "testproject-service1"). Return(moby.ImageInspect{}, nil, nil) api.EXPECT().ContainerStop(gomock.Any(), "123", nil).Return(nil) diff --git a/pkg/compose/image_pruner.go b/pkg/compose/image_pruner.go new file mode 100644 index 0000000000..0d332fa827 --- /dev/null +++ b/pkg/compose/image_pruner.go @@ -0,0 +1,202 @@ +package compose + +import ( + "context" + "fmt" + "sort" + "sync" + + "github.com/compose-spec/compose-go/types" + "github.com/distribution/distribution/v3/reference" + moby "github.com/docker/docker/api/types" + "github.com/docker/docker/api/types/filters" + "github.com/docker/docker/client" + "github.com/docker/docker/errdefs" + "golang.org/x/sync/errgroup" + + "github.com/docker/compose/v2/pkg/api" +) + +// ImagePruneMode controls how aggressively images associated with the project +// are removed from the engine. +type ImagePruneMode string + +const ( + // ImagePruneNone indicates that no project images should be removed. + ImagePruneNone ImagePruneMode = "" + // ImagePruneLocal indicates that only images built locally by Compose + // should be removed. + ImagePruneLocal ImagePruneMode = "local" + // ImagePruneAll indicates that all project-associated images, including + // remote images should be removed. + ImagePruneAll ImagePruneMode = "all" +) + +// ImagePruneOptions controls the behavior of image pruning. +type ImagePruneOptions struct { + Mode ImagePruneMode + + // RemoveOrphans will result in the removal of images that were built for + // the project regardless of whether they are for a known service if true. + RemoveOrphans bool +} + +// ImagePruner handles image removal during Compose `down` operations. +type ImagePruner struct { + client client.ImageAPIClient + project *types.Project +} + +// NewImagePruner creates an ImagePruner object for a project. +func NewImagePruner(imageClient client.ImageAPIClient, project *types.Project) *ImagePruner { + return &ImagePruner{ + client: imageClient, + project: project, + } +} + +// ImagesToPrune returns the set of images that should be removed. +func (p *ImagePruner) ImagesToPrune(ctx context.Context, opts ImagePruneOptions) ([]string, error) { + if opts.Mode == ImagePruneNone { + return nil, nil + } else if opts.Mode != ImagePruneLocal && opts.Mode != ImagePruneAll { + return nil, fmt.Errorf("unsupported image prune mode: %s", opts.Mode) + } + + var images []string + + if opts.Mode == ImagePruneAll { + namedImages, err := p.namedImages(ctx) + if err != nil { + return nil, err + } + images = append(images, namedImages...) + } + + projectImages, err := p.builtImagesForProject(ctx) + if err != nil { + return nil, err + } + + for _, img := range projectImages { + if len(img.RepoTags) == 0 { + // currently, we're only removing the tagged references, but + // if we start removing the dangling images and grouping by + // service, we can remove this (and should rely on `Image::ID`) + continue + } + + removeImage := opts.RemoveOrphans + if !removeImage { + service, err := p.project.GetService(img.Labels[api.ServiceLabel]) + if err == nil && (opts.Mode == ImagePruneAll || service.Image == "") { + removeImage = true + } + } + + if removeImage { + images = append(images, img.RepoTags[0]) + } + } + + fallbackImages, err := p.unlabeledLocalImages(ctx) + if err != nil { + return nil, err + } + images = append(images, fallbackImages...) + + images = normalizeAndDedupeImages(images) + return images, nil +} + +func (p *ImagePruner) builtImagesForProject(ctx context.Context) ([]moby.ImageSummary, error) { + imageListOpts := moby.ImageListOptions{ + Filters: filters.NewArgs( + projectFilter(p.project.Name), + // TODO(milas): we should really clean up the dangling images as + // well (historically we have NOT); need to refactor this to handle + // it gracefully without producing confusing CLI output, i.e. we + // do not want to print out a bunch of untagged/dangling image IDs, + // they should be grouped into a logical operation for the relevant + // service + filters.Arg("dangling", "false"), + ), + } + projectImages, err := p.client.ImageList(ctx, imageListOpts) + if err != nil { + return nil, err + } + return projectImages, nil +} + +func (p *ImagePruner) namedImages(ctx context.Context) ([]string, error) { + var images []string + for _, service := range p.project.Services { + if service.Image == "" { + continue + } + images = append(images, service.Image) + } + return p.filterImagesByExistence(ctx, images) +} + +func (p *ImagePruner) filterImagesByExistence(ctx context.Context, imageNames []string) ([]string, error) { + var mu sync.Mutex + var ret []string + + eg, ctx := errgroup.WithContext(ctx) + for _, img := range imageNames { + img := img + eg.Go(func() error { + _, _, err := p.client.ImageInspectWithRaw(ctx, img) + if errdefs.IsNotFound(err) { + // err on the side of caution: only skip if we successfully + // queried the API and got back a definitive "not exists" + return nil + } + mu.Lock() + defer mu.Unlock() + ret = append(ret, img) + return nil + }) + } + + if err := eg.Wait(); err != nil { + return nil, err + } + + return ret, nil +} + +func (p *ImagePruner) unlabeledLocalImages(ctx context.Context) ([]string, error) { + var images []string + for _, service := range p.project.Services { + if service.Image != "" { + continue + } + img := api.GetImageNameOrDefault(service, p.project.Name) + images = append(images, img) + } + return p.filterImagesByExistence(ctx, images) +} + +func normalizeAndDedupeImages(images []string) []string { + seen := make(map[string]struct{}, len(images)) + for _, img := range images { + // since some references come from user input (service.image) and some + // come from the engine API, we standardize them, opting for the + // familiar name format since they'll also be displayed in the CLI + ref, err := reference.ParseNormalizedNamed(img) + if err == nil { + ref = reference.TagNameOnly(ref) + img = reference.FamiliarString(ref) + } + seen[img] = struct{}{} + } + ret := make([]string, 0, len(seen)) + for v := range seen { + ret = append(ret, v) + } + sort.Strings(ret) + return ret +} diff --git a/pkg/e2e/build_test.go b/pkg/e2e/build_test.go index 178ed7f9b5..706c2f8819 100644 --- a/pkg/e2e/build_test.go +++ b/pkg/e2e/build_test.go @@ -22,6 +22,7 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" "gotest.tools/v3/assert" "gotest.tools/v3/icmd" ) @@ -211,6 +212,10 @@ func TestBuildImageDependencies(t *testing.T) { doTest := func(t *testing.T, cli *CLI) { resetState := func() { cli.RunDockerComposeCmd(t, "down", "--rmi=all", "-t=0") + res := cli.RunDockerOrExitError(t, "image", "rm", "build-dependencies-service") + if res.Error != nil { + require.Contains(t, res.Stderr(), `Error: No such image: build-dependencies-service`) + } } resetState() t.Cleanup(resetState) @@ -229,6 +234,15 @@ func TestBuildImageDependencies(t *testing.T) { "image", "inspect", "--format={{ index .RepoTags 0 }}", "build-dependencies-service") res.Assert(t, icmd.Expected{Out: "build-dependencies-service:latest"}) + + res = cli.RunDockerComposeCmd(t, "down", "-t0", "--rmi=all", "--remove-orphans") + t.Log(res.Combined()) + + res = cli.RunDockerOrExitError(t, "image", "inspect", "build-dependencies-service") + res.Assert(t, icmd.Expected{ + ExitCode: 1, + Err: "Error: No such image: build-dependencies-service", + }) } t.Run("ClassicBuilder", func(t *testing.T) { From 403d691abfff629a40fc4b4c6c5dda51a4f3c31a Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Tue, 13 Sep 2022 18:27:31 +0100 Subject: [PATCH 3/4] small cleanup + godoc Signed-off-by: Milas Bowman --- pkg/compose/image_pruner.go | 84 +++++++++++++------ .../fixtures/build-dependencies/compose.yaml | 2 - 2 files changed, 60 insertions(+), 26 deletions(-) diff --git a/pkg/compose/image_pruner.go b/pkg/compose/image_pruner.go index 0d332fa827..6babb43f62 100644 --- a/pkg/compose/image_pruner.go +++ b/pkg/compose/image_pruner.go @@ -62,7 +62,6 @@ func (p *ImagePruner) ImagesToPrune(ctx context.Context, opts ImagePruneOptions) } else if opts.Mode != ImagePruneLocal && opts.Mode != ImagePruneAll { return nil, fmt.Errorf("unsupported image prune mode: %s", opts.Mode) } - var images []string if opts.Mode == ImagePruneAll { @@ -73,28 +72,38 @@ func (p *ImagePruner) ImagesToPrune(ctx context.Context, opts ImagePruneOptions) images = append(images, namedImages...) } - projectImages, err := p.builtImagesForProject(ctx) + projectImages, err := p.labeledLocalImages(ctx) if err != nil { return nil, err } - for _, img := range projectImages { if len(img.RepoTags) == 0 { - // currently, we're only removing the tagged references, but + // currently, we're only pruning the tagged references, but // if we start removing the dangling images and grouping by // service, we can remove this (and should rely on `Image::ID`) continue } - removeImage := opts.RemoveOrphans - if !removeImage { + var shouldPrune bool + if opts.RemoveOrphans { + // indiscriminately prune all project images even if they're not + // referenced by the current Compose state (e.g. the service was + // removed from YAML) + shouldPrune = true + } else { + // only prune the image if it belongs to a known service for the + // project AND is either an implicitly-named, locally-built image + // or `--rmi=all` has been specified. + // TODO(milas): now that Compose labels the images it builds, this + // makes less sense; arguably, locally-built but explicitly-named + // images should be removed with `--rmi=local` as well. service, err := p.project.GetService(img.Labels[api.ServiceLabel]) if err == nil && (opts.Mode == ImagePruneAll || service.Image == "") { - removeImage = true + shouldPrune = true } } - if removeImage { + if shouldPrune { images = append(images, img.RepoTags[0]) } } @@ -109,7 +118,28 @@ func (p *ImagePruner) ImagesToPrune(ctx context.Context, opts ImagePruneOptions) return images, nil } -func (p *ImagePruner) builtImagesForProject(ctx context.Context) ([]moby.ImageSummary, error) { +// namedImages are those that are explicitly named in the service config. +// +// These could be registry-only images (no local build), hybrid (support build +// as a fallback if cannot pull), or local-only (image does not exist in a +// registry). +func (p *ImagePruner) namedImages(ctx context.Context) ([]string, error) { + var images []string + for _, service := range p.project.Services { + if service.Image == "" { + continue + } + images = append(images, service.Image) + } + return p.filterImagesByExistence(ctx, images) +} + +// labeledLocalImages are images that were locally-built by a current version of +// Compose (it did not always label built images). +// +// The image name could either have been defined by the user or implicitly +// created from the project + service name. +func (p *ImagePruner) labeledLocalImages(ctx context.Context) ([]moby.ImageSummary, error) { imageListOpts := moby.ImageListOptions{ Filters: filters.NewArgs( projectFilter(p.project.Name), @@ -129,17 +159,33 @@ func (p *ImagePruner) builtImagesForProject(ctx context.Context) ([]moby.ImageSu return projectImages, nil } -func (p *ImagePruner) namedImages(ctx context.Context) ([]string, error) { +// unlabeledLocalImages are images that match the implicit naming convention +// for locally-built images but did not get labeled, presumably because they +// were produced by an older version of Compose. +// +// This is transitional to ensure `down` continues to work as expected on +// projects built/launched by previous versions of Compose. It can safely +// be removed after some time. +func (p *ImagePruner) unlabeledLocalImages(ctx context.Context) ([]string, error) { var images []string for _, service := range p.project.Services { - if service.Image == "" { + if service.Image != "" { continue } - images = append(images, service.Image) + img := api.GetImageNameOrDefault(service, p.project.Name) + images = append(images, img) } return p.filterImagesByExistence(ctx, images) } +// filterImagesByExistence returns the subset of images that exist in the +// engine store. +// +// NOTE: Any transient errors communicating with the API will result in an +// image being returned as "existing", as this method is exclusively used to +// find images to remove, so the worst case of being conservative here is an +// attempt to remove an image that doesn't exist, which will cause a warning +// but is otherwise harmless. func (p *ImagePruner) filterImagesByExistence(ctx context.Context, imageNames []string) ([]string, error) { var mu sync.Mutex var ret []string @@ -168,18 +214,7 @@ func (p *ImagePruner) filterImagesByExistence(ctx context.Context, imageNames [] return ret, nil } -func (p *ImagePruner) unlabeledLocalImages(ctx context.Context) ([]string, error) { - var images []string - for _, service := range p.project.Services { - if service.Image != "" { - continue - } - img := api.GetImageNameOrDefault(service, p.project.Name) - images = append(images, img) - } - return p.filterImagesByExistence(ctx, images) -} - +// normalizeAndDedupeImages returns the unique set of images after normalization. func normalizeAndDedupeImages(images []string) []string { seen := make(map[string]struct{}, len(images)) for _, img := range images { @@ -197,6 +232,7 @@ func normalizeAndDedupeImages(images []string) []string { for v := range seen { ret = append(ret, v) } + // ensure a deterministic return result - the actual ordering is not useful sort.Strings(ret) return ret } diff --git a/pkg/e2e/fixtures/build-dependencies/compose.yaml b/pkg/e2e/fixtures/build-dependencies/compose.yaml index c974b72413..7de1960b4e 100644 --- a/pkg/e2e/fixtures/build-dependencies/compose.yaml +++ b/pkg/e2e/fixtures/build-dependencies/compose.yaml @@ -10,5 +10,3 @@ services: build: context: . dockerfile: service.dockerfile - nginx: - image: nginx From 801678686ca423414b8d63e4b8bbb95f63864874 Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Tue, 13 Sep 2022 18:29:03 +0100 Subject: [PATCH 4/4] add license to file Signed-off-by: Milas Bowman --- pkg/compose/image_pruner.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/pkg/compose/image_pruner.go b/pkg/compose/image_pruner.go index 6babb43f62..e828b46a02 100644 --- a/pkg/compose/image_pruner.go +++ b/pkg/compose/image_pruner.go @@ -1,3 +1,19 @@ +/* + Copyright 2022 Docker Compose CLI authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + package compose import (