-
Notifications
You must be signed in to change notification settings - Fork 372
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
ostree, src: support copy of compressed layers #392
ostree, src: support copy of compressed layers #392
Conversation
03fa006
to
15961b8
Compare
15961b8
to
b706d9a
Compare
Looks ok, @mtrmac wdyt? |
ping |
cc @nalind |
5987770
to
5f2b693
Compare
ostree/ostree_dest.go
Outdated
digester := digest.Canonical.Digester() | ||
tee := io.TeeReader(its, digester.Hash()) | ||
|
||
written, err := io.Copy(ioutil.Discard, tee) |
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.
Isn’t this basically io.Copy(digester, its)
?
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.
correct, thanks for finding it. Fixed it in a newer version
ostree/ostree_src.go
Outdated
} | ||
|
||
var schema manifestSchema | ||
if err := json.Unmarshal(manifestBlob, &schema); err != nil { |
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.
Please use the containers/image/manifest
package (manifest.FromBlob
), both for manifest types and for manifest parsing.
This doesn't work as expected because LayersDescriptors
and FSLayers
list layers in the opposite directions; besides, the manifestSchema
franken-type containing a subset of both schema1 and schema2, and undocumented, is pretty surprising.
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.
missed this comment in the last revision. Going to address it now.
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.
done now
ostree/ostree_src.go
Outdated
ref ostreeReference | ||
tmpDir string | ||
repo *C.struct_OstreeRepo | ||
compressed map[digest.Digest]digest.Digest |
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.
Please at least add a comment here saying what is the key and what is the value of the map. (Ideally the field name would be self-explanatory, but that seems difficult.)
ostree/ostree_src.go
Outdated
blob := info.Digest.Hex() | ||
compressedBlob, found := s.compressed[info.Digest] |
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.
It’s not in general guaranteed that LayerInfosForCopy
will be called before GetBlob
; if GetBlob
depends on this map, it may need to build it on demand.
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.
thanks, I've modified it to check if compressed is initialized, and use LayerInfosForCopy
when it is nil
5790ef7
to
961bfca
Compare
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 otherwise.
ostree/ostree_src.go
Outdated
MediaType: layerBlob.MediaType, | ||
} | ||
s.compressed[uncompressedDigest] = layerBlob.Digest | ||
updatedBlobInfos = append([]types.BlobInfo{blobInfo}, updatedBlobInfos...) |
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.
LayerInfosForCopy
should return layers in the order of man.LayerInfos
, not reversed.
ostree/ostree_src.go
Outdated
updatedBlobInfos := []types.BlobInfo{} | ||
manifestBlob, manifestType, err := s.GetManifest(nil) | ||
if err != nil { | ||
return nil |
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.
Not sure about some other cases, but at least in this one it seems clear that err
shouldn’t be thrown away; can you add an error
return value to LayerInfosForCopy
?
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.
this is the signature from the ImageSource
interface. The only user of this function in ostree_src
is GetBlob
and I handle there the nil
return, which is considered an error.
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.
Right; please change the interface. c/i/copy/imageCopier.copyLayers
should be able to tell the difference between “no changes needed” and “reading manifest failed” / ”manifest invalid”.
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.
I've pushed a new change to change the interface
975896a
to
d80ea35
Compare
ostree/ostree_src.go
Outdated
uncompressedSize, err := strconv.ParseInt(uncompressedSizeStr, 10, 64) | ||
if err != nil { | ||
return nil, nil | ||
} |
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.
Is it reasonable to ignore the above errors? At least the last one seems potentially worth failing on.
Up to you.
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.
thanks, I've changed that to return an error. Also, I have added a patch that will trigger the layer to be pulled if this metadata is missing.
@@ -177,18 +177,18 @@ func (s *storageImageSource) GetManifest(instanceDigest *digest.Digest) (manifes | |||
|
|||
// LayerInfosForCopy() returns the list of layer blobs that make up the root filesystem of | |||
// the image, after they've been decompressed. | |||
func (s *storageImageSource) LayerInfosForCopy() []types.BlobInfo { | |||
func (s *storageImageSource) LayerInfosForCopy() ([]types.BlobInfo, error) { |
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.
At least some of the errors in here should probably cause copy.Image
to fail.
Cc: @nalind
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.
I preferred to not touch this part as I am not really familiar with it. I can change it if it is required for this PR
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.
Generally, if storageImageSource.LayerInfosForCopy()
is returning nil
, the copy attempt is going to fail later on when the digests don't match, since I think the majority of images use compressed layers. So yeah, pretty much anywhere we return nil
from there, we should start returning the error value that caused us to do so.
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.
I've added a patch that changes LayerInfosForCopy
in storage/storage_image.go
to propagate the errors.
d80ea35
to
0d3a4d0
Compare
Thanks! LGTM, will wait a bit to give @nalind time to comment on |
👍 for the record; let’s at least track |
@runcom PTAL |
I'm waiting on #392 (comment) |
I took care of the last comment I got on this PR |
@runcom PTAL |
0f47cc9
to
1562f1c
Compare
rebased ⬆️ |
1562f1c
to
a4df631
Compare
we will use it later for LayerInfosForCopy Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
it enables the copy of compressed images. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
a4df631
to
755cd3f
Compare
rebased again. Could we please move this forward? |
@runcom ping |
add implementation for LayerInfosForCopy so we can provide a modified manifest to be used for
copy