-
Notifications
You must be signed in to change notification settings - Fork 40k
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
Normalize apiserver error handling of standard errors #722
Conversation
@@ -336,6 +336,9 @@ type Status struct { | |||
// TODO: if "working", include an operation identifier so final status can be | |||
// checked. | |||
Status string `json:"status,omitempty" yaml:"status,omitempty"` | |||
// An optional machine readable description of why this operation is in the | |||
// "failure" or "working" states. | |||
Reason string `json:"reason,omitempty" yaml:"reason,omitempty"` |
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.
If it's machine readable, should we use an typedef'd string here instead of a raw string?
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.
Yeah, depends on how many we want to spec. I was potentially anticipating hundreds - with components mostly responsible for sending them in the case where there is a clear API subtlety in the error that a client could react to with a more appropriate message. I.e. if the Details says "pod 0294830298420394804 not found" and the Reason is "not_found_pod", a client can choose to say:
switch status.Reason {
case "not_found_pod":
msg = "The requested pod could not be found. It may have been deleted."
default:
msg = "Unable to complete request: " + status.Details
}
In a sense, I'd say this is almost exactly like Golang "error" in practice - you can do a string.Contains() for a specific segment (if the stdlib doesn't give you a type) but in most cases you prefer to do an equality check to a constant, a type cast to a struct, or an interface type cast.
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 feel like this will evolve into a complicated mess, and arguably I started it with the poorly defined "Details" field. I'd argue for making a system where there are two fields. One is a machine readable string enum (of which "other" is one of the choices) and the other is something like the current details field. I think whatever system you use, you should replace the Details field. It's only used for one thing right now, so now is the time to fix it.
{
"status": "Working",
"detailType": "OperationNumber",
"detail": "42",
}
Basically LGTM, but I think that if we are truely considering Reason to be machine readable, we should make it an enum or a struct, rather than a string that has a convention. |
If we do Reason as a struct, I could imagine:
where EDIT: Should reason be optional? That keeps responses clean when a reason is not relevant. |
I like everything except the interaction between the detail and reason fields. |
Unify error handling in apiserver into a single path - RESTStorage objects must provide appropriate errors individually. Ensure ALL errors which can be traced to logical faults with RESTStorage are returned as api.Status objects.
Finally redone, adds some simple error conditions. ptal. |
LGTM |
Will merge on Monday afternoon if there are no other comments. |
Normalize apiserver error handling of standard errors
Make a presubmit script to run all checkswq
* add dns hpa * add addon-manager label and change image name to arg for dns
* add dns hpa * add addon-manager label and change image name to arg for dns
Add feedback in KEP for GMSA support in Kubernetes
Changelog: 4.0.5 add logstash_dateformat as placeholder (kubernetes#718) Tweak travis.yml for suppressing validator warnings and add CI for Linux Arm64 architecture and macOS 10.14 (kubernetes#724) Elasticsearch ruby v7.5 (kubernetes#723) Add Oj serializer testcases for all job (kubernetes#722) Update documentation for ILM (kubernetes#721) 4.0.4 Provide clearing caches timer (kubernetes#719) 4.0.3 Use http.scheme settings for persisent scheme setup (kubernetes#713) 4.0.2 Support TLSv1.3 (kubernetes#710) 4.0.1 Placeholders for template name and customize template (kubernetes#708) Add overwriting ilm policy config parameter (kubernetes#707) Fix a failing ILM config testcase (kubernetes#706) 4.0.0 Restructuring ILM related features (kubernetes#701) Extract placeholders in pipeline parameter (kubernetes#695) fix typo in README.md (kubernetes#698) Reduce log noise when not using rollover_index (kubernetes#692)
Now rebased on top of master, adds a few simple standard errors that can be used by etcd to warn of duplicate creation, update conflict, and not found.