-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
check status of router, registry, metrics, logging, imagestreams in oc cluster status #14436
Conversation
@jim-minter just a general comment... not sure that a warning is the best way to report that the registry/router/etc. is not yet running. It may lead people to think that there's something wrong with their cluster. I would simply report that it's either running or not running. Maybe a warning is warranted only when the uptime of the container exceeds a certain threshold and the registry or router are still not functional. |
pkg/bootstrap/docker/status.go
Outdated
|
||
resp, _ := insecureCli.Get(config.AssetConfig.MetricsPublicURL + "/status") | ||
if resp == nil || resp.StatusCode != http.StatusOK { | ||
fmt.Println("Warning: metrics are not yet successfully deployed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s /are/is/ ... I would tend to think of metrics as a singular component
It would also be helpful to report the status of the persistent volume job (-n default job/persistent-volume-setup) |
@csrwng all done |
So I tried this locally ... I get this output:
So a couple of nits ...
Also, just a question, why are we doing the logging check that way? (Instead of for example hitting a URL like we do with metrics?) |
Because of the oauth proxy, I couldn't find a suitable URL to hit. If you can find one, do let me know! |
@sosiouxme @jcantrill please see above. Is there a reliable check that we can do to ensure that logging is working? |
"Working" comprises a lot of different things, some more subtle than others. But if we're just looking for "present" then why not hit the public logging URL and check that it returns a 302 status (as opposed to 503 if nothing were listening)? Technically the logging stack doesn't have to be deployed in the |
4533795
to
19f0fce
Compare
@csrwng updated and repushed:
|
LGTM |
[Test]ing while waiting on the merge queue |
re-pushed with fix to failing 'oc cluster status' test, ptal/remerge |
[merge] |
pkg/bootstrap/docker/status.go
Outdated
notReady++ | ||
} | ||
|
||
stdout, _, _ = eh.Command(strings.Split("oc get dc router -n default -o template --template {{.status.availableReplicas}}", " ")...).Output() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you using split here and not below? Don't use split.
pkg/bootstrap/docker/status.go
Outdated
|
||
eh := exec.NewExecHelper(dockerClient, openshift.OpenShiftContainer) | ||
|
||
stdout, _, _ := eh.Command(strings.Split("oc get dc docker-registry -n default -o template --template {{.status.availableReplicas}}", " ")...).Output() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use split.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the registry runs as a daemon set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is based on the default set by oc cluster up
.
@csrwng repushed to take care of @smarterclayton's comments above - please remerge |
Evaluated for origin test up to fc526b7 |
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2048/) (Base Commit: 2458531) |
@csrwng ping :) |
[merge] |
#14523 |
Evaluated for origin merge up to fc526b7 |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/978/) (Base Commit: 1284263) (Image: devenv-rhel7_6347) |
fixes #12224
@openshift/devex
@csrwng