From a281be1c11d61deed7cb2853125e6d489f16aa2e Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Thu, 16 Jun 2016 16:06:47 -0700 Subject: [PATCH 1/2] Update rmi logic for canonical references Updates the rmi code to treat canonical references as related to tagged references from the same repository during deletion. Canonical references with a different repository name will be treated as separate references. Updates the remove by ID logic to still remove an image if there is a single tag reference and only canonical references to the same repository remaining. Signed-off-by: Derek McGowan (github: dmcgowan) --- daemon/image_delete.go | 85 +++++++++++++++++++++++++++++------------- 1 file changed, 59 insertions(+), 26 deletions(-) diff --git a/daemon/image_delete.go b/daemon/image_delete.go index 7c6329a669..c1af38bf7f 100644 --- a/daemon/image_delete.go +++ b/daemon/image_delete.go @@ -104,27 +104,34 @@ func (daemon *Daemon) ImageDelete(imageRef string, force, prune bool) ([]types.I repoRefs = daemon.referenceStore.References(imgID) - // If this is a tag reference and all the remaining references - // to this image are digest references, delete the remaining - // references so that they don't prevent removal of the image. + // If a tag reference was removed and the only remaining + // references to the same repository are digest references, + // then clean up those digest references. if _, isCanonical := parsedRef.(reference.Canonical); !isCanonical { - foundTagRef := false + foundRepoTagRef := false for _, repoRef := range repoRefs { - if _, repoRefIsCanonical := repoRef.(reference.Canonical); !repoRefIsCanonical { - foundTagRef = true + if _, repoRefIsCanonical := repoRef.(reference.Canonical); !repoRefIsCanonical && parsedRef.Name() == repoRef.Name() { + foundRepoTagRef = true break } } - if !foundTagRef { + if !foundRepoTagRef { + // Remove canonical references from same repository + remainingRefs := []reference.Named{} for _, repoRef := range repoRefs { - if _, err := daemon.removeImageRef(repoRef); err != nil { - return records, err - } + if _, repoRefIsCanonical := repoRef.(reference.Canonical); repoRefIsCanonical && parsedRef.Name() == repoRef.Name() { + if _, err := daemon.removeImageRef(repoRef); err != nil { + return records, err + } - untaggedRecord := types.ImageDelete{Untagged: repoRef.String()} - records = append(records, untaggedRecord) + untaggedRecord := types.ImageDelete{Untagged: repoRef.String()} + records = append(records, untaggedRecord) + } else { + remainingRefs = append(remainingRefs, repoRef) + + } } - repoRefs = []reference.Named{} + repoRefs = remainingRefs } } @@ -135,11 +142,10 @@ func (daemon *Daemon) ImageDelete(imageRef string, force, prune bool) ([]types.I removedRepositoryRef = true } else { - // If an ID reference was given AND there is exactly one - // repository reference to the image then we will want to - // remove that reference. - // FIXME: Is this the behavior we want? - if len(repoRefs) == 1 { + // If an ID reference was given AND there is at most one tag + // reference to the image AND all references are within one + // repository, then remove all references. + if isSingleReference(repoRefs) { c := conflictHard if !force { c |= conflictSoft &^ conflictActiveReference @@ -148,21 +154,48 @@ func (daemon *Daemon) ImageDelete(imageRef string, force, prune bool) ([]types.I return nil, conflict } - parsedRef, err := daemon.removeImageRef(repoRefs[0]) - if err != nil { - return nil, err + for _, repoRef := range repoRefs { + parsedRef, err := daemon.removeImageRef(repoRef) + if err != nil { + return nil, err + } + + untaggedRecord := types.ImageDelete{Untagged: parsedRef.String()} + + daemon.LogImageEvent(imgID.String(), imgID.String(), "untag") + records = append(records, untaggedRecord) } - - untaggedRecord := types.ImageDelete{Untagged: parsedRef.String()} - - daemon.LogImageEvent(imgID.String(), imgID.String(), "untag") - records = append(records, untaggedRecord) } } return records, daemon.imageDeleteHelper(imgID, &records, force, prune, removedRepositoryRef) } +// isSingleReference returns true when all references are from one repository +// and there is at most one tag. Returns false for empty input. +func isSingleReference(repoRefs []reference.Named) bool { + if len(repoRefs) <= 1 { + return len(repoRefs) == 1 + } + var singleRef reference.Named + canonicalRefs := map[string]struct{}{} + for _, repoRef := range repoRefs { + if _, isCanonical := repoRef.(reference.Canonical); isCanonical { + canonicalRefs[repoRef.Name()] = struct{}{} + } else if singleRef == nil { + singleRef = repoRef + } else { + return false + } + } + if singleRef == nil { + // Just use first canonical ref + singleRef = repoRefs[0] + } + _, ok := canonicalRefs[singleRef.Name()] + return len(canonicalRefs) == 1 && ok +} + // isImageIDPrefix returns whether the given possiblePrefix is a prefix of the // given imageID. func isImageIDPrefix(imageID, possiblePrefix string) bool { From 5cff374b140b4a836b7082d009bcfe9a6e96f1af Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Fri, 17 Jun 2016 09:18:27 -0700 Subject: [PATCH 2/2] Add tests for rmi Add integration test for removing by image id with tag and digest reference to the same repository. Add integration test to ensure only tag to other repository remains after deleting tag with accompanying digest reference. Signed-off-by: Derek McGowan (github: dmcgowan) --- integration-cli/docker_cli_by_digest_test.go | 39 ++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/integration-cli/docker_cli_by_digest_test.go b/integration-cli/docker_cli_by_digest_test.go index 2f71d0f103..b89c76cce3 100644 --- a/integration-cli/docker_cli_by_digest_test.go +++ b/integration-cli/docker_cli_by_digest_test.go @@ -380,9 +380,14 @@ func (s *DockerRegistrySuite) TestDeleteImageByIDOnlyPulledByDigest(c *check.C) dockerCmd(c, "pull", imageReference) // just in case... + dockerCmd(c, "tag", imageReference, repoName+":sometag") + imageID := inspectField(c, imageReference, "Id") dockerCmd(c, "rmi", imageID) + + _, err = inspectFieldWithError(imageID, "Id") + c.Assert(err, checker.NotNil, check.Commentf("image should have been deleted")) } func (s *DockerRegistrySuite) TestDeleteImageWithDigestAndTag(c *check.C) { @@ -412,6 +417,40 @@ func (s *DockerRegistrySuite) TestDeleteImageWithDigestAndTag(c *check.C) { c.Assert(err, checker.NotNil, check.Commentf("image should have been deleted")) } +func (s *DockerRegistrySuite) TestDeleteImageWithDigestAndMultiRepoTag(c *check.C) { + pushDigest, err := setupImage(c) + c.Assert(err, checker.IsNil, check.Commentf("error setting up image")) + + repo2 := fmt.Sprintf("%s/%s", repoName, "repo2") + + // pull from the registry using the @ reference + imageReference := fmt.Sprintf("%s@%s", repoName, pushDigest) + dockerCmd(c, "pull", imageReference) + + imageID := inspectField(c, imageReference, "Id") + + repoTag := repoName + ":sometag" + repoTag2 := repo2 + ":othertag" + dockerCmd(c, "tag", imageReference, repoTag) + dockerCmd(c, "tag", imageReference, repoTag2) + + dockerCmd(c, "rmi", repoTag) + + // rmi should have deleted repoTag and image reference, but left repoTag2 + inspectField(c, repoTag2, "Id") + _, err = inspectFieldWithError(imageReference, "Id") + c.Assert(err, checker.NotNil, check.Commentf("image digest reference should have been removed")) + + _, err = inspectFieldWithError(repoTag, "Id") + c.Assert(err, checker.NotNil, check.Commentf("image tag reference should have been removed")) + + dockerCmd(c, "rmi", repoTag2) + + // rmi should have deleted the tag, the digest reference, and the image itself + _, err = inspectFieldWithError(imageID, "Id") + c.Assert(err, checker.NotNil, check.Commentf("image should have been deleted")) +} + // TestPullFailsWithAlteredManifest tests that a `docker pull` fails when // we have modified a manifest blob and its digest cannot be verified. // This is the schema2 version of the test.