add opt-out ttlSecondsAfterFinished setting for the pre-delete hook #3442

mikeseese · 2023-10-19T22:15:51Z

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 PR adds the agones.crds.cleanupJobTTL Helm value to set the Kubernetes ttlSecondsAfterFinished spec value for the pre_delete_hook Job. This will, by default, cleanup the Job and associated Pods once they complete after 60 seconds, regardless if successful or not. Users can opt out of this functionality by setting agones.crds.cleanupJobTTL to <= 0. The ref doc just states 0 for simplicity, but the Helm template is more forgiving if they want to use null. Also if they put a float or falsey non-integer in for the value (e.g. 3.5 or null or false), it will consider it as 0 in an int conversion and not add the ttl value.

By having this feature, the Jobs, which are likely to succeed, won't pile up and clutter the cluster/dashboards after multiple iterations (particularly on a development cluster).

Which issue(s) this PR fixes:

Closes #3432

Special notes for your reviewer:

markmandel · 2023-10-19T22:20:44Z

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

@@ -100,6 +100,7 @@ The following tables lists the configurable parameters of the Agones chart and t
 |----------------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-------------------------------------------|
 | `agones.crds.install`                                    | Install the CRDs with this chart. Useful to disable if you want to subchart (since crd-install hook is broken), so you can copy the CRDs into your own chart.                                                                                                                                                    | `true`                                    |
 | `agones.crds.cleanupOnDelete`                            | Run the pre-delete hook to delete all GameServers and their backing Pods when deleting the helm chart, so that all CRDs can be removed on chart deletion                                                                                                                                                         | `true`                                    |
+| `agones.crds.cleanupJobTTL`                              | The number of seconds for Kubernetes to delete the associated Job and Pods of the pre-delete hook after it completes, regardless if the Job is successful or not. Set to `0` to disable cleaning up the Job or the associated Pods.                                                                              | `60`                                      |


This looks great - quick thing, can you make a copy of the table add feature codes around the new and old version?

See: https://agones.dev/site/docs/contribute/#within-a-page

We publish the docs on merge with main, so we have a way to keep old features hidden until they are ready in the next release.

Lemme know if you have any questions.

I think I implemented this properly; let me know if I missed something!

Looks good! Will see the preview when the build finishes! 👍🏻

agones-bot · 2023-10-19T22:32:58Z

Build Failed 😱

Build Id: 722ac2c0-0d9d-44bf-acca-bc2a9e602867

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

markmandel · 2023-10-19T22:41:37Z

Looks like a flake, although a funny one:

--- FAIL: TestGameServerAllocationValidate (0.12s)
    gameserverallocation_test.go:1185: 
        	Error Trace:	/go/src/agones.dev/agones/pkg/apis/allocation/v1/gameserverallocation_test.go:1185
        	Error:      	"[spec.scheduling: Unsupported value: "FLERG": supported values: "Packed", "Distributed" spec.required.counters: Forbidden: Feature CountsAndLists must be enabled spec.required.lists: Forbidden: Feature CountsAndLists must be enabled]" should have 1 item(s), but has 3
        	Test:       	TestGameServerAllocationValidate
FAIL
FAIL

agones-bot · 2023-10-20T00:17:05Z

Build Succeeded 👏

Build Id: 3804b4b5-2a7d-42a1-8593-edf06af1bb02

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://c83682c-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3442/head:pr_3442 && git checkout pr_3442
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.36.0-dev-c83682c-amd64

markmandel

Nice!

google-oss-prow · 2023-10-20T00:19:58Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

google-oss-prow · 2023-10-20T00:24:53Z

New changes are detected. LGTM label has been removed.

mikeseese · 2023-10-20T00:26:06Z

Ah, I pressed the "update branch" button, and that seems to have removed the lgtm label. I wasn't sure if that being updated with main was a requirement.

markmandel · 2023-10-20T00:38:13Z

Ah, I pressed the "update branch" button, and that seems to have removed the lgtm label. I wasn't sure if that being updated with main was a requirement.

Doesn't matter - it'll also get automatically updated and merged from here out.

agones-bot · 2023-10-20T01:53:11Z

Build Succeeded 👏

Build Id: c3901a3b-eade-414a-b53c-051d86db16dd

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://8713844-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3442/head:pr_3442 && git checkout pr_3442
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.36.0-dev-8713844-amd64

agones-bot · 2023-10-20T03:47:34Z

Build Succeeded 👏

Build Id: 77c381b5-a9c3-4928-bcd1-7b7f87a372f7

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://c9b8430-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3442/head:pr_3442 && git checkout pr_3442
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.36.0-dev-c9b8430-amd64

add opt-out ttlSecondsAfterFinished setting for the pre-delete hook

7c70658

google-oss-prow bot requested review from aLekSer and cyriltovena October 19, 2023 22:15

google-oss-prow bot added the size/XS label Oct 19, 2023

github-actions bot added the kind/feature New features for Agones label Oct 19, 2023

markmandel reviewed Oct 19, 2023

View reviewed changes

add feature tags for new helm reference doc table

c83682c

google-oss-prow bot added size/S and removed size/XS labels Oct 19, 2023

mikeseese requested a review from markmandel October 19, 2023 22:33

markmandel mentioned this pull request Oct 19, 2023

Flake: TestGameServerAllocationValidate #3443

Merged

markmandel approved these changes Oct 20, 2023

View reviewed changes

google-oss-prow bot assigned markmandel Oct 20, 2023

google-oss-prow bot added the lgtm label Oct 20, 2023

markmandel enabled auto-merge (squash) October 20, 2023 00:19

google-oss-prow bot added the approved label Oct 20, 2023

Merge branch 'main' into feat/cleanup-completed-delete-resources-jobs

8713844

google-oss-prow bot removed the lgtm label Oct 20, 2023

Merge branch 'main' into feat/cleanup-completed-delete-resources-jobs

c9b8430

markmandel merged commit f77482f into googleforgames:main Oct 20, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add opt-out ttlSecondsAfterFinished setting for the pre-delete hook #3442

add opt-out ttlSecondsAfterFinished setting for the pre-delete hook #3442

mikeseese commented Oct 19, 2023 •

edited

Loading

markmandel Oct 19, 2023

mikeseese Oct 19, 2023

markmandel Oct 19, 2023

agones-bot commented Oct 19, 2023

markmandel commented Oct 19, 2023

agones-bot commented Oct 20, 2023

markmandel left a comment

google-oss-prow bot commented Oct 20, 2023

google-oss-prow bot commented Oct 20, 2023

mikeseese commented Oct 20, 2023

markmandel commented Oct 20, 2023

agones-bot commented Oct 20, 2023

agones-bot commented Oct 20, 2023

add opt-out ttlSecondsAfterFinished setting for the pre-delete hook #3442

add opt-out ttlSecondsAfterFinished setting for the pre-delete hook #3442

Conversation

mikeseese commented Oct 19, 2023 • edited Loading

markmandel Oct 19, 2023

Choose a reason for hiding this comment

mikeseese Oct 19, 2023

Choose a reason for hiding this comment

markmandel Oct 19, 2023

Choose a reason for hiding this comment

agones-bot commented Oct 19, 2023

markmandel commented Oct 19, 2023

agones-bot commented Oct 20, 2023

markmandel left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Oct 20, 2023

google-oss-prow bot commented Oct 20, 2023

mikeseese commented Oct 20, 2023

markmandel commented Oct 20, 2023

agones-bot commented Oct 20, 2023

agones-bot commented Oct 20, 2023

mikeseese commented Oct 19, 2023 •

edited

Loading