feature: support set default registry #617

zeppp · 2018-01-19T12:26:35Z

1.Describe what this PR did
Now,pouch support setting default registry and pulling image without registry.
If the image has no prefix,pouch will add a default prefix "registry.hub.docker.com/library/".

2.Does this pull request fix one issue?
fixes part of #589

3.Describe how you did it

4.Describe how to verify it

-> #pouch pull busybox 
registry.hub.docker.com/library/busybox:latest:                                   resolved       |++++++++++++++++++++++++++++++++++++++| 
index-sha256:ac2fc418f3348815e68e266a5aa1b60bc522581c96964912560a0baacc4f5c06:    exists         |++++++++++++++++++++++++++++++++++++++| 
manifest-sha256:4165dea717b4d7ff56ee9f1835a6f02ed98abe04af1a882dfa8f801e82e31b1e: exists         |++++++++++++++++++++++++++++++++++++++| 
layer-sha256:67a0688b88df9791275a3702f902b1f8edebb5d8b5f30da9ca9933c526e6c671:    exists         |++++++++++++++++++++++++++++++++++++++| 
config-sha256:f9b6f7f7b9d34113f66e16a9da3e921a580937aec98da344b852ca540aaa2242:   exists         |++++++++++++++++++++++++++++++++++++++| 
elapsed: 4.1 s                                                                    total:   0.0 B (0.0 B/s)                                         


-> #pouch images
IMAGE ID       IMAGE NAME                                       SIZE
ac2fc418f334   registry.hub.docker.com/library/busybox:latest   710.48 KB

-> #pouch image inspect busybox
{
  "Digest": "sha256:ac2fc418f3348815e68e266a5aa1b60bc522581c96964912560a0baacc4f5c06",
  "ID": "ac2fc418f334",
  "Name": "registry.hub.docker.com/library/busybox:latest",
  "Size": 2699,
}

-># pouch rmi busybox
registry.hub.docker.com/library/busybox:latest

-># pouch images
IMAGE ID   IMAGE NAME   SIZE

设置默认仓库
-># pouchd --default-registry docker.io/library/

-># pouch pull busybox
docker.io/library/busybox:latest:                                                 resolved       |++++++++++++++++++++++++++++++++++++++| 
index-sha256:7a7714572b84fe1fafbc6b4f8374925249da8642b723d941a5ea214dac19b164:    done           |++++++++++++++++++++++++++++++++++++++| 
manifest-sha256:4165dea717b4d7ff56ee9f1835a6f02ed98abe04af1a882dfa8f801e82e31b1e: done           |++++++++++++++++++++++++++++++++++++++| 
layer-sha256:67a0688b88df9791275a3702f902b1f8edebb5d8b5f30da9ca9933c526e6c671:    done           |++++++++++++++++++++++++++++++++++++++| 
config-sha256:f9b6f7f7b9d34113f66e16a9da3e921a580937aec98da344b852ca540aaa2242:   done           |++++++++++++++++++++++++++++++++++++++| 
elapsed: 0.7 s                                                                    total:   0.0 B (0.0 B/s)                                         

-># pouch images
IMAGE ID       IMAGE NAME                         SIZE
7a7714572b84   docker.io/library/busybox:latest   710.48 KB

-># pouch image inspect busybox
{
  "Digest": "sha256:7a7714572b84fe1fafbc6b4f8374925249da8642b723d941a5ea214dac19b164",
  "ID": "7a7714572b84",
  "Name": "docker.io/library/busybox:latest",
  "Size": 2699
}

-># pouch rmi busybox
busybox:latest

-># pouch images
IMAGE ID   IMAGE NAME   SIZE

5.Special notes for reviews
The reference package need to be refactored in the future since it has weak function.
@allencloud

Signed-off-by: zeppp <[email protected]>

codecov-io · 2018-01-19T12:43:43Z

Codecov Report

Merging #617 into master will decrease coverage by 0.48%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #617      +/-   ##
==========================================
- Coverage   13.36%   12.87%   -0.49%     
==========================================
  Files          65       64       -1     
  Lines        3599     3564      -35     
==========================================
- Hits          481      459      -22     
+ Misses       3068     3055      -13     
  Partials       50       50

