Graduate StateAllocationFilter to Stable #3308

Kalaiselvi84 · 2023-08-02T02:17:39Z

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:

Which issue(s) this PR fixes:

Closes #

Special notes for your reviewer:

agones-bot · 2023-08-02T02:25:26Z

Build Failed 😱

Build Id: e1b5fa03-59a6-43d3-bd51-e4ca4186eda4

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

agones-bot · 2023-08-02T15:52:50Z

Build Failed 😱

Build Id: db01691e-0ab3-48ff-a0bc-5fc031a97540

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

markmandel

Great start! Gave some comments on some general cleanup, and also on why those unit tests are failing.

examples/gameserverallocation-deprecated.yaml

examples/gameserverallocation.yaml

markmandel · 2023-08-07T19:42:28Z

site/content/en/docs/Guides/feature-stages.md

@@ -45,7 +45,6 @@ The current set of `alpha` and `beta` feature gates:
 | [GameServer Stable Network ID]({{% ref "/docs/Reference/gameserver.md#stable-network-id" %}})                         | `PodHostname`                  | Enabled  | `Beta`  | 1.32.0 |
 | [Reset Metric Export on Fleet / Autoscaler deletion]({{% relref "./metrics.md#dropping-metric-labels" %}})            | `ResetMetricsOnDelete`         | Enabled  | `Beta`  | 1.32.0 |
 | [Split `agones-controller` ](https://github.com/googleforgames/agones/issues/2797)                                    | `SplitControllerAndExtensions` | Enabled  | `Beta`  | 1.32.0 |
-| [GameServer state filtering on GameServerAllocations](https://github.com/googleforgames/agones/issues/1239)           | `StateAllocationFilter`        | Enabled  | `Beta`  | 1.26.0 |


We'll need to use a feature shortcode for the documentation please.

This line has been added in the expiryVersion shortcode and removed it from the publishVersion shortcode.

This line is in expiryVersion of feature shortcode and lines in expiryVersion shortcode will be removed during the release. Could you please correct me if I am wrong?

pkg/apis/allocation/v1/gameserverallocation.go

pkg/gameserverallocations/allocation_cache_test.go

markmandel · 2023-08-07T19:47:00Z

pkg/gameserverallocations/allocation_cache_test.go

@@ -500,13 +480,10 @@ func TestAllocationCacheCompareGameServers(t *testing.T) {
 	}
 }

-func TestAllocatorRunCacheSyncFeatureStateAllocationFilter(t *testing.T) {
+func TestAllocatorRunCacheSync(t *testing.T) {
 	t.Parallel()

 	// TODO(markmandel): When this feature gets promoted to stable, replace test TestAllocatorRunCacheSync below with this test.


We can drop the TODO as well here please, since we did the replacement 😄

markmandel · 2023-08-07T19:48:34Z

site/content/en/docs/Reference/gameserverallocation.md

+    - matchLabels:
+        game: my-game
+      matchExpressions:
+        - {key: tier, operator: In, values: [cache]}


I might be not seeing something, but shouldn't gameServerState be in here somewhere?

Added it along with description

markmandel · 2023-08-07T19:52:56Z

pkg/allocation/converters/converter_test.go

@@ -216,7 +216,7 @@ func TestConvertAllocationRequestToGameServerAllocation(t *testing.T) {
 		},
 		{
 			name:     "all fields are set",
-			features: fmt.Sprintf("%s=false&%s=false&%s=false", runtime.FeaturePlayerAllocationFilter, runtime.FeatureStateAllocationFilter, runtime.FeatureCountsAndLists),


This test will need to be changed, since it had TestConvertAllocationRequestToGameServerAllocation set to false, so it's expecting a nil GameServer.

Do I need to include something like the below code snippet?
{
name: "nil object",
in: nil,
want: nil,
},

Here is the test implementation:

agones/pkg/allocation/converters/converter_test.go

Lines 565 to 575 in e7d01dc

for _, tc := range tests {

tc := tc

t.Run(tc.name, func(t *testing.T) {

t.Parallel()

runtime.FeatureTestMutex.Lock()

defer runtime.FeatureTestMutex.Unlock()

require.NoError(t, runtime.ParseFeatures(tc.features))

out := ConvertAllocationRequestToGSA(tc.in)

assert.Equal(t, tc.want, out, "mismatch with want after conversion: \"%s\"", tc.name)

This is a really good example of Go table driven tests: https://dave.cheney.net/2019/05/07/prefer-table-driven-tests

Have a look, lemme know if you have questions, and we can go from there 👍🏻

The tests were failing with GameServerState: nil, but they succeeded after updating it to GameServerState: &ready

markmandel · 2023-08-07T19:53:16Z

pkg/allocation/converters/converter_test.go

@@ -366,7 +366,7 @@ func TestConvertAllocationRequestToGameServerAllocation(t *testing.T) {
 		},
 		{
 			name:     "empty fields to GSA with selectors fields",
-			features: fmt.Sprintf("%s=false&%s=false&%s=false", runtime.FeaturePlayerAllocationFilter, runtime.FeatureStateAllocationFilter, runtime.FeatureCountsAndLists),


This test will need to be changed, since it had TestConvertAllocationRequestToGameServerAllocation set to false, so it's expecting a nil GameServer.

agones-bot · 2023-08-07T21:20:57Z

Build Failed 😱

Build Id: 8cb07a2a-df82-44cf-9edc-4fae8d21ad5e

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

agones-bot · 2023-08-09T03:34:09Z

Build Failed 😱

Build Id: cb36d2cd-cae1-4cf6-a9eb-ab9d0d51ad41

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

igooch · 2023-08-09T16:42:22Z

pkg/gameserverallocations/find_test.go

@@ -127,7 +127,7 @@ func TestFindGameServerForAllocationPacked(t *testing.T) {
 				assert.Equal(t, ErrNoGameServer, err)
 				assert.Nil(t, gs)
 			},
-			features: fmt.Sprintf("%s=false&%s=false", runtime.FeaturePlayerAllocationFilter, runtime.FeatureStateAllocationFilter),


You can delete this entire "one label" test, as it is only passing when FeatureStateAllocationFilter=False.

(The test below "one label with player state (StateAllocationFilter)" is for when StateAllocationFilter=True.)

Deleted the "one label" test.

igooch · 2023-08-09T16:44:49Z

pkg/gameserverallocations/find_test.go

@@ -127,7 +127,7 @@ func TestFindGameServerForAllocationPacked(t *testing.T) {
 				assert.Equal(t, ErrNoGameServer, err)
 				assert.Nil(t, gs)
 			},
-			features: fmt.Sprintf("%s=false&%s=false", runtime.FeaturePlayerAllocationFilter, runtime.FeatureStateAllocationFilter),
+			features: fmt.Sprintf("%s=false", runtime.FeaturePlayerAllocationFilter),
 		},
 		"one label with player state (StateAllocationFilter)": {
 			// nolint: dupl


You can also delete this line // nolint: dupl once the "one label" test is deleted.

Deleted // nolint: dupl line

igooch · 2023-08-09T17:37:27Z

pkg/gameserverallocations/find_test.go

@@ -278,7 +278,7 @@ func TestFindGameServerForAllocationPacked(t *testing.T) {
 				assert.Equal(t, gs, list[index])
 				assert.Equal(t, agonesv1.GameServerStateReady, gs.Status.State)
 			},
-			features: fmt.Sprintf("%s=false&%s=false", runtime.FeaturePlayerAllocationFilter, runtime.FeatureStateAllocationFilter),


Change line 273 from require.Len(t, list, 4) to require.Len(t, list, 8).

The previous StateAllocationFilter=false used a cache with matcher gameserverallocations.readyGameServerMatcher which only returns the ready gameservers. The current StateAllocationFilter=true uses a cache with matcher gameserverallocations.readyOrAllocatedGameServerMatcher which returns ready or allocated game servers.

Done!
I have made changes to a few more lines that were indicated in the error log.

igooch · 2023-08-09T18:06:54Z

You'll also need to navigate to agones/build and then run make gen-api-docs since some of the code comments have changed.

I usually do this (and any other generated files) as unique commit within the PR just in case they need to be dropped and regenerated later.

agones-bot · 2023-08-09T18:13:29Z

Build Failed 😱

Build Id: 078051e6-f288-43a4-b986-aa1b1c016edf

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

Kalaiselvi84 · 2023-08-09T18:27:39Z

make gen-api-docs

This command updated aones_crd_api_reference.html file

agones-bot · 2023-08-09T18:28:23Z

Build Failed 😱

Build Id: 551cca44-b99f-44b1-b1dd-60ad1da105b7

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

agones-bot · 2023-08-09T21:53:32Z

Build Failed 😱

Build Id: 4c4bd9b8-69cd-4508-947d-893827fd7b61

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

igooch · 2023-08-09T22:57:22Z

File agones/pkg/gameserverallocations/allocator_test.go has a remaining reference to the FeatureStateAllocationFilter.

Kalaiselvi84 · 2023-08-09T23:48:22Z

File agones/pkg/gameserverallocations/allocator_test.go has a remaining reference to the FeatureStateAllocationFilter.

Excluded the FeatureStateAllocationFilter from agones/pkg/gameserverallocations/allocator_test.go

agones-bot · 2023-08-10T01:32:26Z

Build Succeeded 👏

Build Id: 659baa80-6d75-4414-8b42-84d96e54cfc2

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://e198445-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3308/head:pr_3308 && git checkout pr_3308
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.34.0-dev-e198445-amd64

Kalaiselvi84 · 2023-08-10T19:13:01Z

Please find the error log here. There are 4 test cases failing, but the build has succeeded.

markmandel · 2023-08-10T19:42:39Z

Honestly, I don't usually run the full suite of e2e tests locally - it takes too long, and there are usually some setup that make differ slightly than the prod testing setup (might as well make other computers do all the work while doing other things!).

I usually just focus on https://github.com/googleforgames/agones/blob/main/build/README.md#running-individual-end-to-end-tests if there are specific tests I think are very relevant to what I'm working on or are failing in Cloud Build.

So if you are passing in Cloud Build - you are passing!

igooch · 2023-08-10T19:54:10Z

test/e2e/allocator_test.go

@@ -201,7 +201,7 @@ func TestAllocatorWithSelectors(t *testing.T) {
 		helper.ValidateAllocatorResponse(t, response)

 		// let's do a re-allocation
-		if runtime.FeatureEnabled(runtime.FeatureStateAllocationFilter) && runtime.FeatureEnabled(runtime.FeaturePlayerAllocationFilter) {
+		if runtime.FeatureEnabled(runtime.FeaturePlayerAllocationFilter) {
 			logrus.Info("testing state allocation filter")


We should probably pull the state allocation filter tests out of this if runtime.FeatureEnabled(runtime.FeaturePlayerAllocationFilter) block.

igooch · 2023-08-10T19:55:54Z

test/e2e/allocator_test.go

@@ -106,7 +106,7 @@ func TestAllocatorWithDeprecatedRequired(t *testing.T) {
 		helper.ValidateAllocatorResponse(t, response)

 		// let's do a re-allocation
-		if runtime.FeatureEnabled(runtime.FeatureStateAllocationFilter) && runtime.FeatureEnabled(runtime.FeaturePlayerAllocationFilter) {
+		if runtime.FeatureEnabled(runtime.FeaturePlayerAllocationFilter) {
 			logrus.Info("testing state allocation filter")


We should probably pull the state allocation filter tests out of this if runtime.FeatureEnabled(runtime.FeaturePlayerAllocationFilter) block.

agones-bot · 2023-08-11T00:16:16Z

Build Succeeded 👏

Build Id: 56a46d89-1850-43cb-b874-5b723796c97a

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://61dab35-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3308/head:pr_3308 && git checkout pr_3308
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.34.0-dev-61dab35-amd64

agones-bot · 2023-08-11T02:01:48Z

Build Succeeded 👏

Build Id: ceb462f7-2200-4503-8d4b-d8ecdd40a0dc

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://b43eb8d-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3308/head:pr_3308 && git checkout pr_3308
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.34.0-dev-b43eb8d-amd64

Kalaiselvi84 · 2023-08-11T02:19:33Z

there are usually some setup that make differ slightly than the prod testing setup

In this case, I will move the PR to "Ready for Review"

markmandel

This LGTM! @igooch did you want to take another pass?

igooch

LGTM

google-oss-prow · 2023-08-11T17:39:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: igooch, Kalaiselvi84, markmandel

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]

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

Graduate StateAllocationFilter to Stable

58f8915

Kalaiselvi84 requested a review from markmandel August 2, 2023 02:17

google-oss-prow bot added the do-not-merge/work-in-progress label Aug 2, 2023

google-oss-prow bot requested review from cyriltovena and EricFortin August 2, 2023 02:17

google-oss-prow bot added the size/L label Aug 2, 2023

github-actions bot added the kind/feature New features for Agones label Aug 2, 2023

Kalaiselvi84 removed request for cyriltovena and EricFortin August 2, 2023 02:18

Kalaiselvi84 mentioned this pull request Aug 2, 2023

move StateAllocationFilter to stable #3238

Closed

resolved lint

e7d01dc

markmandel reviewed Aug 7, 2023

View reviewed changes

revised necessary modification

8bc1a1d

markmandel mentioned this pull request Aug 8, 2023

Sort by Priority for strategy Distributed #3296

Merged

GameServerState update

5511fa9

modified expected values in find_test.go

a374be9

igooch requested changes Aug 9, 2023

View reviewed changes

google-oss-prow bot assigned igooch Aug 9, 2023

Kalaiselvi84 added 2 commits August 9, 2023 18:16

one label test deleted

fb37cd7

make gen-api-docs target made change in aones_crd_api_reference.html

2399fd6

google-oss-prow bot added size/XXL and removed size/L labels Aug 9, 2023

google-oss-prow bot added the size/L label Aug 9, 2023

Elimination of FeatureStateAllocationFilter

e198445

igooch reviewed Aug 10, 2023

View reviewed changes

removed log for state allocation filter

61dab35

Kalaiselvi84 added 2 commits August 10, 2023 17:40

Merge branch 'main' into pr_3096

1e8e887

Merge branch 'main' into pr_3096

b43eb8d

Kalaiselvi84 marked this pull request as ready for review August 11, 2023 02:19

google-oss-prow bot removed the do-not-merge/work-in-progress label Aug 11, 2023

google-oss-prow bot requested review from cyriltovena and markmandel August 11, 2023 02:19

markmandel approved these changes Aug 11, 2023

View reviewed changes

google-oss-prow bot assigned markmandel Aug 11, 2023

google-oss-prow bot added lgtm approved labels Aug 11, 2023

igooch approved these changes Aug 11, 2023

View reviewed changes

markmandel merged commit 62d30b3 into googleforgames:main Aug 11, 2023
3 checks passed

markmandel mentioned this pull request Aug 21, 2023

Graduate StateAllocationFilter to Stable #3096

Closed

igooch mentioned this pull request Jan 18, 2024

Request for igooch to become Approver #3600

Closed

5 tasks

Kalaiselvi84 deleted the pr_3096 branch March 15, 2024 01:14

	for _, tc := range tests {
	tc := tc
	t.Run(tc.name, func(t *testing.T) {
	t.Parallel()

	runtime.FeatureTestMutex.Lock()
	defer runtime.FeatureTestMutex.Unlock()
	require.NoError(t, runtime.ParseFeatures(tc.features))

	out := ConvertAllocationRequestToGSA(tc.in)
	assert.Equal(t, tc.want, out, "mismatch with want after conversion: \"%s\"", tc.name)

Graduate StateAllocationFilter to Stable #3308

Graduate StateAllocationFilter to Stable #3308

Conversation

Kalaiselvi84 commented Aug 2, 2023

agones-bot commented Aug 2, 2023

agones-bot commented Aug 2, 2023

markmandel 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agones-bot commented Aug 7, 2023

agones-bot commented Aug 9, 2023

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

igooch commented Aug 9, 2023

agones-bot commented Aug 9, 2023

Kalaiselvi84 commented Aug 9, 2023

agones-bot commented Aug 9, 2023

agones-bot commented Aug 9, 2023

igooch commented Aug 9, 2023

Kalaiselvi84 commented Aug 9, 2023

agones-bot commented Aug 10, 2023

Kalaiselvi84 commented Aug 10, 2023

markmandel commented Aug 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agones-bot commented Aug 11, 2023

agones-bot commented Aug 11, 2023

Kalaiselvi84 commented Aug 11, 2023

markmandel left a comment

Choose a reason for hiding this comment

igooch left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Aug 11, 2023