Skip to content

Adding a "/ready" endpoint for OpenShift #2952

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
Merged
merged 1 commit into from
Jun 9, 2015
Merged

Adding a "/ready" endpoint for OpenShift #2952

merged 1 commit into from
Jun 9, 2015

Conversation

stevekuznetsov
Copy link
Contributor

@stevekuznetsov stevekuznetsov force-pushed the skuznets/issue/1902 branch 2 times, most recently from 4efbcaf to 50bfb82 Compare June 8, 2015 20:10
smarterclayton
smarterclayton reviewed Jun 8, 2015
View reviewed changes
pkg/cmd/server/origin/master.go
func initReadinessCheckRoute(root *restful.WebService, path string, readyFunc func() bool, client *osclient.Client) {
root.Route(root.GET(path).To(func(req *restful.Request, resp *restful.Response) {
var healthzResponse kapi.Status
err := client.Get().AbsPath("/healthz").Do().Into(healthzResponse)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this adds much in retrospect. Let's just do the ready check. We can always come back later and jazz it up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. The Do() returns only the body of the response, not the header, so to parse it we just have two bytes - "ok" instead of also containing the header. Unclear what it contains for messages without a body.

@smarterclayton
Copy link
Contributor

Switch the poll on hack test-cmd and test-end-to-end to wait for the ready check to come back green

@derekwaynecarr
Copy link
Member

@stevekuznetsov - once this merges, do you mind updating the example here:

https://github.com/GoogleCloudPlatform/kubernetes/blob/master/examples/openshift-origin/openshift-controller.yaml

to actually use it?

@stevekuznetsov
Copy link
Contributor Author

@derekwaynecarr Yep, on my radar.

@stevekuznetsov stevekuznetsov force-pushed the skuznets/issue/1902 branch 3 times, most recently from aac6a8b to 4e7250e Compare June 9, 2015 12:54
This was referenced Jun 9, 2015
Closed
Closed
smarterclayton
smarterclayton reviewed Jun 9, 2015
View reviewed changes
pkg/cmd/server/origin/master.go
@@ -361,6 +361,7 @@ func (c *MasterConfig) InstallProtectedAPI(container *restful.Container) []strin
initControllerRoutes(root, "/controllers", c.Options.Controllers != configapi.ControllersDisabled, c.ControllerPlug)
initAPIVersionRoute(root, LegacyOpenShiftAPIPrefix, legacyAPIVersions...)
initAPIVersionRoute(root, OpenShiftAPIPrefix, currentAPIVersions...)
initReadinessCheckRoute(root, "/ready", c.ProjectAuthorizationCache.ReadyForAccess)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor change - can you make this "/healthz/ready"? We had a discussion about it upstream and we agreed readiness is like healthz. You may want to make the NonResourceURLs be properly wild carded (david would know how to do that) for healthz instead of explicitly enumerating the paths

@deads2k
Copy link
Contributor

deads2k commented Jun 9, 2015

https://github.com/openshift/origin/blob/master/pkg/cmd/server/bootstrappolicy/policy.go#L132

Adding "/heathz/*" ought to do it.

@liggitt
Copy link
Contributor

liggitt commented Jun 9, 2015

does that include /healthz?

@stevekuznetsov
Copy link
Contributor Author

/healthz/ in the bootstrap policy returns 403 from both /healthz and /healthz/ready @deads2k @liggitt

@deads2k
Copy link
Contributor

deads2k commented Jun 9, 2015

/heathz/* (note the star) and /heathz

@liggitt
Copy link
Contributor

liggitt commented Jun 9, 2015

typo... missing an l

@stevekuznetsov
Copy link
Contributor Author

Yep, those two together work

smarterclayton
smarterclayton reviewed Jun 9, 2015
View reviewed changes
hack/test-cmd.sh
@@ -155,6 +155,7 @@ export HOME="${FAKE_HOME_DIR}"

wait_for_url "${KUBELET_SCHEME}://${KUBELET_HOST}:${KUBELET_PORT}/healthz" "kubelet: " 0.25 80
wait_for_url "${API_SCHEME}://${API_HOST}:${API_PORT}/healthz" "apiserver: " 0.25 80
wait_for_url "${API_SCHEME}://${API_HOST}:${API_PORT}/healthz/ready" "etcdcaching: " 0.25 80
Copy link
Contributor

Choose a reason for hiding this comment

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

apiserver(ready)

@stevekuznetsov
Copy link
Contributor Author

[test]

@smarterclayton
Copy link
Contributor

Approved LGTM [merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/2352/) (Image: devenv-fedora_1765)

@openshift-bot
Copy link
Contributor

Evaluated for origin up to 2ea6492

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/3017/)

openshift-bot pushed a commit that referenced this pull request Jun 9, 2015
Merge pull request #2952 from stevekuznetsov/skuznets/issue/1902
33e8e5a 
Merged by openshift-bot
@openshift-bot openshift-bot merged commit 33e8e5a into openshift:master Jun 9, 2015
@stevekuznetsov stevekuznetsov deleted the skuznets/issue/1902 branch June 12, 2015 13:45
@stevekuznetsov stevekuznetsov mentioned this pull request Jun 19, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@stevekuznetsov @smarterclayton @derekwaynecarr @deads2k @liggitt @openshift-bot