skopeo: Get uncompressed layer when pulling from containers-storage #210
Conversation
Johan-Liebert1
force-pushed
the
containers-storage-fix
branch
2 times, most recently
from
282f2a7 to
3614a62
Compare
crates/composefs-oci/src/skopeo.rs
Outdated
|
|
||
| /// A backend/transport for OCI/Docker images. | ||
| #[derive(Debug)] | ||
| pub enum Transport { |
There was a problem hiding this comment.
ref youki-dev/oci-spec-rs#205 and we have some duplicate code in ostree-rs-ext too
There was a problem hiding this comment.
In the short term I'd probably say we try to lower this to containers-image-proxy-rs ?
There was a problem hiding this comment.
Or at the very least, this probably shouldn't be pub since we're not exposing it via any API, and it would avoid confusion with the duplicate in ostree-ext.
| let blob_reader = blob_reader.take(descriptor.size()); | ||
| let blob_reader = blob_reader.take(size); | ||
|
|
||
| let bar = self.progress.add(ProgressBar::new(descriptor.size())); |
There was a problem hiding this comment.
This should be updated to use size as above, right? Otherwise the progress bar is going to scale based on the compressed size even in the uncompressed case.
There was a problem hiding this comment.
Right, I missed that
crates/composefs-oci/src/skopeo.rs
Outdated
| let permit = Arc::clone(&sem).acquire_owned().await?; | ||
| let descriptor = mld.clone(); | ||
|
|
||
| // SAFETY: We get descriptor from the manifest_layers itself |
There was a problem hiding this comment.
I haven't looked at the surrounding context of this function, but just based on the local information manifest_layers is passed into the function as &[Descriptor], which could easily be an empty slice and makes this unwrap() safety assumption incorrect.
There was a problem hiding this comment.
Yeah, I think that's fair. Better to print an error than crash
|
This one also relates to bootc-dev/containers-image-proxy-rs#114 and I think it might clean things up a lot to push down into the proxy the handling for decompression too. |
|
Opened bootc-dev/containers-image-proxy-rs#120 moving the |
Johan-Liebert1
force-pushed
the
containers-storage-fix
branch
3 times, most recently
from
eff4067 to
e3b8ac2
Compare
|
Requires a release for |
Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Johan-Liebert1
force-pushed
the
containers-storage-fix
branch
from
e3b8ac2 to
8e5bc2e
Compare
Johan-Liebert1
force-pushed
the
containers-storage-fix
branch
3 times, most recently
from
dd25c1f to
fe40918
Compare
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
This looks good, but I think in a future refactoring we should change things so OpenImage always grabs both the stock and uncompressed manifest and then have the operations default to using it.
When pulling from containers storage we were requesting a layer using the compressed sha digest for that particular layer which caused skopeo to throw the error ``` failed to invoke method GetBlob: locating item named "sha256:<snip>" for image with ID "<snip>" (consider removing the image to resolve the issue): file does not exist ``` To remedy this, when pulling from containers storage we now query using the `get_blob` method using the uncompressed sha digest for the layer Related: bootc-dev/bootc#1703 Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Johan-Liebert1
force-pushed
the
containers-storage-fix
branch
from
fe40918 to
87dece1
Compare
|
@jeckersb blocked on your change request |
|
ping @jeckersb |
jeckersb
left a comment
jeckersb
left a comment
There was a problem hiding this comment.
Doh sorry, still catching up on mail/notifications...
Ship it! 🚢
3 participants
When pulling from containers storage we were requesting a layer using the compressed sha digest for that particular layer which caused skopeo to throw the error
To remedy this, when pulling from containers storage we now query using the
get_blobmethod using the uncompressed sha digest for the layerRelated: bootc-dev/bootc#1703