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: pull non-exist image in container create #1764

Merged
merged 1 commit into from
Jul 24, 2018
Merged

bugfix: pull non-exist image in container create #1764

merged 1 commit into from
Jul 24, 2018

Conversation

Ace-Tang
Copy link
Contributor

Signed-off-by: Ace-Tang [email protected]

Ⅰ. Describe what this PR did

Pull image if not exist in container create process.

# pouch  create   --net=none reg.docker.alibaba-inc.com/base/busybox:latest top
Error: failed to create container: {"message":"image: reg.docker.alibaba-inc.com/base/busybox:latest: not found"}

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@pouchrobot pouchrobot added kind/bug This is bug report for project size/XS labels Jul 22, 2018
@codecov-io
Copy link

codecov-io commented Jul 22, 2018

Codecov Report

Merging #1764 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1764      +/-   ##
==========================================
+ Coverage    56.3%   56.33%   +0.02%     
==========================================
  Files         200      200              
  Lines       15657    15659       +2     
==========================================
+ Hits         8816     8821       +5     
+ Misses       5745     5744       -1     
+ Partials     1096     1094       -2
Flag Coverage Δ
#critest 33.5% <ø> (+0.02%) ⬆️
#integrationtest 37.65% <ø> (-0.01%) ⬇️
#unittest 20.32% <ø> (-0.01%) ⬇️
Impacted Files Coverage Δ
apis/server/utils.go 61.9% <0%> (-4.77%) ⬇️
daemon/logger/jsonfile/utils.go 71.54% <0%> (-1.63%) ⬇️
daemon/containerio/cio.go 62.5% <0%> (-1%) ⬇️
cri/v1alpha2/cri.go 66.13% <0%> (+0.17%) ⬆️
daemon/mgr/container.go 53.46% <0%> (+0.31%) ⬆️
ctrd/container.go 50.17% <0%> (+1.37%) ⬆️

@allencloud
Copy link
Collaborator

Please add an integration test in the CI, so that this case could be covered forever. Thanks a lot.

@@ -60,6 +60,10 @@ func (cc *CreateCommand) runCreate(args []string) error {

ctx := context.Background()
apiClient := cc.cli.Client()
if err := pullMissingImage(ctx, apiClient, config.Image, false); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This functionality is just implemented in the CLI side, not the API server side?
I am wondering if this is reasonable? @Ace-Tang @fuweid
Just double check this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The best will be done in api server, hope someone can do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@allencloud @Ace-Tang I don't think it should be done in the API server, because we don't know the client want to pull the missing image and it will make the API heavy. Currently, the Pull API allows the caller to do the customize like pullMissingImage. It's good enough, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you said, @fuweid , one api just do one thing, seems make sense.

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.

The code change look goods. But please add integration testing for this functionality 😄

@pouchrobot pouchrobot added size/S and removed size/XS labels Jul 24, 2018
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 f369ee7 into AliyunContainerService:master Jul 24, 2018
@Ace-Tang Ace-Tang deleted the create_miss_image branch July 24, 2018 07:57
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/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants