Repository navigation
Skip unreadable entries in image list - #2157
Conversation
3edfdd2 to
fbea4e9
Compare
|
Hi @katiewasnothere, this PR is ready for review.
Could you please approve the pending workflow and review this PR when you have a chance? Thank you! |
481bf16 to
bb235f9
Compare
|
@egernst |
bb235f9 to
b7be756
Compare
|
@Hokila Can you add a reproduction procedure to your description to show how you're triggering the error? That will help us determine whether the changes we merged in apple/containerization#898 resolve this issue for the 1.3.1 release. Once we know how things behave with those changes, let's look at this PR and see what would be the best UX for handling broken resources (images in this case). |
|
Thanks @jglogan. I’ll update the PR description with the exact reproduction procedure and retest it against a build that includes apple/containerization#898. The current regression test covers per-image error isolation in the CLI, but it does not establish whether #898 already prevents or tolerates the underlying broken image-store state. I’ll document the before/after behavior once I’ve verified it with those changes. After that, I’m happy to revisit whether the CLI-side skip-and-warn behavior is still needed and what the best UX should be for broken image resources. |
|
Hi @jglogan, I’ve added the reproduction details and retested against 1.3.0, 1.3.1, and this PR. The issue still reproduces on 1.3.1, while this PR keeps readable images listed and writes the broken image warning to stderr. Would this skip-and-warn behavior be acceptable, or would you prefer a different UX here? |
|
I’ve moved the complete reproduction procedure and the 1.3.0/1.3.1 comparison The reproduction still confirms that containerization#898 does not cover the |
4e77336 to
f123677
Compare
f123677 to
71feeac
Compare
| images: images, | ||
| containerSystemConfig: containerSystemConfig, | ||
| onError: { image, error in | ||
| log.warning( |
There was a problem hiding this comment.
I don't like that the image list command now will always exit with 0 even if there was an error.
There was a problem hiding this comment.
Thanks let me know, fixed in b54383e: the command now outputs readable images, then exits non-zero if any image was skipped.
Code Coverage
|
|
@Hokila merged, thanks for making the image listing more robust! |
Type of Change
Motivation and Context
container image listcurrently fails completely when one image record referencesan unreadable or missing content blob. Healthy images are not rendered, and
structured output such as JSON and YAML is unavailable.
This PR resolves each image independently:
Fixes #2149.
Reproduction
Environment
docker.io/library/alpine:latestexample.invalid/broken:latestreferenced root content blob is absent
I used an isolated application root so that the reproduction does not affect
the normal container state:
I then added the following image record to
$APP_ROOT/state.jsonwithoutcreating its referenced content blob:
{ "example.invalid/broken:latest": { "digest": "sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff", "size": 1, "mediaType": "application/vnd.oci.image.index.v1+json" } }The following file was intentionally absent:
Behavior Before This PR
With container 1.3.0, before
apple/containerization#898, the followingcommands fail before rendering the healthy image:
Each command returns an error similar to:
The command exits with status
1.container image list -qcontinues to work because quiet mode does not resolvethe image content:
Verification Against
containerization#898I repeated the same reproduction with container 1.3.1, which includes the
changes from apple/containerization#898.
The detailed listing commands still fail with the same missing-content-blob
error, while quiet mode remains successful.
This indicates that
containerization#898does not cover this case: the imagestore state record is readable, but the content blob referenced by the image
record is missing. The failure occurs later when
container image listtries toresolve the image content.
Behavior With This PR
With this PR, the healthy image remains available and the broken image is
reported as a warning:
The same skip-and-warn behavior applies to table, verbose, JSON, and YAML
output. Only missing content and content decoding errors are skipped; other
errors, including service interruptions, propagate normally.
For example, JSON output can be captured separately from the warning:
The result is:
1after the readable images are rendered;images.jsoncontains the healthy image and remains valid JSON;warning.logidentifies the skipped image and the underlying error, thenreports that the listing was incomplete.
The healthy image can be inspected from the JSON output:
Scope and Non-Goals
This PR changes image-listing behavior only.
It does not:
container image inspect,pull, orrun;The broken image is still identified, and its error is written to stderr. The
change keeps readable entries available in the listing without corrupting
structured stdout, while returning a failure status for incomplete results.
Testing
swift test --filter ContainerCommandsTests: 41 tests in 12 suites passedmake check: formatting and license checks passedSummary
The underlying broken image resource remains unchanged, but
container image listcan now provide useful results instead of failing completely because of asingle unreadable image entry.
I use Codex help write this description