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: list image should ignore error if containerd can't handle well #1625

Merged
merged 1 commit into from
Jul 5, 2018
Merged

bugfix: list image should ignore error if containerd can't handle well #1625

merged 1 commit into from
Jul 5, 2018

Conversation

fuweid
Copy link
Contributor

@fuweid fuweid commented Jul 5, 2018

Ⅰ. Describe what this PR did

Since the containerd doesn't identify the application/octet-stream media
type, it will cause ListImages API fails to return images to the caller.
In order to avoid odd thing happen and add patch to containerd, we
should ignore the error to make PouchContainer robust.

Ⅱ. Does this pull request fix one issue?

Fixed #1583

Ⅲ. Describe how you did it

Ignore the error.

Ⅳ. Describe how to verify it

  1. pouch pull redis:2
  2. pouch images
➜  pouch git:(bugfix_make_image_list_successful) pouch images
IMAGE ID       IMAGE NAME                                       SIZE
8c811b4aec35   docker.io/library/busybox:1                      710.81 KB
8c811b4aec35   docker.io/library/busybox:1-uclibc               710.81 KB
8c811b4aec35   registry.hub.docker.com/library/busybox:latest   710.81 KB
  1. check the pouch daemon log
WARN[2018-07-05T10:22:06.69910634+08:00] encountered unknown type application/octet-stream; children may not be fetched
WARN[2018-07-05T10:22:06.703451915+08:00] failed to convert containerd image(registry.hub.docker.com/library/redis:2) to ImageInfo during list images: unknown image config media type application/octet-stream

Ⅴ. Special notes for reviews

Since the containerd doesn't identify the application/octet-stream media
type, it will cause ListImages API fails to return images to the caller.
In order to avoid odd thing happen and add patch to containerd, we
should ignore the error to make PouchContainer robust.

Signed-off-by: Wei Fu <fhfuwei@163.com>
@pouchrobot pouchrobot added kind/bug This is bug report for project size/XS labels Jul 5, 2018
@HusterWan
Copy link
Contributor

LGTM, after ci pass, i will merge this PR

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Jul 5, 2018
@@ -150,7 +150,8 @@ func (mgr *ImageManager) ListImages(ctx context.Context, filter ...string) ([]ty
for _, img := range imgs {
imgCfg, err := img.Config(ctx)
if err != nil {
return nil, err
logrus.Warnf("failed to get image config info during list images: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should print some WARN info in cli side , like ctr :

#ctr --address /var/run/containerd.sock images ls
WARN[0000] encountered unknown type application/octet-stream; children may not be fetched
WARN[0000] encountered unknown type application/octet-stream; children may not be fetched

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is quite hard to do so in cli side. Since cli is heavily relying on the api, and if the api side does not return the error or warning message, nothing could be done in cli side, I think.

@codecov-io
Copy link

codecov-io commented Jul 5, 2018

Codecov Report

Merging #1625 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1625      +/-   ##
==========================================
- Coverage   41.99%   41.99%   -0.01%     
==========================================
  Files         270      270              
  Lines       17597    17599       +2     
==========================================
  Hits         7390     7390              
- Misses       9298     9300       +2     
  Partials      909      909
Impacted Files Coverage Δ
daemon/mgr/image.go 68.46% <0%> (-0.63%) ⬇️

@allencloud allencloud merged commit d600783 into AliyunContainerService:master Jul 5, 2018
@fuweid fuweid deleted the bugfix_make_image_list_successful branch August 3, 2018 02:51
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 LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] fail to list images if the image contains the unknown reference
5 participants