Impacted Files	Coverage Δ
daemon/mgr/image.go	`34.06% <0%> (-0.77%)`	⬇️
daemon/mgr/image_utils.go	`0% <0%> (ø)`
daemon/mgr/spec_devices.go	`0% <0%> (ø)`	⬆️
daemon/mgr/spec_process.go	`0% <0%> (ø)`	⬆️
daemon/mgr/spec_cgroup_cpu.go	`0% <0%> (ø)`	⬆️
daemon/mgr/spec_namespace.go	`0% <0%> (ø)`	⬆️
daemon/mgr/spec.go	`0% <0%> (ø)`	⬆️
daemon/mgr/spec_network.go	`0% <0%> (ø)`	⬆️
daemon/mgr/spec_cgroup_memory.go	`0% <0%> (ø)`	⬆️
daemon/mgr/spec_volume.go	`0% <0%> (ø)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c48e28f...6f0d2f0. Read the comment docs.

Letty5411 · 2018-01-22T06:13:08Z

LGTM

allencloud · 2018-01-22T15:54:35Z

Hi, @zeppp
Sorry for the delay. Actually we wish that users could have the ability to set default registry address for pouchd. If we make it, users can directly set DefaultRegistry at the startup of Pouchd, after that users do not need to pull image with registry address in image name. So we has a plan to support this in https://github.com/alibaba/pouch/blob/master/daemon/mgr/image.go#L48.

Could you make DefaultRegistry as a flag in pouchd's to support this, and pass this configuration when initializing the image manager in daemon side?

allencloud · 2018-01-23T02:09:34Z

Thanks a lot for the change.
CI fails, @zeppp

----------------------------------------------------------------------
FAIL: /go/src/github.com/alibaba/pouch/test/api_image_delete_test.go:33: APIImageDeleteSuite.TestDeleteUsingImage
/go/src/github.com/alibaba/pouch/test/api_image_delete_test.go:35:
    CreateBusyboxContainerOk(c, cname)
/go/src/github.com/alibaba/pouch/test/utils.go:135:
    c.Assert(resp.StatusCode, check.Equals, status, check.Commentf("Error:%s", got.Message))
... obtained int = 404
... expected int = 201
... Error:image: registry.hub.docker.com/library/busybox:latest: not found
PASS: /go/src/github.com/alibaba/pouch/test/api_image_inspect_test.go:42: APIImageInspectSuite.TestImageInspectNotFound	0.000s
----------------------------------------------------------------------
FAIL: /go/src/github.com/alibaba/pouch/test/api_image_inspect_test.go:24: APIImageInspectSuite.TestImageInspectOk
/go/src/github.com/alibaba/pouch/test/api_image_inspect_test.go:27:
    c.Assert(resp.StatusCode, check.Equals, 200)
... obtained int = 404
... expected int = 200

zeppp · 2018-01-23T04:30:27Z

Updated @allencloud

allencloud · 2018-01-23T05:32:45Z

daemon/mgr/image_utils.go

+// FIXME: need refactor this function.
+// addRegistry add default registry if needed.
+func (mgr *ImageManager) addRegistry(input string) string {
+	if strings.Contains(input, "/") || isNumericID(input) {


To be honest, I am afraid we can improve this part. Let me introduce one example, pouch pull allencloud/mysql; if I set the registry to be 12.12.12.12:4567 , then I suppose to pull 12.12.12.12:4567/allencloud/mysql. But code here would ignore this, I think. @zeppp

This part should be improved indeed, i agree. But, in fact, when you pull allencloud/mysql, it will not add a registry because the input contains "/". So, you will get allencloud/mysql. Only when you pull image by RepoName like mysql, it will add the registry you set. @allencloud

in fact, when you pull allencloud/mysql, it will not add a registry because the input contains "/". So, you will get allencloud/mysql

My point is that allencloud/mysql does not include a default registry, if we add a function addRegistry, maybe we should add default registry for allencloud/mysql, right?

If you agree with this, this is a part of work we need to finish.

Oh, I got it. This function addRegistry need be refactored while I have no idea about that. @allencloud

@zeppp , could we follow the routine like this?

if isNumberID(ref) { return ref } host, remainder := splitDomainHost(ref) if host.PrefixContain(registry) { return ref } else { return registry + ":" + ref }

I checked the oci-image-spec and found that there is no definition about host....

Maybe we need to make rule in splitDomainHost to define what is host.....

how do you think?

cc @allencloud

It is a better way. I will update soon. @0x04C2

I checked the oci-image-spec and found that there is no definition about host....

@0x04C2 @zeppp I agree with that we should split the host thing. In addition, could we refactor reference package to define:

type Ref struct{ Registry string Namespace string ImageName string Tag string }

for example:

input registry namespace name tag

docker.io/library/nginx:alpine docker.io library nginx latest

localhost:80/nginx:alpine localhost:80 nginx alpine

mysql mysql latest

If we make it, in every place we can use this Ref, and we use the same standard definition when communicating in different modules.

If that, could you help to refactor pkg reference? @0x04C2

Got it. will do

Signed-off-by: zeppp <[email protected]>

allencloud · 2018-01-23T10:53:17Z

The reference package need to be refactored in the future since it has weak function.

Could you share more with us the reason why refactoring reference package? @zeppp
If that, maybe @0x04C2 could help to take a look. At least, we should make functions in pkg robust enough.

zeppp · 2018-01-23T11:33:58Z

Currently, we use reference package to parse a string into reference. Users may pull or remove image by its full name, repo name,ID,digest like"sha256:2a56...", but the funcion Parse() only filter the name and tag. So pouch has not been supporting image ID and digest for now(some supported in daemon side).
I will add some weak function to implement these features like #565, but it may take some mistakes over some special cases, so I think it is important to refactor reference package. @allencloud @0x04C2

fuweid · 2018-01-24T02:15:26Z

Sorry for the late reply.

I followed the oci-image-spec and our backend containerd to implement reference package. To be honest, it is really unfriendly to user. I think we need to refactor it.

The reference should support

only-name parse
only-id parse
tag-name parse

containerd/docker doesn't support pull digest id directly. So only-id parse can used in pouch start/run. However, for the id parser, it need daemon to cache the mapping and client/server make sure that we should not append latest. it will be fine.

@zeppp and @allencloud what do you think?

One more question, the default-registry sounds like user preference, right? I don't think it's good idea to handle in backend....

allencloud · 2018-01-24T06:04:32Z

One more question, the default-registry sounds like user preference, right? I don't think it's good idea to handle in backend....

This flag default-registry is used for pouchd rather than pouch cli, so we plan to handle this is pouchd. While we are open for any better thoughts. @0x04C2

fuweid · 2018-01-24T07:39:18Z

OK. @allencloud

For pouchd, it's easy to handle this. If handle this in pouch, we may involve something like .pouchrc I guess. It's nice to have, but not urgent.

So Let's go ahead. please forget it. haha.

allencloud · 2018-01-29T02:39:55Z

Hi, @0x04C2 I have seen your PR. That is a really good one we need. While I have some information to share with you.

This PR is blocking lots of CRI implementation developing. Currently, we have tested this pr, although it still needs improvement, it works fine for the CRI related thing.

So, I am afraid we will first merge this PR to make CRI related things move on. After that, you have to rebase to the latest master if it is. Sorry for the bothering brought to you.

allencloud · 2018-01-29T02:40:01Z

LGTM

fuweid · 2018-01-29T02:59:08Z

No problem. Will do

feature: support pull image with default registry.

f7ed86f

Signed-off-by: zeppp <[email protected]>

zeppp changed the title ~~feature: support pull image with default registry.~~ feature: support pull image with default registry Jan 19, 2018

pouchrobot added kind/feature size/S labels Jan 19, 2018

pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Jan 22, 2018

pouchrobot added size/M and removed size/S labels Jan 22, 2018

zeppp force-pushed the feature-support-default-registry branch from ae2f873 to c2a0213 Compare January 22, 2018 18:28

pouchrobot added size/S and removed size/M labels Jan 22, 2018

zeppp force-pushed the feature-support-default-registry branch from 597ac68 to 0c50b7f Compare January 22, 2018 18:56

pouchrobot added size/M and removed size/S labels Jan 22, 2018

zeppp force-pushed the feature-support-default-registry branch from 0c50b7f to 7967e6f Compare January 22, 2018 19:14

zeppp changed the title ~~feature: support pull image with default registry~~ feature: support set default registry Jan 22, 2018

zeppp force-pushed the feature-support-default-registry branch from 7967e6f to 6ed4c29 Compare January 23, 2018 04:07

allencloud reviewed Jan 23, 2018

View reviewed changes

feature: support set default registry.

6f0d2f0

Signed-off-by: zeppp <[email protected]>

zeppp force-pushed the feature-support-default-registry branch from 6ed4c29 to 6f0d2f0 Compare January 23, 2018 06:08

allencloud added this to the v0.2-milestone milestone Jan 24, 2018

YaoZengzeng mentioned this pull request Jan 24, 2018

feature: implement execSync of cri manager #629

Merged

allencloud merged commit 0af838a into AliyunContainerService:master Jan 29, 2018

YaoZengzeng mentioned this pull request Jan 29, 2018

feature: return empty if the image doesn't exist in CRI mananger and add "basic operations on container" into CI #659

Merged

pouchrobot mentioned this pull request Feb 2, 2018

Weekly Report of Pouch from 2018-01-26 to 2018-02-02 #690

Closed

zeppp deleted the feature-support-default-registry branch February 7, 2018 02:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: support set default registry #617

feature: support set default registry #617

zeppp commented Jan 19, 2018 •

edited

Loading

codecov-io commented Jan 19, 2018 •

edited

Loading

Letty5411 commented Jan 22, 2018

allencloud commented Jan 22, 2018

allencloud commented Jan 23, 2018

zeppp commented Jan 23, 2018

allencloud Jan 23, 2018

zeppp Jan 23, 2018 •

edited

Loading

allencloud Jan 23, 2018

zeppp Jan 23, 2018 •

edited

Loading

fuweid Jan 24, 2018

zeppp Jan 24, 2018

allencloud Jan 24, 2018

allencloud Jan 24, 2018

fuweid Jan 24, 2018

allencloud commented Jan 23, 2018 •

edited

Loading

zeppp commented Jan 23, 2018 •

edited

Loading

fuweid commented Jan 24, 2018

allencloud commented Jan 24, 2018

fuweid commented Jan 24, 2018

allencloud commented Jan 29, 2018 •

edited

Loading

allencloud commented Jan 29, 2018

fuweid commented Jan 29, 2018

input	registry	namespace	name	tag
docker.io/library/nginx:alpine	docker.io	library	nginx	latest
localhost:80/nginx:alpine	localhost:80		nginx	alpine
mysql			mysql	latest

feature: support set default registry #617

feature: support set default registry #617

Conversation

zeppp commented Jan 19, 2018 • edited Loading

codecov-io commented Jan 19, 2018 • edited Loading

Codecov Report

Letty5411 commented Jan 22, 2018

allencloud commented Jan 22, 2018

allencloud commented Jan 23, 2018

zeppp commented Jan 23, 2018

allencloud Jan 23, 2018

Choose a reason for hiding this comment

zeppp Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

allencloud Jan 23, 2018

Choose a reason for hiding this comment

zeppp Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

fuweid Jan 24, 2018

Choose a reason for hiding this comment

zeppp Jan 24, 2018

Choose a reason for hiding this comment

allencloud Jan 24, 2018

Choose a reason for hiding this comment

allencloud Jan 24, 2018

Choose a reason for hiding this comment

fuweid Jan 24, 2018

Choose a reason for hiding this comment

allencloud commented Jan 23, 2018 • edited Loading

zeppp commented Jan 23, 2018 • edited Loading

fuweid commented Jan 24, 2018

allencloud commented Jan 24, 2018

fuweid commented Jan 24, 2018

allencloud commented Jan 29, 2018 • edited Loading

allencloud commented Jan 29, 2018

fuweid commented Jan 29, 2018

zeppp commented Jan 19, 2018 •

edited

Loading

codecov-io commented Jan 19, 2018 •

edited

Loading

zeppp Jan 23, 2018 •

edited

Loading

zeppp Jan 23, 2018 •

edited

Loading

allencloud commented Jan 23, 2018 •

edited

Loading

zeppp commented Jan 23, 2018 •

edited

Loading

allencloud commented Jan 29, 2018 •

edited

Loading