Fix findOpenPorts portAllocator function #1725

aLekSer · 2020-07-29T18:51:48Z

There could be the possible case when we set PortPolicy as an empty
string on update of GameServer.

What type of PR is this?

/kind bug

What this PR does / Why we need it:

There is a way to cause infinite loop (recursion) in Allocate() function using kubectl edit gameservers, if you update the PortPolicy to an empty string "" and one more field. Then you would receive the state when Agones controller could not allocate any ports and got stuck in this state:

kubectl get gs
NAME                     STATE            ADDRESS         PORT   NODE                                     AGE
simple-udp-2skbm-657tc   PortAllocation                                                                   18m
simple-udp-2skbm-894s9   PortAllocation                                                                   18m
simple-udp-7xxfz         PortAllocation   35.230.21.139          gke-test-cluster-default-5847533f-6z66   73m

Which issue(s) this PR fixes:
none

Special notes for your reviewer:
This is not expected scenario of editing GameServer but I assume that restarting agones controller with deleting bad GameServer would recover from this situation.

agones-bot · 2020-07-29T19:10:06Z

Build Succeeded 👏

Build Id: d5bc282d-f118-475a-9e97-c872153beb12

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

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

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

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1725/head:pr_1725 && git checkout pr_1725
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.8.0-fabc044

aLekSer · 2020-07-29T19:15:02Z

Note that in that case of amount=0 we never return from the recursion as if condition in line https://github.com/googleforgames/agones/blob/master/pkg/gameservers/portallocator.go#L146 would always be false.

agones-bot · 2020-07-29T19:30:19Z

Build Failed 😱

Build Id: 1c8aa1ec-e06a-4a63-ad29-2f241ee39617

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

akremsa · 2020-07-31T13:25:08Z

pkg/gameservers/portallocator_test.go

+		// single port empty
+		fd := fixture.DeepCopy()
+		fd.Spec.Ports[0].PortPolicy = ""
+		pa.Allocate(fd)


Allocate returns GameServer pointer, so it is worth checking it for nil.

akremsa · 2020-07-31T13:25:39Z

pkg/gameservers/portallocator_test.go

+		fd := fixture.DeepCopy()
+		fd.Spec.Ports[0].PortPolicy = ""
+		pa.Allocate(fd)
+		assert.Nil(t, err)


Which error do you check here? Seems this assert is redundant.

akremsa · 2020-07-31T13:26:21Z

pkg/gameservers/portallocator_test.go

+		assert.True(t, cache.WaitForCacheSync(stop, pa.nodeSynced))
+
+		err := pa.syncAll()
+		assert.Nil(t, err)


Use require here, please. If an error happens here there is no point to continue.

Also use NoError over Nil - that way if it fails, we get a better error message.

akremsa · 2020-07-31T13:30:01Z

pkg/gameservers/portallocator_test.go

@@ -39,6 +39,37 @@ var (
 	n3 = corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node3", UID: "node3"}}
 )

+func TestEmptyPortPolicy(t *testing.T) {


I think this test name is not accurate. It should describe which function do you test.
There is a test function TestPortAllocatorAllocate, so you can put this scenario there as a separate t.Run("empty port policy") statement.

"TestPortAllocatorAllocateWithEmptyPolicy" ?

akremsa

several changes have been requested

google-oss-robot · 2020-07-31T13:31:13Z

@akremsa: changing LGTM is restricted to collaborators

In response to this:

several changes have been requested

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

markmandel

Doh! I never submitted this review!

markmandel · 2020-07-31T17:39:11Z

pkg/gameservers/portallocator_test.go

+		fd := fixture.DeepCopy()
+		fd.Spec.Ports[0].PortPolicy = ""
+		pa.Allocate(fd)
+		assert.Nil(t, err)


Suggested change

assert.Nil(t, err)

assert.NoError(t, err)

(gives a better message on failure)

markmandel · 2020-07-31T17:39:38Z

pkg/gameservers/portallocator_test.go

+		assert.True(t, cache.WaitForCacheSync(stop, pa.nodeSynced))
+
+		err := pa.syncAll()
+		assert.Nil(t, err)


Also use NoError over Nil - that way if it fails, we get a better error message.

markmandel · 2020-07-31T17:40:32Z

pkg/gameservers/portallocator_test.go

@@ -39,6 +39,37 @@ var (
 	n3 = corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node3", UID: "node3"}}
 )

+func TestEmptyPortPolicy(t *testing.T) {


"TestPortAllocatorAllocateWithEmptyPolicy" ?

markmandel · 2020-07-31T17:40:47Z

pkg/gameservers/portallocator.go

@@ -121,6 +121,9 @@ func (pa *PortAllocator) Allocate(gs *agonesv1.GameServer) *agonesv1.GameServer
 	// Also the return gives an escape from the double loop
 	findOpenPorts := func(amount int) []pn {
 		var ports []pn
+		if amount <= 0 {


Excellent short circuit! 👍

aLekSer · 2020-08-04T21:03:41Z

Thanks for the review, I have applied both series of comments.

agones-bot · 2020-08-04T21:20:24Z

Build Succeeded 👏

Build Id: 5bcf26fb-3fc7-409e-9441-37a1e70e72c8

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

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

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

https://5296de6-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1725/head:pr_1725 && git checkout pr_1725
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.8.0-5296de6

google-oss-robot · 2020-08-05T10:31:58Z

@akremsa: changing LGTM is restricted to collaborators

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

There could be the possible case when we set PortPolicy as an empty string on update of GameServer.

agones-bot · 2020-08-06T08:20:52Z

Build Succeeded 👏

Build Id: 2eb288d6-4d54-42d9-9b69-fd37ba14576f

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

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

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

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

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1725/head:pr_1725 && git checkout pr_1725
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.8.0-fc49377

markmandel

Love it!

This also ties nicely into #749

google-oss-robot · 2020-08-06T18:35:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akremsa, aLekSer, markmandel

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]

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

* Fix findOpenPorts portAllocator function There could be the possible case when we set PortPolicy as an empty string on update of GameServer.

google-oss-robot requested review from EricFortin and roberthbailey July 29, 2020 18:51

googlebot added the cla: yes label Jul 29, 2020

google-oss-robot added the size/M label Jul 29, 2020

aLekSer requested a review from markmandel July 29, 2020 18:59

akremsa reviewed Jul 31, 2020

View reviewed changes

akremsa suggested changes Jul 31, 2020

View reviewed changes

markmandel reviewed Aug 4, 2020

View reviewed changes

aLekSer force-pushed the fix/findOpenPorts branch from 58ffbae to 5296de6 Compare August 4, 2020 21:01

google-oss-robot added size/S and removed size/M labels Aug 4, 2020

akremsa approved these changes Aug 5, 2020

View reviewed changes

aLekSer added 3 commits August 6, 2020 10:59

Fix findOpenPorts portAllocator function

b0a6ef8

There could be the possible case when we set PortPolicy as an empty string on update of GameServer.

Applying comments from PR

5294800

Fix Nil - NoError

fc49377

aLekSer force-pushed the fix/findOpenPorts branch from 5296de6 to fc49377 Compare August 6, 2020 08:00

markmandel approved these changes Aug 6, 2020

View reviewed changes

google-oss-robot assigned markmandel Aug 6, 2020

google-oss-robot added the lgtm label Aug 6, 2020

google-oss-robot added the approved label Aug 6, 2020

markmandel merged commit c28848a into googleforgames:master Aug 6, 2020

markmandel added area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/bug These are bugs. and removed area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc labels Aug 6, 2020

markmandel added this to the 1.8.0 milestone Aug 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix findOpenPorts portAllocator function #1725

Fix findOpenPorts portAllocator function #1725

aLekSer commented Jul 29, 2020 •

edited

Loading

agones-bot commented Jul 29, 2020

aLekSer commented Jul 29, 2020 •

edited

Loading

agones-bot commented Jul 29, 2020

akremsa Jul 31, 2020

akremsa Jul 31, 2020

akremsa Jul 31, 2020

markmandel Jul 31, 2020

akremsa Jul 31, 2020 •

edited

Loading

markmandel Jul 31, 2020

akremsa left a comment

google-oss-robot commented Jul 31, 2020

markmandel left a comment

markmandel Jul 31, 2020

markmandel Jul 31, 2020

markmandel Jul 31, 2020

markmandel Jul 31, 2020

aLekSer commented Aug 4, 2020

agones-bot commented Aug 4, 2020

google-oss-robot commented Aug 5, 2020

agones-bot commented Aug 6, 2020

markmandel left a comment

google-oss-robot commented Aug 6, 2020

Fix findOpenPorts portAllocator function #1725

Fix findOpenPorts portAllocator function #1725

Conversation

aLekSer commented Jul 29, 2020 • edited Loading

agones-bot commented Jul 29, 2020

aLekSer commented Jul 29, 2020 • edited Loading

agones-bot commented Jul 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akremsa Jul 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akremsa left a comment

Choose a reason for hiding this comment

google-oss-robot commented Jul 31, 2020

markmandel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aLekSer commented Aug 4, 2020

agones-bot commented Aug 4, 2020

google-oss-robot commented Aug 5, 2020

agones-bot commented Aug 6, 2020

markmandel left a comment

Choose a reason for hiding this comment

google-oss-robot commented Aug 6, 2020

aLekSer commented Jul 29, 2020 •

edited

Loading

aLekSer commented Jul 29, 2020 •

edited

Loading

akremsa Jul 31, 2020 •

edited

Loading