Proposal: Update the GameServerAllocation Specification to remove required/preferred

[roberthbailey]

The structure of the GameServerAllocation API was designed in #436 with a spec that includes a required game server selector and a preferred game server selector.

While reviewing the behavior of the allocation code path, I recently filed #2136 to point out our documentation inconsistencies between the API, gRPC, and reference docs. And before that @markmandel created #2029 to clarify the behavior in the website documentation.

I think this points to the fact that the current fields in the API are confusing to users. The implemented behavior (which is best described in the website reference docs) is essentially: iterate through the list of selectors in the preferred field, and if none match try the selector in the required field. Neither field is actually "required", and if both are empty then any Ready game server in the namespace can be allocated.

Searching through the two fields in this way is really no different than having a single field with a list of selectors. What would have previously been put into the required field is now just the last entry in the list, and the beginning of the list is what was previously in the preferred field. The current implementation is much more confusing - hence the need for lots of explanatory text being added as documentation.

Proposed Specification

I'd like to update the spec (and also the proto definition) to consolidate the fields into one. Since the API is stable (v1) we would need to do this in a way that was backwards compatible (and if we ever want to actually drop the old fields from the implementation, we would need to decide if that warrants changing the API to v2). It would be straightforward to extend the existing spec to have a new field to use instead of the two older fields:

// GameServerAllocationSpec is the spec for a GameServerAllocation
type GameServerAllocationSpec struct {
	// MultiClusterPolicySelector if specified, multi-cluster policies are applied.
	// Otherwise, allocation will happen locally.
	MultiClusterSetting MultiClusterSetting `json:"multiClusterSetting,omitempty"`

	// Required The required allocation. Defaults to all GameServers.
	// Deprecated: Please use Selectors instead.
	Required metav1.LabelSelector `json:"required,omitempty"`

	// Preferred ordered list of preferred allocations out of the `required` set.
	// If the first selector is not matched,
	// the selection attempts the second selector, and so on.
	// Deprecated: Please use Selectors instead.
	Preferred []metav1.LabelSelector `json:"preferred,omitempty"`

	// Ordered list of GameServer label selectors.
	// If the first selector is not matched, the selection attempts the second selector, and so on.
	// This is useful for things like smoke testing of new game servers.
	// Note: This field can only be set if neither Required or Preferred is set.
	Selectors []metav1.LabelSelector `json:"selectors,omitempty"`

	// Scheduling strategy. Defaults to "Packed".
	Scheduling apis.SchedulingStrategy `json:"scheduling"`

	// MetaPatch is optional custom metadata that is added to the game server at allocation
	// You can use this to tell the server necessary session data
	MetaPatch MetaPatch `json:"metadata,omitempty"`
}

Implementation

Internally, we would verify that only the old or new fields were set. If the old fields were set, we would simply transform them into the new field by adding the required selectors to the end of the preferred list and storing it in the selectors field. The rest of the code would be updated to only use the selectors field and not reference the legacy fields. This would allow us to drop support for the old fields by simply removing them from the API and this small translation layer.

The implementation would be simplified a bit since it would no longer need to look at the required field after searching through the list; it would simply stop after searching the list.

I'm not sure it is necessary to put this behavior behind a feature gate (although I could be convinced otherwise). Since the change is a matter of renaming fields and not changing any behavior, I would advocate for making the change immediately and having a smaller code delta than duplicating the allocation code to account for both the old and new fields (which would negate the prior paragraph about simplifying the code, at least until the feature was stable).

Backward Compatibility

Backward compatibility would be maintained by keeping the existing fields in place and continuing to support them with no change in behavior. We would, however, update the documentation to mark them as deprecated, and after a few releases remove the website documentation for the old fields entirely.

[roberthbailey]

@ilkercelikyilmaz @markmandel @pooneh-m FYI

[markmandel]

I think this is a great simplification -- and I don't think it will be particularly onerous to maintain backward compatibility here either.

One thing I'll bring up, just so everyone is aware, but I don't think it's blocking in any way, it may just impact order or implementation, for work on #1239 , rather than embedding LabelSelectors directly, I refactored things to use a GameServerSelector instead, that embeds a LabelSelector to maintain backward compatability.

// GameServerSelector contains all the filter options for selecting
// a GameServer for allocation.
type GameServerSelector struct {
	metav1.LabelSelector
	// [Stage:Alpha]
	// [FeatureFlag:StateAllocationFilter]
	// +optional
	GameServerState *agonesv1.GameServerState `json:"gameServerState,omitempty"`
	// [Stage:Alpha]
	// [FeatureFlag:PlayerAllocationFilter]
	// +optional
	Players *PlayerSelector `json:"players,omitempty"`
}

Ultimately this could be done in either order, but just wanted to give a heads up.

One other question: Should this be behind a feature flag and go though alpha/beta/stable? (I would probably advocate so, but figured I would ask the question.)

[roberthbailey]

One other question: Should this be behind a feature flag and go though alpha/beta/stable? (I would probably advocate so, but figured I would ask the question.)

I mentioned feature flags in the write-up, and was hoping to avoid the overhead since the change is mostly re-naming and not behavioral :) But I'm happy to do so if it makes more sense to promote the fields more gradually.

[markmandel]

Sorry I totally missed that section 🤦🏻

I'm not sure it is necessary to put this behavior behind a feature gate (although I could be convinced otherwise). Since the change is a matter of renaming fields and not changing any behavior, I would advocate for making the change immediately and having a smaller code delta than duplicating the allocation code to account for both the old and new fields (which would negate the prior paragraph about simplifying the code, at least until the feature was stable).

Yeah, I could go back and forth on that one. 50/50 either way. How do other people feel?

[kaboing]

Not a long time user here, but I think the proposal looks good, without the need for a feature gate.

[roberthbailey]

We discussed this during the most recent community meeting and there wasn't any dissent. I'm going to let it sit in a "request for comments" state for a bit longer to see if anyone has any objections and/or any better ideas.