Skip to content

Automatically decompress archives for transfer service import#9864

Merged
mxpv merged 1 commit intocontainerd:mainfrom
hinshun:feature/import-compressed
Mar 19, 2024
Merged

Automatically decompress archives for transfer service import#9864
mxpv merged 1 commit intocontainerd:mainfrom
hinshun:feature/import-compressed

Conversation

@hinshun
Copy link
Copy Markdown
Contributor

@hinshun hinshun commented Feb 23, 2024

Currently, ctr image import as well as the transfer service import with NewImageImportStream does not support archives that are compressed (gzip, zstd).

I've tried passing a decompressed stream like so:

	decompressor, err := compression.DecompressStream(f)
	if err != nil {
		return nil, fmt.Errorf("failed to create stream decompressor: %w", err)
	}
	defer decompressor.Close()

	src := tarchive.NewImageImportStream(decompressor, "")

However, I believe there is a bug in the proxy transfer service because it fails during untarBlob with failed to read expected number of bytes: unexpected EOF. Patching containerd, I found it errors in this line with the following parameters: copied 34860544, size 34877440, ws.Offset 0, size-ws.Offset 34877440:

https://github.com/containerd/containerd/blob/v1.7.13/content/helpers.go#L184

When doing the decompression either with a local store local.NewStore or on the server-side (this patch), then it works perfectly fine.

@k8s-ci-robot
Copy link
Copy Markdown

Hi @hinshun. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Comment thread core/transfer/archive/importer.go Outdated
}
return archive.ImportIndex(ctx, store, iis.stream, opts...)

r, err := compression.DecompressStream(iis.stream)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be making use of the media type here? Possibly only running through this logic if the media type indicates compression or is empty.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can check for empty media type, but here the input is a compressed OCI tarball (with possibly uncompressed layers). Typically the media type here is ocispec.MediaTypeImageIndex and there isn't a compressed version afaik.

@hinshun hinshun force-pushed the feature/import-compressed branch from 7dd6887 to 34c5458 Compare March 15, 2024 02:14
@hinshun hinshun requested a review from dmcgowan March 15, 2024 02:14
@mxpv mxpv added this pull request to the merge queue Mar 19, 2024
github-merge-queue Bot pushed a commit that referenced this pull request Mar 19, 2024
Automatically decompress archives for transfer service import
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Mar 19, 2024
@mxpv mxpv added this pull request to the merge queue Mar 19, 2024
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Mar 19, 2024
@mxpv mxpv added this pull request to the merge queue Mar 19, 2024
Merged via the queue into containerd:main with commit 124456e Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants