move StateAllocationFilter to stable #3238

ashutosji · 2023-06-28T06:34:07Z

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

What this PR does / Why we need it:

Which issue(s) this PR fixes:

Closes ##3096

Special notes for your reviewer:

google-oss-prow · 2023-06-28T06:34:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ashutosji
Once this PR has been reviewed and has the lgtm label, please assign alekser for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

agones-bot · 2023-06-28T06:45:20Z

Build Failed 😱

Build Id: 39f18679-2c84-4882-a8ab-fd2b5093cc29

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

agones-bot · 2023-06-28T07:05:40Z

Build Failed 😱

Build Id: 34af16e5-21a4-4164-8bd5-b273e4bf67ac

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

markmandel · 2023-06-28T21:50:40Z

I see a couple of failing tests - TestFindGameServerForAllocationPacked and TestFindGameServerForAllocationPacked/one_label in the unit tests.

Are they also failing for you locally? Are you able to reproduce the issues?

markmandel

Good start! Gave you some direction on how to tidy it up.

Haven't looked into why the tests are failing, but let us know what you find and we'll narrow it down if you are struggling with it.

markmandel · 2023-06-29T19:37:43Z

pkg/gameserverallocations/allocation_cache.go

 		c.matcher = readyOrAllocatedGameServerMatcher
-	}
+


We could clean up up more, and replace the matcher above on line 66 directly with the readyOrAllocatedGameServerMatcher -- and we could delete the readyGameServerMatcher function entirely.

(There's possibly a good reason to get rid of matcher as a data structure entirely, since we don't need to switch it out at runtime anymore).

Yeah, I forgot it. Thanks for reminding i will correct that.

markmandel · 2023-06-29T19:39:17Z

pkg/gameserverallocations/allocation_cache_test.go

 	runtime.FeatureTestMutex.Lock()
 	defer runtime.FeatureTestMutex.Unlock()


Suggested change

runtime.FeatureTestMutex.Lock()

defer runtime.FeatureTestMutex.Unlock()

Since we have no feature flags set, we don't need the mutex anymore.

We do this because Go tests run in parallel, but feature flags are stored globally, so this allows us to have tests that don't collide. No feature flags here anymore means we don't need the mutex anymore!

markmandel · 2023-06-29T19:39:46Z

pkg/gameserverallocations/find_test.go

@@ -94,7 +94,7 @@ func TestFindGameServerForAllocationPacked(t *testing.T) {
 				{ObjectMeta: metav1.ObjectMeta{Name: "gs6", Namespace: "does-not-apply", Labels: oneLabel}, Status: agonesv1.GameServerStatus{NodeName: "node1", State: agonesv1.GameServerStateReady}},
 			},
 			test: func(t *testing.T, list []*agonesv1.GameServer) {
-				assert.Len(t, list, 3)
+				assert.Len(t, list, 5)


Just checking why the change here?

The reason of changing 3 to 5 was the actual and expected outcome was differing. Correct me if i am wrong in assert.Len() function we are passing length as gameserver with state in GameServerStateReady?

markmandel · 2023-06-29T19:41:37Z

site/content/en/docs/Integration Patterns/high-density-gameservers.md

@@ -6,8 +6,7 @@ weight: 70
 description: >
  How to run multiple concurrent game sessions in a single GameServer process.
 ---
-{{< beta title="Allocation State Filter" gate="StateAllocationFilter" >}}
-
+{{% feature publishVersion="1.32.0" %}}


What we would want to do here instead is use feature expiryVersion to wrap the beta shortcode - since we want to remove it up until the next release.

The way it is currently would mean that as soon as we pushed this live, the whole block of docs would be removed.

markmandel · 2023-06-29T19:42:23Z

site/content/en/docs/Integration Patterns/player-capacity.md

@@ -10,7 +10,6 @@ description: >
 {{< alpha
 title="Player Tracking and Allocation Player Filter"
 gate="PlayerTracking,PlayerAllocationFilter" >}}
-{{< beta title="Allocation State Filter" gate="StateAllocationFilter" >}}


Same as above - need to wrap this in a feature shortcode to expire it on the next version, so it doesn't get removed immediately (docs get published immediately on merge to main).

markmandel · 2023-06-29T19:42:37Z

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

@@ -36,8 +36,6 @@ spec:
        game: my-game
      matchExpressions:
        - {key: tier, operator: In, values: [cache]}
-      # [Stage:Beta]
-      # [FeatureFlag:StateAllocationFilter]


All this will need to be feature code wrapped.

ashutosji · 2023-07-03T09:50:04Z

I see a couple of failing tests - TestFindGameServerForAllocationPacked and TestFindGameServerForAllocationPacked/one_label in the unit tests.

Are they also failing for you locally? Are you able to reproduce the issues?

Yeah, It's still passing!

markmandel · 2023-07-06T22:34:28Z

Just touching base to see if you need a hand here. If not no worries, but figured I would check.

Kalaiselvi84 · 2023-08-02T02:20:34Z

Closing this PR now, and will handle this issue in a different PR - #3308

move StateAllocationFilter to stable

5750d8a

google-oss-prow bot requested review from cyriltovena and roberthbailey June 28, 2023 06:34

google-oss-prow bot added the size/L label Jun 28, 2023

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

changes will fix build error

be02e2d

markmandel reviewed Jun 29, 2023

View reviewed changes

Kalaiselvi84 closed this Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move StateAllocationFilter to stable #3238

move StateAllocationFilter to stable #3238

ashutosji commented Jun 28, 2023

google-oss-prow bot commented Jun 28, 2023

agones-bot commented Jun 28, 2023

agones-bot commented Jun 28, 2023

markmandel commented Jun 28, 2023 •

edited

Loading

markmandel left a comment

markmandel Jun 29, 2023

ashutosji Jul 3, 2023

markmandel Jun 29, 2023

markmandel Jun 29, 2023

ashutosji Jul 3, 2023

markmandel Jun 29, 2023

markmandel Jun 29, 2023

markmandel Jun 29, 2023

ashutosji commented Jul 3, 2023

markmandel commented Jul 6, 2023

Kalaiselvi84 commented Aug 2, 2023

		runtime.FeatureTestMutex.Lock()
		defer runtime.FeatureTestMutex.Unlock()

move StateAllocationFilter to stable #3238

move StateAllocationFilter to stable #3238

Conversation

ashutosji commented Jun 28, 2023

google-oss-prow bot commented Jun 28, 2023

agones-bot commented Jun 28, 2023

agones-bot commented Jun 28, 2023

markmandel commented Jun 28, 2023 • edited Loading

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

ashutosji commented Jul 3, 2023

markmandel commented Jul 6, 2023

Kalaiselvi84 commented Aug 2, 2023

markmandel commented Jun 28, 2023 •

edited

Loading