Feat(gameservers): Shared pod IPs with GameServer Addresses

[lacroixthomas]

What type of PR is this?
/kind feature

What this PR does / Why we need it:
Append the Pod IPs to the GameServer's addresses

Which issue(s) this PR fixes:

Closes #3763

Special notes for your reviewer:

[agones-bot]

Build Failed 😱

Build Id: 621fc248-ad56-49a9-8692-f8b3b37aa4b7

• Cloud Build view
• Cloud Build log download

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

[agones-bot]

Build Succeeded 👏

Build Id: 5b0f4219-b594-4034-b104-fe5e8cb63c0c

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

• image: us-docker.pkg.dev/agones-images/ci/agones-controller:1.40.0-dev-13aaa03-amd64
• image: us-docker.pkg.dev/agones-images/ci/agones-extensions:1.40.0-dev-13aaa03-amd64
• image: us-docker.pkg.dev/agones-images/ci/agones-sdk:1.40.0-dev-13aaa03-linux-amd64
• image: us-docker.pkg.dev/agones-images/ci/agones-ping:1.40.0-dev-13aaa03-amd64
• image: us-docker.pkg.dev/agones-images/ci/agones-allocator:1.40.0-dev-13aaa03-amd64
• Linux C++ SDK (build): agonessdk-1.40.0-dev-13aaa03-amd64-linux-arch_64.tar.gz
• SDK Server: agonessdk-server-1.40.0-dev-13aaa03-amd64.zip

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

• https://13aaa03-dot-preview-dot-agones-images.appspot.com/

To install this version:

• git fetch https://github.com/googleforgames/agones.git pull/3764/head:pr_3764 && git checkout pr_3764
• 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-13aaa03-amd64

[markmandel]

Only thing I'd add is a test here:

agones/test/e2e/gameserver_test.go

Line 62 in b946cb3

assert.NotEmpty(t, readyGs.Status.Addresses)

Which checks if there's PodIp record(s), and that it's not empty (can also lookup the pod if we want and do a comparison?)

[lacroixthomas]

Only thing I'd add is a test here:

agones/test/e2e/gameserver_test.go

Line 62 in b946cb3

assert.NotEmpty(t, readyGs.Status.Addresses)

Which checks if there's PodIp record(s), and that it's not empty (can also lookup the pod if we want and do a comparison?)

Hello @markmandel
I've added some check for the PodIPs on the addresses,
I'm not that familiar with the e2e of agones yet (about the framework / helpers etc), I'm gonna check this part to be able to do the lookup of the pod for the comparaison

[agones-bot]

Build Failed 😱

Build Id: 6b3ce305-def9-4b74-8eec-1c31eb153201

• Cloud Build view
• Cloud Build log download

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

[agones-bot]

Build Failed 😱

Build Id: c2142862-2753-4c9d-a6cd-bbb5f716e4d5

• Cloud Build view
• Cloud Build log download

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

[agones-bot]

Build Failed 😱

Build Id: c9d2184e-d5d7-4dfd-b953-27c7da634878

• Cloud Build view
• Cloud Build log download

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

[daniellee]

Before I saw this PR, I had started working on it as well :-) Thanks for jumping on this so quickly - looking forward to seeing it merged soon.

Testing this PR locally, it doesn't seem to populate the gameserver.Status.Addresses field.

I hacked this guard into the syncGameServerStartingState function in pkg/gameservers/controller.go to force it to wait until the pod.Status.PodIPs field has been populated:

if pod.Status.PodIPs == nil || len(pod.Status.PodIPs) == 0 {
	return gs, workerqueue.NewDebugError(errors.Errorf("pod IP not yet populated for Pod %s", pod.ObjectMeta.Name))
}

That's probably not the ideal way to solve it but it identifies the problem. The gameserver's scheduled and ready states are set before the pod.Status.PodIPs field is populated and therefore the gameserver.Status.Addresses field never gets the PodIPs.

[lacroixthomas]

Before I saw this PR, I had started working on it as well :-) Thanks for jumping on this so quickly - looking forward to seeing it merged soon.

Testing this PR locally, it doesn't seem to populate the gameserver.Status.Addresses field.

I hacked this guard into the syncGameServerStartingState function in pkg/gameservers/controller.goin to force it to wait until the pod.Status.PodIPs field has been populated:

if pod.Status.PodIPs == nil || len(pod.Status.PodIPs) == 0 {
	return gs, workerqueue.NewDebugError(errors.Errorf("pod IP not yet populated for Pod %s", pod.ObjectMeta.Name))
}

That's probably not the ideal way to solve it but it identifies the problem. The gameserver's scheduled and ready states are set before the pod.Status.PodIPs field is populated and therefore the gameserver.Status.Addresses field never gets the PodIPs.

Hello, I didn't have time yesterday to investigate what happens, but nice catch, thanks for looking into it ! I'll update my MR with your fix :)

[lacroixthomas]

I will add unit tests for the controller fix *

[agones-bot]

Build Failed 😱

Build Id: e7c5118a-6eee-4a66-8c33-33f9fe5c29b0

• Cloud Build view
• Cloud Build log download

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

[agones-bot]

Build Succeeded 👏

Build Id: e3836b05-5e77-41c3-b3da-9ac3efa71b79

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

• image: us-docker.pkg.dev/agones-images/ci/agones-controller:1.40.0-dev-52b70ae-amd64
• image: us-docker.pkg.dev/agones-images/ci/agones-extensions:1.40.0-dev-52b70ae-amd64
• image: us-docker.pkg.dev/agones-images/ci/agones-sdk:1.40.0-dev-52b70ae-linux-amd64
• image: us-docker.pkg.dev/agones-images/ci/agones-ping:1.40.0-dev-52b70ae-amd64
• image: us-docker.pkg.dev/agones-images/ci/agones-allocator:1.40.0-dev-52b70ae-amd64
• Linux C++ SDK (build): agonessdk-1.40.0-dev-52b70ae-amd64-linux-arch_64.tar.gz
• SDK Server: agonessdk-server-1.40.0-dev-52b70ae-amd64.zip

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

• https://52b70ae-dot-preview-dot-agones-images.appspot.com/

To install this version:

• git fetch https://github.com/googleforgames/agones.git pull/3764/head:pr_3764 && git checkout pr_3764
• 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-52b70ae-amd64

[zmerlynn]

Thank you so much for the submission! This LGTM with a minor nit.

[lacroixthomas]

Thank you so much for the submission! This LGTM with a minor nit.

Thanks for the review ! I've updated the MR following your comments

[agones-bot]

Build Succeeded 👏

Build Id: b21bb160-cd2d-4370-ba84-9359d4a23da4

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

• image: us-docker.pkg.dev/agones-images/ci/agones-controller:1.40.0-dev-c674c20-amd64
• image: us-docker.pkg.dev/agones-images/ci/agones-extensions:1.40.0-dev-c674c20-amd64
• image: us-docker.pkg.dev/agones-images/ci/agones-sdk:1.40.0-dev-c674c20-linux-amd64
• image: us-docker.pkg.dev/agones-images/ci/agones-ping:1.40.0-dev-c674c20-amd64
• image: us-docker.pkg.dev/agones-images/ci/agones-allocator:1.40.0-dev-c674c20-amd64
• Linux C++ SDK (build): agonessdk-1.40.0-dev-c674c20-amd64-linux-arch_64.tar.gz
• SDK Server: agonessdk-server-1.40.0-dev-c674c20-amd64.zip

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

• https://c674c20-dot-preview-dot-agones-images.appspot.com/

To install this version:

• git fetch https://github.com/googleforgames/agones.git pull/3764/head:pr_3764 && git checkout pr_3764
• 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-c674c20-amd64

[zmerlynn]

Since you didn't modify the address function in pkg/gameservers/gameservers.go, I don't think this line is true. However, to avoid a roundtrip across timezones and over a weekend, I may just go ahead and edit this, since the rest LGTM.

[zmerlynn]

(fixed in a671f1b)

[zmerlynn]

LGTM, cc @markmandel

[markmandel]

Looks great!

[agones-bot]

Build Succeeded 👏

Build Id: 53bba61f-c80f-404b-9a02-b4a4ba1c7067

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

• image: us-docker.pkg.dev/agones-images/ci/agones-controller:1.40.0-dev-b2cd697-amd64
• image: us-docker.pkg.dev/agones-images/ci/agones-extensions:1.40.0-dev-b2cd697-amd64
• image: us-docker.pkg.dev/agones-images/ci/agones-sdk:1.40.0-dev-b2cd697-linux-amd64
• image: us-docker.pkg.dev/agones-images/ci/agones-ping:1.40.0-dev-b2cd697-amd64
• image: us-docker.pkg.dev/agones-images/ci/agones-allocator:1.40.0-dev-b2cd697-amd64
• Linux C++ SDK (build): agonessdk-1.40.0-dev-b2cd697-amd64-linux-arch_64.tar.gz
• SDK Server: agonessdk-server-1.40.0-dev-b2cd697-amd64.zip

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

• https://b2cd697-dot-preview-dot-agones-images.appspot.com/

To install this version:

• git fetch https://github.com/googleforgames/agones.git pull/3764/head:pr_3764 && git checkout pr_3764
• 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-b2cd697-amd64