-
Notifications
You must be signed in to change notification settings - Fork 42
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
Fix: archive/tar: write too long
error when using Docker Desktop containerd backend
#275
Conversation
…ntainerd backend Fixes buildpacks/pack#2174 This is somewhat of a band-aid solution and degrades performance because we have to read the uncompressed layer bits to get the uncompressed layer size for layers that we `docker save` out of the daemon (re-used layers and all the base layers). The better longer-term fix will be to send OCI-layout formatted tars when we have all the layers available (when using containerd as the backing store, this will always be true). Signed-off-by: Natalie Arellano <[email protected]>
if err != nil { | ||
return err | ||
} | ||
return i.AddLayerWithHistory(layer, emptyHistory) | ||
} | ||
|
||
func calculateChecksum(path string) (string, 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.
This function relies on the daemon always outputting uncompressed layers, which is no longer true
} | ||
if fileSize == compressedSize { | ||
// the layer is compressed, we don't know the uncompressed size | ||
uncompressedSize = -1 |
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 real fix
@@ -288,11 +289,13 @@ func (s *Store) getLayerSize(layer v1.Layer) (int64, error) { | |||
return 0, err | |||
} | |||
knownLayer, layerFound := s.onDiskLayersByDiffID[diffID] | |||
if layerFound { | |||
if layerFound && knownLayer.uncompressedSize != -1 { |
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 real fix
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.
Can we add a unit test for this? or do we need docker 25 in the runner to be able to test it?
Yeah, we need it. And, containerd should be enabled as the backing storage. It's a bit of a mess. |
Fixes (in part) buildpacks/pack#2174
This is somewhat of a band-aid solution and degrades performance because we have to read the uncompressed layer bits to get the uncompressed layer size for layers that we
docker save
out of the daemon (re-used layers and all the base layers).The better longer-term fix will be to send OCI-layout formatted tars when we have all the layers available (when using containerd as the backing store, this will always be true).