feat: support multiple port ranges #3747

nrwiersma · 2024-04-08T12:44:58Z

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
/kind release

What this PR does / Why we need it:

This adds support for multiple named port ranges that can be configured in the GameServer port using the range property. This is behind a feature flag PortRanges.

Which issue(s) this PR fixes:

Closes #1911

Special notes for your reviewer:

I really dislike how the configuration has ended up, but I did not find a better way to do it. If you have a better idea, I would gladly take it.

github-actions · 2024-04-08T12:45:14Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

agones-bot · 2024-04-08T13:42:40Z

Build Failed 😱

Build Id: d2182235-1425-4ba6-8760-c65ed7c7354a

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

nrwiersma · 2024-04-08T13:57:20Z

I dont really understand what caused the e2e tests to fail. If you could give me a hit where to look, that would be great.

markmandel · 2024-04-08T18:22:29Z

    gameserverallocation_test.go:1442: 
        	Error Trace:	/go/src/agones.dev/agones/test/e2e/gameserverallocation_test.go:1442
        	Error:      	Not equal: 
        	            	expected: 100
        	            	actual  : 98
        	Test:       	TestGameServerAllocationDuringMultipleAllocationClients

uuuh this test is killing me.

markmandel · 2024-04-08T20:48:04Z

Sorry - this a flake #3553 - it's been driving me up the wall.

markmandel

Looks good - I can't think of a better way! Added some questions and tweaks.

Some extra docs we will need:

I think that's about it. I may have some extra tweaks to the docs as I reread, but not seeing any issues. Nice work 👍🏻

site/content/en/docs/Installation/Install Agones/helm.md

install/helm/agones/templates/crds/_gameserverspecschema.yaml

markmandel · 2024-04-08T21:09:48Z

pkg/portallocator/portallocator.go

 // A set of port allocations for a node
 type portAllocation map[int32]bool

 //nolint:govet // ignore fieldalignment, singleton
