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

Don't move Dev GameServer back to Ready always #2545

Merged
merged 2 commits into from
Apr 21, 2022
Merged

Don't move Dev GameServer back to Ready always #2545

merged 2 commits into from
Apr 21, 2022

Conversation

markmandel
Copy link
Member

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

What this PR does / Why we need it:

There was a bug in the implementation of the local development GameServer that would always move a GameServer to Ready any time the state was not currently Ready.

This meant if you tried to Allocate the GameServer, the controller would immediately move it back to being Ready.

This fix locks down that the only path that the controller should affect is moving from Creating state to Ready. This allows the end user to manually manage state on the GameServer as they desire.

Which issue(s) this PR fixes:

Closes #2536

Special notes for your reviewer:

@markmandel markmandel added kind/bug These are bugs. area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc labels Apr 15, 2022
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 90e7de24-cd40-4cf0-b04b-eaeb808bcb37

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/2545/head:pr_2545 && git checkout pr_2545
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.23.0-bcd4bc8

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 81eb1ef1-e2a7-47a5-81d1-c37af7e23c4c

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/2545/head:pr_2545 && git checkout pr_2545
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.23.0-dab0ad8

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 64d8b624-a1b1-4be3-bb67-a3368af22166

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

@markmandel
Copy link
Member Author

Looks like AWS has moved their examples around again

site/docs/advanced/scheduling-and-autoscaling/index.html
  Non-OK status: 404 --- site/docs/advanced/scheduling-and-autoscaling/index.html --> https://github.com/terraform-aws-modules/terraform-aws-eks/tree/master/examples/irsa_autoscale_refresh
site/docs/installation/creating-cluster/eks/index.html
  Non-OK status: 403 --- site/docs/installation/creating-cluster/eks/index.html --> https://itnext.io/kubernetes-is-hard-why-eks-makes-it-easier-for-network-and-security-architects-ea6d8b2ca965
site/docs/installation/creating-cluster/minikube/index.html
  Non-OK status: 403 --- site/docs/installation/creating-cluster/minikube/index.html --> https://blog.thepolyglotprogrammer.com/setting-up-kubernetes-on-wsl-to-work-with-minikube-on-windows-10-90dac3c72fa1
========================================================================

roberthbailey
roberthbailey reviewed Apr 21, 2022
View reviewed changes
pkg/gameservers/controller.go Outdated
c.loggerForGameServer(gs).Debug("GS is a development game server and will not be managed by Agones.")
// Only move from Creating -> Ready. Other manual state changes are up to the end user.
// We also don't want to move from Allocated -> Ready every time someone allocates a GameServer.
if !(gs.Status.State == agonesv1.GameServerStateCreating) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason that this isn't if gs.Status.State != agonesv1.GameServerStateCreating { ... }? The way it's currently written is difficult for my brain to parse.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a really good point. I was just copying the original format, but yeah - I'll make this change.

test/e2e/gameserver_test.go Show resolved Hide resolved
@markmandel
Don't move Dev GameServer back to Ready always
2df8b8e 
There was a bug in the implementation of the [local development
GameServer](https://agones.dev/site/docs/guides/local-game-server/)
that would always move a `GameServer` to `Ready` any time the state was
not currently `Ready`.

This meant if you tried to Allocate the `GameServer`, the controller
would immediately move it back to being `Ready`.

This fix locks down that the only path that the controller should affect
is moving from `Creating` state to `Ready`. This allows the end user to
manually manage state on the `GameServer` as they desire.

Closes googleforgames#2536
@markmandel
Review updates.
d93c763
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: e1d192a2-606d-4ef8-bcab-30eeac699fc4

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/2545/head:pr_2545 && git checkout pr_2545
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.23.0-d93c763

@google-oss-prow google-oss-prow bot added the lgtm label Apr 21, 2022
@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

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,roberthbailey]

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

@roberthbailey roberthbailey merged commit 418aff0 into googleforgames:main Apr 21, 2022
@markmandel markmandel deleted the bug/allocate-dev-gameserver branch April 21, 2022 18:24
Thiryn pushed a commit to Thiryn/agones that referenced this pull request Apr 25, 2022
@markmandel
Don't move Dev GameServer back to Ready always (googleforgames#2545)
8ffb513 
* Don't move Dev GameServer back to Ready always

There was a bug in the implementation of the [local development
GameServer](https://agones.dev/site/docs/guides/local-game-server/)
that would always move a `GameServer` to `Ready` any time the state was
not currently `Ready`.

This meant if you tried to Allocate the `GameServer`, the controller
would immediately move it back to being `Ready`.

This fix locks down that the only path that the controller should affect
is moving from `Creating` state to `Ready`. This allows the end user to
manually manage state on the `GameServer` as they desire.

Closes googleforgames#2536

* Review updates.
@SaitejaTamma SaitejaTamma added this to the 1.23.0 milestone May 3, 2022
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

@aLekSer aLekSer Awaiting requested review from aLekSer

@pooneh-m pooneh-m Awaiting requested review from pooneh-m

Assignees

@roberthbailey roberthbailey

Labels
approved area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/bug These are bugs. lgtm size/M
Projects
None yet
Milestone
1.23.0
Development

Successfully merging this pull request may close these issues.

how to allocate a Local Game Server?
4 participants
@markmandel @agones-bot @roberthbailey @SaitejaTamma