Agones: WatchGameServer should immediately provide the cached GameServer

[TBBle]

Is your feature request related to a problem? Please describe.

I'm always frustrated when I call the WatchGameServer API on the sidecar, and I have to wait up to 30 seconds to receive my current GameServer, if no changes happen.

This is particularly frustrating if the transition to Allocated happened before I called WatchGameServer, which means I'm waiting 30 seconds of _player visible_ time for my GameServer annotations, so I can accept the allocation and start playing.

Describe the solution you'd like

When connecting to the WatchGameServer, the current GameServer resource, if already cached in the sdk server, is immediately sent to the client.

Describe alternatives you've considered

The alternative is to _also_ call GetGameServer. The problem with that is that you must start the Watch first, or you may still miss an update and still have to wait 30 seconds. If you start the Watch first, and an update comes down it while you are calling GetGameServer, then you have to detect this and discard the result of GetGameServer. And if the socket connection is lost for some reason, I have to repeat the process, in case I missed my Allocated update while recalling WatchGameServer. I don't like that approach.

Another alternative would be for every SDK to do the above inside its WatchGameServer wrapper, which would be (I think) needlessly complicated. It also wouldn't help for my use-case, as we are not yet using the Unreal SDK, as (amongst other reasons) it doesn't implement WatchGameServer.

An even worse alternative would be for the resync time to be reduced from 30 seconds to 5 seconds. I'm sure I can find even worse alternatives if I try...

Additional context

In our specific case, we realised we should call WatchGameServer before we announce Ready, and then we don't have the player-visible delay, as long as nothing else goes wrong. But in general, I'd hope WatchGameServer worked more like kubectl get --watch than kubectl get --watch-only.

