feat: allocation response with counters and lists data #3681

katsew · 2024-03-06T09:28:20Z

What type of PR is this?

Uncomment only one /kind <> line, press 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
/kind release

What this PR does / Why we need it:

Currently the CountsAndLists feature does not return the status of counters and lists for GameServer Allocation requests.
This makes it difficult to know how the counters and lists in the GameServer changed before and after the request.
This PR modifies the response to return those data.

Which issue(s) this PR fixes:

Closes #3661

Special notes for your reviewer:

NA

github-actions · 2024-03-06T09:28:38Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

agones-bot · 2024-03-06T09:40:52Z

Build Failed 😱

Build Id: ce442ff8-0d5a-4fc6-ae41-3037a697beef

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

github-actions · 2024-03-06T12:05:48Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

agones-bot · 2024-03-06T13:23:08Z

Build Succeeded 👏

Build Id: 2e525cfd-5707-4dd3-9806-dad09a2b8a17

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

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

https://66cf2cb-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3681/head:pr_3681 && git checkout pr_3681
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.39.0-dev-66cf2cb-amd64

markmandel

This looks amazing! A few of additional things I think we need to finish this off:

Let's add some e2e integration tests:

agones/test/e2e/gameserverallocation_test.go

Line 264 in 12d82a1

func TestCounterAndListGameServerAllocation(t *testing.T) {

May be a good spot to add in some tests for allocation returns.

https://github.com/googleforgames/agones/blob/main/test/e2e/allocator_test.go

I don't think we have a test in here for Counters and Lists (@igooch did I miss a test in here?), so will have to write one -- probably only needs to be something that just checks that conversation between GameServerAllocation and the proto works.

Also, let's add some docs!

https://agones.dev/site/docs/reference/gameserverallocation/#:~:text=Once%20created%20the%20GameServerAllocation%20will%20have%20a%20status%20field%20consisting%20of%20the%20following in this section.

For the new docs, please wrap them in a feature code for the next release as per https://agones.dev/site/docs/contribute/

github-actions · 2024-03-07T12:48:15Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

katsew · 2024-03-07T13:01:55Z

@markmandel

Thank you for your review!
I added the doc and tests, but I misread your comment and wrote the test in allocator_test.go ...
I will write a test in gameserverallocation_test.go soon.
If it is better to leave allocator_test.go as well, I will leave it as it is, but what do you think?

agones-bot · 2024-03-07T13:53:09Z

Build Succeeded 👏

Build Id: 2258167c-bfc1-4a52-bec9-cdcf28aaf437

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

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

https://7c467db-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3681/head:pr_3681 && git checkout pr_3681
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.39.0-dev-7c467db-amd64

markmandel · 2024-03-08T02:53:13Z

@markmandel

Thank you for your review! I added the doc and tests, but I misread your comment and wrote the test in allocator_test.go ... I will write a test in gameserverallocation_test.go soon. If it is better to leave allocator_test.go as well, I will leave it as it is, but what do you think?

This is awesome!

I'd definitely keep everything in allocator_test.go, my thought would be - for:

agones/test/e2e/gameserverallocation_test.go

Line 264 in 1c536c0

func TestCounterAndListGameServerAllocation(t *testing.T) {

Enhance this test to also check expected return values for list and/or counter values for each test case.

I think here would be the right spot:

agones/test/e2e/gameserverallocation_test.go

Lines 603 to 606 in 1c536c0

    
           gsa, err = framework.AgonesClient.AllocationV1().GameServerAllocations(flt.ObjectMeta.Namespace).Create(ctx, gsa.DeepCopy(), metav1.CreateOptions{}) 
        
           require.NoError(t, err) 
        
           assert.Equal(t, string(testCase.wantAllocated), string(gsa.Status.State)) 
        
           assert.Equal(t, gs1.ObjectMeta.Name, gsa.Status.GameServerName)

And I think this would be good to go - and we can squeeze this in before release next Tuesday!

Very excited to see this get added!

github-actions · 2024-03-08T04:38:05Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

katsew · 2024-03-08T04:51:36Z

test/e2e/gameserverallocation_test.go

@@ -589,6 +591,10 @@ func TestCounterAndListGameServerAllocation(t *testing.T) {
 			}
 			require.NoError(t, err)
 			assert.Equal(t, string(testCase.wantAllocated), string(gsa.Status.State))
+			if gsa.Status.State == allocated {


I thought this test is for counters and lists selector, so I check that the status of counters and lists does not change before/after request.
Since GSA may returns UnAllocated state, check the status only if GSA state is Allocated.
Please correct me if i'm wrong.

This makes sense to me!

katsew · 2024-03-08T04:56:25Z

@markmandel

Thanks for the review.
I have written additional tests in gameserverallocation_test.go.
I commented where I wasn't sure, so please let me know if there are any problems.

markmandel

This LGTM!

@igooch did you wan to take a look and see if I missed anything? Otherwise, i think this is good to merge before release next week.

markmandel · 2024-03-08T05:06:03Z

test/e2e/gameserverallocation_test.go

@@ -589,6 +591,10 @@ func TestCounterAndListGameServerAllocation(t *testing.T) {
 			}
 			require.NoError(t, err)
 			assert.Equal(t, string(testCase.wantAllocated), string(gsa.Status.State))
+			if gsa.Status.State == allocated {


This makes sense to me!

agones-bot · 2024-03-08T06:26:21Z

Build Succeeded 👏

Build Id: 3ea62294-ae0d-4a93-9321-18e23ea2ee29

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

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

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3681/head:pr_3681 && git checkout pr_3681
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.39.0-dev-fba6863-amd64

katsew · 2024-03-08T07:32:59Z

Thank you for approval.
I'll rebase my changes into a single commit.
If there are any additional corrections needed, please let me know.

Resolves googleforgames#3661

github-actions · 2024-03-08T07:38:59Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

agones-bot · 2024-03-08T08:42:13Z

Build Succeeded 👏

Build Id: ea24dfbf-0e4e-4bdf-ac2b-b04cb3e1914a

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

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

https://1db67b4-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3681/head:pr_3681 && git checkout pr_3681
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.39.0-dev-1db67b4-amd64

github-actions · 2024-03-11T16:47:36Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

agones-bot · 2024-03-11T17:04:00Z

Build Failed 😱

Build Id: 68d9ec15-3252-4c6a-aaee-d4cbb1d4d375

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

markmandel · 2024-03-11T18:48:31Z

Weird flake with csharp sdk

Could not send ready message. StatusCode=Unavailable, Detail=Error connecting to subchannel.
{"grpcEndpoint":":9005","message":"Starting SDKServer grpc service...","severity":"info","source":"main","time":"2024-03-11T17:00:42.815571669Z"}
{"httpEndpoint":":9105","message":"Starting SDKServer grpc-gateway...","severity":"info","source":"main","time":"2024-03-11T17:00:42.81722091Z"}
make[3]: *** [includes/sdk.mk:91: run-sdk-command] Error 1
{"message":"Shutting down SDK server","severity":"info","source":"main","time":"2024-03-11T17:01:06.038057474Z"}
{"message":"Received requests match expected list. Test run was successful","sdkName":"rust","severity":"info","source":"*sdkserver.LocalSDKServer","time":"2024-03-11T17:01:06.038153154Z"}
{"error":"http: Server closed","message":"http server closed","severity":"info","source":"main","time":"2024-03-11T17:01:06.038219772Z"}

github-actions · 2024-03-11T19:32:01Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

agones-bot · 2024-03-11T21:22:37Z

Build Succeeded 👏

Build Id: 1e4a92f2-c846-44dd-b928-6318beac54cf

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

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

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3681/head:pr_3681 && git checkout pr_3681
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.39.0-dev-c9930e2-amd64

igooch

LGTM

github-actions bot added kind/feature New features for Agones size/XL labels Mar 6, 2024

katsew force-pushed the return-counts-and-lists-data branch from 5d54199 to 66cf2cb Compare March 6, 2024 12:05

markmandel requested a review from igooch March 7, 2024 03:35

markmandel reviewed Mar 7, 2024

View reviewed changes

katsew commented Mar 8, 2024

View reviewed changes

markmandel approved these changes Mar 8, 2024

View reviewed changes

feat: allocation response with counters and lists data

1db67b4

Resolves googleforgames#3661

katsew force-pushed the return-counts-and-lists-data branch from fba6863 to 1db67b4 Compare March 8, 2024 07:38

Merge branch 'main' into return-counts-and-lists-data

8478110

roberthbailey assigned markmandel Mar 11, 2024

Merge branch 'main' into return-counts-and-lists-data

c9930e2

igooch approved these changes Mar 11, 2024

View reviewed changes

markmandel merged commit 8cf0986 into googleforgames:main Mar 11, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allocation response with counters and lists data #3681

feat: allocation response with counters and lists data #3681

katsew commented Mar 6, 2024 •

edited

Loading

github-actions bot commented Mar 6, 2024

agones-bot commented Mar 6, 2024

github-actions bot commented Mar 6, 2024

agones-bot commented Mar 6, 2024

markmandel left a comment

github-actions bot commented Mar 7, 2024

katsew commented Mar 7, 2024

agones-bot commented Mar 7, 2024

markmandel commented Mar 8, 2024

github-actions bot commented Mar 8, 2024

katsew Mar 8, 2024 •

edited

Loading

markmandel Mar 8, 2024

katsew commented Mar 8, 2024

markmandel left a comment

markmandel Mar 8, 2024

agones-bot commented Mar 8, 2024

katsew commented Mar 8, 2024 •

edited

Loading

github-actions bot commented Mar 8, 2024

agones-bot commented Mar 8, 2024

github-actions bot commented Mar 11, 2024

agones-bot commented Mar 11, 2024

markmandel commented Mar 11, 2024

github-actions bot commented Mar 11, 2024

agones-bot commented Mar 11, 2024

igooch left a comment

feat: allocation response with counters and lists data #3681

feat: allocation response with counters and lists data #3681

Conversation

katsew commented Mar 6, 2024 • edited Loading

github-actions bot commented Mar 6, 2024

agones-bot commented Mar 6, 2024

github-actions bot commented Mar 6, 2024

agones-bot commented Mar 6, 2024

markmandel left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 7, 2024

katsew commented Mar 7, 2024

agones-bot commented Mar 7, 2024

markmandel commented Mar 8, 2024

github-actions bot commented Mar 8, 2024

katsew Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

markmandel Mar 8, 2024

Choose a reason for hiding this comment

katsew commented Mar 8, 2024

markmandel left a comment

Choose a reason for hiding this comment

markmandel Mar 8, 2024

Choose a reason for hiding this comment

agones-bot commented Mar 8, 2024

katsew commented Mar 8, 2024 • edited Loading

github-actions bot commented Mar 8, 2024

agones-bot commented Mar 8, 2024

github-actions bot commented Mar 11, 2024

agones-bot commented Mar 11, 2024

markmandel commented Mar 11, 2024

github-actions bot commented Mar 11, 2024

agones-bot commented Mar 11, 2024

igooch left a comment

Choose a reason for hiding this comment

katsew commented Mar 6, 2024 •

edited

Loading

katsew Mar 8, 2024 •

edited

Loading

katsew commented Mar 8, 2024 •

edited

Loading