[Feature Proposal] - GameServer status hook for counter changes

[hongyiweiwu01]

Is your feature request related to a problem? Please describe.
The new counter feature allows us to pack multiple game sessions to one game server. However, it's still not possible to attach the game server's status to the counter atomically. For instance, a game server with max capacity of 8 may want to be marked as "allocated" when there's 1-8 sessions on it and "ready" when there's 0 session such that the game server can be recycled when there's a new deployment, but currently the second part is not supported by the Agones framework and hard to do without race conditions.

Describe the solution you'd like
Some sort of hook that ties the counter value to the status of the game server.

Describe alternatives you've considered
Manually adjusted the server status right before/after modifying the counter value using the SDK, but it introduces race conditions.

Additional context
None.

Link to the Agones Feature Proposal (if any)
N/A.

Discussion Link (if any)
N/A.

[markmandel]

So you've presented a solution - but I don't think I understand the problem.

However, it's still not possible to attach the game server's status to the counter atomically

I don't understand why you would want to?

For instance, a game server with max capacity of 8 may want to be marked as "allocated" when there's 1-8 sessions on it and "ready" when there's 0 session such that the game server can be recycled when there's a new deployment,

Why would you want to do that?

Assuming you have a reason, why not use the fleet overflow functionality?

[hongyiweiwu01]

Why would you want to do that?

Maybe I'm misunderstanding the mechanism here, but let's say I have a fleet with this high-density packing enabled thru the counter, and some fleet members have been allocated, but their sessions finished and the counter is released. At this point they stay as "Allocated" even though they don't have any active sessions on it, so it should be disposable. But because it stays Allocated, if I make an update to the fleet image, it won't be killed until something manually converts it to Ready. I'd like for that "something" to be Agones automatically if possible.

Assuming you have a reason, why not use the fleet overflow functionality?
But then some custom controller is still required to flip the Allocated back to Ready so Agones deletes them during an update? Please correct me if I'm wrong here.

[markmandel]

Maybe I'm misunderstanding the mechanism here, but let's say I have a fleet with this high-density packing enabled thru the counter, and some fleet members have been allocated, but their sessions finished and the counter is released

Why not use the SDK to move it back to Ready?

[hongyiweiwu01]

@markmandel It doesn't seem like the SDK has a way to change the counter AND flipping the state in one request? In that case it's possible for me to flip the state to Ready while the Agones control plane allocates the same server no? Wouldn't that be a race condition?

[markmandel]

It doesn't seem like the SDK has a way to change the counter AND flipping the state in one request? In that case it's possible for me to flip the state to Ready while the Agones control plane allocates the same server no? Wouldn't that be a race condition?

Ah! Now we get to the meat of the problem! This is why it pays to be problem first not solution first -- because now we can talk about solutions to the problem that can resolve this issue - and discuss tradeoffs 👍🏻 😄

So here are several immediate thoughts:

1. Yes, if you try and switch to Ready, there's a potential race condition there -- you are 100% right. There's a chance an allocation could go through, and then the method on the SDK will make it Ready after the fact! Not ideal.
2. Same thing could technically also happen on SDK.Shutdown() as well

A few interesting solutions here:

1. If all you are worried about are Fleet updates - use allocationOverflow on the Fleet to take the Allocated GameServers that should be notified "hey, shutdown soon" to swap the label such that it's no longer in the allocation selection criteria - https://agones.dev/site/docs/guides/fleet-updates/#notifying-gameservers-on-fleet-updatedownscale - that takes you out of the race condition entirely.
2. Use a terminationGracePeriodSeconds such that if you do hit the race condition (assuming a session can end within an ~hour -- depending on Kubernetes provider and config) -- then that's okay, and you have time to finish your session and since the GameServer is being deleted - not being possibly allocated anymore.
3. As you say - make a controller to do the move to Ready, since you can thereby atomically guarantee that the counter is zero, and switch it to Ready appropriately

But this definitely leads me to an interesting path - what if that's not good enough -- how can we give you the opportunity to make a change to the GameServer if their current value(s) are definitely something specific. We don't want to just cover a single scenario (like you have in the solution above) -- we want to cover a wide variety of potential solutions and be as flexible as possible.

The immediate thought that comes to mind is that Kubernetes really already solves this for us, we just need to take advantage of it. The SDK server already does patch operations with tests (see: https://jsonpatch.com/ and/or https://datatracker.ietf.org/doc/html/rfc6902) for making SDK method changes.

.... my thought is - what if we let people put in their own test patches? i.e. "change to shutdown, but test that the gameserver is in a ready state" or "change to ready, but only if this counter is 0 and the list has no values" -- we could do that! And ignore what happens if it fails because the test fails.

Then we could handle all kinds of interesting scenarios!

I'm thinking something like (written off the top of my head, not valid code):

struct Test {
  path string
  value IntOrString
}
SDK.Alpha().ReadyWithTests([]Tests {
  {path="/status/counters/sessions/count", value=IntOrString.FronInt(0)}
}) 

Would solve your race condition issue - you could watch in the GameServer SDK for if the sessions drop to 0 - if they do, you run the above code - if an allocation happens in the meantime - counter gets bumped, the value is no longer 0, and the change never goes through.

I would start with this functionality for both Ready and Shutdown to begin, and can expand if we need to.

What do you think of that solution? Solves the problem, and I think it's super flexible.

If we like it, I'm pretty strongly tempted to implement this once the SidecarContainers feature gate goes to stable - as the sidecar server is a bit of a mess of a variety of edge cases (since we didn't have sidecar containers!) and going to stable will simplify things considerably. We might be able to fast track it, since it's been in beta for four releases now, and haven't heard a peep from anyone on any kind of issue with it.

Let me know your thoughts!