Skip to content

Commit

Permalink
bugfix: pull with RefTag but delete RefDigest
Browse files Browse the repository at this point in the history
Signed-off-by: YaoZengzeng <[email protected]>
  • Loading branch information
YaoZengzeng committed Mar 27, 2018
1 parent 94ee292 commit 0f4d1fd
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 59 deletions.
15 changes: 8 additions & 7 deletions daemon/mgr/cri.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
106 changes: 57 additions & 49 deletions daemon/mgr/image.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}

Expand All @@ -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 {
Expand All @@ -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),
}
}

Expand All @@ -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{}
Expand All @@ -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
Expand All @@ -271,9 +256,13 @@ func (c *imageCache) get(idOrRef string) (*types.ImageInfo, error) {
var id string
if refDigest, ok := refNamed.(reference.Digested); ok {
id = c.refToID[refDigest.String()]
if id == "" {
return nil, errors.Wrap(errtypes.ErrNotfound, "image digest: "+refDigest.String())
}
} else {
refTagged := reference.WithDefaultTagIfMissing(refNamed).(reference.Tagged)
if len(c.refToID[refTagged.String()]) == 0 {
if c.refToID[refTagged.String()] == "" {
// Maybe idOrRef is image ID.
id = idOrRef
} else {
id = c.refToID[refTagged.String()]
Expand Down Expand Up @@ -316,6 +305,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)
Expand All @@ -324,31 +314,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 {
Expand Down
2 changes: 1 addition & 1 deletion hack/cri-test/test-cri.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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"}

Expand Down
5 changes: 3 additions & 2 deletions test/cli_images_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package main
import (
"context"
"encoding/json"
"fmt"
"strings"

"github.com/alibaba/pouch/apis/types"
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 0f4d1fd

Please sign in to comment.