Unity SDK: Added ability to Start / Stop healthchecks. #1598

Davidnovarro · 2020-05-30T09:55:53Z

There was no public method to start/stop health checks manually.

Now we have both options to start/stop health checks manually or automatically.

/kind hotfix

There was no option to start / stop healthchecks manually. Now we have both options to start / stop healthchecs manually or automaticaly.

googlebot · 2020-05-30T09:55:56Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

google-oss-robot · 2020-05-30T09:55:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Davidnovarro
To complete the pull request process, please assign pooneh-m
You can assign the PR to them by writing /assign @pooneh-m 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:

OWNERS

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

agones-bot · 2020-05-30T10:13:15Z

Build Succeeded 👏

Build Id: af8c1e63-2c16-4255-89af-f7eb7511d214

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.7.0-cce7719
image: gcr.io/agones-images/agones-sdk:1.7.0-cce7719
image: gcr.io/agones-images/agones-ping:1.7.0-cce7719
Linux C++ SDK (build): agonessdk-1.7.0-cce7719-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.7.0-cce7719.zip

A preview of the website (the last 30 builds are retained):

https://cce7719-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1598/head:pr_1598 && git checkout pr_1598
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-cce7719

markmandel

Thanks for sending this through 👍 I had some follow up questions attached in my review.

This PR closes the issue #1583

Also, as a note, this doesn't close that issue, that issue is regarding the C# SDK.

I hope you don't mind, I added "Unity SDK" to the ticket title to give it some more context.

Thanks!

markmandel · 2020-06-01T17:08:13Z

sdks/unity/AgonesSdk.cs

        /// </summary>
-        public bool healthEnabled = true;
+        [SerializeField] private bool startHealthChecks = true;


Is there a good reason to change this name?

This would be a breaking change for the SDK, so we would like to avoid that where possible.

Also, changing the encapsulation would also be a breaking change. Is there a good reason to do that as well?

Is there a good reason to change this name?

The name itself is misleading, for example, the user may set it to true externally and expect health checks to start, which won't work because the variable is only checked in the Start method, in case if it was set to true then health checks start.

Also, changing the encapsulation would also be a breaking change. Is there a good reason to do that as well?

I think this variable previously was made public so unity will be able to serialize and show it in the Inspector it basically has no external use case, see code examples below.

