Health check should be moved out of `gameservers/controller.go` about agones

[enocom]

@markmandel How about pkg/health? In the past, I've found health checking often deserves its own package and eventually grows to justify it.

[markmandel]

So I don't see this being a Controller - in the Kubernetes sense (i.e. it doesn't manage resources in the cluster).

So I'd be more inclined to call it something else.

[enocom]

OK. I'll leave the SDK sidecar alone.

Also, I agree: let's try https://github.com/heptiolabs/healthcheck for health-checking as you suggest.

[markmandel]

SGTM.

[enocom]

Does health.NewHealthController or health.HealthController sound like a stutter? Wondering if this work includes renaming health.HealthController to just health.Controller to avoid the stuttering.

[enocom]

Work for this issue is happening here. One thing I'm concerned about is the use of mocks in both the gameservers package tests and now the health package tests.

I see two options:

1. Duplicate test_helpers.go in both packages and remove what is unneeded from both, or
2. Promote test_helpers.go into its own shareable package.

I'll open a PR and we can discuss the best approach over there.

[markmandel]

To go back to design - the original idea was to implement something (or maybe use directly) something more akin to https://github.com/heptiolabs/healthcheck

My thought was:

• cmd/controller would pass this through to each controller (currently there is only one)
• Each controller's responsibility would be to add any checks it has (I'm actually not sure what to check - but that's an interesting discussion)
• cmd/controller would be the one that starts the http endpoint.

Something similar to what I'm doing with the webhook manager is what I was thinking.

WDYT?

[enocom]

That sounds good. So HealthController stays put, but the server which answers requests at /healthz gets moved out, while maybe gaining an AddCheck function that other controllers can use to register their own specialized check. Does that sound like what you're thinking?

[markmandel]

Yeah - sounds like we're on the same page.

And since I'm thinking about it - the pattern could potentially(?) be repeated for the sdk sidecar binary - since that also has a http handler in it (although it actually has 2 health points, one for itself and one as a proxy for the backing game server).

Since that also fires up a http server in it's Run() command, and it also feels a bit icky to me as well.

It may be worth talking over a hangout to discuss the architecture, if you think that would be valuable. Some of the design of the game server health checking is captured in this ticket

[enocom]

This is the branch where I'm making the discussed changes.

[enocom]

Seems like this should be extracted, too. Yes?

[markmandel]

Re: the SDK sidecar, I'm going to say let's leave that alone for now - it's bit more complex, and won't have multiple controllers running through it. But we can definitely review in the future if we see a nice reuse case there.

[enocom]

Resolved by #98