Support gzip and zstd HTTP transport compression to fetch remote resources #8166

laurazard · 2023-02-26T15:31:52Z

Golang net/http already transparently supports gzip compression.
This adds support for zstd compression

This PR takes over #7563 since @ndeloof is no longer working on this as per #7563 (comment).

Also took the chance to address some small comments in the previous PR.

k8s-ci-robot · 2023-02-26T15:32:01Z

Hi @laurazard. 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.

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.

laurazard · 2023-02-26T15:52:46Z

/cc @thaJeztah @AkihiroSuda

thaJeztah · 2023-02-27T12:54:46Z

Something weird in CI (Vagrant, Fedora 37); should not be related to this PR, but wondering if there's an issue somewhere; https://github.com/containerd/containerd/pull/8166/checks?check_run_id=11607343495

default: + chcon -v -t container_runtime_exec_t /usr/local/sbin/runc /usr/local/sbin/runc
    default: changing security context of '/usr/local/sbin/runc'
    default: changing security context of '/usr/local/sbin/runc'
==> default: Running provisioner: install-gotestsum (shell)...
    default: Running: inline script
    default: + /root/go/src/github.com/containerd/containerd/script/setup/install-gotestsum
    default: gotest.tools/gotestsum: go install gotest.tools/gotestsum: build output "/root/go/bin/gotestsum" already exists and is not an object file
The SSH command responded with a non-zero exit status. Vagrant
assumes that this means the command failed. The output for this command
should be in the log above. Please read the output to determine what
went wrong.

laurazard · 2023-02-27T15:12:13Z

Yeah I noticed it, seems like maybe something got stuck/didn't get cleaned up in the runner.

thaJeztah

changes LGTM

thaJeztah · 2023-03-08T12:33:06Z

@dmcgowan I see you added the 2.0 milestone to this; was there a discussion on this one?

dmcgowan · 2023-03-08T16:11:10Z

@thaJeztah no, this one never got discussed, labeled it as needs discussion. All PRs which need discussion got put in 2.0 or removed from 1.7 milestone.

For this PR in particular, I think this is missing a few things still. (1) A timeline for when we would expect official support in Golang, moving from built in protocol support to custom parsing/handling is not preferred for this type of functionality. (2) Justification, as @kzys asked in the previous PR, who is using this or trying to use it. (3) Testing, especially if we are going to accept this custom handling over what the built in http library does for us automatically.

laurazard · 2023-04-19T13:09:14Z

For (2), I've just updated the PR to add some tests for the custom compression handling. Re: (3), I think it's a bit of a chicken-and-egg problem – no reason for registries to support it if there are no clients, and vice versa, which is why it would be good to optionally add support for it.

thoughts @thaJeztah @dmcgowan?

gaby · 2023-04-19T13:16:29Z

@laurazard I'm subscribed to this PR since I'm trying to add zstd support to everything. There is one PR in Docker's official Registry. I'm pretty sure once the official registry add support for zstd others will too.

PR:
distribution/distribution#3784

laurazard · 2023-04-19T13:21:46Z

Thanks for the additional context @gaby! I can try to ping people about distribution/distribution#3784 too, see if we can get that moving again.

gaby · 2023-04-19T13:28:05Z

Found these ones at moby/moby:

moby/moby#43881
moby/moby#43331

laurazard · 2023-06-01T14:33:28Z

distribution/distribution#3900 got merged, maybe we can start taking a look at this again
/cc @dmcgowan

thaJeztah

One nit for the test, but otherwise LGTM

remotes/docker/fetcher_test.go

remotes/docker/fetcher.go

thaJeztah

LGTM

remotes/docker/fetcher.go

gaby · 2023-08-10T02:27:54Z

@laurazard @dmcgowan @thaJeztah Any plans to wrap up this feature? Seems to be ready, but there's a comment still unresolved

laurazard · 2023-09-12T04:55:58Z

@gaby Sorry, I've been out on leave a while and haven't looked at this. I've rebased and addressed the remaining comments now! Thoughts @dmcgowan?

gaby · 2023-09-19T04:08:52Z

@laurazard No worries, looking forward to this feature getting merged :-)

gaby · 2023-11-16T02:56:26Z

@dmcgowan Any chance at looking at this?

cpuguy83

LGTM just a style preference suggestion.

remotes/docker/fetcher.go

…urces Co-authored-by: Nicolas De Loof <nicolas.deloof@gmail.com> Signed-off-by: Laura Brehm <laurabrehm@hey.com>

laurazard · 2023-11-30T13:55:30Z

