From bc806da71258b11832bbcffddcd8a3b219bff904 Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Wed, 7 Sep 2022 16:24:47 -0400 Subject: [PATCH] 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 | 148 ++++++++++++------ pkg/compose/kill_test.go | 1 - .../fixtures/build-dependencies/compose.yaml | 2 + 9 files changed, 260 insertions(+), 71 deletions(-) diff --git a/pkg/api/labels.go b/pkg/api/labels.go index 8e0a8604..4352d9c0 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 5ab257dd..1b9b1aaa 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 d4120ced..6ed66a21 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 e362ae24..156401db 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 603e7057..bf9ec3fe 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 09148040..e5aa45a5 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 } - image = api.GetImageNameOrDefault(service, project.Name) - images[image] = struct{}{} + + if options.Images == "all" { + addImageToRemove(service.Image, true) + } } - return images + + // 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 { diff --git a/pkg/compose/down_test.go b/pkg/compose/down_test.go index e5d527fd..6bbae481 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{ + testContainer("service1", "123", false), + }, nil). + AnyTimes() - api.EXPECT().ContainerList(gomock.Any(), projectFilterListOpt(false)).Return( - []moby.Container{container}, nil) + 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() - 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) + 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().ContainerStop(gomock.Any(), "123", nil).Return(nil) - api.EXPECT().ContainerRemove(gomock.Any(), "123", moby.ContainerRemoveOptions{Force: true}).Return(nil) + api.EXPECT().ImageInspectWithRaw(gomock.Any(), img). + Return(resp, nil, err). + AnyTimes() + } - api.EXPECT().ImageRemove(gomock.Any(), "testproject-service1", moby.ImageRemoveOptions{}).Return(nil, nil) + 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() - err := tested.Down(context.Background(), strings.ToLower(testProject), compose.DownOptions{Images: "local"}) + 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) + + 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) + // 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) + + 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(), "testproject-service1:latest", moby.ImageRemoveOptions{}).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().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) - - err := tested.Down(context.Background(), strings.ToLower(testProject), compose.DownOptions{Images: "all"}) - assert.NilError(t, err) -} diff --git a/pkg/compose/kill_test.go b/pkg/compose/kill_test.go index b5cc8176..e5dc27aa 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 7de1960b..c974b724 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