Check for DeletionTimestamp of fleet and gameserverset before scaling #2526

estebangarcia · 2022-03-31T10:15:59Z

What type of PR is this?

/kind bug

What this PR does / Why we need it:

It checks the DeletionTimestamp of GameServerSet and sets numServersToAdd to 0 and checks DeletionTimestamp of fleets managed by autoscaler and doesn't scale if they are marked for deletion.

This prevents agones from recreating gameservers from fleets marked for deletion by foreground cascading. As reported in #2524

Which issue(s) this PR fixes:
Closes #2524

Special notes for your reviewer:
Tested on AWS EKS 1.21.5

agones-bot · 2022-03-31T10:41:08Z

Build Succeeded 👏

Build Id: b1f530c3-2be9-49df-9d7d-413a0e40f7ba

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

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

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

https://97246f3-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2526/head:pr_2526 && git checkout pr_2526
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.22.0-97246f3

markmandel

This straight makes sense! Thanks so much for the fix!

The only thing that would block this from merge is to add some unit tests to make sure we never mess up this functionality in the future.

(Do we need e2e tests? 🤔 )

agones-bot · 2022-04-01T14:53:00Z

Build Succeeded 👏

Build Id: 8d874f50-be96-41a5-85c0-e5fabca05ed3

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

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

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

https://5a02f8c-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2526/head:pr_2526 && git checkout pr_2526
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.22.0-5a02f8c

estebangarcia · 2022-04-01T15:03:31Z

Hey, @markmandel thanks for taking the time to review. I just added some unit tests to cover this.

I'm not sure if the e2e is necessary as it is a very small change, but I'll let you decide on that :)

markmandel · 2022-04-01T17:59:54Z

This looks good! @roberthbailey how do we feel about including this while we're in feature freeze?

It is a bug fix, and seems to be low risk 🤔 WDYT? Or play it safe and bring it into the next release?

roberthbailey

Should there also be a change in pkg/fleets/controller.go? If you delete a fleet with foreground cascading deletion and then proceed to update the fleet (e.g. change the container image) before the cascading deletion completes will the fleet controller create a new game server set which will then get orphaned?

pkg/fleetautoscalers/controller.go

roberthbailey · 2022-04-01T20:48:54Z

pkg/gameserversets/controller.go