Also this seems like a four-line change to func (s *SDKServer) WatchGameServer that wouldn't break the sidecar's API either. (Context in case I have time to create a PR myself at some future point, if this isn't rejected as a feature)

[aLekSer]

We have discussed this with @akremsa and he can provide a draft with storing this cached gameserver in a memory even if no Watch streams were connected. This can be tested with simple-udp gameserver and simple-udp logs. Actually now we are receiving the results in 15 seconds time on average.
By the way this could introduce a featureFlag for the sidecar, what do you think?

[markmandel]

Not sure why there needs to be extra caching. Why not send through the results of getGameServer() on connection?

The Informer already caches the latest value it has anyway.

And yes to a feature flag 👍 Since this is a change in behaviour.

[markmandel]

@TBBle random thought I had over the weekend.

Where there any concerns / issues that were had around calling SDK.GameServer() to get the latest GameServer before calling WatchGameServer()? (we're essentially going to be doing exactly this on the sdk server end, so just looking for potential footguns).

[TBBle]

My two concerns is that it's twice as many actions for the SDK to do, and it introduces the need to choose which of the two results to take, if a WatchGameServer update comes in parallel to the GameServer response. This should be an easy choice, (WatchGameServer is always going to be up-to-date, I hope), but it's still complexity.

And I'm worried that choice'll get muddied when the APIServer is overloaded, as a colleague of mine raised recently, we're seeing a problem that might be the Informer seeing old ResourceVersions on the GameServers when the API server is overloaded by GameServer updates. I really don't want to be maintaining code to compare versions of a GameServer in an SDK, when the SDKServer _knows_ what the right version is anyway.

[TBBle]

Does this really need to be feature-flagged? The behaviour is WatchGameServer sends GameServer objects when they change. This is a change from null to not-null, that's a reasonable change to make, and more-suits the current behaviour. Our code had not even contemplated the possibility that we'd need a second mechanism to get the _current_ GameServer when calling WatchGameServer.

[markmandel]

I definitely see your point about complexity, and having to do extra work - that's why I can see this being valuable :+1:.

And I'm worried that choice'll get muddied when the APIServer is overloaded

The current game server is backed by an Informer cache, so there is actually no extra effort on the K8s api server no matter what. You're always going to get whatever the latest version the Informer cache has of the GameServer that has been seen by the SDKServer -- which is eventually consistent anyway, because things take time.

we're seeing a problem that might be the Informer seeing old ResourceVersions on the GameServers when the API server is overloaded by GameServer updates.

This is exactly what Kubernetes is built to do by design, so this is perfectly normal. Even when it's not overloaded, race conditions happen. This is why Kubernetes has a controller pattern - so it can self heal. This is how Kubernetes handles race conditions, which are inevitable in a distributed systems.

I really don't want to be maintaining code to compare versions of a GameServer in an SDK, when the SDKServer knows what the right version is anyway.

I'm not quite sure what this means. why would you need this? Can you expand? Maybe there is more work we would need to do that I'm not thinking of in the implementation of this.

when the SDKServer knows what the right version is anyway

Not sure i follow this either. How does the SDKServer know? The SDKServer has the latest GameServer that it is aware of through the Informer.

There is probably an interesting question of small race conditions that might also happen.

Here's what I would probably recommend to implement the change (source), would be something like this (without the feature flag):

// WatchGameServer sends events through the stream when changes occur to the
// backing GameServer configuration / status
func (s *SDKServer) WatchGameServer(e *sdk.Empty, stream sdk.SDK_WatchGameServerServer) error {
    s.logger.Debug("Received WatchGameServer request, adding stream to connectedStreams")
        // new part
        gs, err := s.gGetGameServer(context.Background(), e)
    if err != nil {
        return nil, err
    }
        stream.send(gs)
        // end new part
    s.streamMutex.Lock()
    s.connectedStreams = append(s.connectedStreams, stream)
    s.streamMutex.Unlock()
    // don't exit until we shutdown, because that will close the stream
    <-s.stop
    return nil
}

Is that what you had in mind? Seems like the safest option that will give you the most up to date version that we can.

Does this really need to be feature-flagged?

Well it's a change in behaviour that may break things for our users if it is implemented incorrectly, or has unforseen consequences. So I'd rather not risk breaking a whole release.

This is a change from null to not-null, that's a reasonable change to make, and more-suits the current behaviour.

Sorry, also not following what you mean by "null" to "not-null" ?

@aLekSer & @akremsa - is this also what you had in mind?

[TBBle]

That patch is basically what I expected when I said "like a four-line change", yeah. I imagined that if getGameServer failed (because we haven't gotten the first version from the API Server yet?), we'd still continue the watch, because that first version should still trigger the update-all-connected-clients codepath. But I haven't looked at this code since last week, so maybe that's not a possible flow.

The SDKServer knows what the right version is _because_ of the Informer cache: What it sends as an answer to either GameServer or WatchGameServer will be the same. The problem is if the client asks those two questions, and for load or other reasons receives the answers in a different order than they were sent, it doesn't know if GameServer was the same value as WatchGameServer (because it was answered second) or was an older value because GameServer was answered, and while the answer was in-flight, WatchGameServer was triggered and sent an answer that got there first.

Starting WatchGameServer with a "here's where we are now" is the only way to avoid such race conditions.

The "null" to "not-null" transition is "What is the state of the GameServer" from the client's perspective. Effectively, what I'm saying is that this change has the same behaviour (from a client perspective) as if I was able to get my WatchGameServer connection done _before_ the SDKServer had received its initial GameServer object, so that I observed the initial "null to not-null" transition. i.e. the same transition that changes the GameServer API from blocking to non-blocking.

So if this breaks anything, (assuming it's not implemented wrong, which is a good point), the same breakage is already possible for those clients, if they connect early-enough.

---

The rest of the stuff is related to a discussion I understood you had had with a colleague of mine, where we ran a large-scale test on EKS and saw a _lot_ of API Server load generated by UPDATE on the GameServer from the Controller and the SDKServer, most of which were not 429s but 409s because the sender was sending repeated updates for several minutes with a resource version that it had already sent an update for successfully. This was increasing the load on the API Server, and causing more delays in responses, (etcd was reporting 600ms or more update latency) and put the whole thing in a death-spiral.

We did identify a misbehaviour on our end which contributed to this: high load meant that we were going Unhealthy and Ready at nearly the same time, and triggering a pathological case in the SDK Server which I think put two updates into the worker queue at once, generating a guaranteed conflict, as the SDK Server sent two updates delta'd from the same gGetGameServer state. We never worked out why the ongoing resends had the _same_ resourceVersion though, but our suspicion was that the answer from the API Server was still in-flight for a few minutes, as we were looking at the API Server audit logs, not the Agones Controller/SDK Server logs.

---

Anyway, that aside is a different discussion. We only triggered the load-related issues after we worked around this FR by fixing our code to send Ready immediately, rather than waiting for the WatchGameServer's first response, as the ~27 second delay (i.e. until the next defaultSyncInterval) becomes visible to the AllocateGameServer API if your buffer is exhausted.

[akremsa]

@markmandel yes, this is similar to what I've already implemented and I'm testing this solution right now. I'll create a PR soon.

@TBBle thanks for the detailed explanation.