diff --git a/apis/server/image_bridge.go b/apis/server/image_bridge.go index 7791a49d4..771201eea 100644 --- a/apis/server/image_bridge.go +++ b/apis/server/image_bridge.go @@ -46,7 +46,7 @@ func (s *Server) pullImage(ctx context.Context, rw http.ResponseWriter, req *htt } } // Error information has be sent to client, so no need call resp.Write - if err := s.ImageMgr.PullImage(ctx, image, tag, &authConfig, rw); err != nil { + if err := s.ImageMgr.PullImage(ctx, image+":"+tag, &authConfig, rw); err != nil { logrus.Errorf("failed to pull image %s:%s: %v", image, tag, err) return nil } diff --git a/daemon/mgr/cri.go b/daemon/mgr/cri.go index 0781d852e..85bf4caeb 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 @@ -804,18 +809,24 @@ func (c *CriManager) PullImage(ctx context.Context, r *runtime.PullImageRequest) // TODO: authentication. imageRef := r.GetImage().GetImage() - namedRef, err := reference.ParseNamedReference(imageRef) + refNamed, err := reference.ParseNamedReference(imageRef) if err != nil { return nil, err } - taggedRef := reference.WithDefaultTagIfMissing(namedRef).(reference.Tagged) - err = c.ImageMgr.PullImage(ctx, taggedRef.Name(), taggedRef.Tag(), nil, bytes.NewBuffer([]byte{})) + _, ok := refNamed.(reference.Digested) + if !ok { + // If the imageRef is not a digest. + refTagged := reference.WithDefaultTagIfMissing(refNamed).(reference.Tagged) + imageRef = refTagged.String() + } + + err = c.ImageMgr.PullImage(ctx, imageRef, nil, bytes.NewBuffer([]byte{})) if err != nil { return nil, err } - imageInfo, err := c.ImageMgr.GetImage(ctx, taggedRef.String()) + imageInfo, err := c.ImageMgr.GetImage(ctx, imageRef) if err != nil { return nil, err } @@ -826,19 +837,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/cri_utils.go b/daemon/mgr/cri_utils.go index a1f8e2c2c..75f8c4821 100644 --- a/daemon/mgr/cri_utils.go +++ b/daemon/mgr/cri_utils.go @@ -686,22 +686,28 @@ func imageToCriImage(image *apitypes.ImageInfo) (*runtime.Image, error) { } // ensureSandboxImageExists pulls the image when it's not present. -func (c *CriManager) ensureSandboxImageExists(ctx context.Context, image string) error { - _, err := c.ImageMgr.GetImage(ctx, image) +func (c *CriManager) ensureSandboxImageExists(ctx context.Context, imageRef string) error { + _, err := c.ImageMgr.GetImage(ctx, imageRef) // TODO: maybe we should distinguish NotFound error with others. if err == nil { return nil } - namedRef, err := reference.ParseNamedReference(image) + refNamed, err := reference.ParseNamedReference(imageRef) if err != nil { - return fmt.Errorf("parse image name failed: %v", err) + return err + } + + _, ok := refNamed.(reference.Digested) + if !ok { + // If the imageRef is not a digest. + refTagged := reference.WithDefaultTagIfMissing(refNamed).(reference.Tagged) + imageRef = refTagged.String() } - taggedRef := reference.WithDefaultTagIfMissing(namedRef).(reference.Tagged) - err = c.ImageMgr.PullImage(ctx, taggedRef.Name(), taggedRef.Tag(), nil, bytes.NewBuffer([]byte{})) + err = c.ImageMgr.PullImage(ctx, imageRef, nil, bytes.NewBuffer([]byte{})) if err != nil { - return fmt.Errorf("pull sandbox image %q failed: %v", image, err) + return fmt.Errorf("pull sandbox image %q failed: %v", imageRef, err) } return nil diff --git a/daemon/mgr/image.go b/daemon/mgr/image.go index 263aa0c0b..730a2d8e5 100644 --- a/daemon/mgr/image.go +++ b/daemon/mgr/image.go @@ -22,7 +22,7 @@ import ( // ImageMgr as an interface defines all operations against images. type ImageMgr interface { // PullImage pulls images from specified registry. - PullImage(ctx context.Context, image, tag string, authConfig *types.AuthConfig, out io.Writer) error + PullImage(ctx context.Context, imageRef string, authConfig *types.AuthConfig, out io.Writer) error // ListImages lists images stored by containerd. ListImages(ctx context.Context, filters string) ([]types.ImageInfo, error) @@ -73,7 +73,7 @@ func NewImageManager(cfg *config.Config, client *ctrd.Client) (*ImageManager, er } // PullImage pulls images from specified registry. -func (mgr *ImageManager) PullImage(pctx context.Context, image, tag string, authConfig *types.AuthConfig, out io.Writer) error { +func (mgr *ImageManager) PullImage(pctx context.Context, imageRef string, authConfig *types.AuthConfig, out io.Writer) error { ctx, cancel := context.WithCancel(pctx) stream := jsonstream.New(out) @@ -86,8 +86,8 @@ func (mgr *ImageManager) PullImage(pctx context.Context, image, tag string, auth close(wait) }() - image = mgr.addRegistry(image) - img, err := mgr.client.PullImage(ctx, image+":"+tag, authConfig, stream) + imageRef = mgr.addRegistry(imageRef) + img, err := mgr.client.PullImage(ctx, imageRef, authConfig, stream) // wait goroutine to exit. <-wait @@ -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 @@ -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()] @@ -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) @@ -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 { diff --git a/hack/cri-test/test-cri.sh b/hack/cri-test/test-cri.sh index e52ac02ad..1dfab4d29 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 14d496d57..e11b18c66 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