void Start() { sdk.healthEnabled = true; //There is a 50 % chance that the health checks will start due to Unity random scheduling. } void Update() { if(someCondition) sdk.healthEnabled = true; //This won't work. if(someOtherCondition) sdk.healthEnabled = false; //This doesn't make sense because we can't renew the health checks, instead we can use sdk.Shutdown() }

To be honest, I'm not sure why you would be trying to dynamically decide if health checking is enable or not - you would enable health checking if you have it turned on on Agones. That's really it.

Health checking is not a tool for managing game server lifecycles.

This doesn't sound like a good reason to introduce a breaking change on both naming and encapsulation.

markmandel · 2020-06-01T17:12:22Z

sdks/unity/AgonesSdk.cs

+        /// <summary>
+        /// Start sending health ping to the Agones sidecar. 
+        /// </summary>
+        public void StartHealthChecks()


Thanks for sending this through. I'm curious though - what's the use case for these functions?

Was there a particular scenario in which you would want to manually start the health check process, rather than have it be part of the component?

Also, when would you want to stop a health check?

when would you want to stop a health check?

In my particular case, I do resue my game servers for a long period of time and they all have two external connections with Backend Servers (economy, rewards, etc..) and Party Servers (matchmaking, tickets, backfill).
In case if the GS loses the connection I mark it as non-healthy until the connection is restored.

There may be other use cases like the first match is complete but GS can't load the resources to continue.

Was there a particular scenario in which you would want to manually start the health check process, rather than have it be part of the component?

Yes, like GS was able to restore the connection or load the resources or some background process was revived.

In case if the GS loses the connection I mark it as non-healthy until the connection is restored.

This sounds like a very dangerous approach -- I wouldn't advise it. Unhealthy game servers have failed a health check and are deemed to not be alive. If this GameServer was part of a Fleet, it would immediately be shut down and recreated.

Yes, like GS was able to restore the connection or load the resources or some background process was revived.

I can't advocate for any of these scenarios.

This could be solved using custom labels for example, without abusing health checking abilities.

Hmm, you are right, this PR can be removed.

agones-bot · 2020-06-02T10:05:26Z

Build Succeeded 👏

Build Id: 3837d59c-6090-4071-ab66-e3d92c694669

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.7.0-7d60d4a
image: gcr.io/agones-images/agones-sdk:1.7.0-7d60d4a
image: gcr.io/agones-images/agones-ping:1.7.0-7d60d4a
Linux C++ SDK (build): agonessdk-1.7.0-7d60d4a-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.7.0-7d60d4a.zip

A preview of the website (the last 30 builds are retained):

https://7d60d4a-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1598/head:pr_1598 && git checkout pr_1598
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-7d60d4a

markmandel · 2020-06-03T13:31:48Z

Thanks for working through this with me though! Appreciate you taking the time 👍

Added ability to Start / Stop healthchecks.

cce7719

There was no option to start / stop healthchecks manually. Now we have both options to start / stop healthchecs manually or automaticaly.

google-oss-robot requested review from EricFortin and markmandel May 30, 2020 09:55

google-oss-robot added the size/M label May 30, 2020

Davidnovarro mentioned this pull request Jun 1, 2020

HealthCheckLoop Never invoked in C# SDK #1583

Closed

markmandel changed the title ~~Added ability to Start / Stop healthchecks.~~ UnitySDK: Added ability to Start / Stop healthchecks. Jun 1, 2020

markmandel reviewed Jun 1, 2020

View reviewed changes

markmandel changed the title ~~UnitySDK: Added ability to Start / Stop healthchecks.~~ Unity SDK: Added ability to Start / Stop healthchecks. Jun 1, 2020

Merge branch 'master' into master

7d60d4a

Davidnovarro closed this Jun 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unity SDK: Added ability to Start / Stop healthchecks. #1598

Unity SDK: Added ability to Start / Stop healthchecks. #1598

Davidnovarro commented May 30, 2020 •

edited

Loading

googlebot commented May 30, 2020

google-oss-robot commented May 30, 2020

agones-bot commented May 30, 2020

markmandel left a comment

markmandel Jun 1, 2020

markmandel Jun 1, 2020

Davidnovarro Jun 2, 2020 •

edited

Loading

markmandel Jun 2, 2020 •

edited

Loading

markmandel Jun 1, 2020

Davidnovarro Jun 2, 2020

markmandel Jun 2, 2020

Davidnovarro Jun 3, 2020

agones-bot commented Jun 2, 2020

markmandel commented Jun 3, 2020

Unity SDK: Added ability to Start / Stop healthchecks. #1598

Unity SDK: Added ability to Start / Stop healthchecks. #1598

Conversation

Davidnovarro commented May 30, 2020 • edited Loading

googlebot commented May 30, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

google-oss-robot commented May 30, 2020

agones-bot commented May 30, 2020

markmandel left a comment

Choose a reason for hiding this comment

markmandel Jun 1, 2020

Choose a reason for hiding this comment

markmandel Jun 1, 2020

Choose a reason for hiding this comment

Davidnovarro Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

markmandel Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

markmandel Jun 1, 2020

Choose a reason for hiding this comment

Davidnovarro Jun 2, 2020

Choose a reason for hiding this comment

markmandel Jun 2, 2020

Choose a reason for hiding this comment

Davidnovarro Jun 3, 2020

Choose a reason for hiding this comment

agones-bot commented Jun 2, 2020

markmandel commented Jun 3, 2020

Davidnovarro commented May 30, 2020 •

edited

Loading

Davidnovarro Jun 2, 2020 •

edited

Loading

markmandel Jun 2, 2020 •

edited

Loading