Skip to content
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

Merged
merged 1 commit into from
Aug 11, 2014

Conversation

smarterclayton
Copy link
Contributor

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.

@@ -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"`
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Member

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",
}

@brendandburns
Copy link
Contributor

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.

@smarterclayton
Copy link
Contributor Author

If we do Reason as a struct, I could imagine:

type StatusReason struct {
  Reason string
  Extra map[string]string
}

where Extra represents kv pairs for things like "kind", "id", extended codes.

EDIT: Should reason be optional? That keeps responses clean when a reason is not relevant.

@lavalamp
Copy link
Member

lavalamp commented Aug 1, 2014

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.
@smarterclayton
Copy link
Contributor Author

Finally redone, adds some simple error conditions. ptal.

@lavalamp
Copy link
Member

lavalamp commented Aug 9, 2014

LGTM

@smarterclayton
Copy link
Contributor Author

Will merge on Monday afternoon if there are no other comments.

lavalamp added a commit that referenced this pull request Aug 11, 2014
Normalize apiserver error handling of standard errors
@lavalamp lavalamp merged commit 9050c81 into kubernetes:master Aug 11, 2014
@smarterclayton smarterclayton deleted the improve_errors branch February 11, 2015 02:21
vishh pushed a commit to vishh/kubernetes that referenced this pull request Apr 6, 2016
Make a presubmit script to run all checkswq
mqliang pushed a commit to mqliang/kubernetes that referenced this pull request Dec 8, 2016
* add dns hpa

* add addon-manager label and change image name to arg for dns
mqliang pushed a commit to mqliang/kubernetes that referenced this pull request Mar 3, 2017
* add dns hpa

* add addon-manager label and change image name to arg for dns
seans3 pushed a commit to seans3/kubernetes that referenced this pull request Apr 10, 2019
Add feedback in KEP for GMSA support in Kubernetes
alex0ptr added a commit to alex0ptr/kubernetes that referenced this pull request Mar 11, 2020
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)
b3atlesfan pushed a commit to b3atlesfan/kubernetes that referenced this pull request Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants