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

Sdkserver port configuration #1078

Merged

Conversation

roberthbailey
Copy link
Member

First step of fixing #851.

I'm marking this as draft as I still need to do some testing.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 62834520-d08b-4491-891b-d6e09d03661a

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: da5f5b5b-8f91-42ef-b80d-eeb080781250

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 407e5e4c-4032-4db5-abf8-2d1ac4099dcc

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: bf51cc67-db8e-4134-82dc-5a71fc5206c5

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

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: b43b7269-db4a-4a95-9ae8-1d2dbc22be25

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/1078/head:pr_1078 && git checkout pr_1078
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.1.0-c74dff0

@roberthbailey
Copy link
Member Author

Ok, I've done some testing and this is now ready for review. Note that this only makes the ports configurable in the sdkserver, so currently you need to recompile the gameserver with a new SDK with a different port number to actually take advantage of this feature. I'll add SDK support in separate PRs (one per SDK to make them easier to review in parallel).

Testing cases:

  1. Not setting port, existing SDK: works as before
  2. Setting port, existing SDK: SDK fails to connect (expected) and gs in unhealthy
  3. Setting port, recompiled SDK with new port: works as expected
  4. Not setting port, recompiled SDK with new port: SDK fails to connect (expected) and gs in unhealthy

@roberthbailey
Copy link
Member Author

/assign @markmandel
/assign @aLekSer

@google-oss-robot
Copy link

@roberthbailey: GitHub didn't allow me to assign the following users: aLekSer.

Note that only googleforgames members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @markmandel
/assign @aLekSer

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: ea8026c8-f09a-455a-9818-1573f9936cb4

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/1078/head:pr_1078 && git checkout pr_1078
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.1.0-492e857

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 36ee0343-8c50-4f06-8281-01c28540f124

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/1078/head:pr_1078 && git checkout pr_1078
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.1.0-3f62f45

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.

Just to confirm - this doesn't cover the changes to the SDK(s) so it can connect to the specified ports. I assume that is coming in later PRs? (No problem if it is, just wanted to be sure)

pkg/apis/agones/v1/gameserver.go Show resolved Hide resolved
pkg/apis/agones/v1/gameserver.go Outdated Show resolved Hide resolved
pkg/apis/agones/v1/gameserver.go Outdated Show resolved Hide resolved
site/content/en/docs/Reference/fleet.md Outdated Show resolved Hide resolved
site/content/en/docs/Reference/gameserver.md Show resolved Hide resolved
@roberthbailey
Copy link
Member Author

Just to confirm - this doesn't cover the changes to the SDK(s) so it can connect to the specified ports. I assume that is coming in later PRs? (No problem if it is, just wanted to be sure)

Yes, that's what I was trying to explain here:

Note that this only makes the ports configurable in the sdkserver, so currently you need to recompile the gameserver with a new SDK with a different port number to actually take advantage of this feature. I'll add SDK support in separate PRs (one per SDK to make them easier to review in parallel).

@roberthbailey
Copy link
Member Author

I've rebased, picking up the fix for the spacing issues in the website.

I think there are two outstanding questions:

  1. Defaulting for the sdk server parameters: do we default logging, all, or none?
  2. Name for the log level / logging parameter: do we change it from Logging to LogLevel?

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 44f75acb-1ab1-4b1d-8565-71a22b131637

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

@markmandel
Copy link
Member

Defaulting for the sdk server parameters: do we default logging, all, or none?

I would default all. AFAIK, it seems to be the standard Kubernetes action, so that's what we've followed elsewhere. Also makes it easier to introspect what the values actually are from looking at that final GameServer configuration.

Name for the log level / logging parameter: do we change it from Logging to LogLevel?

I agree - LogLevel is better. 👍

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 25e53c4e-c7e0-407a-b14d-6dce84d25d9a

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/1078/head:pr_1078 && git checkout pr_1078
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.1.0-7b819a1

@roberthbailey roberthbailey force-pushed the sdkserver-port-configuration branch 2 times, most recently from 56c0ca2 to 4243761 Compare September 27, 2019 21:09
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 551da6be-3203-4162-baf5-bf40c9b3a71a

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 40515703-a725-4f81-b720-933841a94151

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

@roberthbailey
Copy link
Member Author

This is ready for another review pass.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 100e245a-4044-471e-aaf0-2750bcb2d9cd

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

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.

Looks like you got caught by a linting fail, but this looks great! Nice job! 😀

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: markmandel, roberthbailey
To complete the pull request process, please assign
You can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found 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

@google-oss-robot
Copy link

New changes are detected. LGTM label has been removed.

@roberthbailey
Copy link
Member Author

Looks like you got caught by a linting fail,

I still haven't figured out how to get GoLand to run gofmt on save.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 33cb6287-e5a5-4a22-b297-0dcaf3892b5a

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/1078/head:pr_1078 && git checkout pr_1078
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.1.0-8e1af2d

@roberthbailey roberthbailey merged commit 03eb7b1 into googleforgames:master Sep 27, 2019
@markmandel markmandel added this to the 1.1.0 milestone Oct 22, 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 Oct 22, 2019
@roberthbailey roberthbailey deleted the sdkserver-port-configuration branch August 24, 2020 23:01
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 size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants