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.

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

Local SDK Server: Add proper GS state handling #979

Merged
merged 2 commits into from
Aug 15, 2019

Conversation

aLekSer
Copy link
Collaborator

@aLekSer aLekSer commented Aug 5, 2019

Update the state of local SDK GS according to calls which were made
by SDK client. Local sidecar now keeps track of the current state.
Part of #958 fix.
Added UTs for localsdk.go.

Need to add transition to Unhealthy state in a separate PR.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 054d7b89-2b94-4b1d-92e7-76daeb35b30e

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/GoogleCloudPlatform/agones.git pull/979/head:pr_979 && git checkout pr_979
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-d381ebd

@roberthbailey
Copy link
Member

Does this fix #958?

@aLekSer
Copy link
Collaborator Author

aLekSer commented Aug 6, 2019

@roberthbailey Yes, it is but after a small update. I need to make validTransition() function consistent to state diagram:
https://development.agones.dev/site/docs/reference/gameserver/

@aLekSer aLekSer force-pushed the local-sdk-server-updates branch 2 times, most recently from 89df81b to 74e0d3f Compare August 6, 2019 13:28
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 5c152218-0701-4ee2-9ce8-f441916bccf9

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: dea6b858-c77c-4333-b73d-0243e79df781

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

@aLekSer aLekSer force-pushed the local-sdk-server-updates branch from 74e0d3f to 9f606ee Compare August 6, 2019 13:38
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: f390bbff-34e0-4b6f-bcd2-46faff946c28

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/GoogleCloudPlatform/agones.git pull/979/head:pr_979 && git checkout pr_979
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.12.0-9f606ee

@aLekSer aLekSer marked this pull request as ready for review August 6, 2019 14:16
m[string(from)+string(to)] = true
}

func initValidStateTransitions() map[string]bool {
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we really care about this?

What are we trying to prevent here? I'm just wondering if this is going put some limitations on what's possible with the local test server, and add extra maintenance down the road for us.

What's the worst that happens if we don't validate state transitions locally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Main intention was to prevent developers from using wrong assumptions while debugging their game servers locally.
Additionally this would be a place in go code where we state all possible edges of state diagram.
I imagine that we can add a flag to validate this transitions or not so that it would be optional.

Copy link
Member

Choose a reason for hiding this comment

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

I'm just trying to think of what could be a bad transition?

Moving from Shutdown -> Ready/Allocated/Reserved ?

Even then - I'm not sure if we should stop people. If the developer wants to go in that loop, they would have to restart the gameserver and sdk every time, and that could slow them down. Fast development iteration loops are a very good thing, so should we be putting extra friction in people's way?

If people really want to move from Shutdown -> Ready they can change the backing file anyway, and it (should) overwrite what's in the local sdk. So I'm not sure this is necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that's right. This is redundant for local development and those steps are well documented in GS State transitions schema.

@aLekSer aLekSer force-pushed the local-sdk-server-updates branch 2 times, most recently from 5ce1b2c to 22ca271 Compare August 15, 2019 12:43
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: a3281ddb-136c-4549-999d-62751fd04cba

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

Update the state of local SDK GS according to calls which were made
by SDK client. Local sidecar now keeps the current state.
For googleforgames#958.
@aLekSer aLekSer force-pushed the local-sdk-server-updates branch from 22ca271 to eaf292f Compare August 15, 2019 12:47
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: b59d9a82-9bed-4509-9ac3-48fa6740cd6d

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

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: c8126e79-49b0-4f0b-a746-fc701d9b7368

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/GoogleCloudPlatform/agones.git pull/979/head:pr_979 && git checkout pr_979
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.0.0-eaf292f

Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

Nice!

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 41db2b40-6085-4afa-8b63-edf97272f3c0

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

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 71570bf9-d7a7-4655-9505-4e314c76d78e

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/GoogleCloudPlatform/agones.git pull/979/head:pr_979 && git checkout pr_979
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.0.0-73e61a8

@markmandel markmandel merged commit 6aab022 into googleforgames:master Aug 15, 2019
@markmandel markmandel added area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/feature New features for Agones labels Aug 15, 2019
@markmandel markmandel added this to the 1.0 milestone Aug 15, 2019
@aLekSer aLekSer deleted the local-sdk-server-updates branch August 15, 2019 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc kind/feature New features for Agones
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants