adding timeout to remote cluster allocate call and adding total timeout to allocate #1815

kdima · 2020-09-15T13:01:40Z

What type of PR is this?
/kind feature

What this PR does / Why we need it:
This PR adds an explicit timeout to the Allocate call. This timeout includes both the RPC itself and creating the connection. I also added the total timeout for Allocate retries.
I think total timeout should be configurable as a command line parameter but I am not sure where to add that. Is cmd/allocator/metrics.go:43 struct the right place?

Which issue(s) this PR fixes:
Closes #1700

Special notes for your reviewer:

googlebot · 2020-09-15T13:01:44Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

agones-bot · 2020-09-15T13:09:32Z

Build Failed 😱

Build Id: d4d76177-4c85-4ceb-9bae-61d687f262cb

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

googlebot · 2020-09-15T13:33:25Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

agones-bot · 2020-09-15T13:44:30Z

Build Failed 😱

Build Id: c1b9fed2-5c34-4a8c-a8e5-89d79e1128be

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

agones-bot · 2020-09-15T14:30:28Z

Build Succeeded 👏

Build Id: ec29894a-f815-4afa-b8fc-574372fddcd1

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

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

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

https://464d177-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1815/head:pr_1815 && git checkout pr_1815
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.9.0-464d177

agones-bot · 2020-09-15T14:42:15Z

Build Succeeded 👏

Build Id: 4f63122b-89ec-4597-9df3-87ae4ee1d4e2

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

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

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

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1815/head:pr_1815 && git checkout pr_1815
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.9.0-da989a1

markmandel · 2020-09-15T15:55:12Z

Thanks for your contribution! Should this timeout be documented somewhere?

pooneh-m

@kdima thanks for the change.

pooneh-m · 2020-09-15T16:56:25Z

pkg/gameserverallocations/allocator.go

+	// Timeout for an individual Allocate RPC call can take
+	remoteAllocateTimeout = 10 * time.Second
+	// Total timeout for allocation including retries
+	totalRemoteAllocationTimeout = 30 * time.Second


The timeout is set, but not used?
For the total timeout you can check the time passed in the retry: https://github.com/googleforgames/agones/blob/master/pkg/gameserverallocations/allocator.go#L340

Fail with DEADLINE_EXCEEDED when the request does not complete by 30s.

Thanks for catching this. Added.

pooneh-m · 2020-09-15T17:12:18Z

pkg/gameserverallocations/allocator.go

@@ -82,6 +82,13 @@ const (
 	batchWaitTime         = 500 * time.Millisecond
 )

+const (
+	// Timeout for an individual Allocate RPC call can take
+	remoteAllocateTimeout = 10 * time.Second


Also the client timeout needs to be set on the request context: https://grpc.io/blog/deadlines/#go

I think I am setting it

agones/pkg/gameserverallocations/allocator.go

Line 153 in cd28618

return grpcClient.Allocate(allocationCtx, request)

Unless you mean something else?

robbieheywood · 2020-09-16T08:24:20Z

Looks really good Dima - +1 to making this a CLI argument if possible (we'd probably want to set this lower than the defaults here - maybe something like 1s). Also, is there any way to add a metric for this (i.e. number of allocation timeouts)?

agones-bot · 2020-09-16T08:56:52Z

Build Failed 😱

Build Id: 6aa7e7ee-1237-4466-835d-0903410fe31d

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

agones-bot · 2020-09-16T09:56:21Z

Build Failed 😱

Build Id: b43f3dc5-5582-401d-aed4-f5270bcdb154

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

agones-bot · 2020-09-17T09:29:27Z

Build Succeeded 👏

Build Id: 93d2f848-f37e-40dc-977c-efb3a39934fe

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

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

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

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1815/head:pr_1815 && git checkout pr_1815
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.9.0-c25c5cf

pooneh-m

Thanks for the update. Can we add testing for it?
For the env flag and Helm config, here is an example of plumbing through a config value: https://github.com/googleforgames/agones/pull/1777/files

pooneh-m · 2020-09-17T16:29:37Z

pkg/gameserverallocations/allocator.go

@@ -82,6 +82,13 @@ const (
 	batchWaitTime         = 500 * time.Millisecond
 )

+const (
+	// Timeout for an individual Allocate RPC call can take
+	remoteAllocateTimeout = 10 * time.Second


pooneh-m · 2020-09-17T19:36:30Z

pkg/gameserverallocations/allocator.go

+		select {
+		case <-ctx.Done():
+			return status.Errorf(codes.DeadlineExceeded, "remote allocation retry timeout exceeded")
+		default:


Shouldn't this be after defer cancel()?

I changed the code a fair bit, see if it makes sense now

agones-bot · 2020-10-16T14:22:56Z

Build Failed 😱

Build Id: 5d773430-7522-462e-afd4-f69c441c5235

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

pooneh-m · 2020-10-16T17:22:50Z

pkg/gameserverallocations/allocator.go

@@ -62,6 +62,8 @@ var (
 	ErrNoGameServerReady = errors.New("Could not find a Ready GameServer")
 	// ErrConflictInGameServerSelection is returned when the candidate gameserver already allocated
 	ErrConflictInGameServerSelection = errors.New("The Gameserver was already allocated")
+	// ErrTotalTimeoutExceeded is used to signal that total retry timeout has been exceeded and no additional retries should be made
+	ErrTotalTimeoutExceeded          = status.Errorf(codes.DeadlineExceeded, "remote allocation retry timeout exceeded")


s/retry/total

pooneh-m · 2020-10-16T17:30:44Z

I added custom ErrTotalTimeoutExceeded that follow the pattern similar to the one from ErrNoGameServerReady this way error is reported as DeadlineExceeded which I agree is much better for the user. And we still have the test that verifies that no allocation calls are made after the total timeout is exceeded.

Looks great, thanks for the change! You are missing a test coverage for per request timeout. Please add a test that validates that retries happen when the allocation request has exceeds the remoteAllocationTimeout timeout limit.

agones-bot · 2020-10-19T09:53:40Z

Build Failed 😱

Build Id: ec4289f1-f5cb-4812-9e82-108aad3869e4

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

agones-bot · 2020-10-19T10:01:51Z

Build Failed 😱

Build Id: 43703567-32bc-4385-89f3-7cb5b598b60c

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

agones-bot · 2020-10-19T10:17:14Z

Build Failed 😱

Build Id: a2d86d19-43f6-4e23-9b21-9b0a75a5cc9e

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

agones-bot · 2020-10-19T14:12:28Z

Build Failed 😱

Build Id: e5d5436e-893c-4a4c-bc05-4871e9ac08de

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

agones-bot · 2020-10-19T14:50:52Z

Build Failed 😱

Build Id: b42f23ee-135b-46d3-85a7-83d6c420d4f1

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

pooneh-m

Great addition. Thank you!

pooneh-m · 2020-10-19T17:11:39Z

pkg/gameserverallocations/controller_test.go

+		assert.Equal(t, st.Code(), codes.DeadlineExceeded)
+		assert.Equal(t, 0, calls)
+	})
+	t.Run("No allocations called after total timeout", func(t *testing.T) {


The description of the test should be updated?

pooneh-m · 2020-10-19T17:15:22Z

pkg/gameserverallocations/controller_test.go

+		assert.Equal(t, 0, calls)
+	})
+	t.Run("No allocations called after total timeout", func(t *testing.T) {
+		c, m := newFakeControllerWithTimeout(10*time.Second, 10*time.Second)


Should we use newFakeController if the values are not used?

pooneh-m · 2020-10-19T17:16:13Z

pkg/gameserverallocations/controller_test.go

+		calls := 0
+		c.allocator.remoteAllocationCallback = func(ctx context.Context, endpoint string, dialOpt grpc.DialOption, request *pb.AllocationRequest) (*pb.AllocationResponse, error) {
+			if calls == 0 {
+				calls++


you can move calls to the top of the func and condition of calls == 1. Please also leave a comment on what the function is doing.

pooneh-m · 2020-10-19T17:18:08Z

pkg/gameserverallocations/controller_test.go

+
+		// Allocation policy reactor
+		secretName := clusterName + "secret"
+		m.AgonesClient.AddReactor("list", "gameserverallocationpolicies", func(action k8stesting.Action) (bool, k8sruntime.Object, error) {


Can you refactor the reactors to a method call?

google-oss-robot · 2020-10-19T17:19:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kdima, 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~~ [pooneh-m]

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

pooneh-m · 2020-10-19T17:21:35Z

It looks like your changes is causing TestAllocatorCrossNamespace to fail. Please take a look.

google-oss-robot · 2020-10-20T15:47:47Z

New changes are detected. LGTM label has been removed.

agones-bot · 2020-10-20T16:09:02Z

Build Succeeded 👏

Build Id: 568eeb41-017e-44c1-aacd-1e8148534530

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-8bdf18c
image: gcr.io/agones-images/agones-sdk:1.10.0-8bdf18c
image: gcr.io/agones-images/agones-ping:1.10.0-8bdf18c
Linux C++ SDK (build): agonessdk-1.10.0-8bdf18c-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.10.0-8bdf18c.zip

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

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

To install this version:

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

…ut to allocate (googleforgames#1815) * adding timeout to allocate call and adding total timeout to allocate * adding explicit context check to retry * added part of plumbing for timeout flags * allocation * generated install.yaml * retry test * Added test * cleanup of fake controller * test improvement * Added docs * linting * lint * fixing yamls * fixes * testcase for illustration * custom error used to exit Retry * comment * test * fixes * fixes * fixes * fixes * potential e2e test

google-oss-robot requested review from dzlier-gcp and markmandel September 15, 2020 13:01

googlebot added the cla: no label Sep 15, 2020

google-oss-robot added the size/S label Sep 15, 2020

googlebot added cla: yes and removed cla: no labels Sep 15, 2020

adding timeout to allocate call and adding total timeout to allocate

da989a1

kdima force-pushed the new-timeout branch from 464d177 to da989a1 Compare September 15, 2020 14:12

markmandel requested review from pooneh-m and removed request for dzlier-gcp September 15, 2020 15:54

pooneh-m reviewed Sep 15, 2020

View reviewed changes

adding explicit context check to retry

cd28618

google-oss-robot added size/M and removed size/S labels Sep 16, 2020

Merge branch 'master' into new-timeout

7b92a82

kdima requested a review from pooneh-m September 16, 2020 10:04

Merge branch 'master' into new-timeout

c25c5cf

pooneh-m reviewed Sep 17, 2020

View reviewed changes

added part of plumbing for timeout flags

078048e

Merge branch 'master' into new-timeout

4f36c33

pooneh-m reviewed Oct 16, 2020

View reviewed changes

Dmytro Kislov added 2 commits October 19, 2020 10:44

test

7e4b9f8

fixes

e35092a

fixes

9f466db

fixes

4a910fd

fixes

b3e2817

Merge branch 'master' into new-timeout

e16e75f

pooneh-m approved these changes Oct 19, 2020

View reviewed changes

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

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

google-oss-robot added the approved label Oct 19, 2020

potential e2e test

8bdf18c

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

pooneh-m merged commit 926e277 into googleforgames:master Oct 20, 2020

markmandel added this to the 1.10.0 milestone Nov 3, 2020

markmandel added area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/feature New features for Agones labels Nov 3, 2020

adding timeout to remote cluster allocate call and adding total timeout to allocate #1815

adding timeout to remote cluster allocate call and adding total timeout to allocate #1815

Conversation

kdima commented Sep 15, 2020

googlebot commented Sep 15, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

agones-bot commented Sep 15, 2020

googlebot commented Sep 15, 2020

agones-bot commented Sep 15, 2020

agones-bot commented Sep 15, 2020

agones-bot commented Sep 15, 2020

markmandel commented Sep 15, 2020

pooneh-m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbieheywood commented Sep 16, 2020

agones-bot commented Sep 16, 2020

agones-bot commented Sep 16, 2020

agones-bot commented Sep 17, 2020

pooneh-m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agones-bot commented Oct 16, 2020

Choose a reason for hiding this comment

pooneh-m commented Oct 16, 2020

agones-bot commented Oct 19, 2020

agones-bot commented Oct 19, 2020

agones-bot commented Oct 19, 2020

agones-bot commented Oct 19, 2020

agones-bot commented Oct 19, 2020

pooneh-m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-oss-robot commented Oct 19, 2020

pooneh-m commented Oct 19, 2020

google-oss-robot commented Oct 20, 2020

agones-bot commented Oct 20, 2020