Fixes for imgutil from recent refactor #267
Conversation
Signed-off-by: Natalie Arellano <[email protected]>
| @@ -46,6 +46,11 @@ func (i *Image) Identifier() (imgutil.Identifier, error) { | |||
| }, nil | |||
| } | |||
|
|
|||
| func (i *Image) Valid() bool { | |||
| // local images are actually always invalid, because they are missing a manifest, but let's ignore that for now | |||
There was a problem hiding this comment.
This was causing the analyzer not to ever find the previous image
| layer, err := store.LayerByDiffID(diffID) | ||
| if err == nil { | ||
| return layer.Size() | ||
| } | ||
| if err = store.downloadLayersFor(imageID); err != nil { |
There was a problem hiding this comment.
Oops! We may ask a layer for its size just to construct a (missing stuff but good enough for our purposes) manifest. I didn't catch this in my earlier performance testing because the tests for WithPreviousImage (where this is relevant) use a hard-coded image, not the "runnable base image".
Previously, we forced the layer to be downloaded by asking for its size. But, this has performance implications when you only need the size to construct a manifest. Now, we force the layer to be downloaded by opening a ReadCloser to the layer, and size only reports if the layer is present on disk, it doesn't cause the layer to be downloaded. Signed-off-by: Natalie Arellano <[email protected]>
| if size == -1 { // layer facade fronting empty layer | ||
| layerName = fmt.Sprintf("blank_%d", blankIdx) | ||
| blankIdx++ | ||
| hdr := &tar.Header{Name: layerName, Mode: 0644, Size: 0} | ||
| if err := tw.WriteHeader(hdr); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Moved to s.addLayerToTar
| func (s *Store) addLayerToTar(tw *tar.Writer, layer v1.Layer, blankIdx *int) (string, error) { | ||
| // If the layer is a previous image layer that hasn't been downloaded yet, | ||
| // cause ALL the previous image layers to be downloaded by grabbing the ReadCloser. | ||
| layerReader, err := layer.Uncompressed() |
There was a problem hiding this comment.
Previously, we forced the layer to be downloaded by asking for its size.
But, this has performance implications when you only need the size to construct a manifest.
Now, we force the layer to be downloaded by opening a ReadCloser to the layer,
and size only reports if the layer is present on disk, it doesn't cause the layer to be downloaded.
I'm not sure how this ever worked Signed-off-by: Natalie Arellano <[email protected]>
|
@jabrown85 @jjbustamante one more... 🤦🏼♀️ |
As surfaced in buildpacks/lifecycle#1335