Applied suggestions + rebased ✅

thaJeztah

still LGTM

k8s-ci-robot added the needs-ok-to-test label Feb 26, 2023

k8s-ci-robot requested review from AkihiroSuda and thaJeztah February 26, 2023 15:52

dmcgowan added this to the 2.0 milestone Feb 28, 2023

thaJeztah approved these changes Mar 8, 2023

View reviewed changes

dmcgowan added the status/needs-discussion Needs discussion and decision from maintainers label Mar 8, 2023

laurazard force-pushed the gzip-gstd-compression branch from 2571776 to e424bef Compare April 19, 2023 12:59

k8s-ci-robot requested a review from dmcgowan June 1, 2023 14:33

thaJeztah reviewed Jun 1, 2023

View reviewed changes

remotes/docker/fetcher_test.go Outdated Show resolved Hide resolved

remotes/docker/fetcher.go Show resolved Hide resolved

laurazard force-pushed the gzip-gstd-compression branch from e424bef to aa3aad8 Compare June 2, 2023 08:37

thaJeztah approved these changes Jun 2, 2023

View reviewed changes

dmcgowan reviewed Jun 6, 2023

View reviewed changes

remotes/docker/fetcher.go Outdated Show resolved Hide resolved

laurazard force-pushed the gzip-gstd-compression branch 2 times, most recently from 3fe668d to 8d0b06c Compare September 12, 2023 04:55

cpuguy83 approved these changes Nov 16, 2023

View reviewed changes

remotes/docker/fetcher.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase label Nov 29, 2023

Support gzip and zstd HTTP transport compression to fetch remote reso…

2ead895

…urces Co-authored-by: Nicolas De Loof <nicolas.deloof@gmail.com> Signed-off-by: Laura Brehm <laurabrehm@hey.com>

laurazard force-pushed the gzip-gstd-compression branch from 8d0b06c to 2ead895 Compare November 30, 2023 13:51

k8s-ci-robot added size/L and removed needs-rebase labels Nov 30, 2023

laurazard self-assigned this Nov 30, 2023

estesp approved these changes Nov 30, 2023

View reviewed changes

thaJeztah approved these changes Nov 30, 2023

View reviewed changes

dmcgowan approved these changes Nov 30, 2023

View reviewed changes

dmcgowan added this pull request to the merge queue Nov 30, 2023

Merged via the queue into containerd:main with commit b8e3259 Nov 30, 2023
40 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support gzip and zstd HTTP transport compression to fetch remote resources #8166

Support gzip and zstd HTTP transport compression to fetch remote resources #8166

laurazard commented Feb 26, 2023 •

edited

k8s-ci-robot commented Feb 26, 2023

laurazard commented Feb 26, 2023

thaJeztah commented Feb 27, 2023

laurazard commented Feb 27, 2023

thaJeztah left a comment

thaJeztah commented Mar 8, 2023

dmcgowan commented Mar 8, 2023

laurazard commented Apr 19, 2023 •

edited

gaby commented Apr 19, 2023

laurazard commented Apr 19, 2023

gaby commented Apr 19, 2023

laurazard commented Jun 1, 2023

thaJeztah left a comment

thaJeztah left a comment

gaby commented Aug 10, 2023

laurazard commented Sep 12, 2023

gaby commented Sep 19, 2023

gaby commented Nov 16, 2023

cpuguy83 left a comment

laurazard commented Nov 30, 2023

thaJeztah left a comment

Support gzip and zstd HTTP transport compression to fetch remote resources #8166

Support gzip and zstd HTTP transport compression to fetch remote resources #8166

Conversation

laurazard commented Feb 26, 2023 • edited

k8s-ci-robot commented Feb 26, 2023

laurazard commented Feb 26, 2023

thaJeztah commented Feb 27, 2023

laurazard commented Feb 27, 2023

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Mar 8, 2023

dmcgowan commented Mar 8, 2023

laurazard commented Apr 19, 2023 • edited

gaby commented Apr 19, 2023

laurazard commented Apr 19, 2023

gaby commented Apr 19, 2023

laurazard commented Jun 1, 2023

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

gaby commented Aug 10, 2023

laurazard commented Sep 12, 2023

gaby commented Sep 19, 2023

gaby commented Nov 16, 2023

cpuguy83 left a comment

Choose a reason for hiding this comment

laurazard commented Nov 30, 2023

thaJeztah left a comment

Choose a reason for hiding this comment

laurazard commented Feb 26, 2023 •

edited

laurazard commented Apr 19, 2023 •

edited