reword error message when cluster state exit code is not found #9390
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
do-not-merge/invalid-commit-message
|
Hi @prezha. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
k8s-ci-robot
added
the
needs-ok-to-test
k8s-ci-robot
added
the
size/XS
|
Can one of the admins verify this patch? |
|
/assign @priyawadhwa |
priyawadhwa
reviewed
Oct 5, 2020
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
kvm2 Driver |
medyagh
requested changes
Oct 6, 2020
|
no worries @medyagh: i believe those are actual warning messages that came through (some examples are given in the pr description above), ie, not related to a container being created/starting up? |
what I got wrong is, We explicitly called out that stderr will always have unexpected artifacts, like panic's, in the design doc I think we had expected the user to parse only the stdout for json and ignore the stderr |
|
ok, that makes perfect sense for a requested json format output |
k8s-ci-robot
added
needs-rebase
|
kvm2 Driver Times for Minikube (PR 9390): 24.0s 62.0s 59.7s Averages Time Per Log docker Driver Times for Minikube (PR 9390): 30.1s 27.5s 27.1s Averages Time Per Log |
|
kvm2 Driver Times for Minikube (PR 9390): 60.0s 61.3s 62.3s Averages Time Per Log docker Driver Times for Minikube (PR 9390): 28.4s 27.8s 28.6s Averages Time Per Log |
@medyagh instead of 'forcing' user to use something like
example
|
|
kvm2 Driver Times for Minikube (PR 9390): 59.5s 57.3s 61.2s Averages Time Per Log docker Driver Times for Minikube (PR 9390): 28.9s 28.9s 29.6s Averages Time Per Log |
additionally: - issue warning insted of error if 'exitCode' is not found - rebase: use klog instead of glog don't pollute status json output with errors
prezha
force-pushed
the
json-output-cleanup
branch
from
a96c94f to
e7f9b3e
Compare
k8s-ci-robot
removed
do-not-merge/invalid-commit-message
prezha
changed the title
tstromberg
suggested changes
Oct 21, 2020
There was a problem hiding this comment.
This PR has some nice things - but please keep in mind that stderr is not designed to be parsed in UNIX. There will always be unhandled messages, such as panics, that are emitted to stderr - for any command-line, not just minikube.
Most JSON users will be coming from languages where stdout and stderr are handled separately because of this. Even shells handle this properly.
pkg/minikube/out/out.go
Outdated
| @@ -191,6 +191,17 @@ func SetOutFile(w fdWriter) { | |||
| func SetJSON(j bool) { | |||
| klog.Infof("Setting JSON to %v", j) | |||
| JSON = j | |||
| if JSON { | |||
| if err := flag.Set("logtostderr", "false"); err != nil { | |||
There was a problem hiding this comment.
In Go, libraries should never set or read flags - only main.
There was a problem hiding this comment.
thanks for this comment - i've removed it altogether (as per your general comment), and this specifically is because libraries should never (unexpectedly and uncontrollably) change the behaviour of the mail app?
|
@tstromberg thank you very much for your comments! |
k8s-ci-robot
added
size/XS
|
kvm2 Driver Times for Minikube (PR 9390): 63.9s 60.2s 62.2s Averages Time Per Log docker Driver Times for Minikube (PR 9390): 28.5s 27.3s 29.3s Averages Time Per Log |
priyawadhwa
reviewed
Nov 6, 2020
priyawadhwa
reviewed
Nov 6, 2020
prezha
changed the title
yes, you are right - i've renamed the pr accordingly |
|
kvm2 Driver Times for Minikube (PR 9390): 54.6s 57.5s 56.4s Averages Time Per Log docker Driver Times for Minikube (PR 9390): 29.1s 27.1s 27.6s Averages Time Per Log |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: prezha, priyawadhwa 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 |
Travis tests have failedHey @prezha, 1st Buildmake testTravisBuddy Request Identifier: e8b0ee90-2089-11eb-a20b-45518f503c27 |
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes: #9374
fixes #8933
as described by @medyagh in issue #9374, json output contains
unable to convert exit code to int: strconv.Atoi: parsing "": invalid syntaxerror messages while the container is starting, and they should be treated as non-errors (consequentially not showing in json output)original output (before pr) - example:
new output (after pr):
minikube status -o=json -l=cluster | jq .StatusNamedoes not contain this errorminikube status -o=json -l=cluster --alsologtostderrwould log it as message from data map(transient code 100 ("Starting"): Waiting for container(s) to start (exitcode=""))edit: i wasn't able to replicate case when
data["exitcode"]was set to"", just the complete absence ofdata["exitcode"](that manifests the same like""value in maps) - eg:therefore i've amended pr to, in case that
data["exitcode"]is set to""or is unset (which boils down again to be "", just being explicit), print warning entry with the data map instead of the specific message (and potentially wrong/misleading message i've initially mentioned above - ie, waiting for container to start...)-- with default (docker) driver:
-- with kvm2 driver: