GSA: Switch LabelSelector to GameServerSelector #2166

markmandel · 2021-07-01T19:23:25Z

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:

This embeds the LabelSelector into a GameServerSelector struct, along with the new player and state selector features we need for advanced allocation, and utilising it in the preferred and required sections of a GameServerAllocation.

This means the CRD doesn't change, but the Go api surface does.

This also moves the logic of validation and selection matching into the GameServerSelector so it can be more easily reused.

GameServerAllocations are used pretty much everywhere, so a good chunk of the PR is making that api change.

Which issue(s) this PR fixes:

Work on #1239

Special notes for your reviewer:

Sorry for the chunky PR.

agones-bot · 2021-07-01T19:37:14Z

Build Failed 😱

Build Id: 743c8ce7-cb7d-4f61-9d8f-0e721e04a6d8

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

agones-bot · 2021-07-01T20:24:36Z

Build Succeeded 👏

Build Id: a833e522-8f50-41d9-bc9e-493784c31cd1

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.16.0-2e82bdf
image: gcr.io/agones-images/agones-ping:1.16.0-2e82bdf
Linux C++ SDK (build): agonessdk-1.16.0-2e82bdf-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.16.0-2e82bdf.zip

A preview of the website (the last 30 builds are retained):

https://2e82bdf-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2166/head:pr_2166 && git checkout pr_2166
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.16.0-2e82bdf

roberthbailey · 2021-07-01T22:32:58Z

This is a large PR... I'll try to get to it before the weekend.

pkg/apis/allocation/v1/gameserverallocation.go

markmandel · 2021-07-01T22:45:23Z

pkg/allocation/converters/converter.go

@@ -64,7 +64,7 @@ func ConvertAllocationRequestToGSA(in *pb.AllocationRequest) *allocationv1.GameS
 	}

 	if ls := convertLabelSelectorToInternalLabelSelector(in.GetRequiredGameServerSelector()); ls != nil {
-		gsa.Spec.Required = *ls
+		gsa.Spec.Required = allocationv1.GameServerSelector{LabelSelector: *ls}


This code will get updates once again once the proto changes, but this allows current functionality to continue working as expected.

roberthbailey · 2021-07-08T22:02:21Z

pkg/apis/allocation/v1/gameserverallocation.go

@@ -144,5 +265,10 @@ func (gsa *GameServerAllocation) Validate() ([]metav1.StatusCause, bool) {
 			Message: fmt.Sprintf("Invalid value: %s, value must be either Packed or Distributed", gsa.Spec.Scheduling)})
 	}

+	causes, _ = gsa.Spec.Required.Validate("spec.required", causes)


This will overwrite causes if it's non empty. Both here and below you need to append to causes (like is done on line 263).

(update: re-reading the validate function, I can see why this works, but it's really confusing to read. I think it's simpler to have validate not take causes and have this function do the appending).

I agree! I will make this change! Same as comment below 👍🏻

pkg/apis/allocation/v1/gameserverallocation.go

roberthbailey · 2021-07-08T22:08:38Z

pkg/allocation/converters/converter_test.go

+								MatchLabels: map[string]string{
+									"g": "h",
+								},
+							}},


What about adding conversion tests for the new fields?

Coming soon to a theatre near you! But once the allocation proto changes are in, because there's nothing to convert things to otherwise.

This embeds the LabelSelector into a GameServerSelector struct, along with the new player and state selector features we need for advanced allocation, and utilising it in the `preferred` and `required` sections of a `GameServerAllocation`. This means the CRD doesn't change, but the Go api surface does. This also moves the logic of validation and selection matching into the GameServerSelector so it can be more easily reused. GameServerAllocations are used pretty much everywhere, so a good chunk of the PR is making that api change. Work on googleforgames#1239

markmandel · 2021-07-09T23:29:52Z

Updated. I'm 50/50 on whether this should be included in this RC or not 🤔

Do we give some end users some pain now in preparation, or wait until the feature is ready to go?

agones-bot · 2021-07-09T23:53:11Z

Build Succeeded 👏

Build Id: 6b522700-32b2-46b9-9452-43189d92aa35

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.16.0-4bce072
image: gcr.io/agones-images/agones-ping:1.16.0-4bce072
Linux C++ SDK (build): agonessdk-1.16.0-4bce072-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.16.0-4bce072.zip

A preview of the website (the last 30 builds are retained):

https://4bce072-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2166/head:pr_2166 && git checkout pr_2166
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.16.0-4bce072

agones-bot · 2021-07-20T22:08:56Z

Build Succeeded 👏

Build Id: 0f87c2dc-0f98-4f16-b89b-ceab150797c4

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.17.0-e310229
image: gcr.io/agones-images/agones-ping:1.17.0-e310229
Linux C++ SDK (build): agonessdk-1.17.0-e310229-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.17.0-e310229.zip

A preview of the website (the last 30 builds are retained):

https://e310229-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2166/head:pr_2166 && git checkout pr_2166
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.17.0-e310229

google-oss-robot · 2021-07-21T00:19:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markmandel, roberthbailey

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,roberthbailey]

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

markmandel requested a review from roberthbailey July 1, 2021 19:23

google-cla bot added the cla: yes label Jul 1, 2021

markmandel added kind/feature New features for Agones and removed cla: yes labels Jul 1, 2021

google-oss-robot added the size/XL label Jul 1, 2021

google-cla bot added the cla: yes label Jul 1, 2021

google-oss-robot requested review from aLekSer and pooneh-m July 1, 2021 19:23

google-oss-robot added the approved label Jul 1, 2021

markmandel force-pushed the feature/gameserverselector branch from 31a1736 to 2e82bdf Compare July 1, 2021 19:49

google-oss-robot added size/XXL and removed size/XL labels Jul 1, 2021

markmandel commented Jul 1, 2021

View reviewed changes

pkg/apis/allocation/v1/gameserverallocation.go Show resolved Hide resolved

markmandel commented Jul 1, 2021

View reviewed changes

roberthbailey reviewed Jul 8, 2021

View reviewed changes

markmandel added 2 commits July 9, 2021 15:22

Review updates.

4bce072

markmandel force-pushed the feature/gameserverselector branch from 2e82bdf to 4bce072 Compare July 9, 2021 23:28

roberthbailey added the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Jul 13, 2021

roberthbailey removed the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Jul 20, 2021

Merge branch 'main' into feature/gameserverselector

e310229

roberthbailey approved these changes Jul 21, 2021

View reviewed changes

google-oss-robot assigned roberthbailey Jul 21, 2021

google-oss-robot added the lgtm label Jul 21, 2021

roberthbailey merged commit 429de93 into googleforgames:main Jul 21, 2021

markmandel deleted the feature/gameserverselector branch July 21, 2021 03:06

roberthbailey added this to the 1.17.0 milestone Jul 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GSA: Switch LabelSelector to GameServerSelector #2166

GSA: Switch LabelSelector to GameServerSelector #2166

markmandel commented Jul 1, 2021

agones-bot commented Jul 1, 2021

agones-bot commented Jul 1, 2021

roberthbailey commented Jul 1, 2021

markmandel Jul 1, 2021

roberthbailey Jul 8, 2021

markmandel Jul 9, 2021

roberthbailey Jul 8, 2021

markmandel Jul 9, 2021

markmandel commented Jul 9, 2021

agones-bot commented Jul 9, 2021

agones-bot commented Jul 20, 2021

google-oss-robot commented Jul 21, 2021

GSA: Switch LabelSelector to GameServerSelector #2166

GSA: Switch LabelSelector to GameServerSelector #2166

Conversation

markmandel commented Jul 1, 2021

agones-bot commented Jul 1, 2021

agones-bot commented Jul 1, 2021

roberthbailey commented Jul 1, 2021

markmandel Jul 1, 2021

Choose a reason for hiding this comment

roberthbailey Jul 8, 2021

Choose a reason for hiding this comment

markmandel Jul 9, 2021

Choose a reason for hiding this comment

roberthbailey Jul 8, 2021

Choose a reason for hiding this comment

markmandel Jul 9, 2021

Choose a reason for hiding this comment

markmandel commented Jul 9, 2021

agones-bot commented Jul 9, 2021

agones-bot commented Jul 20, 2021

google-oss-robot commented Jul 21, 2021