Use github.com/heptio/healthcheck #98
Conversation
|
There will be conflicts with this PR and #95. Let's merge #95 first and I'll deal with the conflicts. One thing to note about this PR is the significant number of transitive dependencies that Heptio's healthcheck library pulls in:
If that list seems too big, we might consider the hand-rolled approach after all. |
|
#95 is merged! 🔨 😁 |
Gopkg.toml
Outdated
| version = "1.8.0" |
There was a problem hiding this comment.
Super, super nitpicky, but can we keep a line between each constraint?
Having them right next to each other freaks me out ;)
There was a problem hiding this comment.
Absolutely. Sorry for missing this. I agree it's nicer.
cmd/controller/main.go
Outdated
| err = c.Run(2, stop) | ||
| if err != nil { | ||
| logrus.WithError(err).Fatal("Could not run gameserver controller") | ||
| } | ||
|
|
||
| logrus.Info("Shut down gameserver controller") | ||
| } | ||
|
|
||
| func startHealthCheck(h healthcheck.Handler) { |
There was a problem hiding this comment.
Curious why you wrote your own function, rather than just a:
go http.ListenAndServe("0.0.0.0:8086", health)
Keeping the logging? (Not a bad thing)
There was a problem hiding this comment.
It's mostly a matter of style. I like having all the things together so as to avoid any subtle closure state, which in turn makes it easy to extract the thing if there's ever a need. That said, I see the webhook goroutine is inline. So in the interest of consistency, I'll follow that pattern and we can move things around later if need be.
cmd/controller/main.go
Outdated
| } | ||
| defer srv.Close() // nolint: errcheck | ||
|
|
||
| if err := srv.ListenAndServe(); err != nil { |
There was a problem hiding this comment.
I believe this will change the health check paths.
If so we'll need to update install.yaml to point to /live , otherwise this will break the controller and it would also be good to have a new readiness check too since this now exists (and because of #95 we actually have a service that this sits behind as well).
There was a problem hiding this comment.
I'll update install.yaml. What would a good readiness check of the game server be? Or a liveness check?
|
@enocom curious what your concern is over the extra dependencies. It's not something that has really bugged me in the past, but would be interested to hear what issues you might see arising from it. |
|
@markmandel I like to be careful about pulling in dependencies just because they represent a certain amount of liability and risk. But the |
|
We need to decide on a good liveness check and maybe a readiness check too and presumably this PR will be done. |
|
I don't know if this is possible, but what would make this controller healthy is that the goroutines generated in this code are still running and haven't exited in any way. I just can't think of a way of doing that? |
There were multiple places where queue and worker goroutines pattern for controllers (and related) were repeated in almost exactly the same ways. Since there are more controllers coming, it made sense to consolidate this logic into a centralised place that could be reused to make authoring them easier. This may also allow us to centralise some health checking in #98
There were multiple places where queue and worker goroutines pattern for controllers (and related) were repeated in almost exactly the same ways. Since there are more controllers coming, it made sense to consolidate this logic into a centralised place that could be reused to make authoring them easier. This may also allow us to centralise some health checking in #98
There were multiple places where queue and worker goroutines pattern for controllers (and related) were repeated in almost exactly the same ways. Since there are more controllers coming, it made sense to consolidate this logic into a centralised place that could be reused to make authoring them easier. This may also allow us to centralise some health checking in #98
There were multiple places where queue and worker goroutines pattern for controllers (and related) were repeated in almost exactly the same ways. Since there are more controllers coming, it made sense to consolidate this logic into a centralised place that could be reused to make authoring them easier. This may also allow us to centralise some health checking in #98
There were multiple places where queue and worker goroutines pattern for controllers (and related) were repeated in almost exactly the same ways. Since there are more controllers coming, it made sense to consolidate this logic into a centralised place that could be reused to make authoring them easier. This may also allow us to centralise some health checking in #98
There were multiple places where queue and worker goroutines pattern for controllers (and related) were repeated in almost exactly the same ways. Since there are more controllers coming, it made sense to consolidate this logic into a centralised place that could be reused to make authoring them easier. This may also allow us to centralise some health checking in #98
There were multiple places where queue and worker goroutines pattern for controllers (and related) were repeated in almost exactly the same ways. Since there are more controllers coming, it made sense to consolidate this logic into a centralised place that could be reused to make authoring them easier. This may also allow us to centralise some health checking in #98
|
@markmandel In the interest of finishing the 0.1 milestone, how would you feel if we moved adding a particular health check to the GameServer into a separate issue and associated PR? |
|
@enocom Assuming you meant the healthcheck for the sdk sidecar, I think this makes a lot of sense. If this feature also should be pushed to 0.2, that's also an option. |
Rather than a handrolled solution, this commit uses heptio's healthcheck library. Future consumers of the health check handler need only to register liveness or readiness probes. Update install.yaml to use /live for liveness check. Also, inline health handler function for consistent style.
|
@markmandel I propose merging this PR with the heptio health check library and adding a liveness check to a 0.2 milestone. Since this PR touches a few places in the |
|
SGTM! |
Rather than a handrolled solution, this commit uses heptio's healthcheck
library. Future consumers of the health check handler need only to
register liveness or readiness probes.