Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix: convert container err into pouchd manager error #2034

Merged
merged 1 commit into from
Aug 10, 2018

Conversation

allencloud
Copy link
Collaborator

Signed-off-by: Allen Sun allensun.shl@alibaba-inc.com

Ⅰ. Describe what this PR did

This pull request converts the error from containerd side to pouch manager side.

For the error part, we have the following part:

  • the containerd's part which returns grpc code;
  • containerd client part, which wraps the grpc code to be error in github.com/containerd/containerd/errdefs
  • mgrs's error types from github.com/alibaba/pouch/pkg/errtypes, this is used for API layers to construct http status code in ./apis/server/routes.go

So if we need to make use of error returned from containerd client which is ctrd, then we should convert those errors into errtypes which could be used in mgrs. This will transfer errors from containerd correctly to mgrs and API layers.

Ⅱ. Does this pull request fix one issue?

none

Ⅲ. Describe how you did it

none

Ⅳ. Describe how to verify it

none

Ⅴ. Special notes for reviews

none

@pouchrobot pouchrobot added kind/bug This is bug report for project size/XL labels Aug 2, 2018
@codecov-io
Copy link

codecov-io commented Aug 2, 2018

Codecov Report

Merging #2034 into master will increase coverage by 0.01%.
The diff coverage is 61.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2034      +/-   ##
==========================================
+ Coverage   64.05%   64.06%   +0.01%     
==========================================
  Files         202      202              
  Lines       15510    15588      +78     
==========================================
+ Hits         9935     9987      +52     
- Misses       4338     4352      +14     
- Partials     1237     1249      +12
Flag Coverage Δ
#criv1alpha1test 33.69% <22.1%> (-0.05%) ⬇️
#criv1alpha2test 34.21% <22.1%> (-0.02%) ⬇️
#integrationtest 38.58% <49.47%> (+0.03%) ⬆️
#unittest 22.67% <12.63%> (-0.05%) ⬇️
Impacted Files Coverage Δ
ctrd/utils.go 64.7% <100%> (+4.7%) ⬆️
ctrd/container.go 51.96% <52.83%> (+1.79%) ⬆️
ctrd/image.go 76.85% <60%> (-4.43%) ⬇️
cri/v1alpha1/cri.go 65.36% <0%> (+0.17%) ⬆️
daemon/mgr/system.go 78.63% <0%> (+1.7%) ⬆️

ctrd/image.go Outdated
@@ -170,6 +220,9 @@ func (c *Client) PullImage(ctx context.Context, ref string, authConfig *types.Au

// start to pull image.
img, err := c.pullImage(ctx, wrapperCli, ref, options)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need to convert err here. the error is in the jsonstream message, not http status

ctrd/utils.go Outdated
"github.com/alibaba/pouch/pkg/utils"
"github.com/pkg/errors"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move it into next group

}

if errdefs.IsAlreadyExists(err) {
return errors.Wrap(errtypes.ErrAlreadyExisted, err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it;s ok to have duplicate error message. we might find the way to hack it and remove the duplicated message

@allencloud allencloud force-pushed the convert-err branch 7 times, most recently from 002144c to e6141d0 Compare August 6, 2018 03:23
Signed-off-by: Allen Sun <allensun.shl@alibaba-inc.com>
@allencloud
Copy link
Collaborator Author

ping @fuweid PTAL

Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fuweid fuweid merged commit 9482e6a into AliyunContainerService:master Aug 10, 2018
@allencloud allencloud deleted the convert-err branch August 10, 2018 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is bug report for project size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants