fix(*): add readiness & liveness probes #149

arschles · 2016-02-08T19:35:52Z

Fixes #92
Fixes #89
Ref deis/deis#4809
When this is done, merge deis/charts#93

PR Overview

Added a health check server with a single endpoint (/healthz), intended to be used by the livenessProbe and readinessProbe.
- This endpoint checks the accessibility of the Kubernetes API & the object storage system, and checks that the SSH server is ready to serve
- The health check server utilizes two interfaces - for S3 interaction and Kubernetes API interaction - so that those two components can be mocked and the server can be more easily unit tested
- The handler for the /healthz endpoint does all three checks concurrently, and only fails if one or more of them don't complete before a hard-coded timeout, or any of them fail before the timeout
Added a (github.com/deis/builder/pkg/sshd).Circuit type, which acts as a simple, concurrency-safe on/off switch for communication across concurrency barriers. A Circuit is used to allow the SSH server to tell the health check server when it's ready to serve
Modified boot.go to serve the SSH server and the health check server concurrently, but to kill the process with an error code if either fails
Modified the manifests to closely resemble those in github.com/deis/charts (bring manifests up to date with deis/charts #89)
Added liveness and readiness checks to the deis-builder-rc.yaml manifest
Modified the Makefile to only look for non-vendored packages when make test is run, which makes all runs faster

Still TODO

Check k8s server on probe request
Check that the server is actually running on probe request
Tests for healthZHandler
TestOpenCloseConcurrent
make test output: pkg/sshd/server_test.go:106: not enough arguments in call to Serve

aledbf · 2016-02-09T21:28:47Z

pkg/healthsrv/healthz_handler.go

+			rsp.S3Buckets = append(rsp.S3Buckets, convertBucket(buck))
+		}
+
+		nsList, err := nsLister.List(labels.Everything(), fields.Everything())


Why not framework.NewInformer like here to not hit the apiserver unnecessarily

I'm unfamiliar with k8s.io/kubernetes/pkg/controller/framework, but it looks like it does not hit the API server, which I do need to do. Did I misunderstand something?

Also, if you know of a cheaper operation than listing namespaces (I'm assuming the common case is that there will not be many), I'd love to hear it!

@arschles I will post a snippet for listing namespaces using controller.framework. The idea behind this abstraction is that uses watch and an implicit cache that reduces the round trips to api-server and transfer size

That would be great, thanks. I'd definitely like to reduce general load on the k8s master.

Sent from my iPhone

On Feb 9, 2016, at 16:21, Manuel Alejandro de Brito Fontes [email protected] wrote:

In pkg/healthsrv/healthz_handler.go:

http.Error(w, str, http.StatusServiceUnavailable)

return

}

lbOut, err := bLister.ListBuckets(&s3.ListBucketsInput{})

if err != nil {

str := fmt.Sprintf("Error listing buckets (%s)", err)

log.Err(str)

http.Error(w, str, http.StatusServiceUnavailable)

return

}

var rsp healthZResp

for _, buck := range lbOut.Buckets {

rsp.S3Buckets = append(rsp.S3Buckets, convertBucket(buck))

}

nsList, err := nsLister.List(labels.Everything(), fields.Everything())
@arschles I will post a snippet for listing namespaces using controller.framework. The idea behind this abstraction is that uses watch and an implicit cache that reduces the round trips to api-server and transfer size

—
Reply to this email directly or view it on GitHub.

krancour · 2016-02-10T20:41:35Z

Is this still WIP? The WIP in the title and the "awaiting review" tag are giving me mixed signals.

arschles · 2016-02-10T20:43:23Z

@krancour sorry about that. Yes, still in progress. I changed the label to indicate so.

arschles · 2016-02-11T22:33:29Z

Tested myself. To test yourself, spin up a deis-dev cluster and kd delete rc deis-builder && kd create -f manifests/deis-builder-rc.yaml && kd get pod -w. Once you see the pod come up, wait for it to go into Running and 1/1 containers up.

arschles · 2016-02-11T23:05:30Z

@krancour this is now ready for review

smothiki · 2016-02-11T23:23:53Z

pkg/sshd/circuit.go

+type Circuit struct {
+	bit uint32
+}
+


why not byte or bool?

see below - the atomic package has no facilities for manipulating either one of those types

Okay I didn't know that trying to learn

basically, I'm using atomic here because the circuit can "get away" with simple bit flipping - and using atomic operations can easily do that in a cheaper way than with a mutex

smothiki · 2016-02-11T23:34:03Z

why not modify fetcher to health serv. Do we need another http server to handle health serv. Already fetcher has a health check handler

smothiki · 2016-02-11T23:38:18Z

Anyways we are thinking of moving away from fetcher . So please Ignore my previous comment

smothiki · 2016-02-12T00:01:06Z

boot.go

+				healthSrvCh := make(chan error)
+				go func() {
+					if err := healthsrv.Start(cnf.HealthSrvPort, kubeClient.Namespaces(), s3Client, circ); err != nil {
+						healthSrvCh <- err


Do we have to check if namepsace is present or not ? builder won't start if namespace is not present or k8s cluster is not working

Well, the goal here is to check that the k8s API is accessible. Given that the builder has started, we can assume that the namespace under which it runs (also specified in POD_NAMESPACE) exists. The call that the health server makes is to just get a list of namespaces, and the goal there is only to see if the master is accessible.

I think @aledbf had an idea for a cheaper method of determining whether the API server was accessible, but if you have ideas I'd like to hear them too.

@arschles if the goal is just check the status of API server why not just run:

res := c.Get().AbsPath("/healthz").Do() if res.Error() != nil{ // not possible to reach the server }

@aledbf that's much better - thanks!

@aledbf here's the rendered error when that code runs:

couldn't get version/kind; json parse error: invalid character 'o' looking for beginning of value

I'd like to keep this code as-is for now, and put in an issue to use the /healthz way later. Objections?

Objections?

None

smothiki · 2016-02-12T00:32:31Z

If k8s API is not accessible how come it's builder fault . Also it not making sense to me to check if API is accessible or not as builder wont start in the first place.
I agree checking S3 storage is accessible or not
This is overall amazing code. I feel its more complex design
If we are coming up a different health server why do we need an entire class to atomically set a value. We can just write one more handler in the health server that sets the circuit state value and ssh.server.go calls that handler before this https://github.com/deis/builder/blob/master/pkg/sshd/server.go#L73 and health check handle polls change of circuit state till the timeout is over.

smothiki · 2016-02-12T00:40:09Z

Also liveness probes shouldn't be checking S3 API accessibility

before, we ran ‘glide nv’ before *all* commands, but it was only used in the ‘test’ target. this commit only runs it as part of that target

bacongobbler · 2016-02-12T19:22:43Z

manifests/deis-builder-rc.yaml

            - containerPort: 3000
+              name: fetcher


Looks like we missed a spot in #171 :)

Thanks - removed in 958e0b1

The fetcher was removed in deis#171, but the port wasn’t removed from the manifest in that PR. This removes it

bacongobbler · 2016-02-12T19:47:45Z

pkg/sshd/circuit.go

+)
+
+// CircuitState represents the state of a Circuit
+type CircuitState uint32


Maybe I'm not understanding the lower level synchronization types here, but from reading up to understand this bit of code from sync/atomic's docs:

These functions require great care to be used correctly. Except for special, low-level applications, synchronization is better done with channels or the facilities of the sync package. Share memory by communicating; don't communicate by sharing memory.

Not saying this should be refactored (this looks good to me), but I'm curious on your decision to go this route instead of using a combination of a boolean and mutal exclusion locks instead. Looking at the code, it looks like we only need two circuit states: open and closed.

Yup, good question.

A bool/sync.RWMutex or channel-based implementation would work just as well here, and both would fit fine into this abstraction. I went the sync/atomic route mainly because there's no need to protect a critical section of logic - just the mutation of a single boolean - and sync/atomic can do concurrency-safe bit-twiddling well.

Basically, I originally checked this code in thinking it would be simplest and not because I had any performance or specific sync/atomic benefits in mind. Now that I've gotten a few questions on it, I wonder if it'd be best to refactor it using a mutex, which seems to be more familiar. Thoughts?

Here's some more on sync/atomic if you care:

Atomic integer and pointer manipulation usually does lend itself to lower level systems problems like databases, etc..., but the core concept still applies here - atomically manipulating bits. We're just approximating that single bit as a uint32.

Also, on many systems, the sync/atomic approach is more performant in a few different regards, but that's getting into scheduler and OS level implementation details, which is where I get fuzzy anyway.

Now that I've gotten a few questions on it, I wonder if it'd be best to refactor it using a mutex, which seems to be more familiar. Thoughts?

Honestly it's all bikeshedding, which I hate. I'm quite happy with this implementation, and it serves its purpose as required, so it looks good to me. This was more of an educational exercise to understand why you chose this implementation is all. :)

On the plus side, I just learned about a new package today! 🎉

Alrighty, thanks. Enjoy your new atomicity?

arschles · 2016-02-12T20:09:47Z

@bacongobbler are you referring to the AbsPath("/healthz") bug?

bacongobbler · 2016-02-12T20:30:42Z

Sorry, that comment was in regards to refactoring out pkg/log to use the standard log package as in this commit. Github doesn't really show well that I was making a comment on that commit.

arschles · 2016-02-12T20:33:41Z

Ah, got it. I see the tiny text where it says you commented on the commit now.

Filed deis/pkg#19 to track

aledbf · 2016-02-12T21:14:25Z

Code LGTM

arschles · 2016-02-12T21:32:36Z

Gonna hold off on merging this until the discussion at #149 (comment) is complete

technosophos · 2016-02-16T19:36:16Z

FWIW, here is my view on health checking logic:

The readiness check should ONLY pass if the underlying system believes itself to be ready to successfully handle a request.
A liveness probe should ONLY pass if the system is in a condition where it could successfully handle a request.

Neither probes are about establishing "fault", the purpose of the probes is to indicate that the system believes itself to be in a position to fulfill its service contract. That's it. If it can't reach a database, cache, API endpoint, etc., it should fail its check.

That said, I am a little concerned with probes being dependent on the state of Kubernetes itself. So the k8s API service availability is sort of a gray area. Originally, I was opposed to failing healthz based on k8s API server problems, but I've changed my mind. Here's the use case that caused me to change my mind:

Builder is working just fine.
A well-meaning admin tries to lock down the API server by enforcing stricter security policies
Builder can no longer hit the API server.

In this case, builder SHOULD fail because it can no longer fulfill its service contract. If a policy was changed, k8s itself will restart the pod and it will come up successful. Otherwise, it will go into crash backoff, which is exactly what ought to happen.

helgi · 2016-02-16T19:43:55Z

A liveness probe should not fail if k8s is down or s3 is reachable and here is why:

A liveness probe will cause a pod to restart when those external components fail, which in turn means the user can not even do git push but instead gets a unreachable hostname if there are no builders to service the request.

As a user (developer) I want to do a git push which fails gracefully if dependant services are down with a good error message, along the lines of "unable to process the git push right now, X and Y are down".

Builder is alive and can give good information back to the user, making the liveness probe depend on 3rd party things is a bad UX since the whole builder becomes unavailable at that point. I'm proposing we do the deeper health check during git push and readiness probe but not liveness probe. Different purposes and different external informational needs by devs / operators.

I'd like to hear the opposite side on how we'd handle the scenario of builder being down and telling the developers (and operators) what is going on and how they would effectively debug things when the builder is effectively left in a "running but not ready" state until s3 / k8s / etc comes up?

bacongobbler · 2016-02-16T20:23:19Z

To chime in my thoughts as well:

In this case, builder SHOULD fail because it can no longer fulfill its service contract. If a policy was changed, k8s itself will restart the pod and it will come up successful. Otherwise, it will go into crash backoff, which is exactly what ought to happen.

This is great for an administrator, but not for a user. In the current implementation the user has no view into why the builder is down. If we were to go down this route, a status page (similar to status pages like http://status.docker.com/) would be invaluable to a user to see what systems are down, bridging the communication gap between users and administrators.

I'd like to hear the opposite side on how we'd handle the scenario of builder being down and telling the developers (and operators) what is going on and how they would effectively debug things when the builder is effectively left in a "running but not ready" state until s3 / k8s / etc comes up?

^^

Another proposed solution offline was to gracefully fail a git push, but internally continuously perform health checks on upstream services like k8s/minio, which would then be logged if they're reporting as down. That way we can pick that up in https://github.com/deis/monitor with alerting (not implemented in telegraf AFAIK but not something that we couldn't do ourselves)

arschles · 2016-02-16T21:32:17Z

Ok, after extensive internal discussion, consensus is as follows:

For now, and probably through the v2-beta release, the logic in this PR will suffice as-is
I will be creating issues for the following enhancements, to be done at various points in the release cycle
- Have health probes check the status of the controller (Have probes check the status of the controller #181)
- Have health probes ping the local SSH server (instead of what they do now, which is only verify that it has started properly) (Have probes ping the SSH server #182)
- Have the git push logic respond with an informative error message during downstream dependency failures (Allow git pushes to respond with an informative failure message #183)

fix(*): add readiness & liveness probes

arschles added enhancement in progress labels Feb 8, 2016

arschles self-assigned this Feb 8, 2016

arschles added this to the v2.0-beta1 milestone Feb 8, 2016

arschles changed the title ~~fix(pkg/healthz): add readiness & liveness probes (WIP)~~ fix(*): add readiness & liveness probes (WIP) Feb 9, 2016

arschles added awaiting review and removed in progress labels Feb 9, 2016

arschles force-pushed the probes branch from 9f986dc to 1b58590 Compare February 9, 2016 20:51

aledbf reviewed Feb 9, 2016
View reviewed changes

arschles mentioned this pull request Feb 9, 2016

feat(deis-dev/manifests/deis-builder-rc.yaml): add liveness and readiness probes deis/charts#93

Merged

arschles force-pushed the probes branch 3 times, most recently from aeba9b4 to 89c3a29 Compare February 10, 2016 18:39

arschles added in progress and removed awaiting review labels Feb 10, 2016

arschles force-pushed the probes branch from 10c7202 to 905a3b1 Compare February 11, 2016 00:12

arschles added awaiting review and removed in progress labels Feb 11, 2016

arschles changed the title ~~fix(*): add readiness & liveness probes (WIP)~~ fix(*): add readiness & liveness probes Feb 11, 2016

smothiki reviewed Feb 11, 2016
View reviewed changes

smothiki reviewed Feb 12, 2016
View reviewed changes

arschles force-pushed the probes branch from c20fdb4 to ef5ddbc Compare February 12, 2016 00:06

Aaron Schlesinger added 3 commits February 12, 2016 09:12

fix(Makefile): speed up all makefile builds

c925148

before, we ran ‘glide nv’ before *all* commands, but it was only used in the ‘test’ target. this commit only runs it as part of that target

fix(pkg/healthsrv/server.go): remove unused constants

2b8fc3b

doc(pkg/sshd/circuit.go): add more godocs

8952b25

arschles force-pushed the probes branch from 4c15c8d to 8952b25 Compare February 12, 2016 17:14

arschles mentioned this pull request Feb 12, 2016

Use a cheaper way to ping the Kubernetes API in the health check endpoint #180

Open

bacongobbler reviewed Feb 12, 2016
View reviewed changes

fix(manifests/deis-builder-rc.yaml): remove fetcher port

958e0b1

The fetcher was removed in deis#171, but the port wasn’t removed from the manifest in that PR. This removes it

bacongobbler reviewed Feb 12, 2016
View reviewed changes

bacongobbler added the LGTM1 label Feb 12, 2016

aledbf added the LGTM2 label Feb 12, 2016

bacongobbler removed the awaiting review label Feb 12, 2016

arschles added a commit that referenced this pull request Feb 16, 2016

Merge pull request #149 from arschles/probes

3d5feb3

fix(*): add readiness & liveness probes

arschles merged commit 3d5feb3 into deis:master Feb 16, 2016

arschles deleted the probes branch February 16, 2016 21:32

This was referenced Feb 16, 2016

Have probes check the status of the controller #181

Closed

Have probes ping the SSH server #182

Open

Allow git pushes to respond with an informative failure message #183

Open

This was referenced Mar 21, 2018

Allow git pushes to respond with an informative failure message teamhephy/builder#33

Open

Have probes ping the SSH server teamhephy/builder#34

Open

fix(*): add readiness & liveness probes #149

fix(*): add readiness & liveness probes #149

Conversation

arschles commented Feb 8, 2016

PR Overview

Still TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krancour commented Feb 10, 2016

arschles commented Feb 10, 2016

arschles commented Feb 11, 2016

arschles commented Feb 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smothiki commented Feb 11, 2016

smothiki commented Feb 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smothiki commented Feb 12, 2016

smothiki commented Feb 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arschles commented Feb 12, 2016

bacongobbler commented Feb 12, 2016

arschles commented Feb 12, 2016

aledbf commented Feb 12, 2016

arschles commented Feb 12, 2016

technosophos commented Feb 16, 2016

helgi commented Feb 16, 2016

bacongobbler commented Feb 16, 2016

arschles commented Feb 16, 2016