-
Notifications
You must be signed in to change notification settings - Fork 243
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
Fall back from partial pull when on VFS #2140
Conversation
Possibly fixes containers/podman#24308 . |
Confirming that this fixes the single reproducer in containers/podman#24308 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
return nil, errors.New("both zstd:chunked and eStargz TOC found") | ||
} | ||
|
||
// convertImages also serves as a “must not fallback to non-partial pull” option (?!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, for now at least it does. Because if that is set, we expect that every image is pulled using the partial pull code path, so that we can enforce composefs.
/lgtm |
... to centralize the fallback allowed / required logic. Should not change behavior, apart from maybe some error text. Signed-off-by: Miloslav Trmač <[email protected]>
Instead of sharing the badRequestErr logic, duplicate it. That's a bit ugly, but we get better debug messages and a more traditional control flow. Should not change behavior, except for debug messages. Signed-off-by: Miloslav Trmač <[email protected]>
Use switch to handle the various TOC presence cases Signed-off-by: Miloslav Trmač <[email protected]>
... and other graph drivers which don't support partial pulls. Signed-off-by: Miloslav Trmač <[email protected]>
> go mod edit -replace github.com/containers/storage=github.com/mtrmac/storage@vfs-fallback Signed-off-by: Miloslav Trmač <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Merge at will.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, mtrmac, 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 |
Signed-off-by: Miloslav Trmač <[email protected]>
Signed-off-by: Miloslav Trmač <[email protected]>
First refactor the logic a bit, to separate “are partial pulls allowed” from “set up partial pulls”.
See individual commits.
@giuseppe RFC on the logic, WRT composefs integrity enforcement.
Currently untested, that’s going to happen in containers/podman#24287 .