fix: pull should return 404 when image not exists #2914

ZYecho · 2019-06-18T14:34:57Z

Signed-off-by: zhangyue zy675793960@yeah.net

Ⅰ. Describe what this PR did

fix bugs in #2913, when error happens, we should not write anything to Writer, if you write anything, then we will get resp code 200.
In this pr, add a resolveImage interface to check the ref is exists or not, if not, will return err.

Ⅱ. Does this pull request fix one issue?

fix bugs in #2913

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

takes on #2913

Ⅳ. Describe how to verify it

wait for CI

Ⅴ. Special notes for reviews

when image pull No.5 layer error, the error msg still in json stream, this pr doesn't handle this case.

codecov · 2019-06-18T14:35:00Z

Codecov Report

Merging #2914 into master will decrease coverage by 0.08%.
The diff coverage is 85%.

@@            Coverage Diff             @@
##           master    #2914      +/-   ##
==========================================
- Coverage   68.23%   68.14%   -0.09%     
==========================================
  Files         291      291              
  Lines       18328    18337       +9     
==========================================
- Hits        12506    12496      -10     
- Misses       4368     4377       +9     
- Partials     1454     1464      +10

Flag	Coverage Δ
#criv1alpha2_test	`34.79% <45%> (-0.04%)`	⬇️
#integration_test_0	`35.92% <85%> (-0.15%)`	⬇️
#integration_test_1	`35.57% <45%> (+0.07%)`	⬆️
#integration_test_2	`35.95% <45%> (-0.08%)`	⬇️
#integration_test_3	`35.61% <85%> (-0.03%)`	⬇️
#node_e2e_test	`34.15% <75%> (-0.21%)`	⬇️
#unittest	`27.97% <0%> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
ctrd/utils.go	`76.12% <100%> (+0.15%)`	⬆️
daemon/mgr/image.go	`58.83% <100%> (-2.54%)`	⬇️
apis/server/image_bridge.go	`78.03% <66.66%> (-0.44%)`	⬇️
ctrd/image.go	`67.76% <80%> (+0.23%)`	⬆️
cri/ocicni/netns.go	`58.1% <0%> (-2.71%)`	⬇️
ctrd/container.go	`51.62% <0%> (-2.68%)`	⬇️
cri/ocicni/cni_manager.go	`61.32% <0%> (-0.95%)`	⬇️
cri/v1alpha2/cri.go	`63.53% <0%> (-0.51%)`	⬇️
daemon/mgr/container.go	`59.93% <0%> (-0.42%)`	⬇️
... and 5 more

allencloud · 2019-06-18T15:05:30Z

PTAL @fuweid thanks a lot.

fuweid · 2019-06-18T15:37:57Z

apis/server/image_bridge.go

@@ -58,7 +58,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, &authConfig, newWriteFlusher(rw)); err != nil {
 		logrus.Errorf("failed to pull image %s: %v", image, err)
-		return nil
+		return err


well. if we write something into response, the http code will be 200 which can never be changed.
so we use jsonstream to return error. the client needs to parse the json stream to get error.

when you pull the No. 5 layer and get something wrong like connection refuse, your change is not helpful. it always be 200, not 500

fuweid · 2019-06-18T15:39:33Z

test/api_image_create_test.go

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


we need to parse the json stream here

ZYecho · 2019-06-19T09:09:45Z

@fuweid PTAL

fuweid · 2019-06-20T02:41:30Z

test/api_image_create_test.go

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


is it 400? not 500?

fuweid · 2019-06-20T02:42:50Z

ctrd/image.go

 	if err != nil {
-		return nil, fmt.Errorf("failed to get a containerd grpc client: %v", err)
+		return nil, "", err


I think we should check the status and make sure that it is 404

when resolveImage return err, means that it can't find available ref to use.

cri/v1alpha2/cri.go

cri/v1alpha2/cri_utils.go

zhuangqh · 2019-06-20T11:46:02Z

ctrd/utils.go

@@ -166,7 +165,8 @@ func (c *Client) getResolver(ctx context.Context, authConfig *types.AuthConfig,
 	}

 	if availableRef == "" {
-		return nil, "", fmt.Errorf("there is no available image reference after trying %+q", refs)
+		logrus.Errorf("there is no available image reference after trying %+q", refs)
+		return nil, "", errtypes.ErrNotfound


prefer errors.Wrapf(errtypes.ErrNotfound, "there is no available image reference after trying %+q", refs) here
util function just directly return error, log or not should be decided by upper.

I change to logrus.warnf, add an another log in upperand. But I think errtypes.ErrNotfound is enough here, user didn't care about how you find a available ref, just tell them not found.

zhuangqh · 2019-06-20T11:48:33Z

could you show me the UX when pulling a not-exist image? What is the error message display in terminal after doing pouch pull xxx. I am afraid some internal test would fail

ZYecho · 2019-06-20T12:02:36Z

could you show me the UX when pulling a not-exist image? What is the error message display in terminal after doing pouch pull xxx. I am afraid some internal test would fail

sudo pouch pull fsadfsdafsdaf:fsdfdsaf
Error: failed to pull image: {"message":"not found"}

ZYecho · 2019-06-22T01:52:28Z

failed test try to pull reg.docker.alibaba-inc.com/base/hello-world， I try to pull it use docker， also didn't download it， seems it was an inner registry？

Signed-off-by: zhangyue <zy675793960@yeah.net>

zhuangqh

LGTM

pouchrobot added kind/bug This is bug report for project size/S labels Jun 18, 2019

ZYecho mentioned this pull request Jun 18, 2019

test: check pull non-existent image response code #2913

Closed

allencloud assigned fuweid Jun 18, 2019

fuweid reviewed Jun 18, 2019

View reviewed changes

fuweid suggested changes Jun 18, 2019

View reviewed changes

ZYecho force-pushed the fix-pull-resp branch from 84e3189 to 1a6685e Compare June 19, 2019 07:41

pouchrobot added size/M and removed size/S labels Jun 19, 2019

ZYecho force-pushed the fix-pull-resp branch from 1a6685e to 3e96e67 Compare June 19, 2019 07:44

ZYecho changed the title ~~fix: pull should return err and not write msg~~ fix: pull should return 500 when image not exists Jun 19, 2019

ZYecho force-pushed the fix-pull-resp branch 2 times, most recently from 40d3ec2 to 77c60f6 Compare June 20, 2019 01:08

fuweid reviewed Jun 20, 2019

View reviewed changes

ZYecho force-pushed the fix-pull-resp branch from 77c60f6 to 18cbe93 Compare June 20, 2019 07:16

ZYecho changed the title ~~fix: pull should return 500 when image not exists~~ fix: pull should return 404 when image not exists Jun 20, 2019

ZYecho force-pushed the fix-pull-resp branch 2 times, most recently from 5501241 to 93c4e50 Compare June 20, 2019 08:50

zhuangqh reviewed Jun 20, 2019

View reviewed changes

ZYecho force-pushed the fix-pull-resp branch 2 times, most recently from 1b9816f to 5299cfa Compare June 21, 2019 01:32

fix: pull should return 404 when image not exists

21a279d

Signed-off-by: zhangyue <zy675793960@yeah.net>

ZYecho force-pushed the fix-pull-resp branch from 5299cfa to 21a279d Compare June 22, 2019 01:57

zhuangqh approved these changes Jun 22, 2019

View reviewed changes

weekly-digest bot mentioned this pull request Jun 23, 2019

Weekly Digest (16 June, 2019 - 23 June, 2019) #2925

Closed

fuweid merged commit 44be01d into AliyunContainerService:master Jun 27, 2019

ZYecho deleted the fix-pull-resp branch June 27, 2019 11:49

pouchrobot mentioned this pull request Jun 28, 2019

WeeklyReport of PouchContainer from 2019-06-21 to 2019-06-28 #2935

Closed

weekly-digest bot mentioned this pull request Jun 30, 2019

Weekly Digest (23 June, 2019 - 30 June, 2019) #2936

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: pull should return 404 when image not exists #2914

fix: pull should return 404 when image not exists #2914

ZYecho commented Jun 18, 2019 •

edited

codecov bot commented Jun 18, 2019 •

edited

allencloud commented Jun 18, 2019

fuweid Jun 18, 2019

fuweid Jun 18, 2019

fuweid Jun 18, 2019

ZYecho commented Jun 19, 2019

fuweid Jun 20, 2019

fuweid Jun 20, 2019

ZYecho Jun 20, 2019

zhuangqh Jun 20, 2019

ZYecho Jun 20, 2019

zhuangqh Jun 20, 2019

zhuangqh commented Jun 20, 2019 •

edited

ZYecho commented Jun 20, 2019

ZYecho commented Jun 22, 2019

zhuangqh left a comment

fix: pull should return 404 when image not exists #2914

fix: pull should return 404 when image not exists #2914

Conversation

ZYecho commented Jun 18, 2019 • edited

Ⅰ. 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 Jun 18, 2019 • edited

Codecov Report

allencloud commented Jun 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZYecho commented Jun 19, 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

zhuangqh commented Jun 20, 2019 • edited

ZYecho commented Jun 20, 2019

ZYecho commented Jun 22, 2019

zhuangqh left a comment

Choose a reason for hiding this comment

ZYecho commented Jun 18, 2019 •

edited

codecov bot commented Jun 18, 2019 •

edited

zhuangqh commented Jun 20, 2019 •

edited