feature: support push image in daemon side #2641

Ace-Tang · 2019-01-02T03:57:30Z

implement push command in daemon side, since containerd v1.0.3
version push get some problem when push image with index.json which
have multiple platforms. But it works good with only one platform,
this feature is our need.

fixes: #2099

Signed-off-by: Ace-Tang aceapril@126.com

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

fixes: #2099

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

not easy to add ci test, since it need a registry

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov · 2019-01-02T03:57:31Z

Codecov Report

Merging #2641 into master will decrease coverage by 0.34%.
The diff coverage is 26.95%.

@@            Coverage Diff             @@
##           master    #2641      +/-   ##
==========================================
- Coverage   69.46%   69.12%   -0.35%     
==========================================
  Files         283      285       +2     
  Lines       18827    18957     +130     
==========================================
+ Hits        13079    13104      +25     
- Misses       4286     4379      +93     
- Partials     1462     1474      +12

Flag	Coverage Δ
#criv1alpha1test	`31.16% <6.38%> (-0.37%)`	⬇️
#criv1alpha2test	`35.46% <6.38%> (-0.1%)`	⬇️
#integrationtest	`41.96% <19.14%> (-0.22%)`	⬇️
#nodee2etest	`32.04% <6.38%> (-0.31%)`	⬇️
#unittest	`26.96% <12.5%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pkg/jsonstream/image_progress.go	`0% <0%> (ø)`	⬆️
pkg/jsonstream/image_push.go	`0% <0%> (ø)`
apis/server/router.go	`91.35% <100%> (+0.05%)`	⬆️
ctrd/utils.go	`100% <100%> (ø)`	⬆️
ctrd/image.go	`69.26% <15.38%> (-9.74%)`	⬇️
daemon/mgr/image.go	`59.45% <55.55%> (-0.1%)`	⬇️
client/image_push.go	`57.89% <57.89%> (ø)`
apis/server/image_bridge.go	`74.19% <61.53%> (-1.49%)`	⬇️
cri/ocicni/cni_manager.go	`58.82% <0%> (-11.77%)`	⬇️
apis/server/utils.go	`71.15% <0%> (-3.85%)`	⬇️
... and 7 more

allencloud · 2019-01-02T03:59:24Z

apis/server/image_bridge.go

+	// no need return error here
+	if err := s.ImageMgr.PushImage(ctx, name, tag, &authConfig, newWriteFlusher(rw)); err != nil {
+		logrus.Errorf("failed to push image %s with tag %s: %v", name, tag, err)
+		return nil


return err or return nil?

I think like pull and exec, return nil here

could we check image before push? PushImage will return 404 as error. And the user doesn't know error because we don't return it.

@fuweid , have do the check, if containerd can get image, we will push

I re-check the code here, we use jsonstream to pass infos to client, but stream only start after push image, that means no other error can not be send to client, so I think should be return error here

fuweid · 2019-01-06T11:22:55Z

client/image_push_test.go

+	}
+}
+
+func TestImagePushWrongError(t *testing.T) {


I think this case is duplicated with TestImagePushServerError. It doesn't help. Could we consider to remove this? WDYT?

fuweid · 2019-01-06T11:29:46Z

apis/server/image_bridge.go

+	// no need return error here
+	if err := s.ImageMgr.PushImage(ctx, name, tag, &authConfig, newWriteFlusher(rw)); err != nil {
+		logrus.Errorf("failed to push image %s with tag %s: %v", name, tag, err)
+		return nil


could we check image before push? PushImage will return 404 as error. And the user doesn't know error because we don't return it.

fuweid · 2019-01-06T11:35:33Z

daemon/mgr/image.go

+
+	stream := jsonstream.New(out, nil)
+	err = mgr.client.PushImage(ctx, ref.String(), authConfig, stream)
+	stream.Wait()


I don't think we should wait here. If the image is not found, the stream.Wait() will be dead end. WDYT?

I think it ok here, since in stream.New(), it have do the add, but move it to ctrd/image.go make more sense.

// New creates a 'JSONStream' instance and use a goroutine to recv/send data. func New(out io.Writer, f Formater) *JSONStream { format := f if format == nil { format = newDefaultFormat() } stream := &JSONStream{ w: out, cache: make(chan interface{}, 16), f: format, } stream.wg.Add(1)

fuweid · 2019-01-06T11:49:53Z

ctrd/utils.go

@@ -16,6 +16,9 @@ import (
 	"github.com/pkg/errors"
 )

+// pushTracker records push progress detail
+var pushTracker = docker.NewInMemoryTracker()


I am not sure that it is good to use global variables. There is one case:

Someone has pushed image A with desc A1. If other one tries to push image B which also contains desc A1, the status might be done. But it might be under committing.

It remind me, looks like a parallel caused bug, we can also do a fix for containerd

please remove this line since you uses temp var.

containerd ctr is command , not a server implement. it doesn't have the issue.

fuweid · 2019-01-06T11:51:53Z

@Ace-Tang please rebase it with current master to make the CI happy.

ZYecho · 2019-01-07T13:33:02Z

ctrd/image.go

@@ -176,6 +178,63 @@ func (c *Client) importImage(ctx context.Context, importer ctrdmetaimages.Import
 	return imgs, nil
 }

+// PushImage pushs image to registry


ZYecho · 2019-01-07T13:35:06Z

ctrd/image.go

+
+	defer stream.Close()
+	if err != nil {
+		// Send Error information to client through stream


s/Error/error?

ZYecho · 2019-01-07T13:38:19Z

ctrd/image.go

@@ -13,6 +14,7 @@ import (

 	"github.com/containerd/containerd"
 	"github.com/containerd/containerd/errdefs"
+	"github.com/containerd/containerd/images"


seems this line duplicate with next line?

ZYecho · 2019-01-07T13:39:31Z

daemon/mgr/image.go

@@ -43,6 +43,9 @@ type ImageMgr interface {
 	// PullImage pulls images from specified registry.
 	PullImage(ctx context.Context, ref string, authConfig *types.AuthConfig, out io.Writer) error

+	// PushImage push image to specified registry.


ZYecho · 2019-01-07T13:40:00Z

ctrd/interface.go

@@ -89,6 +89,8 @@ type ImageAPIClient interface {
 	SaveImage(ctx context.Context, exporter ctrdmetaimages.Exporter, ref string) (io.ReadCloser, error)
 	// Commit commits an image from a container.
 	Commit(ctx context.Context, config *CommitConfig) (digest.Digest, error)
+	// PushImage pushs a image to registry


ZYecho · 2019-01-07T13:40:43Z

daemon/mgr/image.go

@@ -174,6 +177,25 @@ func (mgr *ImageManager) PullImage(ctx context.Context, ref string, authConfig *
 	return mgr.StoreImageReference(ctx, img)
 }

+// PushImage push image to specified registry.


Ace-Tang · 2019-01-08T12:11:36Z

updated， @fuweid @ZYecho

fuweid

please repush to re-trigger travis-ci. thanks!

fuweid · 2019-01-13T07:00:29Z

ctrd/utils.go

@@ -16,6 +16,9 @@ import (
 	"github.com/pkg/errors"
 )

+// pushTracker records push progress detail
+var pushTracker = docker.NewInMemoryTracker()


please remove this line since you uses temp var.

fuweid · 2019-01-13T07:01:18Z

test/api_image_push_test.go

+	SkipIfFalse(c, environment.IsLinux)
+}
+
+// TestImageCreateOk tests creating an image is OK.


wrong comment

fuweid · 2019-01-13T07:02:00Z

test/api_image_push_test.go

+	name := "not_exist_image"
+	resp, err := request.Post("/images/" + name + "/push")
+	c.Assert(err, check.IsNil)
+	CheckRespStatus(c, resp, 500)


why using 500 instead of 404 for not-exist-image?

yes, should be 404

404 means no such route

I am afraid that 404 mean image not found rather than endpoint route not found.

fuweid · 2019-01-13T07:03:16Z

ctrd/utils.go

@@ -16,6 +16,9 @@ import (
 	"github.com/pkg/errors"
 )

+// pushTracker records push progress detail
+var pushTracker = docker.NewInMemoryTracker()


containerd ctr is command , not a server implement. it doesn't have the issue.

allencloud

Please add this HTTP endpoint in swagger.yml first to define this API explicitly. Thanks. @Ace-Tang

ctrd/utils.go

allencloud · 2019-01-15T04:09:22Z

pkg/jsonstream/image_push.go

+)
+
+// Pushjobs defines a job in upload progress
+type Pushjobs struct {


use the camel way of PushJobs instead of Pushjobs?

allencloud · 2019-01-15T06:49:01Z

apis/swagger.yml

+      summary: "push an image on the registry"
+      description: "push an image on the registry"
+      consumes:
+        - "application/json"


Here maybe we need some discussion. I think there the in HTTP request, we are not consuming application/json, and actually it is application/octet-stream that consumes type of file, like that described in moby: https://github.com/moby/moby/blob/master/api/swagger.yaml#L6735 .

And please add

- name: "X-Registry-Auth" in: "header" description: "A base64-encoded auth configuration. [See the authentication section for details.](#section/Authentication)" type: "string" required: true

fuweid · 2019-01-16T09:38:18Z

apis/server/image_bridge.go

+	}
+
+	// Error information has be sent to client, so no need call resp.Write
+	// no need return error here


wrong comment here?

ctrd/image.go

test/api_image_push_test.go

fuweid

Basicly, LGTM

implement push command in daemon side, since containerd v1.0.3 version push get some problem when push image with index.json which have multiple platforms. But it works good with only one platform, this feature is our need. fixes: #2099 Signed-off-by: Ace-Tang <aceapril@126.com>

fuweid

LGTM

pouchrobot added kind/feature size/XL labels Jan 2, 2019

allencloud reviewed Jan 2, 2019

View reviewed changes

fuweid reviewed Jan 6, 2019

View reviewed changes

allencloud assigned ZYecho Jan 7, 2019

ZYecho reviewed Jan 7, 2019

View reviewed changes

fuweid added this to the v1.2.0 milestone Jan 8, 2019

fuweid suggested changes Jan 8, 2019

View reviewed changes

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Jan 10, 2019

fuweid reviewed Jan 13, 2019

View reviewed changes

allencloud requested changes Jan 15, 2019

View reviewed changes

allencloud reviewed Jan 15, 2019

View reviewed changes

ctrd/utils.go Show resolved Hide resolved

allencloud reviewed Jan 15, 2019

View reviewed changes

fuweid reviewed Jan 16, 2019

View reviewed changes

fuweid reviewed Jan 17, 2019

View reviewed changes

test/api_image_push_test.go Show resolved Hide resolved

fuweid reviewed Jan 17, 2019

View reviewed changes

allencloud approved these changes Jan 17, 2019

View reviewed changes

allencloud merged commit ff8b8fb into AliyunContainerService:master Jan 17, 2019

pouchrobot mentioned this pull request Jan 18, 2019

WeeklyReport of PouchContainer from 2019-01-11 to 2019-01-18 #2676

Closed

feature: support push image in daemon side #2641

feature: support push image in daemon side #2641

Conversation

Ace-Tang commented Jan 2, 2019

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov bot commented Jan 2, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid commented Jan 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ace-Tang commented Jan 8, 2019

fuweid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ace-Tang Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid left a comment

Choose a reason for hiding this comment

fuweid left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 2, 2019 •

edited

Loading

Ace-Tang Jan 15, 2019 •

edited

Loading

allencloud Jan 15, 2019 •

edited

Loading