change the "too old resource version" error from NewInternalError to NewBadRequest #16628

caesarxuchao · 2015-10-30T22:34:09Z

fix #14384

cc @erictune

k8s-github-robot · 2015-10-30T22:34:33Z

Labelling this PR as size/XS

k8s-bot · 2015-10-30T22:36:00Z

GCE e2e build/test failed for commit 7de7ec8413eb237e7baa6ae08010cc9a502b1782.

caesarxuchao · 2015-10-30T22:42:38Z

Sorry. I should have tested it locally first. I will fix it.
[update]
Fixed.

k8s-bot · 2015-10-30T23:26:56Z

GCE e2e test build/test passed for commit 0feb4e8e8c043ccb0ff09244d7a197c242092d02.

wojtek-t · 2015-11-02T15:46:40Z

pkg/storage/watch_cache.go

@@ -266,7 +266,7 @@ func (w *watchCache) GetAllEventsSinceThreadUnsafe(resourceVersion uint64) ([]wa
 		return result, nil
 	}
 	if resourceVersion < oldest {
-		return nil, errors.NewInternalError(fmt.Errorf("too old resource version: %d (%d)", resourceVersion, oldest))
+		return nil, errors.NewBadRequest(fmt.Sprintf("too old resource version: %d (%d)", resourceVersion, oldest))


To be honest - I'm not convinced about this change.
From the logic point of view - it's not a bad request - it's totally valid request.
It's the implementation limitation, that we cannot handle this request, because we don't have old enough data.

So for me it look more like "InternalError" - the request is correct - we just aren't good enough to serve it :)

Yeah I don't think it's a bad request. Also I think InternalError will give you a stack trace in apiserver logs, I would like to avoid this if it's the case. I suggest 410 if it avoids the stack trace.

Thanks @bprashanth. 410 seems to be most accurate one. Currently we don't have a NewGone method. I'll add one.

Yeah - 410 sounds good.

Actually what do you guys think about "412 Precondition Failed"-- I think
410 Gone sounds much too permanent?

On Mon, Nov 2, 2015 at 10:32 AM, Wojciech Tyczynski <
notifications@github.com> wrote:

In pkg/storage/watch_cache.go
#16628 (comment)
:

@@ -266,7 +266,7 @@ func (w *watchCache) GetAllEventsSinceThreadUnsafe(resourceVersion uint64) ([]wa
return result, nil
}
if resourceVersion < oldest {

return nil, errors.NewInternalError(fmt.Errorf("too old resource version: %d (%d)", resourceVersion, oldest))

return nil, errors.NewBadRequest(fmt.Sprintf("too old resource version: %d (%d)", resourceVersion, oldest))

Yeah - 410 sounds good.

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/16628/files#r43662572.

"416 Requested Range Not Satisfiable" and "417 Expectation Failed" both
sound reasonable, too. Unfortunately all 3 are talking about headers and
we are doing this via a query parameter.

On Mon, Nov 2, 2015 at 10:52 AM, Daniel Smith dbsmith@google.com wrote:

Actually what do you guys think about "412 Precondition Failed"-- I think
410 Gone sounds much too permanent?

On Mon, Nov 2, 2015 at 10:32 AM, Wojciech Tyczynski <
notifications@github.com> wrote:

In pkg/storage/watch_cache.go
#16628 (comment)
:

@@ -266,7 +266,7 @@ func (w *watchCache) GetAllEventsSinceThreadUnsafe(resourceVersion uint64) ([]wa
return result, nil
}
if resourceVersion < oldest {

return nil, errors.NewInternalError(fmt.Errorf("too old resource version: %d (%d)", resourceVersion, oldest))

return nil, errors.NewBadRequest(fmt.Sprintf("too old resource version: %d (%d)", resourceVersion, oldest))

Yeah - 410 sounds good.

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/16628/files#r43662572.

As long as it's not one of the more frequently used return codes I don't have a preference. 404 is what you generally use for things like http://www.google.com/lkajsdlkf, and of course a 5xx means we messed up. I agree 410 is generally what one would apply for: I know for sure this resource used to exist, but it's been deleted, and for it to apply in this scenario the "resource" in question needs to be the in-memory watch window etcd has flushed.

lavalamp · 2015-11-11T20:57:52Z

OK, I guess let's go with 410 Gone. We can change it later if we get complaints. I don't know that we currently consider HTTP response codes part of our API, though perhaps we should.

caesarxuchao · 2015-11-11T21:44:29Z

pkg/api/unversioned/types.go

+	// StatusReasonGone means the item is no longer available at the server and no
+	// forwarding address is known.
+	// Status code 410
+	StatusReasonGone StatusReason = "Gone"


@lavalamp, this actually is an API change. [edit] Let me know if it's appropriate.

Right, it's totally fine to add this status code to the api. The thing I was talking about was the range of codes we could possibly return from a given request. I don't think we consider that part of the API, or at least it's not written down places. I could be wrong?

Hmm..The status code that will be sent by the server is listed In the api-converions doc, and 410 is not there. https://github.com/kubernetes/kubernetes/blob/master/docs/devel/api-conventions.md#http-status-codes. Does that make the status code part of the API?

Good catch. Please include a change to that doc in this PR!

On Wed, Nov 11, 2015 at 2:27 PM, Chao Xu notifications@github.com wrote:

In pkg/api/unversioned/types.go
#16628 (comment)
:

@@ -162,6 +162,11 @@ const (
// Status code 409
StatusReasonConflict StatusReason = "Conflict"

// StatusReasonGone means the item is no longer available at the server and no

// forwarding address is known.

// Status code 410

StatusReasonGone StatusReason = "Gone"

Hmm..Status code that will be sent by the server is listed In the
api-converions doc, and 410 is not there.
https://github.com/kubernetes/kubernetes/blob/master/docs/devel/api-conventions.md#http-status-codes.
Does that make the status code part of the API?

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/16628/files#r44596262.

k8s-github-robot · 2015-11-11T21:45:10Z

Labelling this PR as size/S

k8s-bot · 2015-11-11T21:56:42Z

GCE e2e build/test failed for commit 0a50730fb9a756344125411d584bbca8c482ed8a.

caesarxuchao · 2015-11-11T22:29:12Z

@k8s-bot test this please.

k8s-bot · 2015-11-11T23:12:50Z

GCE e2e build/test failed for commit 0a50730fb9a756344125411d584bbca8c482ed8a.

caesarxuchao · 2015-11-13T00:50:31Z

docs/devel/api-conventions.md

+* `410 StatusGone`
+  * Indicates that the item is no longer available at the server and no forwarding address is known.
+  * Suggested client recovery behavior
+    * Do not retry. Fix the request.


The status code is added to the api-conventions.md. PTAL. I'll squash later.

k8s-bot · 2015-11-13T01:18:37Z

GCE e2e test build/test passed for commit 4c38b2b0f4a89c3d685f902128a0002dcd0b982e.

wojtek-t · 2015-11-25T15:49:26Z

@caesarxuchao - LGTM

Can you please squash the commits? (I think that we should merge it finally).

… Gone.

caesarxuchao · 2015-11-25T18:28:26Z

Thanks @wojtek-t. Squashed. I'll ping lavalamp after the holiday.

k8s-bot · 2015-11-25T18:56:00Z

GCE e2e test build/test passed for commit a470070.

lavalamp · 2015-11-30T18:53:49Z

LGTM. I'm pretty sure that clients (ours, at least) will treat this the same, as evidenced by the fact that you didn't update any client code, so there shouldn't be a compatibility problem.

k8s-github-robot · 2015-11-30T18:56:15Z

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

k8s-bot · 2015-11-30T19:28:10Z

GCE e2e test build/test passed for commit a470070.

wojtek-t · 2015-11-30T19:35:36Z

@k8s-bot unit test this please

k8s-github-robot · 2015-12-01T00:45:40Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2015-12-01T01:21:28Z

GCE e2e test build/test passed for commit a470070.

k8s-github-robot · 2015-12-01T01:21:51Z

Automatic merge from submit-queue

Auto commit by PR queue bot

caesarxuchao · 2015-12-01T01:56:09Z

@k8s-oncall, why did the bot merge this even if the unit test failed?

caesarxuchao · 2015-12-01T01:57:18Z

cc @eparis @ixdy

ixdy · 2015-12-01T02:01:22Z

#16662 also merged despite unit tests failing. I suspect mergebot is not looking at the unit tests result, which is worrying.

eparis · 2015-12-01T14:19:30Z

@ixdy Basically yes. Merge bot looks a unit+e2e+travis when it decides to retest. It only looks at e2e on the re-test. I will switch it today to look at e2e+unit on retest.

eparis · 2015-12-01T14:30:57Z

(this logic was built back when we had shippable doing unit testing and didn't have a reasonable mechanism to retest)

Auto commit by PR queue bot

caesarxuchao assigned lavalamp Oct 30, 2015

googlebot added the cla: yes label Oct 30, 2015

k8s-github-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 30, 2015

caesarxuchao force-pushed the change-error-type branch from 7de7ec8 to 0feb4e8 Compare October 30, 2015 22:57

wojtek-t reviewed Nov 2, 2015
View reviewed changes

caesarxuchao closed this Nov 2, 2015

caesarxuchao reopened this Nov 2, 2015

caesarxuchao reviewed Nov 11, 2015
View reviewed changes

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 11, 2015

caesarxuchao reviewed Nov 13, 2015
View reviewed changes

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Nov 24, 2015

change the "too old resource version" error from InternalError to 410…

a470070

… Gone.

caesarxuchao force-pushed the change-error-type branch from 4c38b2b to a470070 Compare November 25, 2015 18:27

lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2015

k8s-github-robot pushed a commit that referenced this pull request Dec 1, 2015

Merge pull request #16628 from caesarxuchao/change-error-type

55f5e48

Auto commit by PR queue bot

k8s-github-robot merged commit 55f5e48 into kubernetes:master Dec 1, 2015

wojtek-t mentioned this pull request Mar 11, 2016

"error that is not an unversioned.Status: too old resource version" #22830

Closed

lavalamp mentioned this pull request Mar 11, 2016

Sporadic "too old resource version" errors from master pods #22024

Closed

xingzhou pushed a commit to xingzhou/kubernetes that referenced this pull request Dec 15, 2016

Merge pull request kubernetes#16628 from caesarxuchao/change-error-type

e2203f9

Auto commit by PR queue bot

change the "too old resource version" error from NewInternalError to NewBadRequest #16628

change the "too old resource version" error from NewInternalError to NewBadRequest #16628

Conversation

caesarxuchao commented Oct 30, 2015

k8s-github-robot commented Oct 30, 2015

k8s-bot commented Oct 30, 2015

caesarxuchao commented Oct 30, 2015

k8s-bot commented Oct 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Nov 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-github-robot commented Nov 11, 2015

k8s-bot commented Nov 11, 2015

caesarxuchao commented Nov 11, 2015

k8s-bot commented Nov 11, 2015

Choose a reason for hiding this comment

k8s-bot commented Nov 13, 2015

wojtek-t commented Nov 25, 2015

caesarxuchao commented Nov 25, 2015

k8s-bot commented Nov 25, 2015

lavalamp commented Nov 30, 2015

k8s-github-robot commented Nov 30, 2015

k8s-bot commented Nov 30, 2015

wojtek-t commented Nov 30, 2015

k8s-github-robot commented Dec 1, 2015

k8s-bot commented Dec 1, 2015

k8s-github-robot commented Dec 1, 2015

caesarxuchao commented Dec 1, 2015

caesarxuchao commented Dec 1, 2015

ixdy commented Dec 1, 2015

eparis commented Dec 1, 2015

eparis commented Dec 1, 2015