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

Implement gRPC Health Checking Protocol + add readiness/liveness probes to vizier-core #270

Merged
merged 8 commits into from
Dec 5, 2018
Merged

Implement gRPC Health Checking Protocol + add readiness/liveness probes to vizier-core #270

merged 8 commits into from
Dec 5, 2018

Conversation

lkpdn
Copy link
Contributor

@lkpdn lkpdn commented Dec 3, 2018 •
edited by jlewi
Loading

(1) and (2) listed in Ref: #268. Plus, set timeout (https://github.com/go-sql-driver/mysql/#timeout) parameter so that vizier-core never get stuck in a fruitless and harmful manner.

This change is Reviewable

@k8s-ci-robot
Copy link

Hi @lkpdn. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@lkpdn lkpdn changed the title Add rediness and liveness probes to vizier core Add readiness/liveness probes to vizier core Dec 3, 2018
@lkpdn lkpdn changed the title Add readiness/liveness probes to vizier core Add readiness/liveness probes to vizier-core Dec 3, 2018
@YujiOshima
Copy link
Contributor

@lkpdn Thank you for create this PR.
Could you implement gRPC Health Checking Protocol and check healthy with grpc_health_probe ?

@lkpdn
Copy link
Contributor Author

lkpdn commented Dec 4, 2018

@YujiOshima OK, I'll push a commit later. Thanks.

@lkpdn lkpdn changed the title Add readiness/liveness probes to vizier-core Implement gRPC Health Checking Protocol + add readiness/liveness probes to vizier-core Dec 4, 2018
Koichiro Den added 5 commits December 4, 2018 22:17
Ensure vizier-core never been stuck too long waiting for DB conn
9ee326b 
Signed-off-by: Koichiro Den <den@valinux.co.jp>
Add standard Health gRPC service
356482d 
Signed-off-by: Koichiro Den <den@valinux.co.jp>
Change db.New to return error instead of exit(1) with log.Fatal
65c3406 
Signed-off-by: Koichiro Den <den@valinux.co.jp>
Add SelectOne() to VizierDBInterface
61ac560 
Signed-off-by: Koichiro Den <den@valinux.co.jp>
Rename import for later convenience
d2c92c6 
Signed-off-by: Koichiro Den <den@valinux.co.jp>
@lkpdn lkpdn force-pushed the add-rediness-and-liveness-probes-to-vizier-core branch from 7d4709f to 2fea722 Compare December 4, 2018 13:18
@lkpdn
Copy link
Contributor Author

lkpdn commented Dec 4, 2018

@YujiOshima patch size gets quite heavier so I split it up into seven logical commits. Please take another look, thanks.

YujiOshima
YujiOshima reviewed Dec 4, 2018
View reviewed changes
cmd/manager/main.go Outdated Show resolved Hide resolved
cmd/manager/Dockerfile Outdated Show resolved Hide resolved
@lkpdn lkpdn force-pushed the add-rediness-and-liveness-probes-to-vizier-core branch 2 times, most recently from 0d1e0cb to a71f195 Compare December 4, 2018 23:25
Koichiro Den added 2 commits December 5, 2018 08:56
Implement and register Health Server for Katib manager
3cdf216 
Signed-off-by: Koichiro Den <den@valinux.co.jp>
Add readiness/liveness probes to vizier-core
d72d268 
Signed-off-by: Koichiro Den <den@valinux.co.jp>
@lkpdn lkpdn force-pushed the add-rediness-and-liveness-probes-to-vizier-core branch from a71f195 to d72d268 Compare December 4, 2018 23:57
@lkpdn
Copy link
Contributor Author

lkpdn commented Dec 5, 2018

Sorry about repeated forced push. Now all fixed, though I've just done testrun on my local minikube.
Thanks for reviewing, PTAL.

@YujiOshima
Copy link
Contributor

Thanks!!
/ok-to-test

Update test codebase
25012d4 
Fixes: 61ac560 ("Add SelectOne() to VizierDBInterface")
Signed-off-by: Koichiro Den <den@valinux.co.jp>
@lkpdn
Copy link
Contributor Author

lkpdn commented Dec 5, 2018

Sorry I forgot to update some test codebase. Pushed a commit.: 25012d4

@lkpdn
Copy link
Contributor Author

lkpdn commented Dec 5, 2018

/retest

@YujiOshima
Copy link
Contributor

@lkpdn Thank you!! This is very helpful.
/lgtm
/approve

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: YujiOshima

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 36d8d25 into kubeflow:master Dec 5, 2018
@lkpdn
Copy link
Contributor Author

lkpdn commented Dec 5, 2018

Thanks for the review!

@lkpdn lkpdn deleted the add-rediness-and-liveness-probes-to-vizier-core branch December 5, 2018 09:15
@pdmack pdmack mentioned this pull request Jan 13, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YujiOshima YujiOshima YujiOshima left review comments

@richardsliu richardsliu Awaiting requested review from richardsliu

@texasmichelle texasmichelle Awaiting requested review from texasmichelle

Assignees

@YujiOshima YujiOshima

Labels
approved lgtm ok-to-test size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lkpdn @k8s-ci-robot @YujiOshima