Content encoding negotiation added to archive request. #36164

emil2k · 2018-01-31T13:19:00Z

- What I did
Enabled content encoding negotiation to speed up retrieval of container archives.

A better solution might be, some sort of middleware applied to most endpoints on the server and from the client side an Accept-Encoding header could be set either using a client's customHTTPHeaders attribute or in individual methods.

I'm not really sure this is the best way to do this, but I'm posting this here to get feedback and guidance.

- How I did it
Parses the Accept-Encoding header in handler for GET /containers/{id}/archive requests, determines the best encoding, compresses the files, and adds the appropriate Content-Encoding header.

- How to verify it
I'm not sure how to add a test for this, looking for feedback.

- Description for the changelog
Content encoding negotiation added to archive request.

dnephin · 2018-01-31T17:44:09Z

hmm, it looks like our vendor check is broken. I believe github.com/golang/gddo was added as a new dependency but not added to vendor.conf. vendor should have failed because of this.

dnephin

The new dependency needs to be added to vendor.conf I will look into why it didn't fail.

The approach seems reasonable to me. A middleware might be too much because it's not necessarily safe for all cases, but moving it out to a function should at least let us re-use it.

dnephin · 2018-01-31T20:04:36Z

api/server/router/container/copy.go

This suggests that maybe the content-type should change as well?

I think since the Content-Encoding header is being set to gzip the Content-Type should stay as application/x-tar as it is the underlying content type after decoding:

The Content-Encoding entity header is used to compress the media-type. When present, its value indicates which encodings were applied to the entity-body. It lets the client know, how to decode in order to obtain the media-type referenced by the Content-Type header.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Encoding

dnephin · 2018-01-31T20:11:42Z

api/server/router/container/copy.go

I think this code should go into a function so that we'll be able to re-use it later.

func writeCompressedResponse(r *http.Request, w http.ResponseWriter, body io.Reader) error { ... }

dnephin · 2018-01-31T20:20:15Z

client/container_copy.go

Interesting!

We have ioutils.ReadCloserWrapper from github.com/docker/docker/pkg/ioutils which does this as well without the anonymous struct. I think it would be good to use that.

Is it really safe to defer gr.Close() here? Doesn't that close the Reader before it's used? I think you would need to use a closer on ioutils.ReadCloserWrapper that closes both Readers.

Good catch on the closer. I need to setup some stuff to develop on the server side, so I hadn't really tested this.

dnephin · 2018-01-31T20:20:17Z

client/container_copy.go

should also be a new function readResponsebody(resp serverResponse) (io.ReadCloser, error).

emil2k · 2018-02-05T15:38:12Z

@dnephin I made some fixes as you suggested.

On the client side, as long as the http.Transport's DisableCompression field is set to false there is no need to manually set Accept-Encoding and then decode based on the Content-Encoding header so all I did was set DisableCompression explicitly to false in the client constructors.

dnephin

Looks good.

I think it could use a test for writeCompressedResponse(), but feel free to wait until someone else has reviewed the design

dnephin · 2018-02-05T20:43:24Z

client/client.go

Nice find! I didn't know about this

emil2k · 2018-02-14T14:01:37Z

Resolved conflicts and rebased this on master.

vdemeester

LGTM 🐸
Should we split the vendoring to the code in two separate commits ? 👼

emil2k · 2018-02-16T15:49:52Z

@vdemeester It is up to you guys, I have no preference on splitting up the commit.

thaJeztah · 2018-02-19T15:56:49Z

Given that this was opened to improve performance, do we need bench tests for this?

Silly question; is the compression done "streaming" or is the content read into memory first?

thaJeztah · 2018-02-19T15:57:06Z

ping @unclejack @cpuguy83 PTAL

emil2k · 2018-02-19T21:07:52Z

Given that this was opened to improve performance, do we need bench tests for this?

I'm not sure benchmarking is necessary in this case, because it would simply be testing the compression rate which would depend on the particular data chosen for the benchmark.

Silly question; is the compression done "streaming" or is the content read into memory first?

The data is being compressed in as a stream as it is written to the client.

emil2k · 2018-02-22T12:07:46Z

Rebased it on master to resolve conflicts.

thaJeztah · 2018-02-22T19:34:13Z

client/client.go

This should be the default, so not needed

I specified it to be explicit that the proper behavior of the client depends on that property value.

Also, if for whatever reason the DisableCompression field is removed or renamed the build would break and then things could be corrected.

It's up to you though, do you want me to remove the setting?

if for whatever reason the DisableCompression field is removed or renamed

I regard the Go team highly enough that I'm confident that would never happen. I think it's safe to remove.

Ok, I have removed this setting and rebased on master.

thaJeztah · 2018-02-22T19:34:23Z

client/client.go

tiborvass · 2018-02-22T19:41:48Z

LGTM

codecov · 2018-02-24T05:36:09Z

Codecov Report

Merging #36164 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #36164      +/-   ##
==========================================
+ Coverage   34.64%   34.65%   +<.01%     
==========================================
  Files         612      612              
  Lines       45310    45310              
==========================================
+ Hits        15696    15700       +4     
+ Misses      27556    27553       -3     
+ Partials     2058     2057       -1

Signed-off-by: Emil Davtyan <emil2k@gmail.com>

emil2k requested a review from dnephin as a code owner January 31, 2018 13:19

GordonTheTurtle added the status/0-triage label Jan 31, 2018

emil2k force-pushed the gzip-archive branch 6 times, most recently from 091e283 to 59b9e7e Compare January 31, 2018 15:29

dnephin reviewed Jan 31, 2018

View reviewed changes

yongtang added status/2-code-review and removed status/0-triage labels Feb 1, 2018

dnephin mentioned this pull request Feb 2, 2018

Fix vendor validation #36190

Merged

emil2k force-pushed the gzip-archive branch 3 times, most recently from 2440772 to 394116d Compare February 5, 2018 12:54

dnephin reviewed Feb 5, 2018

View reviewed changes

client/client.go Outdated

Copy link

Member

dnephin Feb 5, 2018

Choose a reason for hiding this comment

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

Nice find! I didn't know about this

dnephin requested review from thaJeztah and vdemeester February 5, 2018 20:45

emil2k force-pushed the gzip-archive branch from 394116d to a82253e Compare February 14, 2018 10:32

vdemeester approved these changes Feb 14, 2018

View reviewed changes

GordonTheTurtle assigned vieux Feb 15, 2018

emil2k force-pushed the gzip-archive branch from a82253e to b62545c Compare February 22, 2018 12:06

thaJeztah reviewed Feb 22, 2018

View reviewed changes

emil2k force-pushed the gzip-archive branch from b62545c to 0d42bb5 Compare February 24, 2018 05:36

Content encoding negotiation added to archive request.

117cd7f

Signed-off-by: Emil Davtyan <emil2k@gmail.com>

emil2k force-pushed the gzip-archive branch from 0d42bb5 to 117cd7f Compare February 24, 2018 05:42

tiborvass approved these changes Feb 24, 2018

View reviewed changes

thaJeztah added the rebuild/* label Feb 24, 2018

GordonTheTurtle removed the rebuild/* label Feb 24, 2018

yongtang merged commit 20c4efe into moby:master Feb 24, 2018

emil2k deleted the gzip-archive branch February 25, 2018 09:34

thaJeztah added area/api API docs/revisit impact/changelog labels Feb 28, 2018

rnorth mentioned this pull request Aug 4, 2018

Docker 18.06: copyArchiveFromContainerCmd fails due to presence of gzip encoding docker-java/docker-java#1079

Closed

thaJeztah mentioned this pull request Feb 25, 2020

Very slow download speed when fetching 'tar' from container with encoding headers #40577

Open

Content encoding negotiation added to archive request. #36164

Content encoding negotiation added to archive request. #36164

Uh oh!

Conversation

emil2k commented Jan 31, 2018

Uh oh!

dnephin commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emil2k commented Feb 5, 2018

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emil2k commented Feb 14, 2018

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

emil2k commented Feb 16, 2018

Uh oh!

thaJeztah commented Feb 19, 2018

Uh oh!

thaJeztah commented Feb 19, 2018

Uh oh!

emil2k commented Feb 19, 2018

Uh oh!

emil2k commented Feb 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiborvass commented Feb 22, 2018

Uh oh!

codecov bot commented Feb 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dnephin commented Jan 31, 2018 •

edited

Loading

codecov bot commented Feb 24, 2018 •

edited

Loading