Skip to content
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

resolver: disable http2 for pushing #1420

Merged
merged 1 commit into from
Apr 6, 2020

Conversation

fuweid
Copy link
Contributor

@fuweid fuweid commented Mar 31, 2020

The golang net/http package uses http2 client to serve https by default,
if let Transport.TLSNextProto is nil. And net/http package doesn't
provide tunnable value for http2 flow control which will limit push
performance.

Before this commit, use GODEBUG="http2debug=1" buildkitd to pushing
one image from dockerfile like

$ about 700MB
FROM scratch
ADD ./golang-1.13.0-stretch.tar.gzip /

and use ifstat to monitor network interface and found that

$ ifstat -i enp0s3
      enp0s3
 KB/s in  KB/s out
    0.47      0.67
    0.44      0.51
   19.72     11.56
   62.25   2184.41
   96.34   3514.28
   93.89   3508.31
   95.41   3515.53
   91.61   3433.22
   95.82   3579.68
   90.36   3388.89
   93.64   3513.03
   93.32   3478.04
   ...

$ log from buildkitd
2020/03/31 17:40:33 http2: Transport received WINDOW_UPDATE stream=11 len=4 incr=32768
2020/03/31 17:40:33 http2: Transport received WINDOW_UPDATE len=4 (conn) incr=271
2020/03/31 17:40:33 http2: Transport received WINDOW_UPDATE stream=11 len=4 incr=271
2020/03/31 17:40:33 http2: Transport received WINDOW_UPDATE len=4 (conn) incr=32496
2020/03/31 17:40:33 http2: Transport received WINDOW_UPDATE stream=11 len=4 incr=32496
...

The registry will update window size of flow control when receives each
frame data. The sender need wait for receiver update the window size if
the sender runs out of buffer of flow control. But the increase value
for buffer by WINDOW_UPDATE frame is too small and slow which impacts
push performance.

Before net/http package provides tunnable value for flow control, we
should disable http2 for https request.

And with this commit, the performance will be better like:

$ ifstat -i enp0s3
      enp0s3
 KB/s in  KB/s out
    0.56      0.61
   16.13      5.55
   18.89      9.23
  218.84   7832.80
  338.56  13074.04
  302.39  11713.83
  231.62   8964.60
  356.50  13504.02
  298.14  11401.81
  311.24  11783.26
  333.01  12710.17
  329.64  12630.40
  305.87  11662.04
  292.53  11118.04

Signed-off-by: Wei Fu fuweid89@gmail.com

The golang net/http package uses http2 client to serve https by default,
if let Transport.TLSNextProto is nil. And net/http package doesn't
provide tunnable value for http2 flow control which will limit push
performance.

Before this commit, use GODEBUG="http2debug=1" buildkitd to pushing
one image from dockerfile like

```
$ about 700MB
FROM scratch
ADD ./golang-1.13.0-stretch.tar.gzip /
```

and use ifstat to monitor network interface and found that

```
$ ifstat -i enp0s3
      enp0s3
 KB/s in  KB/s out
    0.47      0.67
    0.44      0.51
   19.72     11.56
   62.25   2184.41
   96.34   3514.28
   93.89   3508.31
   95.41   3515.53
   91.61   3433.22
   95.82   3579.68
   90.36   3388.89
   93.64   3513.03
   93.32   3478.04
   ...

$ log from buildkitd
2020/03/31 17:40:33 http2: Transport received WINDOW_UPDATE stream=11 len=4 incr=32768
2020/03/31 17:40:33 http2: Transport received WINDOW_UPDATE len=4 (conn) incr=271
2020/03/31 17:40:33 http2: Transport received WINDOW_UPDATE stream=11 len=4 incr=271
2020/03/31 17:40:33 http2: Transport received WINDOW_UPDATE len=4 (conn) incr=32496
2020/03/31 17:40:33 http2: Transport received WINDOW_UPDATE stream=11 len=4 incr=32496
...
```

The registry will update window size of flow control when receives each
frame data. The sender need wait for receiver update the window size if
the sender runs out of buffer of flow control. But the increase value
for buffer by WINDOW_UPDATE frame is too small and slow which impacts
push performance.

Before net/http package provides tunnable value for flow control, we
should disable http2 for https request.

And with this commit, the performance will be better like:

```
$ ifstat -i enp0s3
      enp0s3
 KB/s in  KB/s out
    0.56      0.61
   16.13      5.55
   18.89      9.23
  218.84   7832.80
  338.56  13074.04
  302.39  11713.83
  231.62   8964.60
  356.50  13504.02
  298.14  11401.81
  311.24  11783.26
  333.01  12710.17
  329.64  12630.40
  305.87  11662.04
  292.53  11118.04
```

Signed-off-by: Wei Fu <fuweid89@gmail.com>
//
// NOTE: For push, there must disable http2 for https because the flow control
// will limit data transfer. The net/http package doesn't provide http2 tunable
// settings which limits push performance.
Copy link
Member

Choose a reason for hiding this comment

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

Is there just no way to force it to open up multiple connections? The flow control will actually make the number of bytes sent over the wire smaller than it would on a single connection or is there multiplexing overhead?

TLSHandshakeTimeout: 10 * time.Second,
ExpectContinueTimeout: 5 * time.Second,
DisableKeepAlives: true,
TLSNextProto: make(map[string]func(authority string, c *tls.Conn) http.RoundTripper),
Copy link
Member