-type portAllocator struct {
+type portRangeAllocator struct {


I like this refactor 👍🏻

pkg/cloudproduct/cloudproduct.go

cmd/controller/main.go

github-actions · 2024-04-09T11:41:37Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2024-04-09T11:43:19Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

agones-bot · 2024-04-09T12:49:27Z

Build Succeeded 👏

Build Id: e1643044-d05d-4165-8eb9-b7ca62640cd1

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

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

https://8cd9b47-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3747/head:pr_3747 && git checkout pr_3747
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.40.0-dev-8cd9b47-amd64

pkg/util/runtime/features.go

install/helm/agones/templates/controller.yaml

zmerlynn · 2024-04-09T17:29:59Z

Oh, one last request (and thank you for your patience): Please add e2e tests. It's a little tricky because for normal e2e tests we use the same install, but here you need to install it a different way and then test. You can add "custom" tests in this area:

agones/build/Makefile

Lines 304 to 308 in c1320ee

    
           test-e2e: 
        
           	$(MAKE) DOCKER_RUN_ARGS="$(DOCKER_RUN_ARGS)" test-e2e-integration 
        
           	$(MAKE) DOCKER_RUN_ARGS="$(DOCKER_RUN_ARGS)" test-e2e-failure 
        
           	$(MAKE) DOCKER_RUN_ARGS="$(DOCKER_RUN_ARGS)" test-e2e-ha-extensions 
        
           	$(MAKE) DOCKER_RUN_ARGS="$(DOCKER_RUN_ARGS)" test-e2e-allocator-crash

github-actions · 2024-04-10T11:56:08Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

agones-bot · 2024-04-10T12:11:15Z

Build Failed 😱

Build Id: 92265a73-517a-41eb-bbae-71e87e9295d1

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

nrwiersma · 2024-04-10T18:20:04Z

A hair brained idea. Given the config for this feature is really not great, and e2e tests seem to be tricky at best, what if the config for this feature came in the form of a CRD. The port ranges could be made immutable, and still checked for overlap via validation webhooks. It does, without a massive amount of thought, seems like it would solve a couple issues here. It does open a few new and exciting routes to failure, but I think some of those are somewhat unavoidable in any case.

markmandel · 2024-04-16T22:57:42Z

A hair brained idea. Given the config for this feature is really not great, and e2e tests seem to be tricky at best, what if the config for this feature came in the form of a CRD.

Given that the rest of the config for Agones is provided via Helm configuration parameters it seems weird to seperate this out.

That being said, seems like a good time to start a https://helm.sh/docs/topics/charts/#schema-files ?

markmandel · 2024-04-16T22:59:26Z

Oh, one last request (and thank you for your patience): Please add e2e tests. It's a little tricky because for normal e2e tests we use the same install, but here you need to install it a different way and then test. You can add "custom" tests in this area:

@zmerlynn could we add some port ranges to
https://github.com/googleforgames/agones/blob/05d8232c937d756d9b8e848915e04e4159924b7b/build/e2e-image/e2e.sh#L25C18-L25C25

And call it a day? Does it need to be a separate install?

markmandel

I've lost track through this, but a capturing some more thoughts @zmerlynn where chatting about and wanted to get your take.

the current minPort and maxPort range should be called default - that way if people are naming ranges, they can use default (which would also be the default, so it's backward compatible).
For validation for GameServerSpec on Autopilot, we could then say "if you pick a port range other than default we will return an invalid configuration" (with appropriate documentation).

This means we don't have to validate that they are on autopilot at the helm install layer, and the end user gets a nice error message if they try and do something they shouldn't / can't on Autopilot that isn't as scary as a controller crash or similar.

WDYT?

install/helm/agones/values.yaml

github-actions · 2024-04-18T09:54:26Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

agones-bot · 2024-04-18T10:09:12Z

Build Failed 😱

Build Id: 03b0d831-7828-4ac0-ae74-235b97f29c91

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

github-actions · 2024-04-18T11:12:26Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

agones-bot · 2024-04-18T12:22:23Z

Build Failed 😱

Build Id: f582e71c-d62e-4d07-b5ad-f168747c17f9

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

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

markmandel · 2024-04-19T00:46:41Z

Just to make life easy - here's the validation block for GameServerSpec:

agones/pkg/apis/agones/v1/gameserver.go

Line 511 in 4bb6731

    
           func (gss *GameServerSpec) Validate(apiHooks APIHooks, devAddress string, fldPath *field.Path) field.ErrorList {

Which feeds into:

agones/pkg/cloudproduct/gke/gke.go

Line 134 in 4bb6731

    
           func (g *gkeAutopilot) ValidateGameServerSpec(gss *agonesv1.GameServerSpec, fldPath *field.Path) field.ErrorList {

For the Autopilot specific validation.

(I don't think you implemented this yet)

agones-bot · 2024-04-19T02:33:04Z

Build Succeeded 👏

Build Id: 5fa30dc4-feee-4186-bd4c-2cd3fefd55e4

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

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

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3747/head:pr_3747 && git checkout pr_3747
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.40.0-dev-e00771a-amd64

nrwiersma · 2024-04-19T04:23:02Z

For the Autopilot specific validation.

I did this: https://github.com/googleforgames/agones/pull/3747/files#diff-6df711a53a9866de14c530a4b1f9316d142ead0bc9a8e9d668c50a2ff28673b2R142

Is that not correct, or did I missing something extra I need to do?

github-actions · 2024-04-19T05:58:52Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

agones-bot · 2024-04-19T07:08:15Z

Build Succeeded 👏

Build Id: 9a979f38-4314-4f4a-8f00-c2c246921013

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

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

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3747/head:pr_3747 && git checkout pr_3747
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.40.0-dev-f03c1e2-amd64

github-actions · 2024-05-02T06:22:35Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

agones-bot · 2024-05-02T07:46:14Z

Build Succeeded 👏

Build Id: 2eaa7a79-d971-48d3-9bb1-9c91aee44ac8

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

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

https://894e516-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3747/head:pr_3747 && git checkout pr_3747
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.41.0-dev-894e516-amd64

github-actions · 2024-05-03T11:06:30Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

agones-bot · 2024-05-03T11:18:39Z

Build Failed 😱

Build Id: ff547f82-31c0-4a55-860f-3e065a80898c

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

zmerlynn

LGTM, thank you!

github-actions · 2024-05-03T12:47:36Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2024-05-03T13:09:16Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

nrwiersma · 2024-05-03T13:10:34Z

@zmerlynn It turns out there was a test flake due to assuming the order of the input map. I removed the offending assertions, as the assertions above already prove they came from the correct port ranges.

zmerlynn · 2024-05-03T13:14:34Z

@nrwiersma Thanks for fixing it, LGTM!

agones-bot · 2024-05-03T14:52:57Z

Build Succeeded 👏

Build Id: 026db8dc-cdba-4444-9d9d-5fbd2aed2362

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

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

https://10f071b-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3747/head:pr_3747 && git checkout pr_3747
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.41.0-dev-10f071b-amd64

github-actions bot added kind/feature New features for Agones size/XL labels Apr 8, 2024

markmandel reviewed Apr 8, 2024

View reviewed changes

zmerlynn requested changes Apr 9, 2024

View reviewed changes

pkg/util/runtime/features.go Show resolved Hide resolved

install/helm/agones/templates/controller.yaml Outdated Show resolved Hide resolved

markmandel reviewed Apr 17, 2024

View reviewed changes

install/helm/agones/values.yaml Outdated Show resolved Hide resolved

nrwiersma force-pushed the port-ranges branch from f77e244 to e00771a Compare April 18, 2024 11:12

markmandel reviewed Apr 19, 2024

View reviewed changes

site/content/en/docs/Reference/gameserver.md Outdated Show resolved Hide resolved

nrwiersma force-pushed the port-ranges branch from a847561 to 894e516 Compare May 2, 2024 06:22

nrwiersma added 12 commits May 3, 2024 13:01

feat: add port range allocation

b1b9c5e

feat: update install

8aa5f46

feat: update docs

f4b1447

fix: cr suggestions

d719356

fix: cr suggestions

4b3e88f

fix: cr suggestions

9dc69b2

fix: cr suggestions

08327d2

fix: range reference default

9d3db9f

fix: cr suggestions

a31b251

fix: flaky test due to map

0785662

fix: handle release changes

393dc3d

fix: feature flags in docs

1367d86

nrwiersma force-pushed the port-ranges branch from 894e516 to 1367d86 Compare May 3, 2024 11:06

zmerlynn approved these changes May 3, 2024

View reviewed changes

zmerlynn enabled auto-merge (squash) May 3, 2024 12:16

Merge branch 'main' into port-ranges

338397d

fix: test flake

10f071b

auto-merge was automatically disabled May 3, 2024 13:08
Head branch was pushed to by a user without write access

zmerlynn merged commit 26b17c3 into googleforgames:main May 3, 2024
4 checks passed

feat: support multiple port ranges #3747

feat: support multiple port ranges #3747

Conversation

nrwiersma commented Apr 8, 2024

github-actions bot commented Apr 8, 2024

agones-bot commented Apr 8, 2024

nrwiersma commented Apr 8, 2024

markmandel commented Apr 8, 2024

markmandel commented Apr 8, 2024

markmandel left a comment

Choose a reason for hiding this comment

markmandel Apr 8, 2024

Choose a reason for hiding this comment

github-actions bot commented Apr 9, 2024

github-actions bot commented Apr 9, 2024

agones-bot commented Apr 9, 2024

zmerlynn commented Apr 9, 2024

github-actions bot commented Apr 10, 2024

agones-bot commented Apr 10, 2024

nrwiersma commented Apr 10, 2024

markmandel commented Apr 16, 2024

markmandel commented Apr 16, 2024

markmandel left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 18, 2024

agones-bot commented Apr 18, 2024

github-actions bot commented Apr 18, 2024

agones-bot commented Apr 18, 2024

markmandel commented Apr 19, 2024

agones-bot commented Apr 19, 2024

nrwiersma commented Apr 19, 2024

github-actions bot commented Apr 19, 2024

agones-bot commented Apr 19, 2024

github-actions bot commented May 2, 2024

agones-bot commented May 2, 2024

github-actions bot commented May 3, 2024

agones-bot commented May 3, 2024

zmerlynn left a comment

Choose a reason for hiding this comment

github-actions bot commented May 3, 2024

github-actions bot commented May 3, 2024

nrwiersma commented May 3, 2024

zmerlynn commented May 3, 2024

agones-bot commented May 3, 2024