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
cmd/kube-scheduler: return error instead of os.Exit when something goes wrong #104503
cmd/kube-scheduler: return error instead of os.Exit when something goes wrong #104503
Conversation
@sanposhiho: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
Hi @sanposhiho. 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. |
@@ -79,15 +79,14 @@ suitable Node. Multiple different schedulers may be used within a cluster; | |||
kube-scheduler is the reference implementation. | |||
See [scheduling](https://kubernetes.io/docs/concepts/scheduling-eviction/) | |||
for more information about scheduling and the kube-scheduler component.`, | |||
Run: func(cmd *cobra.Command, args []string) { | |||
RunE: func(cmd *cobra.Command, args []string) error { |
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 suggest to not change the error messages. Just return the errors to preserve the current behavior, whatever it is.
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.
Fixed thanks
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.
You may also want to set SilenceErrors
and SilenceUsage
to avoid Cobra printing the error and the usage information on the error.
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.
Ok, fixed.
As you said, if we don't set SilenceErrors/SilenceUsage, it seems that cobra changes error msg and adds usage message on error msg. thanks
https://github.com/spf13/cobra/blob/v1.2.1/command.go#L975-L994
/ok-to-test |
/retest |
1 similar comment
/retest |
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.
/lgtm
Thanks! did you manually try to test the changes? can you show us the difference between the old and new behavior? |
Hello @ahg-g. I've tried it briefly 1. When cobra.Command.RunE return an error.(For this case, I made RunE always return an error(
This is the same output as old. (but, internally, 2. When user specify wrong flag
This output is different from old (see below). This is because we set SilenceErrors and SilenceUsage true.
But if we don't set SilenceErrors and SilenceUsage on this PR, usage will be print when cobra.Command.RunE return an error. (And this is different output from old)
|
Thanks! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, sanposhiho 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 |
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.
/hold
}, | ||
SilenceErrors: true, |
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 does this mean exactly?
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.
When SilenceErrors = false (or not set), an error will be printed with the prefix Error:
. In the above example, it prints Error: something goes wrong
.
We print error on this line, so I think we don't need this.
https://github.com/spf13/cobra/blob/v1.2.1/command.go#L983-L987
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.
So, if we set false (or don't set SilenceErrors), error is printed twice like this.
$ ./_output/bin/kube-scheduler
Error: something goes wrong
something goes wrong
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.
thanks for the explanation
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'm going to replace this with a more generic solution that'll work for most of the Kubernetes commands in #105076.
It would be good to get some additional eyes on that. Perhaps someone can even try out the PR?
I ran tests manually with kube-scheduler as example and I think it still works as you intended here.
/hold cancel |
Should we add this change to the release notes? (Like |
sure, wont hurt to do that. s/don't/doesn't |
Hello team.
What type of PR is this?
/kind bug
What this PR does / why we need it:
The defer function will not be called if os.Exit.
https://play.golang.org/p/f7IjXeb62dd
Which issue(s) this PR fixes:
part of #102231
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: