images: Fix read past end of file for pull --cache #152
Conversation
|
Testing here: cilium/cilium#30447 EDIT: The PR / workflow above is invoking the |
|
OK, looks like the latest version here was now successful, by manually overriding the LVH binary in the workflow: https://github.com/cilium/cilium/pull/30447/files |
Pull has two modes:
- No cache: Extract and decompress the target VM image from the docker
filesystem to the host filesystem. Result: One file, image.qcow.
- Cache: Same as no cache, but also copy the compressed image to the
host filesystem. Result: two files, image.qcow{,.zst}
Previously, the logic for extracting the target VM image from the docker
image with --cache enabled was to:
- Extract and decompress the target file directly from inside the docker
image filesystem to the host filesystem
- Do not seek the source Reader back to the start of the file
- Attempt (and fail) to copy the entire compressed image also to the
host filesystem, but starting from the end of the source file.
- This resulted in an EOF error because the reader wasn't reset to start
from the beginning.
Unfortunately, tar.Reader doesn't expose a way to seek back to the start
of the source file. We could perhaps get an additional reference to a
reader starting from the beginning of the current tar header in order to
reset back to the beginning of the target file in cache mode, but this
was getting a bit complicated.
The solution in this patch is to:
- Copy the compressed file to a temporary path on the host filesystem
- Extract the compressed file from this temporary host-side cache to the
intended destination directory.
- If in cache mode, move the temporary file also to the output
directory. Otherwise, delete the temporary file.
There's one minor extra case, if the qcow inside the docker image is not
compressed at all then we skip decompression, and in that case we also
need to move the temporary path to the destination directory.
Fixes: ea09cb7 ("images: Add docker pull support")
Reported-by: Lorenz Bauer <[email protected]>
Signed-off-by: Joe Stringer <[email protected]>
lmb
force-pushed
the
pr/joe/fix-pull-cache-eof
branch
from
e3a5248 to
1dfca60
Compare
|
👋 I'm here as cilium/cilium#30478 is updating the lvh action to |
|
i think the issue is that the action is still using v0.0.14: https://github.com/cilium/cilium/actions/runs/7708590978/job/21008029780#step:8:77. i opened #157 to update the release process so that the docker image tag matches the action version. would be interesting to see if cilium/cilium#30478 works if it uses |
|
Yeah it looks like the action has a variable input for |
|
still failing with v0.0.15. struggle: https://github.com/cilium/cilium/actions/runs/7714824322/job/21028190169 |
Pull has two modes:
filesystem to the host filesystem. Result: One file, image.qcow.
host filesystem. Result: two files, image.qcow{,.zst}
Previously, the logic for extracting the target VM image from the docker
image with --cache enabled was to:
image filesystem to the host filesystem
host filesystem, but starting from the end of the source file.
from the beginning.
Unfortunately, tar.Reader doesn't expose a way to seek back to the start
of the source file. We could perhaps get an additional reference to a
reader starting from the beginning of the current tar header in order to
reset back to the beginning of the target file in cache mode, but this
was getting a bit complicated.
The solution in this patch is to:
intended destination directory.
directory. Otherwise, delete the temporary file.
There's one minor extra case, if the qcow inside the docker image is not
compressed at all then we skip decompression, and in that case we also
need to move the temporary path to the destination directory regardless
of the cache flag setting.
Fixes: #133
Fixes: ea09cb7 ("images: Add docker pull support")
Reported-by: Lorenz Bauer [email protected]
Signed-off-by: Joe Stringer [email protected]