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.

Sign up for GitHub

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

Jump to bottom

session: fix grpc message size limits for tar streams #4313

Merged
merged 3 commits into from
Nov 13, 2023
Merged

session: fix grpc message size limits for tar streams #4313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
329e945
filesync: split stream data into 3MB chunks to avoid message limits
tonistiigi Oct 9, 2023
ac33aec
session: raise grpc message size limits for session endpoint
tonistiigi Oct 9, 2023
463bdab
uploadprovider: avoid too big grpc messages on tar upload
tonistiigi Oct 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions session/filesync/diffcopy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,22 @@ type streamWriterCloser struct {
}

func (wc *streamWriterCloser) Write(dt []byte) (int, error) {
// grpc-go has a 4MB limit on messages by default. Split large messages
// so we don't get close to that limit.
const maxChunkSize = 3 * 1024 * 1024
Copy link
Member

Choose a reason for hiding this comment

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

Just a note - this logic feels very similar to what's already implemented in fsutil.Send with a buffer pool. I wonder if (as a follow-up) we could push the logic to send/receive single files into fsutil (which could let us re-use the same limiting logic, so if we want to update the size of the messages we send, we don't have to do it everywhere).

if len(dt) > maxChunkSize {
n1, err := wc.Write(dt[:maxChunkSize])
if err != nil {
return n1, err
}
dt = dt[maxChunkSize:]
var n2 int
if n2, err = wc.Write(dt); err != nil {
return n1 + n2, err
}
return n1 + n2, nil
Comment on lines +60 to +63
Copy link
Member

Choose a reason for hiding this comment

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

nit: could we change this recursion into a loop? If we get an error deep in the stack if we have very large messages, the stack will be pretty unreadable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Realistically, I would never expect to get too big messages in here. The limit is 16MB anyway so anything bigger than 5x would already start to fail on these conditions.

}

if err := wc.ClientStream.SendMsg(&BytesMessage{Data: dt}); err != nil {
// SendMsg return EOF on remote errors
if errors.Is(err, io.EOF) {
Expand Down
3 changes: 3 additions & 0 deletions session/grpc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"sync/atomic"
"time"

"github.com/containerd/containerd/defaults"
grpc_middleware "github.com/grpc-ecosystem/go-grpc-middleware"
"github.com/moby/buildkit/util/bklog"
"github.com/moby/buildkit/util/grpcerrors"
Expand Down Expand Up @@ -44,6 +45,8 @@ func grpcClientConn(ctx context.Context, conn net.Conn) (context.Context, *grpc.
dialOpts := []grpc.DialOption{
dialer,
grpc.WithTransportCredentials(insecure.NewCredentials()),
grpc.WithDefaultCallOptions(grpc.MaxCallRecvMsgSize(defaults.DefaultMaxRecvMsgSize)),
jedevc marked this conversation as resolved.
Show resolved Hide resolved
grpc.WithDefaultCallOptions(grpc.MaxCallSendMsgSize(defaults.DefaultMaxSendMsgSize)),
}

if span := trace.SpanFromContext(ctx); span.SpanContext().IsValid() {
Expand Down
14 changes: 14 additions & 0 deletions session/upload/uploadprovider/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,20 @@ type writer struct {
}

func (w *writer) Write(dt []byte) (int, error) {
// avoid sending too big messages on grpc stream
const maxChunkSize = 3 * 1024 * 1024
if len(dt) > maxChunkSize {
n1, err := w.Write(dt[:maxChunkSize])
if err != nil {
return n1, err
}
dt = dt[maxChunkSize:]
var n2 int
if n2, err := w.Write(dt); err != nil {
return n1 + n2, err
}
return n1 + n2, nil
}
if err := w.SendMsg(&upload.BytesMessage{Data: dt}); err != nil {
return 0, err
}
Expand Down
Loading