Add QPS settings to Allocation endpoints #1863

markmandel · 2020-10-22T21:38:11Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking

/kind bug

/kind cleanup
/kind documentation
/kind feature
/kind hotfix

What this PR does / Why we need it:

Allocation endpoints where throttled to the default ~4qps for a Kubernetes client.

Matching the controller settings on standard QPS and Burst to allow higher throughput.

Which issue(s) this PR fixes:

Closes #1852

Special notes for your reviewer:

Code can be reviewed, but we should wait until we get the benchmarking tool from @ilkercelikyilmaz to confirm throughput before merging.

roberthbailey

LGTM (it matches https://github.com/googleforgames/agones/blob/master/cmd/controller/main.go).

markmandel · 2020-10-22T21:43:12Z

LGTM (it matches https://github.com/googleforgames/agones/blob/master/cmd/controller/main.go).

Copy paste driven development 😁

agones-bot · 2020-10-22T22:07:45Z

Build Succeeded 👏

Build Id: 5b4309c9-c535-46c1-8a7c-770515e73f86

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.10.0-8aaf76d
image: gcr.io/agones-images/agones-sdk:1.10.0-8aaf76d
image: gcr.io/agones-images/agones-ping:1.10.0-8aaf76d
Linux C++ SDK (build): agonessdk-1.10.0-8aaf76d-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.10.0-8aaf76d.zip

A preview of the website (the last 30 builds are retained):

https://8aaf76d-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1863/head:pr_1863 && git checkout pr_1863
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.10.0-8aaf76d

pooneh-m

Thanks for the fix.

pooneh-m · 2020-10-22T22:30:31Z

cmd/allocator/metrics.go

@@ -65,6 +71,8 @@ func parseEnvFlags() config {
 	viper.SetDefault(remoteAllocationTimeoutFlag, 10*time.Second)
 	viper.SetDefault(totalRemoteAllocationTimeoutFlag, 30*time.Second)

+	pflag.Int32(apiServerSustainedQPSFlag, 100, "Maximum sustained queries per second to send to the API server")
+	pflag.Int32(apiServerBurstQPSFlag, 200, "Maximum burst queries per second to send to the API server")


viper.GetInt(apiServerBurstQPSFlag) to follow the pattern?
Is there any case that the defaults are used? Why not set defaults similar to the helm default?

No reason - I literally just copied exactly what was in the controller/main.go - figured might as well keep it all consistent.
https://github.com/googleforgames/agones/blob/master/cmd/controller/main.go#L258

I think we should use the same default across stack, if the default values are not meant to be different, regardless of the controller.

If we set the default to 100 here and 400 as the environment variable, I can imagine looking at the code, one may assume the default is 100 as they have not set that on the environment variable.

I also thought this was confusing, but it's consistent with the agones controller. Maybe a cleanup there would be better than propagating confusing dueling default values?

Do we feel we should cleanup here, or do it in a subsequent PR? I don't mind either way tbh.

For allocator, because we are making the change in this PR, we should fix it in this PR. For controller, we can fix it in a later PR.

Thought I hit this, and apparently I missed it. Fix incoming!

pooneh-m · 2020-10-22T22:35:13Z

Can you please add the Helm config documentations as well?

markmandel · 2020-10-22T22:37:52Z

Can you please add the Helm config documentations as well?

🤦 I knew I forgot something.

agones-bot · 2020-10-22T23:04:27Z

Build Succeeded 👏

Build Id: 16c1f228-d1fb-4144-97c3-6998b2cf1893

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.10.0-e3140c3
image: gcr.io/agones-images/agones-sdk:1.10.0-e3140c3
image: gcr.io/agones-images/agones-ping:1.10.0-e3140c3
Linux C++ SDK (build): agonessdk-1.10.0-e3140c3-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.10.0-e3140c3.zip

A preview of the website (the last 30 builds are retained):

https://e3140c3-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1863/head:pr_1863 && git checkout pr_1863
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.10.0-e3140c3

pooneh-m · 2020-10-23T05:33:26Z

Looks pretty good. Thanks for the change!

agones-bot · 2020-10-24T08:04:33Z

Build Failed 😱

Build Id: 6dee8416-e990-4cb4-a2ec-2a85b74687ac

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

markmandel · 2020-10-27T23:39:10Z

Just did a test with the tooling from #1867

root@fba8ccd77f67:/go/src/agones.dev/agones/test/load/allocation/grpc# TESTRUNSCOUNT=1 ./runAllocation.sh 40 100
Run number:  1
started: 2020-10-27 23:25:28.81615566 +0000 UTC m=+0.000918033
(failed(client=2,allocation=79): rpc error: code = Unknown desc = error updating allocated gameserver: Operation cannot be fulfilled on gameservers.agones.dev "load-test-fleet-bx24p-t7sd9": the object has been modified; please apply your changes to the latest version and try again
...
(failed(client=6,allocation=90): rpc error: code = Unknown desc = error updating allocated gameserver: Operation cannot be fulfilled on gameservers.agones.dev "load-test-fleet-bx24p-z25b7": the object has been modified; please apply your changes to the latest version and try again
finished: 2020-10-27 23:26:31.579609788 +0000 UTC m=+62.764372203

So that was 3980 allocation in 1 minute 3 seconds, so 63 QPS. (This is also from my home laptop while I'm on a hangout 😄)

So looks like once I handle the above comments, this PR should be good to go!

agones-bot · 2020-10-28T00:06:15Z

Build Succeeded 👏

Build Id: 7c99adb4-b665-4fc0-ba11-fc7e47d5b805

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.10.0-0b3c82d
image: gcr.io/agones-images/agones-sdk:1.10.0-0b3c82d
image: gcr.io/agones-images/agones-ping:1.10.0-0b3c82d
Linux C++ SDK (build): agonessdk-1.10.0-0b3c82d-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.10.0-0b3c82d.zip

A preview of the website (the last 30 builds are retained):

https://0b3c82d-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1863/head:pr_1863 && git checkout pr_1863
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.10.0-0b3c82d

Allocation endpoints where throttled to the default ~4qps for a Kubernetes client. Matching the controller settings on standard QPS and Burst to allow higher throughput. Closes googleforgames#1852

google-oss-robot · 2020-10-28T17:12:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markmandel, pooneh-m

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [markmandel,pooneh-m]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

agones-bot · 2020-10-28T17:21:02Z

Build Failed 😱

Build Id: a367188d-1a88-4590-a4e1-268bbcefde8e

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

agones-bot · 2020-10-28T17:44:33Z

Build Succeeded 👏

Build Id: f1e60b75-338b-4086-8613-a111b7e9f716

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.10.0-6836e0e
image: gcr.io/agones-images/agones-sdk:1.10.0-6836e0e
image: gcr.io/agones-images/agones-ping:1.10.0-6836e0e
Linux C++ SDK (build): agonessdk-1.10.0-6836e0e-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.10.0-6836e0e.zip

A preview of the website (the last 30 builds are retained):

https://6836e0e-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1863/head:pr_1863 && git checkout pr_1863
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.10.0-6836e0e

google-oss-robot · 2020-10-28T18:07:53Z

New changes are detected. LGTM label has been removed.

agones-bot · 2020-10-28T18:32:16Z

Build Succeeded 👏

Build Id: a569f684-10e0-4852-bc11-4441d0c249af

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.10.0-d485bbc
image: gcr.io/agones-images/agones-sdk:1.10.0-d485bbc
image: gcr.io/agones-images/agones-ping:1.10.0-d485bbc
Linux C++ SDK (build): agonessdk-1.10.0-d485bbc-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.10.0-d485bbc.zip

A preview of the website (the last 30 builds are retained):

https://d485bbc-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1863/head:pr_1863 && git checkout pr_1863
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.10.0-d485bbc

markmandel added kind/bug These are bugs. area/performance Anything to do with Agones being slow, or making it go faster. labels Oct 22, 2020

google-cla bot added the cla: yes label Oct 22, 2020

google-oss-robot requested review from pooneh-m and roberthbailey October 22, 2020 21:38

google-oss-robot added approved size/S labels Oct 22, 2020

roberthbailey reviewed Oct 22, 2020

View reviewed changes

pooneh-m reviewed Oct 22, 2020

View reviewed changes

markmandel force-pushed the bug/allocator-qps branch from 8aaf76d to e3140c3 Compare October 22, 2020 22:43

google-oss-robot added size/M and removed size/S labels Oct 22, 2020

markmandel mentioned this pull request Oct 27, 2020

Allocation Endpoint - Better load balancing for gRPC connection #1872

Closed

markmandel force-pushed the bug/allocator-qps branch from 3c9e050 to 0b3c82d Compare October 27, 2020 23:43

markmandel force-pushed the bug/allocator-qps branch from 0b3c82d to 1dcd7b9 Compare October 28, 2020 17:07

markmandel added 2 commits October 28, 2020 10:08

Add QPS settings to Allocation endpoints

d211b17

Allocation endpoints where throttled to the default ~4qps for a Kubernetes client. Matching the controller settings on standard QPS and Burst to allow higher throughput. Closes googleforgames#1852

Review updates.

6836e0e

markmandel force-pushed the bug/allocator-qps branch from 1dcd7b9 to 6836e0e Compare October 28, 2020 17:08

pooneh-m approved these changes Oct 28, 2020

View reviewed changes

google-oss-robot assigned pooneh-m Oct 28, 2020

google-oss-robot added the lgtm label Oct 28, 2020

Merge branch 'master' into bug/allocator-qps

d485bbc

google-oss-robot removed the lgtm label Oct 28, 2020

pooneh-m merged commit 8c2fa6f into googleforgames:master Oct 28, 2020

markmandel deleted the bug/allocator-qps branch October 28, 2020 18:34

markmandel added this to the 1.10.0 milestone Nov 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add QPS settings to Allocation endpoints #1863

Add QPS settings to Allocation endpoints #1863

markmandel commented Oct 22, 2020

roberthbailey left a comment

markmandel commented Oct 22, 2020

agones-bot commented Oct 22, 2020

pooneh-m left a comment

pooneh-m Oct 22, 2020 •

edited

Loading

markmandel Oct 22, 2020

pooneh-m Oct 23, 2020

roberthbailey Oct 24, 2020

markmandel Oct 26, 2020

pooneh-m Oct 26, 2020

markmandel Oct 28, 2020

pooneh-m commented Oct 22, 2020

markmandel commented Oct 22, 2020

agones-bot commented Oct 22, 2020

pooneh-m commented Oct 23, 2020

agones-bot commented Oct 24, 2020

markmandel commented Oct 27, 2020

agones-bot commented Oct 28, 2020

google-oss-robot commented Oct 28, 2020

agones-bot commented Oct 28, 2020

agones-bot commented Oct 28, 2020

google-oss-robot commented Oct 28, 2020

agones-bot commented Oct 28, 2020

Add QPS settings to Allocation endpoints #1863

Add QPS settings to Allocation endpoints #1863

Conversation

markmandel commented Oct 22, 2020

roberthbailey left a comment

Choose a reason for hiding this comment

markmandel commented Oct 22, 2020

agones-bot commented Oct 22, 2020

pooneh-m left a comment

Choose a reason for hiding this comment

pooneh-m Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

markmandel Oct 22, 2020

Choose a reason for hiding this comment

pooneh-m Oct 23, 2020

Choose a reason for hiding this comment

roberthbailey Oct 24, 2020

Choose a reason for hiding this comment

markmandel Oct 26, 2020

Choose a reason for hiding this comment

pooneh-m Oct 26, 2020

Choose a reason for hiding this comment

markmandel Oct 28, 2020

Choose a reason for hiding this comment

pooneh-m commented Oct 22, 2020

markmandel commented Oct 22, 2020

agones-bot commented Oct 22, 2020

pooneh-m commented Oct 23, 2020

agones-bot commented Oct 24, 2020

markmandel commented Oct 27, 2020

agones-bot commented Oct 28, 2020

google-oss-robot commented Oct 28, 2020

agones-bot commented Oct 28, 2020

agones-bot commented Oct 28, 2020

google-oss-robot commented Oct 28, 2020

agones-bot commented Oct 28, 2020

pooneh-m Oct 22, 2020 •

edited

Loading