Return api/errors instead of raw etcd errors #10246

nikhiljindal · 2015-06-23T18:48:13Z

For #10064

Wrapping etcd errors in errors.NewInternalError(err)

nikhiljindal · 2015-06-23T18:48:20Z

cc @bgrant0607 @lavalamp

k8s-bot · 2015-06-23T19:05:35Z

GCE e2e build/test failed for commit d3eca56f71dc15684bce5f3fab767dfd99b8d659.

k8s-bot · 2015-06-23T21:35:19Z

GCE e2e build/test passed for commit a43f8b3c68b0bd45551fd40be5d658bcfc040894.

k8s-bot · 2015-06-24T00:31:35Z

GCE e2e build/test failed for commit 5a29a715e117630079e90487f7ae5d5e1ded4dd9.

k8s-bot · 2015-06-24T01:05:21Z

GCE e2e build/test failed for commit fa1f8d7aba7777d5e456af6feed93b77a59b3ca7.

k8s-bot · 2015-06-24T18:33:53Z

GCE e2e build/test failed for commit f609e8f6762ca581b6478e6db4dcf5f25166775d.

nikhiljindal · 2015-06-25T20:34:57Z

Adding to v1 since the associated bug is marked v1.

k8s-bot · 2015-06-25T21:04:07Z

GCE e2e build/test failed for commit f9eaf959d03e1570ddcc6f95c749bff0bfd063ea.

lavalamp · 2015-06-25T21:07:01Z

pkg/registry/service/allocator/etcd/etcd.go

@@ -31,7 +30,7 @@ import (
 )

 var (
-	errorUnableToAllocate = errors.New("unable to allocate")
+	errorUnableToAllocate = k8serr.NewInternalError(fmt.Errorf("unable to allocate"))


Prefer errors.New if you don't have parameters to pass.

It will still be converted to an InternalError, even if we do not generate one here.
The advantage of generating an InternalError here is that we can then compare that the returned error == errorUnableToAllocate.
Otherwise, we will have to do
error.Error() == NewInternalError(errorUnableToAllocate).Error()

Please add a comment that this error shouldn't ever be returned to the user.

k8s-bot · 2015-06-25T21:58:36Z

GCE e2e build/test failed for commit f9eaf959d03e1570ddcc6f95c749bff0bfd063ea.

k8s-bot · 2015-06-25T23:13:34Z

GCE e2e build/test failed for commit f9eaf959d03e1570ddcc6f95c749bff0bfd063ea.

nikhiljindal · 2015-06-29T23:25:32Z

Rebased and updated code as per comments.

k8s-bot · 2015-06-29T23:34:47Z

GCE e2e build/test failed for commit 26d87b37379626554171907c32d53f58ad6aaabc.

k8s-bot · 2015-06-29T23:36:03Z

GCE e2e build/test passed for commit 4b7f7ce.

bgrant0607 · 2015-06-29T23:38:23Z

Good enough for now. LGTM.

bgrant0607 · 2015-06-29T23:38:45Z

Re-assigned to @lavalamp for another LGTM

lavalamp · 2015-06-30T18:52:40Z

lgtm

Return api/errors instead of raw etcd errors

smarterclayton · 2015-07-01T00:47:02Z

I missed the review of this but this is a pretty substantial change to the intent of the etcd methods - those were transform only. This change prevents other code from further refining those errors. I would have been a -1 on this - we should be handling non API errors higher in the stack, or via a different method. We use this method to filter out known errors, not wrap all errors.

nikhiljindal · 2015-07-01T01:28:41Z

Sorry about that @smarterclayton I thought that I had cc'd you on this but looks like I didnt.
Does this break Openshift? Do you want me to revert the PR?

Can you give an example where other code wants to refine the etcd error? Are you talking about refining an etcd error into another etcd error?
Since the InterpretError functions were already converting etcd errors to apierrors, it seemed that thats the intent of those functions. If thats not the case, then we can introduce a new EtcdToApiError() function, that should be called just before returning from the registry functions (ex Update function here: https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/registry/generic/etcd/etcd.go#L267). This function will ensure that we are not returning raw etcd errors above the registry.
The etcd errors can be refined before this function is called. InterpretError functions will not convert etcd errors to api errors.

Does that sound fine?

bgrant0607 · 2015-07-01T02:38:10Z

Let's revert and figure out the right solution.

smarterclayton · 2015-07-01T03:09:55Z

I don't know that we need to revert right now.

Basically, the original idea was that those methods would filter known
errors into a transformed error - otherwise it would leave the error
alone. This would allow you to safely wrap etcd calls with the method, but
if you had a method that called etcd and also returned non etcd errors
(like a typed error from etcd helper that was ErrStopped or
ErrExplicitQuit) you could invoke InterpretError first, then check for
those typed errors. My concern would be any code that made that assumption
in Kube, as well as OpenShift. Let me do a quick scan - in the event that
we were not doing that anymore, I think this change is probably safe - if
in the future, we need to do that, we dan check for typed errors first.

At one point we called this method before a switch somewhere (switch
InterpretEtcdErr(err)) but that may have gone away as we refactored the
etcd libraries.

On Jun 30, 2015, at 9:28 PM, Nikhil Jindal notifications@github.com wrote:

Sorry about that @smarterclayton https://github.com/smarterclayton I
tought that I had cc'd you on this but looks like I didnt.
Does this break Openshift? Do you want me to revert the PR?

Can you give an example where other code wants to refine the etcd error?
Are you talking about refining an etcd error into another etcd error?
Since the Interpret
_Error functions were already converting etcd errors to apierrors, it
seemed that thats the intent of those functions. If thats not the case,
then we can introduce a new EtcdToApiError() function, that should be
called just before returning from the registry functions (ex Update
function here:
https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/registry/generic/etcd/etcd.go#L267
https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/registry/generic/etcd/etcd.go#L267).
This function will ensure that we are not returning raw etcd errors above
the registry. The etcd errors can be refined before this function is
called. Interpret_Error functions will not convert etcd errors to api
errors.

Does that sound fine?

—
Reply to this email directly or view it on GitHub
#10246 (comment)
.

bgrant0607 · 2015-07-01T05:04:41Z

We should document that, and ideally test it. It was non-obvious.

smarterclayton · 2015-07-01T14:11:35Z

It's broken in BindingREST assignPod:

err = etcderr.InterpretGetError(err, "pod", podID)
err = etcderr.InterpretUpdateError(err, "pod", podID)

and in generic/etcd Create():

err = etcderr.InterpretCreateError(err, e.EndpointName, name)
err = rest.CheckGeneratedNameError(e.CreateStrategy, err, obj)

On Wed, Jul 1, 2015 at 1:04 AM, Brian Grant notifications@github.com
wrote:

We should document that, and ideally test it. It was non-obvious.

—
Reply to this email directly or view it on GitHub
#10246 (comment)
.

Clayton Coleman | Lead Engineer, OpenShift

smarterclayton · 2015-07-01T14:12:19Z

Also in generic/etcd Update():

if creating {
err = etcderr.InterpretCreateError(err, e.EndpointName, name)
err = rest.CheckGeneratedNameError(e.CreateStrategy, err, obj)
} else {
err = etcderr.InterpretUpdateError(err, e.EndpointName, name)
}

On Wed, Jul 1, 2015 at 10:11 AM, Clayton Coleman ccoleman@redhat.com
wrote:

It's broken in BindingREST assignPod:
err = etcderr.InterpretGetError(err, "pod", podID)
err = etcderr.InterpretUpdateError(err, "pod", podID)
and in generic/etcd Create():
err = etcderr.InterpretCreateError(err, e.EndpointName, name)
err = rest.CheckGeneratedNameError(e.CreateStrategy, err, obj)
On Wed, Jul 1, 2015 at 1:04 AM, Brian Grant notifications@github.com
wrote:

We should document that, and ideally test it. It was non-obvious.

—
Reply to this email directly or view it on GitHub
#10246 (comment)
.

Clayton Coleman | Lead Engineer, OpenShift

Clayton Coleman | Lead Engineer, OpenShift

smarterclayton · 2015-07-01T14:12:55Z

I don't see any other obvious uses of it in Kube.

On Wed, Jul 1, 2015 at 10:12 AM, Clayton Coleman ccoleman@redhat.com
wrote:

Also in generic/etcd Update():
if creating {
err = etcderr.InterpretCreateError(err, e.EndpointName, name)
err = rest.CheckGeneratedNameError(e.CreateStrategy, err, obj)
} else {
err = etcderr.InterpretUpdateError(err, e.EndpointName, name)
}
On Wed, Jul 1, 2015 at 10:11 AM, Clayton Coleman ccoleman@redhat.com
wrote:
It's broken in BindingREST assignPod:
err = etcderr.InterpretGetError(err, "pod", podID)
err = etcderr.InterpretUpdateError(err, "pod", podID)
and in generic/etcd Create():
err = etcderr.InterpretCreateError(err, e.EndpointName, name)
err = rest.CheckGeneratedNameError(e.CreateStrategy, err, obj)
On Wed, Jul 1, 2015 at 1:04 AM, Brian Grant notifications@github.com
wrote:

We should document that, and ideally test it. It was non-obvious.

—
Reply to this email directly or view it on GitHub
#10246 (comment)
.

Clayton Coleman | Lead Engineer, OpenShift
Clayton Coleman | Lead Engineer, OpenShift

Clayton Coleman | Lead Engineer, OpenShift

smarterclayton · 2015-07-01T14:13:40Z

I do think it's valuable for there to be a "turn etcd error into api error
for the given method type" - I'd like to have it be clear there is a catch
all.

On Wed, Jul 1, 2015 at 10:12 AM, Clayton Coleman ccoleman@redhat.com
wrote:

I don't see any other obvious uses of it in Kube.

On Wed, Jul 1, 2015 at 10:12 AM, Clayton Coleman ccoleman@redhat.com
wrote:
Also in generic/etcd Update():
if creating {
err = etcderr.InterpretCreateError(err, e.EndpointName, name)
err = rest.CheckGeneratedNameError(e.CreateStrategy, err, obj)
} else {
err = etcderr.InterpretUpdateError(err, e.EndpointName, name)
}
On Wed, Jul 1, 2015 at 10:11 AM, Clayton Coleman ccoleman@redhat.com
wrote:
It's broken in BindingREST assignPod:
err = etcderr.InterpretGetError(err, "pod", podID)
err = etcderr.InterpretUpdateError(err, "pod", podID)
and in generic/etcd Create():
err = etcderr.InterpretCreateError(err, e.EndpointName, name)
err = rest.CheckGeneratedNameError(e.CreateStrategy, err, obj)
On Wed, Jul 1, 2015 at 1:04 AM, Brian Grant notifications@github.com
wrote:

We should document that, and ideally test it. It was non-obvious.

—
Reply to this email directly or view it on GitHub
#10246 (comment)
.

Clayton Coleman | Lead Engineer, OpenShift
Clayton Coleman | Lead Engineer, OpenShift
Clayton Coleman | Lead Engineer, OpenShift

Clayton Coleman | Lead Engineer, OpenShift

This reverts commit a902a2f, reversing changes made to 7df8d76.

nikhiljindal · 2015-07-01T18:11:37Z

Thanks for those pointers @smarterclayton.
I am reverting this PR in #10632. Will send another PR with better changes.

Revert "Merge pull request #10246 from nikhiljindal"

googlebot added the cla: yes label Jun 23, 2015

nikhiljindal closed this Jun 23, 2015

nikhiljindal reopened this Jun 23, 2015

nikhiljindal force-pushed the minionPrefix branch from d3eca56 to a43f8b3 Compare June 23, 2015 21:17

nikhiljindal force-pushed the minionPrefix branch from a43f8b3 to 5a29a71 Compare June 24, 2015 00:30

nikhiljindal force-pushed the minionPrefix branch from 5a29a71 to fa1f8d7 Compare June 24, 2015 00:35

j3ffml added this to the v1.0-candidate milestone Jun 24, 2015

j3ffml added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jun 24, 2015

j3ffml assigned bgrant0607 Jun 24, 2015

nikhiljindal mentioned this pull request Jun 24, 2015

Returning api/errors from event/rest.go #10269

Merged

nikhiljindal force-pushed the minionPrefix branch from fa1f8d7 to f609e8f Compare June 24, 2015 18:12

nikhiljindal force-pushed the minionPrefix branch from f609e8f to f9eaf95 Compare June 25, 2015 20:30

nikhiljindal modified the milestones: v1.0, v1.0-candidate Jun 25, 2015

lavalamp reviewed Jun 25, 2015
View reviewed changes

ixdy mentioned this pull request Jun 25, 2015

GitHub authentication failures updating statuses after upgrading to 1.24 jenkinsci/ghprb-plugin#117

Closed

nikhiljindal force-pushed the minionPrefix branch from f9eaf95 to 6240623 Compare June 26, 2015 21:03

bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 29, 2015

bgrant0607 assigned lavalamp and unassigned bgrant0607 Jun 29, 2015

nikhiljindal closed this Jun 30, 2015

nikhiljindal reopened this Jun 30, 2015

lavalamp added the ok-to-merge label Jun 30, 2015

zmerlynn added a commit that referenced this pull request Jun 30, 2015

Merge pull request #10246 from nikhiljindal/minionPrefix

a902a2f

Return api/errors instead of raw etcd errors

zmerlynn merged commit a902a2f into kubernetes:master Jun 30, 2015

nikhiljindal mentioned this pull request Jul 1, 2015

Shouldnt be returning etcd errors #10064

Closed

nikhiljindal added a commit to nikhiljindal/kubernetes that referenced this pull request Jul 1, 2015

Revert "Merge pull request kubernetes#10246 from nikhiljindal"

d82d5d1

This reverts commit a902a2f, reversing changes made to 7df8d76.

nikhiljindal mentioned this pull request Jul 1, 2015

Revert "Merge pull request #10246 from nikhiljindal" #10632

Merged

zmerlynn added a commit that referenced this pull request Jul 1, 2015

Merge pull request #10632 from nikhiljindal/revert-etcderrors

9db58ff

Revert "Merge pull request #10246 from nikhiljindal"

nikhiljindal unassigned lavalamp Aug 12, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return api/errors instead of raw etcd errors #10246

Return api/errors instead of raw etcd errors #10246

nikhiljindal commented Jun 23, 2015

nikhiljindal commented Jun 23, 2015

k8s-bot commented Jun 23, 2015

k8s-bot commented Jun 23, 2015

k8s-bot commented Jun 24, 2015

k8s-bot commented Jun 24, 2015

k8s-bot commented Jun 24, 2015

nikhiljindal commented Jun 25, 2015

k8s-bot commented Jun 25, 2015

lavalamp Jun 25, 2015

nikhiljindal Jun 26, 2015

bgrant0607 Jun 29, 2015

nikhiljindal Jun 29, 2015

k8s-bot commented Jun 25, 2015

k8s-bot commented Jun 25, 2015

nikhiljindal commented Jun 29, 2015

k8s-bot commented Jun 29, 2015

k8s-bot commented Jun 29, 2015

bgrant0607 commented Jun 29, 2015

bgrant0607 commented Jun 29, 2015

lavalamp commented Jun 30, 2015

smarterclayton commented Jul 1, 2015

nikhiljindal commented Jul 1, 2015

bgrant0607 commented Jul 1, 2015

smarterclayton commented Jul 1, 2015

bgrant0607 commented Jul 1, 2015

smarterclayton commented Jul 1, 2015

smarterclayton commented Jul 1, 2015

smarterclayton commented Jul 1, 2015

smarterclayton commented Jul 1, 2015

nikhiljindal commented Jul 1, 2015

Return api/errors instead of raw etcd errors #10246

Return api/errors instead of raw etcd errors #10246

Conversation

nikhiljindal commented Jun 23, 2015

nikhiljindal commented Jun 23, 2015

k8s-bot commented Jun 23, 2015

k8s-bot commented Jun 23, 2015

k8s-bot commented Jun 24, 2015

k8s-bot commented Jun 24, 2015

k8s-bot commented Jun 24, 2015

nikhiljindal commented Jun 25, 2015

k8s-bot commented Jun 25, 2015

lavalamp Jun 25, 2015

Choose a reason for hiding this comment

nikhiljindal Jun 26, 2015

Choose a reason for hiding this comment

bgrant0607 Jun 29, 2015

Choose a reason for hiding this comment

nikhiljindal Jun 29, 2015

Choose a reason for hiding this comment

k8s-bot commented Jun 25, 2015

k8s-bot commented Jun 25, 2015

nikhiljindal commented Jun 29, 2015

k8s-bot commented Jun 29, 2015

k8s-bot commented Jun 29, 2015

bgrant0607 commented Jun 29, 2015

bgrant0607 commented Jun 29, 2015

lavalamp commented Jun 30, 2015

smarterclayton commented Jul 1, 2015

nikhiljindal commented Jul 1, 2015

bgrant0607 commented Jul 1, 2015

smarterclayton commented Jul 1, 2015

bgrant0607 commented Jul 1, 2015

smarterclayton commented Jul 1, 2015

smarterclayton commented Jul 1, 2015

smarterclayton commented Jul 1, 2015

smarterclayton commented Jul 1, 2015

nikhiljindal commented Jul 1, 2015