enhance: adjust data stream from pouch pull api #1586

fuweid · 2018-06-23T16:44:45Z

Ⅰ. Describe what this PR did

Since the existing swarm, mesos or other scheduled system maybe use the docker client to pull image from PouchContainer, PouchContainer should change data stream to help user to use PouchContainer.

It can help user to migrate to PouchContainer from docker.

Ⅱ. Does this pull request fix one issue?

fixes #1579

Ⅲ. Describe how you did it

With original design, the PouchContainer uses the array as data packet in the stream. However, the docker client can only consume the json as one packet in the stream. Before this change, we will got the error like that:

docker -H {Pouch Daemon Host} pull busybox
json: cannot unmarshal array into Go value of type jsonmessage.JSONMessage

In order to make it happen, we need to adjust data stream with one json object as packet.

Ⅳ. Describe how to verify it

Basically, we need to confirm the pouch cli does work well.

➜  ~ pouch pull ruby
registry.hub.docker.com/library/ruby:latest:                                      resolved |++++++++++++++++++++++++++++++++++++++|
index-sha256:15cf422c8a386c5dc4ffc51875d09f3e70a9350575aabe710117de452ec36159:    done     |++++++++++++++++++++++++++++++++++++++|
manifest-sha256:d2c3ac773b65fc114065f1b649a71f4183d68eb3e56ca44afefe129b56c5824d: done     |++++++++++++++++++++++++++++++++++++++|
layer-sha256:e6fd54c4bb16195b2277ee283c59ea8cab4bc5d3fad9294d0b78c28b48f4e259:    done     |++++++++++++++++++++++++++++++++++++++|
layer-sha256:231cb0e216d30ea48044d44d37fba016eb67eca9b19b29a741d95775359d3533:    done
layer-sha256:3d2aa70286b89febc36370098220c9b2960cc67c03375c9df4e82736519f1e8a:    done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:e80dfb6a4adf7a00bab2a596e518acdf66c94fd2534023225f6e1a1861f72417:    done           |++++++++++++++++++++++++++++++++++++++|
config-sha256:857bc7ff918f67a8e74abd01dc02308040f7f7f9b99956a815c5a9cb6393f11f:   done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:9fdce3e4507c8079e74b796cebdd63826089d548bb8dfe64a062c08f3a137d7d:    done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:590d63dda050c79d76f1e39724ad28d12f66d358d7e6aa08ff9ca84e0401fc24:    done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:d2c05365ee2a2245bb9f6786bc88aa12bf64da676a52668424437826d0f0cb92:    done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:cc1a78bfd46becbfc3abb8a74d9a70a0e0dc7a5809bbd12e814f9382db003707:    done           |++++++++++++++++++++++++++++++++++++++|
elapsed: 99.6s


➜  ~ pouch pull unknown
registry.hub.docker.com/library/unknown:latest: resolving      |--------------------------------------|
elapsed: 2.4 s                                  total:   0.0 B (0.0 B/s)
Error: failed to display progress: failed to pull image: server message: insufficient_scope: authorization failed
➜  ~ pouch pull busybox:unknown
registry.hub.docker.com/library/busybox:unknown: resolving      |--------------------------------------|
elapsed: 2.4 s                                   total:   0.0 B (0.0 B/s)
Error: failed to display progress: failed to pull image: registry.hub.docker.com/library/busybox:unknown not found

For the docker client,

➜  ~ docker -H tcp://localhost:5000 pull busybox:1.25
registry.hub.docker.com/library/busybox:1.25: resolved
manifest-sha256:29f5d56d12684887bdfa50dcd29fc31eea4aaf4ad3bec43daf19026a7ce69912: done [==================================================>]     527B/527B
layer-sha256:56bec22e355981d8ba0878c6c2f23b21f422f30ab0aba188b54f1ffeff59c190: done [==================================================>]  668.2kB/668.2kB
config-sha256:e02e811dd08fd49e7f6032625495118e63f597eb150403d02e3238af1df240ba: done [==================================================>]  1.464kB/1.464kB

➜  ~ docker -H tcp://localhost:5000 pull busybox:unknown
registry.hub.docker.com/library/busybox:unknown: resolving
failed to pull image: registry.hub.docker.com/library/busybox:unknown not found

➜  ~ docker -H tcp://localhost:5000 pull unknown
Using default tag: latest
registry.hub.docker.com/library/unknown:latest: resolving
failed to pull image: server message: insufficient_scope: authorization failed
➜  ~ echo $?
1

Ⅴ. Special notes for reviews

codecov-io · 2018-06-24T11:00:15Z

Codecov Report

Merging #1586 into master will decrease coverage by 3.97%.
The diff coverage is 44.33%.

@@            Coverage Diff             @@
##           master    #1586      +/-   ##
==========================================
- Coverage   41.92%   37.95%   -3.98%     
==========================================
  Files         270      275       +5     
  Lines       17635    19451    +1816     
==========================================
- Hits         7394     7382      -12     
- Misses       9333    11154    +1821     
- Partials      908      915       +7

