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

Jump to bottom

fix(boot.go,src/healthsrv): add readiness and liveness probes #57

Merged
merged 14 commits into from Feb 5, 2016
Merged

fix(boot.go,src/healthsrv): add readiness and liveness probes #57

merged 14 commits into from Feb 5, 2016

Conversation

arschles
Copy link
Member

@arschles arschles commented Feb 4, 2016

Fixes #54

After this is merged and a new container is built, deis/charts#86 needs to be done

@arschles arschles self-assigned this Feb 4, 2016
@arschles arschles added this to the v2.0-beta1 milestone Feb 4, 2016
@arschles arschles mentioned this pull request Feb 4, 2016
Closed
@arschles arschles changed the title fix(boot.go,src/healthsrv): add readiness and liveness probes (WIP) fix(boot.go,src/healthsrv): add readiness and liveness probes Feb 5, 2016
bacongobbler
bacongobbler reviewed Feb 5, 2016
View reviewed changes
glide.lock
@@ -229,7 +226,7 @@ imports:
- name: github.com/feyeleanor/slices
version: bb44bb2e4817fe71ba7082d351fd582e7d40e3ea
- name: github.com/fsouza/go-dockerclient
version: 76fd6c68cf24c48ee6a2b25def997182a29f940e
version: ""
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure why glide did this. Investigating...

Copy link
Member Author

Choose a reason for hiding this comment

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

@bacongobbler after running glide up again (more precisely, make glideup), this version number didn't come back. In arschles@08099c9, however, another version number did come back for a launchpad.net dependency...

@technosophos suggested that I file an issue on glide for this, so I'm working on getting a solid set of repro steps down before I do.

@krancour
Copy link
Contributor

krancour commented Feb 5, 2016

I'm torn on whether it might be better to create the minio client used by the health check in / closer to the healthcheck code instead of doing so in boot.go and passing it in. On the one hand, the complexity of creating that client seems like it should be handled a little closer to where it's used rather than the boot script which really handles overall orchestration. On the other hand, secrets are used in constructing the minio client and you probably don't want to moves dependencies on kubernetes down into the healthcheck code. Given that I can see both sides of this, I wouldn't let this stand in the way of my LGTM, but I just figured I'd mention it in case it inspires you in some way.

@krancour krancour added the LGTM1 label Feb 5, 2016
Aaron Schlesinger added 11 commits February 5, 2016 09:41
fix(boot.go,src/healthsrv): add health check server
f46d64b 
and a single health check endpoint ‘/healthz’
fix(boot.go): add log line to indicate starting the minio server
040206b
fix(glide.lock,glide.yaml): add minio-go dependency
abdbcc1 
so that the health check server can use it to check the health of the
minio server
fix(boot.go,src/healthsrv): use the minio client to test minio on hea…
a0c231a 
…lth checks
fix(src/healthsrv/healthz_handler.go): add logging for health check e…
7597cb8 
…rrors
fix(Makefile): add ability to run go unit tests
e4376bd
fix(boot.go,boot_test.go): move minio client creation logic out
ed3bc61 
and add a test for it
fix(manifests/deis-minio-rc.yaml): add liveness and readiness probes
566a81e 
also specify the health check server port
fix(manifests/deis-minio-rc.yaml): change the health check port to ma…
acdbcf7 
…tch the default

…while still keeping it explicitly defined
fix(src/healthsrv/healtz_handler.go,src/storage): create a new interf…
eef0711 
…ace for listing buckets

The Go minio client adheres to it, and it will be used for more easily
testing the healthz endpoint handler. The alternative was to use this
interface:
https://godoc.org/github.com/minio/minio-go#CloudStorageClient

…
fix(src/healthsrv/healthz_handler_test.go): add test for the healthz …
4e774e9 
…handler
@arschles arschles mentioned this pull request Feb 5, 2016
Closed
@arschles
Copy link
Member Author

arschles commented Feb 5, 2016

@krancour good point, and thanks for that. I think the other option for minio client creation location is in the Serve function in the ./src/healthsrv package. I don't think it would harm anything, but it seemed like it was more prudent to create it inside boot.go, since it may be used elsewhere in the future. Also, it may be beneficial for tests, in the case where I may want to test the Serve function in the future (I don't right now though since it's very basic)

fix(glide.lock): re-run glide up
08099c9 
gets a version back for launchpad
@mboersma
Copy link
Member

mboersma commented Feb 5, 2016

This code looks good to me once the glide issue with go-dockerclient is sorted out.

@mboersma mboersma added the LGTM2 label Feb 5, 2016
fix(glide.yaml,glide.lock): add utils and aboutme sub-packages
c2353e4 
also re-run ‘glide up’, which adds versions back in for github
repositories.

finally, set the minio version to latest (as of this writing)
@arschles
Copy link
Member Author

arschles commented Feb 5, 2016

@mboersma @bacongobbler after running a glide up, it looks like there are no more empty versions for github projects left

bacongobbler
bacongobbler reviewed Feb 5, 2016
View reviewed changes
glide.lock
version: 68415e7123da32b07eab49c96d2c4d6158360e9b
repo: https://github.com/golang/protobuf
version: ""
repo: https://code.google.com/p/goprotobuf
Copy link
Member

Choose a reason for hiding this comment

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

code.google.com no longer exists. Any idea how this would occur?

Copy link
Member Author

Choose a reason for hiding this comment

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

no idea. glide seems to toggle that just about every time I run glide up

arschles added a commit that referenced this pull request Feb 5, 2016
@arschles
Merge pull request #57 from arschles/health-check
8a743ba 
fix(boot.go,src/healthsrv): add readiness and liveness probes
@arschles arschles merged commit 8a743ba into deis:master Feb 5, 2016
@arschles arschles deleted the health-check branch February 5, 2016 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@arschles arschles

Labels
bug LGTM1 LGTM2
Projects
None yet
Milestone
v2.0-beta1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@arschles @krancour @mboersma @bacongobbler