diff --git a/daemon/mgr/cri.go b/daemon/mgr/cri.go index 0781d852ee..05fe630f3c 100644 --- a/daemon/mgr/cri.go +++ b/daemon/mgr/cri.go @@ -765,7 +765,12 @@ func (c *CriManager) ListImages(ctx context.Context, r *runtime.ListImagesReques images := make([]*runtime.Image, 0, len(imageList)) for _, i := range imageList { - image, err := imageToCriImage(&i) + // NOTE: we should query image cache to get the correct image info. + imageInfo, err := c.ImageMgr.GetImage(ctx, strings.TrimPrefix(i.ID, "sha256:")) + if err != nil { + continue + } + image, err := imageToCriImage(imageInfo) if err != nil { // TODO: log an error message? continue @@ -826,19 +831,15 @@ func (c *CriManager) PullImage(ctx context.Context, r *runtime.PullImageRequest) // RemoveImage removes the image. func (c *CriManager) RemoveImage(ctx context.Context, r *runtime.RemoveImageRequest) (*runtime.RemoveImageResponse, error) { imageRef := r.GetImage().GetImage() - ref, err := reference.Parse(imageRef) - if err != nil { - return nil, err - } - imageInfo, err := c.ImageMgr.GetImage(ctx, strings.TrimPrefix(ref.String(), "sha256:")) + imageInfo, err := c.ImageMgr.GetImage(ctx, strings.TrimPrefix(imageRef, "sha256:")) if err != nil { // TODO: seperate ErrImageNotFound with others. // Now we just return empty if the error occured. return &runtime.RemoveImageResponse{}, nil } - err = c.ImageMgr.RemoveImage(ctx, imageInfo, strings.TrimPrefix(ref.String(), "sha256:"), &ImageRemoveOption{}) + err = c.ImageMgr.RemoveImage(ctx, imageInfo, strings.TrimPrefix(imageRef, "sha256:"), &ImageRemoveOption{}) if err != nil { return nil, err } diff --git a/daemon/mgr/image.go b/daemon/mgr/image.go index 263aa0c0b4..649f38697b 100644 --- a/daemon/mgr/image.go +++ b/daemon/mgr/image.go @@ -132,13 +132,8 @@ func (mgr *ImageManager) RemoveImage(ctx context.Context, image *types.ImageInfo // name is image ID if strings.HasPrefix(strings.TrimPrefix(image.ID, "sha256:"), name) { refs := mgr.cache.getIDToRefs(image.ID) - for _, ref := range refs { - if err := mgr.client.RemoveImage(ctx, ref); err != nil { - return err - } - } mgr.cache.remove(image) - return nil + return mgr.client.RemoveImage(ctx, refs[0]) } // name is tagged or digest @@ -151,14 +146,15 @@ func (mgr *ImageManager) RemoveImage(ctx context.Context, image *types.ImageInfo var ref string if refDigest, ok := refNamed.(reference.Digested); ok { ref = refDigest.String() - } else if refTagged, ok := reference.WithDefaultTagIfMissing(refNamed).(reference.Tagged); ok { + } + if refTagged, ok := refNamed.(reference.Tagged); ok { ref = refTagged.String() } if err := mgr.client.RemoveImage(ctx, ref); err != nil { return err } - mgr.cache.untagged(ref) + mgr.cache.untagged(refNamed) return nil } @@ -183,6 +179,7 @@ type imageCache struct { idToDigests map[string]map[string]bool // store repoDigests by id, the repoDigest is ref if bool is true. refToID map[string]string // store refs by id. tagToDigest map[string]string // store the mapping repoTag to repoDigest. + digestToTag map[string]string // store the mapping repoDigest to repoTag, } func newImageCache() *imageCache { @@ -192,6 +189,7 @@ func newImageCache() *imageCache { idToDigests: make(map[string]map[string]bool), refToID: make(map[string]string), tagToDigest: make(map[string]string), + digestToTag: make(map[string]string), } } @@ -204,40 +202,26 @@ func (c *imageCache) put(image *types.ImageInfo) { repoDigests := image.RepoDigests repoTags := image.RepoTags + // NOTE: actually we simplify something, we assume that + // tag and digest are one-to-one mapping and we can only + // atmost one tag and one digest at a time. if len(repoTags) > 0 { - for _, repoTag := range repoTags { - if len(c.idToTags[id]) == 0 { - tag := make(map[string]bool) - tag[repoTag] = true - c.idToTags[id] = tag - } else { - c.idToTags[id][repoTag] = true - } - - if len(c.idToDigests[id]) == 0 { - digest := make(map[string]bool) - digest[repoDigests[0]] = false - c.idToDigests[id] = digest - } else { - c.idToDigests[id][repoDigests[0]] = false - } - - c.tagToDigest[repoTag] = repoDigests[0] - c.refToID[repoTag] = id - } - } else { - if len(c.idToDigests[id]) == 0 { - digest := make(map[string]bool) - digest[repoDigests[0]] = true - c.idToDigests[id] = digest - } else { - c.idToDigests[id][repoDigests[0]] = true + // Pull with TagRef. + if c.idToTags[id] == nil { + c.idToTags[id] = make(map[string]bool) } + c.idToTags[id][repoTags[0]] = true + c.tagToDigest[repoTags[0]] = repoDigests[0] + c.digestToTag[repoDigests[0]] = repoTags[0] + c.refToID[repoTags[0]] = id + } - c.refToID[repoDigests[0]] = id + if c.idToDigests[id] == nil { + c.idToDigests[id] = make(map[string]bool) } + c.idToDigests[id][repoDigests[0]] = true + c.refToID[repoDigests[0]] = id - // get repoTags and repoDigest from idToTags and idToDigests if item := c.ids.Get(patricia.Prefix(id)); item != nil { repoTags := []string{} repoDigests := []string{} @@ -248,6 +232,7 @@ func (c *imageCache) put(image *types.ImageInfo) { repoDigests = append(repoDigests, digest) } + // Reset the image's RepoTags and RepoDigests. if img, ok := item.(*types.ImageInfo); ok { img.RepoTags = repoTags img.RepoDigests = repoDigests @@ -268,16 +253,15 @@ func (c *imageCache) get(idOrRef string) (*types.ImageInfo, error) { return nil, err } - var id string + id := idOrRef if refDigest, ok := refNamed.(reference.Digested); ok { id = c.refToID[refDigest.String()] - } else { - refTagged := reference.WithDefaultTagIfMissing(refNamed).(reference.Tagged) - if len(c.refToID[refTagged.String()]) == 0 { - id = idOrRef - } else { - id = c.refToID[refTagged.String()] - } + } + if refTagged, ok := refNamed.(reference.Tagged); ok { + id = c.refToID[refTagged.String()] + } + if id == "" { + return nil, errors.Wrap(errtypes.ErrNotfound, "image: "+id) } // use trie to fetch image @@ -316,6 +300,7 @@ func (c *imageCache) remove(image *types.ImageInfo) { } for _, v := range image.RepoDigests { delete(c.refToID, v) + delete(c.digestToTag, v) } delete(c.idToTags, id) delete(c.idToDigests, id) @@ -324,31 +309,49 @@ func (c *imageCache) remove(image *types.ImageInfo) { } // untagged is used to remove the deleted repoTag or repoDigest from image. -func (c *imageCache) untagged(ref string) { +func (c *imageCache) untagged(refNamed reference.Named) { c.Lock() defer c.Unlock() + var ref, tag, digest string + if refDigest, ok := refNamed.(reference.Digested); ok { + ref = refDigest.String() + digest = ref + tag = c.digestToTag[digest] + } else if refTagged, ok := reference.WithDefaultTagIfMissing(refNamed).(reference.Tagged); ok { + ref = refTagged.String() + tag = ref + digest = c.tagToDigest[tag] + } + id := c.refToID[ref] - delete(c.idToTags[id], ref) - delete(c.idToDigests[id], c.tagToDigest[ref]) - delete(c.refToID, ref) - delete(c.refToID, c.tagToDigest[ref]) - delete(c.tagToDigest, ref) + delete(c.idToTags[id], tag) + delete(c.idToDigests[id], digest) + delete(c.refToID, tag) + delete(c.refToID, digest) + delete(c.tagToDigest, tag) + delete(c.digestToTag, digest) if len(c.idToTags[id]) == 0 && len(c.idToDigests[id]) == 0 { c.ids.Delete(patricia.Prefix(id)) return } - // get repoTags and repoDigest from idToTags and idToDigests + // Delete the corresponding tag and digest from idToTags and idToDigests if item := c.ids.Get(patricia.Prefix(id)); item != nil { repoTags := []string{} repoDigests := []string{} - for tag := range c.idToTags[id] { - repoTags = append(repoTags, tag) + for t := range c.idToTags[id] { + if t == tag { + continue + } + repoTags = append(repoTags, t) } - for digest := range c.idToDigests[id] { - repoDigests = append(repoDigests, digest) + for d := range c.idToDigests[id] { + if d == digest { + continue + } + repoDigests = append(repoDigests, d) } if img, ok := item.(*types.ImageInfo); ok { diff --git a/hack/cri-test/test-cri.sh b/hack/cri-test/test-cri.sh index e52ac02ad2..1dfab4d29b 100755 --- a/hack/cri-test/test-cri.sh +++ b/hack/cri-test/test-cri.sh @@ -26,7 +26,7 @@ POUCH_SOCK="/var/run/pouchcri.sock" CRI_FOCUS=${CRI_FOCUS:-} # CRI_SKIP skips the test to skip. -CRI_SKIP=${CRI_SKIP:-"RunAsUserName|seccomp localhost|SELinux|public image with digest|listImage should get exactly 2 repoTags"} +CRI_SKIP=${CRI_SKIP:-"RunAsUserName|seccomp localhost|SELinux|listImage should get exactly 2 repoTags"} # REPORT_DIR is the the directory to store test logs. REPORT_DIR=${REPORT_DIR:-"/tmp/test-cri"} diff --git a/test/cli_images_test.go b/test/cli_images_test.go index 14d496d577..e11b18c66c 100644 --- a/test/cli_images_test.go +++ b/test/cli_images_test.go @@ -3,7 +3,6 @@ package main import ( "context" "encoding/json" - "fmt" "strings" "github.com/alibaba/pouch/apis/types" @@ -107,7 +106,9 @@ func (suite *PouchImagesSuite) TestInspectImage(c *check.C) { // inspect image name output = command.PouchRun("image", "inspect", "-f", "{{.RepoTags}}", busyboxImage).Stdout() - c.Assert(output, check.Equals, fmt.Sprintf("[%s]\n", busyboxImage)) + if !strings.Contains(output, busyboxImage) { + c.Fatalf("output %s should contains %s", output, busyboxImage) + } } // TestLoginAndLogout is to test login and logout command