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

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

Closed
wants to merge 2 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cce7719
Added ability to Start / Stop healthchecks.
May 30, 2020
7d60d4a
Merge branch 'master' into master
Davidnovarro Jun 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 37 additions & 6 deletions sdks/unity/AgonesSdk.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,36 +38,47 @@ public class AgonesSdk : MonoBehaviour
[Range(0.01f, 5)] public float healthIntervalSecond = 5.0f;

/// <summary>
/// Whether the server sends a health ping to the Agones sidecar.
/// Whether to start sending health ping to the Agones sidecar.
/// </summary>
public bool healthEnabled = true;
[SerializeField] private bool startHealthChecks = true;
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 good reason to change this name?

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

@Davidnovarro Davidnovarro Jun 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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() 
}

Copy link
Member

@markmandel markmandel Jun 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.


/// <summary>
/// Debug Logging Enabled. Debug logging for development of this Plugin.
/// </summary>
public bool logEnabled = false;

/// <summary>
/// Indicates whether health checks are currently started.
/// </summary>
public bool HealthIsStarted { get; private set; }

private string sidecarAddress;
private readonly CancellationTokenSource cancellationTokenSource = new CancellationTokenSource();

private struct KeyValueMessage
{
public string key;
public string value;
public KeyValueMessage(string k, string v) => (key, value) = (k, v);

public KeyValueMessage(string key, string value)
{
this.key = key;
this.value = value;
}
}

#region Unity Methods
// Use this for initialization.
private void Awake()
{
String port = Environment.GetEnvironmentVariable("AGONES_SDK_HTTP_PORT");
string port = Environment.GetEnvironmentVariable("AGONES_SDK_HTTP_PORT");
sidecarAddress = "http://localhost:" + (port ?? "9358");
}

private void Start()
{
HealthCheckAsync();
if (startHealthChecks)
StartHealthChecks();
}

private void OnApplicationQuit()
Expand Down Expand Up @@ -212,6 +223,26 @@ public async Task<bool> Reserve(TimeSpan duration)
return await SendRequestAsync("/reserve", json).ContinueWith(task => task.Result.ok);
}

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

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

{
if (HealthIsStarted)
return;

HealthIsStarted = true;
HealthCheckAsync();
}

/// <summary>
/// Stop sending health ping to the Agones sidecar.
/// </summary>
public void StopHealthChecks()
{
HealthIsStarted = false;
}

/// <summary>
/// WatchGameServerCallback is the callback that will be executed every time
/// a GameServer is changed and WatchGameServer is notified
Expand All @@ -237,7 +268,7 @@ public void WatchGameServer(WatchGameServerCallback callback)

private async void HealthCheckAsync()
{
while (healthEnabled)
while (HealthIsStarted)
{
await Task.Delay(TimeSpan.FromSeconds(healthIntervalSecond));

Expand Down