refactor: image manager #1267

fuweid · 2018-05-03T12:28:28Z

Ⅰ. Describe what this PR did

Refactor image manager and make it stable.

Before this PR, we always meet the "not found" during pouch image inspect, pouch rmi. We have several PR to fix the problem. But it always show up. In order to make the pouch stable, I decides to refactor.

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Describe how you did it

I redesign the imageStore in ImageMgr. The design is here:

Primary reference is the reference used for pulling the image. For example,

 	pouch pull busybox:1.26

the busybox:1.26 is the primary reference. However, the user can use the following references to consume the busybox:1.26:

        busybox:1.26
        busybox@sha256:be3c11fdba7cfe299214e46edc642e09514dbb9bbefcd0d3836c05a1e0cd0642
        c30178c5239f

The Name[:Tag][@Digest] are the references generated from busybox:1.26. 
Therefore, We can call it searchable references, which allows users use different reference to use the busybox:1.26.

Searchable reference can be named by user. 
For example, the localhost:5000/busybox:latest is the searchable reference for the busybox:1.26. 
Then the localhost:5000/busybox:latest is like alias. You can remove the alias and the image is still here.

However, if we remove the primary reference, bot the image and all searchable references will be removed.

In the Containerd, the Name to be pulled is used as primary key. 
We can only use the name to search the image in Containerd. 
Based on this, we should make primary and searchable references concept here.

Besides this, the user can use busybox[:whatever]@digest to pull image. 
The registry doesn't care the tag information if you provide digest. 
The tag maybe wrong. In order to avoid this kind of data, we will trim tag information from Name:Tag@Digest reference.

Ⅳ. Describe how to verify it

First, pull the reg.docker.alibaba-inc.com/base/busybox:latest

➜  ~ pouch pull reg.docker.alibaba-inc.com/base/busybox:latest
reg.docker.alibaba-inc.com/base/busybox:latest:                                   resolved       |++++++++++++++++++++++++++++++++++++++|
manifest-sha256:68effe31a4ae8312e47f54bec52d1fc925908009ce7e6f734e1b54a4169081c5: done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:04176c8b224aa0eb9942af765f66dae866f436e75acef028fe44b8a98e045515:    done           |++++++++++++++++++++++++++++++++++++++|
config-sha256:00f017a8c2a6e1fe2ffd05c281f27d069d2a99323a8cd514dd35f228ba26d2ff:   done           |++++++++++++++++++++++++++++++++++++++|
elapsed: 0.0 s

About inspect

The following commands have the same result.

pouch image inspect reg.docker.alibaba-inc.com/base/busybox:latest
pouch image inspect reg.docker.alibaba-inc.com/base/busybox@sha256:68effe31a4ae8312e47f54bec52d1fc925908009ce7e6f734e1b54a4169081c5
pouch image inspect reg.docker.alibaba-inc.com/base/busybox
pouch image inspect 00f017a8c2a6e1

About rmi

The following commands have the same result.

pouch rmi reg.docker.alibaba-inc.com/base/busybox:latest
pouch rmi reg.docker.alibaba-inc.com/base/busybox
pouch rmi 00f017a8c2a6e1

However, the reg.docker.alibaba-inc.com/base/busybox@sha256:68effe31a4ae8312e47f54bec52d1fc925908009ce7e6f734e1b54a4169081c5 is the searchable reference. If we remove this, the image is still here because the primary reference is still here.

➜  ~ pouch rmi reg.docker.alibaba-inc.com/base/busybox@sha256:68effe31a4ae8312e47f54bec52d1fc925908009ce7e6f734e1b54a4169081c5
reg.docker.alibaba-inc.com/base/busybox@sha256:68effe31a4ae8312e47f54bec52d1fc925908009ce7e6f734e1b54a4169081c5
➜  ~ pouch images --digest
IMAGE ID       IMAGE NAME                                       DIGEST   SIZE
00f017a8c2a6   reg.docker.alibaba-inc.com/base/busybox:latest   <none>   686.62 KB

About pull

pouch pull busybox # equals => pouch pull `registry.hub.docker.com/library/busybox:latest`
pouch pull busybox:1.28 # equals => pouch pull `registry.hub.docker.com/library/busybox:1.228`

User can use Name:Tag@Digest to pull image. However, the register doesn't care the tag information if the reference has the digest. In order to avoid the wrong tag information, we remove tag information in digest before pull.

Therefore, the following commands has the same result

pouch pull busybox:wrong@sha256:58ac43b2cc92c687a32c8be6278e50a063579655fe3090125dcb2af0ff9e1a64
pouch pull busybox@sha256:58ac43b2cc92c687a32c8be6278e50a063579655fe3090125dcb2af0ff9e1a64

➜  ~ pouch images
IMAGE ID       IMAGE NAME                                                                                                        SIZE
8ac48589692a   registry.hub.docker.com/library/busybox@sha256:58ac43b2cc92c687a32c8be6278e50a063579655fe3090125dcb2af0ff9e1a64   710.83 KB

Ⅴ. Special notes for reviews

codecov-io · 2018-05-03T12:54:33Z

Codecov Report

Merging #1267 into master will increase coverage by 0.38%.
The diff coverage is 30.22%.

@@            Coverage Diff             @@
##           master    #1267      +/-   ##
==========================================
+ Coverage   15.21%   15.59%   +0.38%     
==========================================
  Files         172      173       +1     
  Lines       10735    10819      +84     
==========================================
+ Hits         1633     1687      +54     
- Misses       8981     9013      +32     
+ Partials      121      119       -2

Impacted Files	Coverage Δ
daemon/mgr/cri_utils.go	`29.31% <0%> (+0.45%)`	⬆️
ctrd/image.go	`0% <0%> (ø)`	⬆️
daemon/mgr/cri.go	`0% <0%> (ø)`	⬆️
cli/pull.go	`0% <0%> (ø)`	⬆️
daemon/mgr/image.go	`0% <0%> (-31.96%)`	⬇️
cli/upgrade.go	`0% <0%> (ø)`	⬆️
cli/image_list.go	`0% <0%> (ø)`	⬆️
daemon/mgr/container.go	`0% <0%> (ø)`	⬆️
apis/server/image_bridge.go	`15% <20%> (-6.54%)`	⬇️
daemon/mgr/image_utils.go	`40% <40%> (+40%)`	⬆️
... and 4 more

allencloud · 2018-05-04T12:23:25Z

daemon/mgr/image.go

 	GetImage(ctx context.Context, idOrRef string) (*types.ImageInfo, error)

 	// RemoveImage deletes an image by reference.
-	RemoveImage(ctx context.Context, image *types.ImageInfo, name string, option *ImageRemoveOption) error
+	RemoveImage(ctx context.Context, idOrref string, force bool) error


s/idOrref/idOrRef ?
A minor wish of update.

allencloud · 2018-05-04T12:24:06Z

daemon/mgr/image.go

@@ -13,377 +12,361 @@ import (
 	"github.com/alibaba/pouch/pkg/errtypes"
 	"github.com/alibaba/pouch/pkg/jsonstream"
 	"github.com/alibaba/pouch/pkg/reference"
-	"github.com/alibaba/pouch/registry"
+	"github.com/alibaba/pouch/pkg/utils"
+	"github.com/sirupsen/logrus"


Please add a blank line of "github.com/sirupsen/logrus" and remove the blank line of it.

allencloud · 2018-05-04T12:36:16Z

Primary reference is the reference used for pulling the image. For example,
pouch pull busybox:1.26
the busybox:1.26 is the primary reference.

Is busybox:1.26 the only primary reference for the corresponding image? @fuweid

And if we execute pouch pull busybox, in the command what is busybox?

fuweid · 2018-05-05T08:56:32Z

HI, @allencloud sorry to reply late.

Is busybox:1.26 the only primary reference for the corresponding image? @fuweid

No. Both busybox@digest and busybox[:whatever]@digest can be the primary reference for the corresponding image. One image has many primary references:

Name:Tag
Name@Digest
Name:WrongTag@RightDigest [The Registry service doesn't care the tag information correct or not if the reference contains the digest]

In order to make the primary reference simple, the image, which comes from one registry service, has two primary references in pouchd:

Name:Tag
Name@Digest

BTW, the primary reference means that you use it to pull the image. :)

And if we execute pouch pull busybox, in the command what is busybox?

If we execute pouch pull busybox, the pouchd will use registry.hub.docker.com/library/busybox:latest as the primary reference.

Does it make senses to you?

YaoZengzeng · 2018-05-07T08:35:47Z

daemon/mgr/image_store.go

+		if oldP.String() == trimRef.String() {
+			if oldP.String() != trimPrimaryRef.String() {
+
+				return pkgerrors.Wrap(errtypes.ErrInvalidType, "invalid reference: cannot replace primary reference")


The following command

pouch pull registry.hub.docker.com/library/busybox@sha256:552644f12d0cb92449213f701c88fabf8e5a7acfae3fcefd144b592293ded713 pouch pull busybox:1-uclibc

will make the pouchd report error like:

ERRO[2018-05-07T16:22:07.055029807+08:00] failed to pull image busybox:1-uclibc:1-uclibc: invalid reference: cannot replace primary reference: invalid type

From my point of view, if a Ref has already been a PrimaryRef, we may not need to maintain its relationship with other PrimaryRefs.

So maybe it is more reasonable that just return nil other then error here.

@fuweid WDYT?

@YaoZengzeng , I think we should maintain the relationship.
If not, the searchable reference can override the primary relationship. In this case, we cannot clean the image in the containerd side. It maybe cause the dirty data in the host. WDYT?

I have updated my code. In this case, we should check the reference before we add it. Please take a look.

Thanks for the suggestion.

Redesign the imageStore in image mgr and make it stable. In the imageStore, we design primary reference and searchable reference. The primary reference is used for pull in containerd and searchable references are the alias the primary reference. We can use different references to find the same image so that it provides the better UEX command line. Signed-off-by: Wei Fu <[email protected]>

YaoZengzeng · 2018-05-07T09:33:43Z

LGTM

allencloud · 2018-05-07T12:27:29Z

Thanks for your review, @YaoZengzeng
We are merging this.

pouchrobot added kind/refactor size/XXL labels May 3, 2018

fuweid changed the title ~~[WIP] refactor: image mgr~~ [WIP] refactor: image manager May 4, 2018

fuweid changed the title ~~[WIP] refactor: image manager~~ refactor: image manager May 4, 2018

fuweid requested review from YaoZengzeng, allencloud and HusterWan May 4, 2018 07:58

allencloud reviewed May 4, 2018

View reviewed changes

YaoZengzeng reviewed May 7, 2018

View reviewed changes

allencloud merged commit d05bc85 into AliyunContainerService:master May 7, 2018

fuweid deleted the refactor_image_mgr branch May 7, 2018 13:10

pouchrobot mentioned this pull request May 10, 2018

Weekly Report of Pouch from 2018-05-03 to 2018-05-10 #1297

Closed

fuweid mentioned this pull request May 11, 2018

refactor: redesign the image structure and related code #1160

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: image manager #1267

refactor: image manager #1267

fuweid commented May 3, 2018 •

edited

Loading

codecov-io commented May 3, 2018 •

edited

Loading

allencloud May 4, 2018

allencloud May 4, 2018

allencloud commented May 4, 2018 •

edited

Loading

fuweid commented May 5, 2018

YaoZengzeng May 7, 2018

fuweid May 7, 2018

YaoZengzeng commented May 7, 2018

allencloud commented May 7, 2018

refactor: image manager #1267

refactor: image manager #1267

Conversation

fuweid commented May 3, 2018 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

About inspect

About rmi

About pull

Ⅴ. Special notes for reviews

codecov-io commented May 3, 2018 • edited Loading

Codecov Report

allencloud May 4, 2018

Choose a reason for hiding this comment

allencloud May 4, 2018

Choose a reason for hiding this comment

allencloud commented May 4, 2018 • edited Loading

fuweid commented May 5, 2018

YaoZengzeng May 7, 2018

Choose a reason for hiding this comment

fuweid May 7, 2018

Choose a reason for hiding this comment

YaoZengzeng commented May 7, 2018

allencloud commented May 7, 2018

fuweid commented May 3, 2018 •

edited

Loading

codecov-io commented May 3, 2018 •

edited

Loading

allencloud commented May 4, 2018 •

edited

Loading