cluster: set bigger grpc limit for array requests #38103

tonistiigi · 2018-10-30T01:19:20Z

Add bigger limits in the swarmkit calls that can return a lot of data. This is a regression in the grpc library update where the previous version didn't have a limit in this case while new has a 4MB limit. https://github.com/grpc/grpc-go/pull/1165/files#diff-e1550a73f5d25064c8b586ec68d81a64R105

This can be followed up by adding better defaults in swarmkit on the dialer level.

@anshulpundir @andrewhsu

Signed-off-by: Tonis Tiigi [email protected]

andrewhsu

LGTM

looks like this will also address #37997 (to some extent)

cpuguy83 · 2018-10-30T05:31:07Z

Can someone please comment on these issues with the analysis?

thaJeztah

LGTM

thaJeztah

Actually; @tonistiigi could you update the commit description to include some information about this change, and explain why we have to set this limit now?

i.e., that it was previously not limited by grpc-go, but as of grpc/grpc-go@50d4175 / grpc/grpc-go@6f8b553 (grpc/grpc-go#1165) is.

Looking at git blame, it looks like this change was brought in as part of the containerd 1.1 bump in commit 52ed3e0 (#36895), which is part of Docker 18.06 and up.

codecov · 2018-10-30T16:31:21Z

Codecov Report

❗ No coverage uploaded for pull request base (master@b8e87cf). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #38103   +/-   ##
=========================================
  Coverage          ?    36.1%           
=========================================
  Files             ?      610           
  Lines             ?    45216           
  Branches          ?        0           
=========================================
  Hits              ?    16327           
  Misses            ?    26650           
  Partials          ?     2239

tonistiigi · 2018-10-30T16:32:26Z

@thaJeztah Updated commit description with the same message as in the PR description

cpuguy83

LGTM

thaJeztah

LGTM, thank you

cpuguy83 · 2018-10-30T17:08:16Z

Opened to address issues with large list operations moby/swarmkit#2774

thaJeztah · 2018-10-30T17:30:41Z

Alternative approach; moby/swarmkit#2773

tonistiigi · 2018-10-30T17:35:51Z

@andrewhsu I'm not really sure what the highest limit is that users could reach under heavy load so maybe I'll just restore it to maxuint32 (instead of 32MB) like it was before(before 18.06), effectively disabling all limits. Then in the future releases, we can figure out what API changes are needed(moby/swarmkit#2774 (comment)) to get these response sizes down to something normal(and possibly stream them as well).

andrewhsu · 2018-10-30T17:50:12Z

With about 70 services with average of 20 tasks each, we're getting errors that the message size of 5MB is above the current 4MB limit, so increasing the message size limit to 32MB is a good start to buy us some breathing room until we get some more analysis on the grpc message size, tracked here.

cpuguy83 · 2018-10-30T18:02:39Z

The problem is the limit breaks existing setups and we already know that people will be broken at 32MB (or at least that is what is claimed).

I'd vote for setting back to the old value for this reason.

cpuguy83 · 2018-10-30T18:04:09Z

This should be back ported to 18.06, IHMO, to unblock people without forcing a major update.

andrewhsu · 2018-10-30T22:22:05Z

I talked with @anshulpundir and @tonistiigi about this and came to consensus that bringing the max message size to the old original value of max of int32 is the best solution for this PR's desired fix. (i lost the arm-wrestling match)

I'll update backports after this PR is adjusted.

4MB client side limit was introduced in vendoring go-grpc#1165 (v1.4.0) making these requests likely to produce errors Signed-off-by: Tonis Tiigi <[email protected]>

tonistiigi · 2018-10-30T23:03:40Z

Updated

cpuguy83

LGTM again.

thaJeztah

still LGTM

anshulpundir

LGTM again

thaJeztah · 2018-10-31T09:41:58Z

PowerPC failure is a flaky test; #34988

00:39:03 FAIL: docker_api_swarm_test.go:359: DockerSwarmSuite.TestAPISwarmRaftQuorum
00:39:03 
00:39:03 [dda1effb20dae] waiting for daemon to start
00:39:03 [dda1effb20dae] daemon started
00:39:03 
00:39:03 [dd36cc83874c7] waiting for daemon to start
00:39:03 [dd36cc83874c7] daemon started
00:39:03 
00:39:03 [d7fc52a3c7429] waiting for daemon to start
00:39:03 [d7fc52a3c7429] daemon started
00:39:03 
00:39:03 [dd36cc83874c7] exiting daemon
00:39:03 [d7fc52a3c7429] exiting daemon
00:39:03 docker_api_swarm_test.go:385:
00:39:03     // d1 will eventually step down from leader because there is no longer an active quorum, wait for that to happen
00:39:03     waitAndAssert(c, defaultReconciliationTimeout, func(c *check.C) (interface{}, check.CommentInterface) {
00:39:03         _, err = cli.ServiceCreate(context.Background(), service.Spec, types.ServiceCreateOptions{})
00:39:03         return err.Error(), nil
00:39:03     }, checker.Contains, "Make sure more than half of the managers are online.")
00:39:03 docker_utils_test.go:435:
00:39:03     c.Assert(v, checker, args...)
00:39:03 ... obtained string = "Error response from daemon: rpc error: code = DeadlineExceeded desc = context deadline exceeded"
00:39:03 ... substring string = "Make sure more than half of the managers are online."
00:39:03 
00:39:03 [dda1effb20dae] exiting daemon

WindowsRS5 CI is broken currently, and needs to be looked into. Opened an issue for that; #38114

23:11:20 INFO: Base image for tests is microsoft/windowsservercore
23:11:20 INFO: Loading windowsservercore .tar from disk into the daemon under test. This may take some time...
23:12:08 
23:12:08 
23:12:08 ERROR: Failed 'ERROR: Failed to load c:\baseimages\windowsservercore.tar into daemon under test' at 10/30/2018 23:12:08
23:12:08 At C:\gopath\src\github.com\docker\docker\hack\ci\windows.ps1:705 char:21
23:12:08 + ...             Throw $("ERROR: Failed to load $readBaseFrom`:\baseimages ...
23:12:08 +                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Changes included; - moby/swarmkit#2735 Assign secrets individually to each task - moby/swarmkit#2759 Adding a new `Deallocator` component - moby/swarmkit#2738 Add additional info for secret drivers - moby/swarmkit#2775 Increase grpc max recv message size - addresses moby#37941 - addresses moby#37997 - follow-up to moby#38103 Signed-off-by: Sebastiaan van Stijn <[email protected]>

Changes included; - moby/swarmkit#2735 Assign secrets individually to each task - moby/swarmkit#2759 Adding a new `Deallocator` component - moby/swarmkit#2738 Add additional info for secret drivers - moby/swarmkit#2775 Increase grpc max recv message size - addresses moby/moby#37941 - addresses moby/moby#37997 - follow-up to moby/moby#38103 Signed-off-by: Sebastiaan van Stijn <[email protected]> Upstream-commit: be3843c8c8fb30b4a604dae9d0dad3d393db717c Component: engine

Changes included; - moby/swarmkit#2735 Assign secrets individually to each task - moby/swarmkit#2759 Adding a new `Deallocator` component - moby/swarmkit#2738 Add additional info for secret drivers - moby/swarmkit#2775 Increase grpc max recv message size - addresses moby#37941 - addresses moby#37997 - follow-up to moby#38103 Signed-off-by: Sebastiaan van Stijn <[email protected]>

…3 branch) full diff: moby/swarmkit@4fb9e96...bbe3418 changes included: - moby/swarmkit#2889 [19.03 backport] Fix update out of sequence and increase max recv gRPC message size for nodes and secrets Which relates to - moby#39531 integration-cli: fix swarm tests flakiness - docker-archive#345 [19.03 backport] integration-cli: fix swarm tests flakiness And includes backports of - moby/swarmkit#2808 Fix flaky tests - moby/swarmkit#2866 Swap gometalinter for golangci-lint - moby/swarmkit#2869 Increase max recv gRPC message size to initialize connection broker - related / similar to moby#38103 / docker-archive#102 cluster: set bigger grpc limit for array requests - related / similar to moby#39306 Increase max recv gRPC message size for nodes and secrets - fixes moby/swarmkit#2733 Error generated when messages size is too big - moby/swarmkit#2870 Fix update out of sequence Signed-off-by: Sebastiaan van Stijn <[email protected]>

…3 branch) full diff: moby/swarmkit@4fb9e96...bbe3418 changes included: - moby/swarmkit#2889 [19.03 backport] Fix update out of sequence and increase max recv gRPC message size for nodes and secrets Which relates to - moby/moby#39531 integration-cli: fix swarm tests flakiness - docker-archive/engine#345 [19.03 backport] integration-cli: fix swarm tests flakiness And includes backports of - moby/swarmkit#2808 Fix flaky tests - moby/swarmkit#2866 Swap gometalinter for golangci-lint - moby/swarmkit#2869 Increase max recv gRPC message size to initialize connection broker - related / similar to moby/moby#38103 / docker-archive/engine#102 cluster: set bigger grpc limit for array requests - related / similar to moby/moby#39306 Increase max recv gRPC message size for nodes and secrets - fixes moby/swarmkit#2733 Error generated when messages size is too big - moby/swarmkit#2870 Fix update out of sequence Signed-off-by: Sebastiaan van Stijn <[email protected]> Upstream-commit: f7dbee3eeaa1dd218116f85b8f60361acbd5b214 Component: engine

…v18.09) full diff: moby/swarmkit@142a737...5c86095 - moby/swarmkit#2892 [18.09 backport] Remove hardcoded IPAM config subnet value for ingress network - backport of moby/swarmkit#2890 Remove hardcoded IPAM config subnet value for ingress network - fixes [ENGORC-2651](https://docker.atlassian.net/browse/ENGORC-2651) - moby/swarmkit#2836 [18.09 backport] Switch to go 1.11 - backport of moby/swarmkit#2752 Switch to go 1.11 - moby/swarmkit#2901 [18.09 backport] Bump to golang 1.12.9 - backport of moby/swarmkit#2880 Bump to golang 1.12.9 - moby/swarmkit#2900 [18.09 backport] Fix update out of sequence and increase max recv gRPC message size for nodes and secrets - backport of moby/swarmkit#2762 Increased wait time on test utils WaitForCluster and WatchTaskCreate - backport of moby/swarmkit#2771 Allow using Configs as CredentialSpecs - **second commit only** (attempt to fix weirdly broken tests) - backport of moby/swarmkit#2808 Fix flaky tests - backport of moby/swarmkit#2866 Swap gometalinter for golangci-lint - backport of moby/swarmkit#2869 Increase max recv gRPC message size to initialize connection broker - related / similar to moby#38103 / docker-archive#102 cluster: set bigger grpc limit for array requests - related / similar to moby#39306 Increase max recv gRPC message size for nodes and secrets - fixes moby/swarmkit#2733 Error generated when messages size is too big - backport of moby/swarmkit#2870 Fix update out of sequence Signed-off-by: Sebastiaan van Stijn <[email protected]>

…v18.09) full diff: moby/swarmkit@142a737...5c86095 - moby/swarmkit#2892 [18.09 backport] Remove hardcoded IPAM config subnet value for ingress network - backport of moby/swarmkit#2890 Remove hardcoded IPAM config subnet value for ingress network - fixes [ENGORC-2651](https://docker.atlassian.net/browse/ENGORC-2651) - moby/swarmkit#2836 [18.09 backport] Switch to go 1.11 - backport of moby/swarmkit#2752 Switch to go 1.11 - moby/swarmkit#2901 [18.09 backport] Bump to golang 1.12.9 - backport of moby/swarmkit#2880 Bump to golang 1.12.9 - moby/swarmkit#2900 [18.09 backport] Fix update out of sequence and increase max recv gRPC message size for nodes and secrets - backport of moby/swarmkit#2762 Increased wait time on test utils WaitForCluster and WatchTaskCreate - backport of moby/swarmkit#2771 Allow using Configs as CredentialSpecs - **second commit only** (attempt to fix weirdly broken tests) - backport of moby/swarmkit#2808 Fix flaky tests - backport of moby/swarmkit#2866 Swap gometalinter for golangci-lint - backport of moby/swarmkit#2869 Increase max recv gRPC message size to initialize connection broker - related / similar to moby/moby#38103 / docker-archive/engine#102 cluster: set bigger grpc limit for array requests - related / similar to moby/moby#39306 Increase max recv gRPC message size for nodes and secrets - fixes moby/swarmkit#2733 Error generated when messages size is too big - backport of moby/swarmkit#2870 Fix update out of sequence Signed-off-by: Sebastiaan van Stijn <[email protected]> Upstream-commit: e06f07ef337ab890f211397d6b408b75a2512dc5 Component: engine

GordonTheTurtle added the status/0-triage label Oct 30, 2018

andrewhsu mentioned this pull request Oct 30, 2018

[18.09] cluster: set bigger grpc limit for array requests docker-archive/engine#99

Merged

andrewhsu approved these changes Oct 30, 2018

View reviewed changes

anshulpundir approved these changes Oct 30, 2018

View reviewed changes

thaJeztah approved these changes Oct 30, 2018

View reviewed changes

thaJeztah added status/2-code-review area/swarm and removed status/0-triage labels Oct 30, 2018

thaJeztah requested changes Oct 30, 2018

View reviewed changes

thaJeztah mentioned this pull request Oct 30, 2018

Update containerd to 1.1 #36895

Merged

tonistiigi force-pushed the cluster-grpc-limits branch from f0c115e to f1f50ed Compare October 30, 2018 16:31

cpuguy83 approved these changes Oct 30, 2018

View reviewed changes

thaJeztah approved these changes Oct 30, 2018

View reviewed changes

thaJeztah added status/4-merge and removed status/2-code-review labels Oct 30, 2018

andrewhsu mentioned this pull request Oct 30, 2018

[18.06] cluster: set bigger grpc limit for array requests docker-archive/engine#102

Merged

andrewhsu mentioned this pull request Oct 30, 2018

Request each service's tasks separately docker/cli#1489

Closed

cluster: set bigger grpc limit for array requests

489b8ed

4MB client side limit was introduced in vendoring go-grpc#1165 (v1.4.0) making these requests likely to produce errors Signed-off-by: Tonis Tiigi <[email protected]>

tonistiigi force-pushed the cluster-grpc-limits branch from f1f50ed to 489b8ed Compare October 30, 2018 23:02

cpuguy83 approved these changes Oct 30, 2018

View reviewed changes

thaJeztah approved these changes Oct 30, 2018

View reviewed changes

anshulpundir approved these changes Oct 30, 2018

View reviewed changes

thaJeztah mentioned this pull request Oct 31, 2018

WindowsRS5 CI is borked #38114

Closed

thaJeztah merged commit 547f11d into moby:master Oct 31, 2018

This was referenced Nov 1, 2018

Bump SwarmKit to 8d8689d5a94ac42406883a4cef89b3a5eaec3d11 #38123

Merged

docker service list fails with "rpc error: code = Unavailable desc = transport is closing" #37849

Closed

dani-docker mentioned this pull request Feb 26, 2019

Increase grpc limit for GetConfigs api #38800

Merged

thaJeztah mentioned this pull request Sep 6, 2019

[19.03 backport] Fix update out of sequence and increase max recv gRPC message size for nodes and secrets moby/swarmkit#2889

Merged

thaJeztah mentioned this pull request Sep 12, 2019

[19.03 backport] bump swarmkit to bbe341867eae1615faf8a702ec05bfe986e73e06 (bump_v19.03 branch) docker-archive/engine#353

Merged

This was referenced Sep 25, 2019

[18.09 backport] Fix update out of sequence and increase max recv gRPC message size for nodes and secrets moby/swarmkit#2900

Merged

[18.03 backport] Fix update out of sequence moby/swarmkit#2902

Merged

thaJeztah mentioned this pull request Oct 2, 2019

[18.09 backport] integration-cli: fix swarm tests flakiness docker-archive/engine#346

Merged

cluster: set bigger grpc limit for array requests #38103

cluster: set bigger grpc limit for array requests #38103

Uh oh!

Conversation

tonistiigi commented Oct 30, 2018

Uh oh!

andrewhsu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Oct 30, 2018

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

tonistiigi commented Oct 30, 2018

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Oct 30, 2018

Uh oh!

thaJeztah commented Oct 30, 2018

Uh oh!

tonistiigi commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewhsu commented Oct 30, 2018

Uh oh!

cpuguy83 commented Oct 30, 2018

Uh oh!

cpuguy83 commented Oct 30, 2018

Uh oh!

andrewhsu commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tonistiigi commented Oct 30, 2018

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

anshulpundir left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

andrewhsu left a comment •

edited

Loading

thaJeztah left a comment •

edited

Loading

codecov bot commented Oct 30, 2018 •

edited

Loading

tonistiigi commented Oct 30, 2018 •

edited

Loading

andrewhsu commented Oct 30, 2018 •

edited

Loading