@@ -307,6 +307,12 @@ func (c *Controller) syncGameServerSet(ctx context.Context, key string) error {

 	numServersToAdd, toDelete, isPartial := computeReconciliationAction(gsSet.Spec.Scheduling, list, c.counter.Counts(),
 		int(gsSet.Spec.Replicas), maxGameServerCreationsPerBatch, maxGameServerDeletionsPerBatch, maxPodPendingCount)
+
+	// GameserverSet is marked for deletion then don't add gameservers.
+	if !gsSet.DeletionTimestamp.IsZero() {


Similar question here - should we check for the finalizer being set?

markmandel · 2022-04-01T22:04:02Z

Should there also be a change in pkg/fleets/controller.go?

Oooh! That is an excellent question. I would argue yes - even if just to ensure no extra resource creation in general when a Fleet is being deleted (either foreground or background).

estebangarcia · 2022-04-02T19:04:19Z

Should there also be a change in pkg/fleets/controller.go?

I agree with this. I've just committed a change that considers this as well.

agones-bot · 2022-04-02T19:21:10Z

Build Failed 😱

Build Id: 55dec21f-58a6-4b12-ae6f-87f8d9be69fc

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

estebangarcia · 2022-04-02T19:27:43Z

@markmandel the e2e tests on Cloud Build seemed to have failed I see a lot of x509: certificate signed by unknown authority errors but I don't know if that's expected or not.

agones-bot · 2022-04-04T20:47:58Z

Build Failed 😱

Build Id: 7b89e579-1841-4ec4-964f-03ca01514c44

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

markmandel · 2022-04-04T20:55:31Z

Looks like the deity of flakiness has decided to target this PR. Rerunning.

agones-bot · 2022-04-04T21:23:02Z

Build Succeeded 👏

Build Id: 273c82cc-396b-4f86-89bf-2295d4238ce5

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

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

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

https://4286a22-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2526/head:pr_2526 && git checkout pr_2526
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.22.0-4286a22

agones-bot · 2022-04-05T19:40:36Z

Build Failed 😱

Build Id: b11be78b-571b-4270-b292-89355272654b

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

estebangarcia · 2022-04-06T08:53:17Z

I merged main to my fork, build is failing again. Going to need you to retry it when you can @markmandel

google-oss-prow · 2022-04-07T21:57:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: estebangarcia, 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

markmandel · 2022-04-07T21:57:54Z

We are past feature freeze - and I fixed the 404 in our site testing, so this is good to merge 👍🏻

google-oss-prow · 2022-04-07T21:58:01Z

New changes are detected. LGTM label has been removed.

agones-bot · 2022-04-07T22:24:48Z

Build Succeeded 👏

Build Id: d126582e-f88c-4fac-9b91-f15b2653b946

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

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

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

https://3ce3604-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2526/head:pr_2526 && git checkout pr_2526
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.23.0-3ce3604

…googleforgames#2526) Co-authored-by: Mark Mandel <markmandel@google.com>

google-oss-prow bot added the size/S label Mar 31, 2022

google-oss-prow bot requested review from cyriltovena and EricFortin March 31, 2022 10:16

markmandel reviewed Mar 31, 2022

View reviewed changes

estebangarcia force-pushed the main branch from 97246f3 to 5a02f8c Compare April 1, 2022 14:25

google-oss-prow bot added size/L and removed size/S labels Apr 1, 2022

roberthbailey reviewed Apr 1, 2022

View reviewed changes

Check for DeletionTimestamp of fleet and gameserverset before scaling

7d40f2c

estebangarcia force-pushed the main branch from 5a02f8c to 7d40f2c Compare April 2, 2022 19:03

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

Merge branch 'main' into main

4286a22

Merge branch 'main' into main

c655d0b

SaitejaTamma removed the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Apr 5, 2022

markmandel approved these changes Apr 7, 2022

View reviewed changes

google-oss-prow bot assigned markmandel Apr 7, 2022

google-oss-prow bot added the lgtm label Apr 7, 2022

google-oss-prow bot added the approved label Apr 7, 2022

Merge branch 'main' into main

3ce3604

google-oss-prow bot removed the lgtm label Apr 7, 2022

markmandel merged commit e47df78 into googleforgames:main Apr 7, 2022

Thiryn pushed a commit to Thiryn/agones that referenced this pull request Apr 25, 2022

Check for DeletionTimestamp of fleet and gameserverset before scaling (…

3d3a07f

…googleforgames#2526) Co-authored-by: Mark Mandel <markmandel@google.com>

SaitejaTamma added this to the 1.23.0 milestone May 3, 2022

WontonSam mentioned this pull request Dec 20, 2023

[Snyk] Fix for 8 vulnerabilities WontonSam/Phiahplay-#11

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for DeletionTimestamp of fleet and gameserverset before scaling #2526

Check for DeletionTimestamp of fleet and gameserverset before scaling #2526

estebangarcia commented Mar 31, 2022

agones-bot commented Mar 31, 2022

markmandel left a comment

agones-bot commented Apr 1, 2022

estebangarcia commented Apr 1, 2022

markmandel commented Apr 1, 2022

roberthbailey left a comment

roberthbailey Apr 1, 2022

markmandel commented Apr 1, 2022

estebangarcia commented Apr 2, 2022

agones-bot commented Apr 2, 2022

estebangarcia commented Apr 2, 2022

agones-bot commented Apr 4, 2022

markmandel commented Apr 4, 2022

agones-bot commented Apr 4, 2022

agones-bot commented Apr 5, 2022

estebangarcia commented Apr 6, 2022

google-oss-prow bot commented Apr 7, 2022

markmandel commented Apr 7, 2022

google-oss-prow bot commented Apr 7, 2022

agones-bot commented Apr 7, 2022

Check for DeletionTimestamp of fleet and gameserverset before scaling #2526

Check for DeletionTimestamp of fleet and gameserverset before scaling #2526

Conversation

estebangarcia commented Mar 31, 2022

agones-bot commented Mar 31, 2022

markmandel left a comment

Choose a reason for hiding this comment

agones-bot commented Apr 1, 2022

estebangarcia commented Apr 1, 2022

markmandel commented Apr 1, 2022

roberthbailey left a comment

Choose a reason for hiding this comment

roberthbailey Apr 1, 2022

Choose a reason for hiding this comment

markmandel commented Apr 1, 2022

estebangarcia commented Apr 2, 2022

agones-bot commented Apr 2, 2022

estebangarcia commented Apr 2, 2022

agones-bot commented Apr 4, 2022

markmandel commented Apr 4, 2022

agones-bot commented Apr 4, 2022

agones-bot commented Apr 5, 2022

estebangarcia commented Apr 6, 2022

google-oss-prow bot commented Apr 7, 2022

markmandel commented Apr 7, 2022

google-oss-prow bot commented Apr 7, 2022

agones-bot commented Apr 7, 2022