Choose a reason for hiding this comment

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

So is it TLSNextProto or DisableKeepAlives that is important in here? In Docker I don't see call to TLSNextProto so not sure why that isn't affected then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The onceSetNextProtoDefaults will be called once at the beginning of RoundTrip, which used to setup http2 RoundTrip transport for https request if possible. And the TLSNextProto is mentioned in net/http package document. But there are several way to disable http2, for instance, dockerd uses non-nil TLSClientConfig to avoid http2.

roundTrip in golang1.13 and golang1.14
onceSetNextProtoDefaults in golang1.13 and golang1.14

// roundTrip implements a RoundTripper over HTTP.
func (t *Transport) roundTrip(req *Request) (*Response, error) {
	t.nextProtoOnce.Do(t.onceSetNextProtoDefaults)
        ...
}

func (t *Transport) onceSetNextProtoDefaults() {
...
        if t.TLSNextProto != nil {
		// This is the documented way to disable http2 on a
		// Transport.
		return
	}
	if !t.ForceAttemptHTTP2 && (t.TLSClientConfig != nil || t.Dial != nil || t.DialContext != nil || t.hasCustomTLSDialer()) {
		// Be conservative and don't automatically enable
		// http2 if they've specified a custom TLS config or
		// custom dialers. Let them opt-in themselves via
		// http2.ConfigureTransport so we don't surprise them
		// by modifying their tls.Config. Issue 14275.
		// However, if ForceAttemptHTTP2 is true, it overrides the above checks.
		return
	}
....
}

If the http2 is enabled, the http2 RoundTripp transport will use http2clientStream to write request body to server.

writeRequestBody in golang1.13 and golang1.14

func (cs *http2clientStream) writeRequestBody(body io.Reader, bodyCloser io.Closer) (err error) {
	cc := cs.cc
	sentEnd := false // whether we sent the final DATA frame w/ END_STREAM
	buf := cc.frameScratchBuffer()
	defer cc.putFrameScratchBuffer(buf)

	....

	var sawEOF bool
	for !sawEOF {
		n, err := body.Read(buf)
		if err == io.EOF {
			sawEOF = true
			err = nil
		} else if err != nil {
			cc.writeStreamReset(cs.ID, http2ErrCodeCancel, err)
			return err
		}

		remain := buf[:n]
		for len(remain) > 0 && err == nil {
			var allowed int32
			allowed, err = cs.awaitFlowControl(len(remain))
			...
			cc.wmu.Lock()
			data := remain[:allowed]
			remain = remain[allowed:]
			sentEnd = sawEOF && len(remain) == 0 && !hasTrailers
			err = cc.fr.WriteData(cs.ID, sentEnd, data)
			...
			cc.wmu.Unlock()
		}
		
	}
        ...
}

For writing data frame, the client needs to wait for available buffer from flow control by awaitFlowControl. The buffer from flow control will be updated by WINDOW_UPDATE frame which will be send by server when it receives data frame. However, the increase for buffer is too small and slow than client uploads. Sometimes, the client needs to wait for 200ms to write 30kb data. It is unacceptable.

For http1, just use io.Copy to upload data and let TCP proto to handle flow control.

writeBody in golang1.13 and golang1.14

func (t *transferWriter) writeBody(w io.Writer) error {
	var err error
	var ncopy int64

	// Write body. We "unwrap" the body first if it was wrapped in a
	// nopCloser. This is to ensure that we can take advantage of
	// OS-level optimizations in the event that the body is an
	// *os.File.
	if t.Body != nil {
		var body = t.unwrapBody()
		...
		} else {
			ncopy, err = t.doBodyCopy(w, io.LimitReader(body, t.ContentLength))
			if err != nil {
				return err
			}
			var nextra int64
			nextra, err = t.doBodyCopy(ioutil.Discard, body)
			ncopy += nextra
		}
		...
	}

cc @dmcgowan

Copy link
Contributor Author

@fuweid fuweid Apr 2, 2020

Choose a reason for hiding this comment

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

The golang http2 just setup 64KB buffer for flow control and client needs to wait for WINDOW_UPDATE frame to update the buffer before upload. The init buffer size cannot be configurable. it is painful.

And the docker/distribution is setting TLS on listener not for http.Server so that the server will setup http2 by default for https request.

https://github.com/docker/distribution/blob/release/2.6/registry/registry.go#L98

@fuweid
Copy link
Contributor Author

fuweid commented Apr 2, 2020

ping @AkihiroSuda

PTAL

@fuweid
Copy link
Contributor Author

fuweid commented Apr 6, 2020

ping @AkihiroSuda

@AkihiroSuda AkihiroSuda merged commit 4201939 into moby:master Apr 6, 2020
@fuweid fuweid deleted the disable-http2-for-https branch April 6, 2020 13:08
@fuweid
Copy link
Contributor Author

fuweid commented Apr 6, 2020

Thanks! @tonistiigi @AkihiroSuda

@tonistiigi
Copy link
Member

@fuweid closes #1192 ?

@fuweid
Copy link
Contributor Author

fuweid commented Apr 23, 2020

@tonistiigi we are testing about limitation of pushing goroutine like existing docker behaviour. after that, we will close it. Thanks for reminder!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants