Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

simple-game-server: Adds a graceful termination delay #3436

Merged
merged 5 commits into from
Oct 18, 2023
Merged

simple-game-server: Adds a graceful termination delay #3436

merged 5 commits into from
Oct 18, 2023

Conversation

zmerlynn
Copy link
Collaborator

  • Adds gracefulTerminationDelaySec to imitate a gameserver delaying after SIGTERM (eviction).

  • Adds test/eviction/evictpod.go, which I used to test the above.

This should allow easier testing in autoscaling scenarios (including eventually possibly e2e tests with eviction: safe: Always).

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: c999224b-e8d3-4a38-8b3b-a55c2fe9edb8

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):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/3436/head:pr_3436 && git checkout pr_3436
  • 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-676dd6f-amd64

roberthbailey
roberthbailey reviewed Oct 18, 2023
View reviewed changes
test/eviction/evictpod.go Outdated Show resolved Hide resolved
test/eviction/evictpod.go Outdated Show resolved Hide resolved
test/eviction/evictpod.go Outdated Show resolved Hide resolved
test/eviction/evictpod.go Outdated Show resolved Hide resolved
test/eviction/evictpod.go Outdated Show resolved Hide resolved
test/eviction/evictpod.go Outdated Show resolved Hide resolved
@zmerlynn
simple-game-server: Adds a graceful termination delay
c516736 
* Adds `gracefulTerminationDelaySec` to imitate a gameserver delaying
after SIGTERM (eviction).

* Adds test/eviction/evictpod.go, which I used to test the above.

This should allow easier testing in autoscaling scenarios (including
eventually possibly e2e tests with eviction: safe: Always).
@zmerlynn
Fix review comments
f9b064a
roberthbailey
roberthbailey reviewed Oct 18, 2023
View reviewed changes
Copy link
Member

@roberthbailey roberthbailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few nits. I also verified that passing a negative time to sleep is ok and doesn't need any additional error checking.

test/eviction/evictpod.go Outdated Show resolved Hide resolved
test/eviction/evictpod.go Outdated Show resolved Hide resolved
roberthbailey
roberthbailey approved these changes Oct 18, 2023
View reviewed changes
Copy link
Member

@roberthbailey roberthbailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once this merges and we push the new image version, we need to update all of the references across the code base. Can you do that in a follow up PR?

@google-oss-prow google-oss-prow bot added the lgtm label Oct 18, 2023
@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: roberthbailey, zmerlynn

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:

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

@zmerlynn
Copy link
Collaborator Author

Once this merges and we push the new image version, we need to update all of the references across the code base. Can you do that in a follow up PR?

Yup, that was the plan!

@zmerlynn zmerlynn enabled auto-merge (squash) October 18, 2023 16:46
@zmerlynn zmerlynn enabled auto-merge (squash) October 18, 2023 16:46
@google-oss-prow google-oss-prow bot removed the lgtm label Oct 18, 2023
@google-oss-prow
Copy link

New changes are detected. LGTM label has been removed.

@zmerlynn zmerlynn merged commit f3ed290 into googleforgames:main Oct 18, 2023
3 checks passed
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 181b4c0a-213d-4b6e-845c-ccfe6c2bcc16

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):

To install this version:

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

zmerlynn added a commit to zmerlynn/agones that referenced this pull request Oct 19, 2023
@zmerlynn
Bump simple-game-server references to 0.19
e660011 
I pushed simple-game-server:0.19 after googleforgames#3436 was merged, update all
references to verify it still works with everything.
@zmerlynn zmerlynn mentioned this pull request Oct 19, 2023
Merged
zmerlynn added a commit that referenced this pull request Oct 19, 2023
@zmerlynn
Bump simple-game-server references to 0.19 (#3439)
34f0463 
I pushed simple-game-server:0.19 after #3436 was merged, update all
references to verify it still works with everything.
@Kalaiselvi84 Kalaiselvi84 added kind/feature New features for Agones and removed kind/other labels Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roberthbailey roberthbailey roberthbailey approved these changes

@igooch igooch Awaiting requested review from igooch

@aLekSer aLekSer Awaiting requested review from aLekSer

@markmandel markmandel Awaiting requested review from markmandel

Assignees

@markmandel markmandel

@roberthbailey roberthbailey

Labels
approved kind/feature New features for Agones size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zmerlynn @agones-bot @roberthbailey @markmandel @Kalaiselvi84