Impacted Files	Coverage Δ
cli/pull.go	`0% <0%> (ø)`	⬆️
pkg/jsonstream/image_progress.go	`0% <0%> (ø)`
pkg/jsonstream/types.go	`0% <0%> (ø)`
pkg/jsonstream/stream.go	`68.62% <100%> (+3.92%)`	⬆️
apis/server/image_bridge.go	`53.06% <100%> (-12.77%)`	⬇️
daemon/mgr/image.go	`54.09% <100%> (-14.38%)`	⬇️
pkg/jsonstream/format.go	`100% <100%> (+19.04%)`	⬆️
ctrd/image.go	`75.27% <86.04%> (-4.16%)`	⬇️
apis/opts/config/blkio.go	`0% <0%> (-69.67%)`	⬇️
pkg/utils/filters/filter.go	`22.5% <0%> (-45%)`	⬇️
... and 28 more

YaoZengzeng · 2018-07-03T03:08:16Z

pkg/jsonstream/format.go

@@ -23,46 +22,21 @@ var (
 	jsonSep        = []byte(",")


jsonBeginDelim, jsonEndDelim and jsonSep seems have been deprecated?

And the Formater seems not appropriate as well 😄 . Only the Write method of it is necessary.

I will remove the jsonBeginDelim, jsonEndDelim, jsonSep. Maybe the requirement is to render the array json to the caller. But for this case, we don't need the BeginWrite function.

YaoZengzeng · 2018-07-03T03:15:08Z

pkg/jsonstream/image_progress.go

+	PullStatusExists = "exists"
+	// PullStatusDone represents done status.
+	PullStatusDone = "done"
+)


Maybe define a special type for these status? like type PullStatus string ?

It's const var. I think both are ok right now. If we need to upgrade the definition with format like progress.Bar, we will use the specific type for the status. WDTY?

Yes, agree.

YaoZengzeng · 2018-07-03T03:19:18Z

Actually I'm not very skilled at this part 😄 , but apart from the minor issue mentioned above, LGTM.

shaloulcy · 2018-07-05T03:31:54Z

ctrd/image.go

@@ -212,11 +203,13 @@ outer:
 				}
 				// update status of active entries!
 				for _, active := range active {


maybe we should rename the later active as activities. It's a little misunderstanding

Have updated

In order to help user migrate to PouchContainer from docker world, PouchContainer need to adjust data stream from pull image api so that the docker pull can consume PouchContainer data steam. Signed-off-by: Wei Fu <[email protected]>

fuweid · 2018-07-05T11:57:22Z

@YaoZengzeng and @shaloulcy I have updated the code. PTAL

yyb196 · 2018-07-06T02:15:17Z

LGTM

shaloulcy · 2018-07-06T02:15:55Z

LGTM

pouchrobot added areas/test size/XL labels Jun 23, 2018

fuweid changed the title ~~--wip-- [skip ci]~~ enhance: adjust data stream from pouch pull api Jun 24, 2018

pouchrobot added the areas/images label Jun 24, 2018

fuweid requested review from allencloud, yyb196 and YaoZengzeng June 27, 2018 07:19

YaoZengzeng reviewed Jul 3, 2018

View reviewed changes

shaloulcy reviewed Jul 5, 2018

View reviewed changes

enhance: adjust data stream from pull image api

f66c62c

In order to help user migrate to PouchContainer from docker world, PouchContainer need to adjust data stream from pull image api so that the docker pull can consume PouchContainer data steam. Signed-off-by: Wei Fu <[email protected]>

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

yyb196 merged commit b58ff0c into AliyunContainerService:master Jul 6, 2018

pouchrobot mentioned this pull request Jul 12, 2018

Weekly Report of PouchContainer from 2018-07-05 to 2018-07-12 #1722

Closed

fuweid deleted the enhance_adjust_data_stream_from_pull_api branch August 3, 2018 02:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhance: adjust data stream from pouch pull api #1586

enhance: adjust data stream from pouch pull api #1586

fuweid commented Jun 23, 2018 •

edited

Loading

codecov-io commented Jun 24, 2018 •

edited

Loading

YaoZengzeng Jul 3, 2018

fuweid Jul 5, 2018

YaoZengzeng Jul 3, 2018

fuweid Jul 5, 2018

YaoZengzeng Jul 5, 2018

YaoZengzeng commented Jul 3, 2018

shaloulcy Jul 5, 2018

fuweid Jul 5, 2018

fuweid commented Jul 5, 2018

yyb196 commented Jul 6, 2018

shaloulcy commented Jul 6, 2018

enhance: adjust data stream from pouch pull api #1586

enhance: adjust data stream from pouch pull api #1586

Conversation

fuweid commented Jun 23, 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

Ⅴ. Special notes for reviews

codecov-io commented Jun 24, 2018 • edited Loading

Codecov Report

YaoZengzeng Jul 3, 2018

Choose a reason for hiding this comment

fuweid Jul 5, 2018

Choose a reason for hiding this comment

YaoZengzeng Jul 3, 2018

Choose a reason for hiding this comment

fuweid Jul 5, 2018

Choose a reason for hiding this comment

YaoZengzeng Jul 5, 2018

Choose a reason for hiding this comment

YaoZengzeng commented Jul 3, 2018

shaloulcy Jul 5, 2018

Choose a reason for hiding this comment

fuweid Jul 5, 2018

Choose a reason for hiding this comment

fuweid commented Jul 5, 2018

yyb196 commented Jul 6, 2018

shaloulcy commented Jul 6, 2018

fuweid commented Jun 23, 2018 •

edited

Loading

codecov-io commented Jun 24, 2018 •

edited

Loading