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

image list: check for all errors #8151

Merged
merged 1 commit into from
Oct 27, 2020

Conversation

vrothberg
Copy link
Member

@vrothberg vrothberg commented Oct 26, 2020

For unknown historical reasons, some errors were ignored when listing
images. I assume that the basic assumption was that if we can properly
list images, we can also successfully compute their sizes which turned
out to be wrong.

Signed-off-by: Valentin Rothberg [email protected]

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2020
@edsantiago
Copy link
Member

Close:

$ ./bin/podman images
Error: top layer 1f832d5208105d5dde3f814d391ff7b4ddb557fd3bdbcb79418906242772dc73 of image a7a37f74ff864eec55891b64ad360d07020827486e30a92ea81d16459645b26a not found in layer tree
$ ./bin/podman images -a
ERRO[0000] Error getting image size: unable to determine size: error locating layer with ID "1f832d5208105d5dde3f814d391ff7b4ddb557fd3bdbcb79418906242772dc73": layer not known
REPOSITORY                TAG       IMAGE ID      CREATED       SIZE
quay.io/libpod/testimage  20200929  766ff5a3a7e4  3 weeks ago   7.75 MB
quay.io/libpod/fedora     31        a7a37f74ff86  5 months ago  0 B

@vrothberg
Copy link
Member Author

@edsantiago, I update the errors. Does that meet your expectations?

@edsantiago
Copy link
Member

With 9029493 I get:

$ ./bin/podman images
Error: top layer 1f832d5208105d5dde3f814d391ff7b4ddb557fd3bdbcb79418906242772dc73 of image a7a37f74ff864eec55891b64ad360d07020827486e30a92ea81d16459645b26a not found in layer tree
$ ./bin/podman images -a
Error: error retrieving size of image "a7a37f74ff864eec55891b64ad360d07020827486e30a92ea81d16459645b26a": you may need to remove the image to resolve the error: unable to determine size: error locating layer with ID "1f832d5208105d5dde3f814d391ff7b4ddb557fd3bdbcb79418906242772dc73": layer not known
$ ./bin/podman image inspect a7a
Error: stat /home/esm/.local/share/containers/storage/overlay/1f832d5208105d5dde3f814d391ff7b4ddb557fd3bdbcb79418906242772dc73: no such file or directory

@vrothberg
Copy link
Member Author

I think we should split that in two work items. First, we need to get the nil deref fixed. Dealing with a corrupted image seems to open a whole universe of ugly messages.

The latter will be tricky.

@edsantiago
Copy link
Member

Okay, in order of decreasing priority (IMHO):

  1. podman images -a should not crash
  2. podman images should at a very minimum show existing valid images
  3. podman images should deal with rotten images. Maybe show them as errored, maybe ignore them, maybe ignore them with a warning, I don't know.

But right now I have a system that is not in good shape, and I don't want any end users winding up in the same situation.

@vrothberg
Copy link
Member Author

vrothberg commented Oct 26, 2020

Sounds reasonable to me. This PR gets us to 1).

2) and 3) will need some tinkering in the backend. I like the idea to show only "healthy" images and list the others separately.

So far, I have seen such corrupted images only when builds, or image commits in general (also happens during pull), have been killed or been interrupted (OEM kills, power outage, etc.). In that case, removing the image is the only option I can think of. But I'd leave that decision open to users.

@mtrmac @rhatdan PTAL

@edsantiago
Copy link
Member

Is there some easy way to get podman images to emit something at least partly useful? Right now (9029493) I, a simple naive user, am getting the top layer error with no indication of what is wrong; and it is not obvious to me that I should run podman images -a to get a different slightly less unhelpful error message.

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

The error checks LGTM.

I haven’t checked in detail whether removing the image is likely to resolve the error – or if it is even always possible; it does seem reasonable just based on the quoted error messages.

@edsantiago
Copy link
Member

It is not (possible to remove the image):

$ podman rmi a7a3
Error: 1 error occurred:
        * top layer 1f832d5208105d5dde3f814d391ff7b4ddb557fd3bdbcb79418906242772dc73 of image a7a37f74ff864eec55891b64ad360d07020827486e30a92ea81d16459645b26a not found in layer tree

Even -a -f fails:

$ podman rmi -a -f
Error: 3 errors occurred:
        * top layer 1f832d5208105d5dde3f814d391ff7b4ddb557fd3bdbcb79418906242772dc73 of image a7a37f74ff864eec55891b64ad360d07020827486e30a92ea81d16459645b26a not found in layer tree
        * top layer 1f832d5208105d5dde3f814d391ff7b4ddb557fd3bdbcb79418906242772dc73 of image a7a37f74ff864eec55891b64ad360d07020827486e30a92ea81d16459645b26a not found in layer tree
        * unable to delete all images, check errors and re-run image removal if needed

I could rm -rf ~/.local/share/containers but I don't think we should force end users to do (or even know about) this.

@edsantiago
Copy link
Member

I'm tentatively ok with this PR, but please remove the 'Fixes' comment from both the commit message and the Github description.

For unknown historical reasons, some errors were ignored when listing
images.  I assume that the basic assumption was that if we can properly
list images, we can also successfully compute their sizes which turned
out to be wrong.

Signed-off-by: Valentin Rothberg <[email protected]>
@vrothberg
Copy link
Member Author

I'm tentatively ok with this PR, but please remove the 'Fixes' comment from both the commit message and the Github description.

done

@edsantiago
Copy link
Member

/lgtm
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 27, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 27, 2020
@mheon
Copy link
Member

mheon commented Oct 27, 2020

LGTM
/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 27, 2020
@openshift-merge-robot openshift-merge-robot merged commit 0f0d857 into containers:master Oct 27, 2020
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants