Avoid double decoding all client responses #36001

smarterclayton · 2016-11-01T20:44:03Z

The linked issue uncovered that we were always double decoding the response in restclient for get, list, update, create, and patch. That's fairly expensive, most especially for list. This PR refines the behavior of the rest client to avoid double decoding, and does so while minimizing the changes to rest client consumers.

restclient must be able to deal with multiple types of servers. Alter the behavior of restclient.Result#Raw() to not process the body on error, but instead to return the generic error (which still matches the error checking cases in api/error like IsBadRequest). If the caller uses
.Error(), .Into(), or .Get(), try decoding the body as a Status.

For older servers, continue to default apiVersion "v1" when calling restclient.Result#Error(). This was only for 1.1 servers and the extensions group, which we have since fixed.

This removes a double decode of very large objects (like LIST) - we were trying to DecodeInto status, but that ends up decoding the entire result and then throwing it away. This makes the decode behavior specific to the type of action the user wants.

The error handling behavior of `pkg/client/restclient.Result` has changed.  Calls to `Result.Raw()` will no longer parse the body, although they will still return errors that react to `pkg/api/errors.Is*()` as in previous releases.  Callers of `Get()` and `Into()` will continue to receive errors that are parsed from the body if the kind and apiVersion of the body match the `Status` object.

This more closely aligns rest client as a generic RESTful client, while preserving the special Kube API extended error handling for the `Get` and `Into` methods (which most Kube clients use).

This change is

smarterclayton · 2016-11-01T20:45:13Z

@kubernetes/sig-api-machinery @wojtek-t this doubles client read performance (even versioned clients).

smarterclayton · 2016-11-01T21:06:42Z

I added a release note that describes how error handling has changed - all calls to errors.IsNotFound/* should be the same for Raw, but the body will no longer be parsed unless clients explicitly ask for it. I did a quick review of all the places where we call Raw and didn't find any places that could be broken by this (/healthz, /version, /metrics, /swagger, and test metrics loads)

k8s-ci-robot · 2016-11-01T21:20:31Z

Jenkins GCE e2e failed for commit 88ef1a8b5bd2fbe229aee818cc2addcbab084b8e. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-11-01T21:21:20Z

Jenkins GCI GCE e2e failed for commit 88ef1a8b5bd2fbe229aee818cc2addcbab084b8e. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-11-01T21:51:58Z

Jenkins unit/integration failed for commit 4836d5ed3baa2edf979b113a7803444f929944b5. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

caesarxuchao · 2016-11-01T21:47:33Z

pkg/client/restclient/request.go

 // See the Request.Do() comment for what errors you might get.
 func (r Result) Error() error {
+	// if we have received an unexpected server error, and we have a body and decoder, we can try to extract
+	// a Status object.
+	if r.err == nil || !errors.IsUnexpectedServerError(r.err) || len(r.body) == 0 || r.decoder == nil {


Why IsUnexpectedServerError? I think we should just check if r.err implements APIStatus interface.

No, the error in this case is set by the thing that creates Result (which calls newGeneric...Error, which is UnexpectedServerError).

UnexpectedServerError literally means - we got an error from the server but we couldn't / didn't decode the body. So this code is looking for errors where before we didn't try to decode it, and try to decode it.

Ok, the logic is correct. The name UnexpectedServerError is misleading IMO, the name makes sense for the "couldn't decode the body" case, but doesn't make sense for the "didn't decode the body" case.

caesarxuchao · 2016-11-01T21:53:29Z

pkg/client/restclient/request_test.go

+			},
+		},
+		{
+			// successful status is ignored


When will this happen in real life?

Delete returns a status object, as do a few others.

caesarxuchao · 2016-11-01T22:29:01Z

LGTM. @deads2k if you want to take another look.

smarterclayton · 2016-11-02T00:36:22Z

Found a bug - authorization didn't register Status. Is authorization new for 1.5 or did we also fail to register it in 1.4?

liggitt · 2016-11-02T00:40:33Z

Was in 1.4

smarterclayton · 2016-11-02T00:41:48Z

I'm going to fix it in 1.5 but that means 1.4 clients can't get Status messages from subject access review. Was it alpha?

smarterclayton · 2016-11-02T01:41:12Z

Ok, that was fun.

ExportOptions wasn't registered in all groups - fixed that.
Added a test to verify ExportOptions was in all groups and could decode / encode correctly
Authorization wasn't registered in testapi
Found a bug with how we treat Status - it's projected into all groups as v1, when it really should be the group version. If you called runtime.ConvertToVersion with "your group version" it would fail to convert a v1.Status response into unversioned.Status. Added a case to convertToVersion to bandaid over this until I add the server api group version and rip out unversioned.

Ready for re-review. No backwards compatibility impact now.

k8s-ci-robot · 2016-11-02T02:01:51Z

Jenkins GCE Node e2e failed for commit 40220c917efec57c4384f4c4850d6f76b666bd5c. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-11-02T02:04:50Z

Jenkins verification failed for commit 40220c917efec57c4384f4c4850d6f76b666bd5c. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

restclient must be able to deal with multiple types of servers. Alter the behavior of restclient.Result#Raw() to not process the body on error, but instead to return the generic error (which still matches the error checking cases in api/error like IsBadRequest). If the caller uses .Error(), .Into(), or .Get(), try decoding the body as a Status. For older servers, continue to default apiVersion "v1" when calling restclient.Result#Error(). This was only for 1.1 servers and the extensions group, which we have since fixed. This removes a double decode of very large objects (like LIST).

Status is exposed as v1 in the current schema (so all groups are returning v1.Status). However, if you give a codec only "mygroup" "myversion", it will fail to convert Status to v1. For now, unversioned types should be allowed to be projected into all group versions, and when we add the server group we'll rip out the unversioned concept entirely.

deads2k · 2016-11-02T11:53:11Z

pkg/api/testapi/testapi.go

@@ -279,6 +290,7 @@ func init() {
 	Rbac = Groups[rbac.GroupName]
 	Storage = Groups[storage.GroupName]
 	ImagePolicy = Groups[imagepolicy.GroupName]
+	Authorization = Groups[authorization.GroupName]


If nobody uses this, don't add it. We want less code to touch for each group you add, not more.

deads2k · 2016-11-02T11:53:49Z

pkg/api/testapi/testapi.go

@@ -258,6 +260,15 @@ func init() {
 			externalTypes:        api.Scheme.KnownTypes(externalGroupVersion),
 		}
 	}
+	if _, ok := Groups[authorization.GroupName]; !ok {


It'd be nice to have a unit test somewhere that makes sure this parallel registration stays in sync.

deads2k · 2016-11-02T12:05:22Z

nits, lgtm.

smarterclayton · 2016-11-02T15:48:28Z

I need it to properly test authorization implements ExportOptions, which is
what this issue uncovered.

On Wed, Nov 2, 2016 at 7:53 AM, David Eads notifications@github.com wrote:

@deads2k commented on this pull request.

In pkg/api/testapi/testapi.go
#36001 (review)
:

@@ -279,6 +290,7 @@ func init() {
Rbac = Groups[rbac.GroupName]
Storage = Groups[storage.GroupName]
ImagePolicy = Groups[imagepolicy.GroupName]

Authorization = Groups[authorization.GroupName]

If nobody uses this, don't add it. We want less code to touch for each
group you add, not more.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#36001 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p89wRSx18Qsc9yIqaCAFfpyZ5kZBks5q6Hm2gaJpZM4Kmjnm
.

smarterclayton · 2016-11-02T15:49:49Z

I agree. Do you have an issue that covers your concerns with testapi that
I can add to?

On Wed, Nov 2, 2016 at 7:54 AM, David Eads notifications@github.com wrote:

@deads2k commented on this pull request.

In pkg/api/testapi/testapi.go
#36001 (review)
:

@@ -258,6 +260,15 @@ func init() {
externalTypes: api.Scheme.KnownTypes(externalGroupVersion),
}
}

if _, ok := Groups[authorization.GroupName]; !ok {

It'd be nice to have a unit test somewhere that makes sure this parallel
registration stays in sync.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#36001 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p3Ovqif8TFW6SW325KaNQWv3qq1wks5q6HncgaJpZM4Kmjnm
.

smarterclayton · 2016-11-02T15:49:59Z

As per comments

/lgtm

On Wed, Nov 2, 2016 at 11:48 AM, Clayton Coleman ccoleman@redhat.com
wrote:

I agree. Do you have an issue that covers your concerns with testapi that
I can add to?

On Wed, Nov 2, 2016 at 7:54 AM, David Eads notifications@github.com
wrote:

@deads2k commented on this pull request.

In pkg/api/testapi/testapi.go
#36001 (review)
:

@@ -258,6 +260,15 @@ func init() {
externalTypes: api.Scheme.KnownTypes(externalGroupVersion),
}
}

if _, ok := Groups[authorization.GroupName]; !ok {

It'd be nice to have a unit test somewhere that makes sure this parallel
registration stays in sync.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#36001 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p3Ovqif8TFW6SW325KaNQWv3qq1wks5q6HncgaJpZM4Kmjnm
.

deads2k · 2016-11-02T16:28:08Z

I agree. Do you have an issue that covers your concerns with testapi that
I can add to?

Old PR trying to remove the package entirely: #34644 . I'm still not sure that I'm convinced of the value it adds. I thought new groups would be able to ignore the package, so I didn't press on it.

caesarxuchao · 2016-11-02T17:52:22Z

ExportOptions wasn't registered in all groups - fixed that.

Can you add ExportOptions to this line to test it's registered in all groups?

caesarxuchao · 2016-11-02T17:52:48Z

Sorry, I saw you did that :)

caesarxuchao · 2016-11-02T17:53:28Z

Fix #27886.

k8s-github-robot · 2016-11-02T18:36:41Z

Automatic merge from submit-queue

googlebot added the cla: yes label Nov 1, 2016

smarterclayton added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. labels Nov 1, 2016

smarterclayton added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Nov 1, 2016

k8s-github-robot assigned caesarxuchao Nov 1, 2016

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 1, 2016

smarterclayton force-pushed the change_double_decode branch from 88ef1a8 to 4836d5e Compare November 1, 2016 21:24

caesarxuchao reviewed Nov 1, 2016

View reviewed changes

smarterclayton force-pushed the change_double_decode branch from 4836d5e to 40220c9 Compare November 2, 2016 01:38

k8s-github-robot added the kind/new-api label Nov 2, 2016

smarterclayton added 5 commits November 1, 2016 22:55

ExportOptions should be registered to all public schemas

f0fa26b

Improve the common kinds API test to catch more errors

8987019

Authorization should be registered in testapi

95116c7

smarterclayton force-pushed the change_double_decode branch from 40220c9 to 0513826 Compare November 2, 2016 02:56

deads2k reviewed Nov 2, 2016

View reviewed changes

smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2016

k8s-github-robot merged commit 7d14b56 into kubernetes:master Nov 2, 2016

chentao1596 mentioned this pull request Dec 5, 2016

WIP:kubelet: support multi-headers when getting pod from HTTP source #38089

Closed

pacoxu mentioned this pull request Dec 14, 2020

this is wrong, but legacy clients expect it pacoxu/kubernetes#9

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid double decoding all client responses #36001

Avoid double decoding all client responses #36001

smarterclayton commented Nov 1, 2016 •

edited

Loading

smarterclayton commented Nov 1, 2016

smarterclayton commented Nov 1, 2016

k8s-ci-robot commented Nov 1, 2016

k8s-ci-robot commented Nov 1, 2016

k8s-ci-robot commented Nov 1, 2016

caesarxuchao Nov 1, 2016

smarterclayton Nov 1, 2016

caesarxuchao Nov 1, 2016

caesarxuchao Nov 1, 2016

smarterclayton Nov 1, 2016

caesarxuchao commented Nov 1, 2016

smarterclayton commented Nov 2, 2016

liggitt commented Nov 2, 2016

smarterclayton commented Nov 2, 2016

smarterclayton commented Nov 2, 2016

k8s-ci-robot commented Nov 2, 2016

k8s-ci-robot commented Nov 2, 2016

deads2k Nov 2, 2016

deads2k Nov 2, 2016

deads2k commented Nov 2, 2016

smarterclayton commented Nov 2, 2016

@deads2k commented on this pull request.

smarterclayton commented Nov 2, 2016

@deads2k commented on this pull request.

smarterclayton commented Nov 2, 2016

@deads2k commented on this pull request.

deads2k commented Nov 2, 2016

caesarxuchao commented Nov 2, 2016

caesarxuchao commented Nov 2, 2016

caesarxuchao commented Nov 2, 2016

k8s-github-robot commented Nov 2, 2016

Avoid double decoding all client responses #36001

Avoid double decoding all client responses #36001

Conversation

smarterclayton commented Nov 1, 2016 • edited Loading

smarterclayton commented Nov 1, 2016

smarterclayton commented Nov 1, 2016

k8s-ci-robot commented Nov 1, 2016

k8s-ci-robot commented Nov 1, 2016

k8s-ci-robot commented Nov 1, 2016

caesarxuchao Nov 1, 2016

Choose a reason for hiding this comment

smarterclayton Nov 1, 2016

Choose a reason for hiding this comment

caesarxuchao Nov 1, 2016

Choose a reason for hiding this comment

caesarxuchao Nov 1, 2016

Choose a reason for hiding this comment

smarterclayton Nov 1, 2016

Choose a reason for hiding this comment

caesarxuchao commented Nov 1, 2016

smarterclayton commented Nov 2, 2016

liggitt commented Nov 2, 2016

smarterclayton commented Nov 2, 2016

smarterclayton commented Nov 2, 2016

k8s-ci-robot commented Nov 2, 2016

k8s-ci-robot commented Nov 2, 2016

deads2k Nov 2, 2016

Choose a reason for hiding this comment

deads2k Nov 2, 2016

Choose a reason for hiding this comment

deads2k commented Nov 2, 2016

smarterclayton commented Nov 2, 2016

@deads2k commented on this pull request.

smarterclayton commented Nov 2, 2016

@deads2k commented on this pull request.

smarterclayton commented Nov 2, 2016

@deads2k commented on this pull request.

deads2k commented Nov 2, 2016

caesarxuchao commented Nov 2, 2016

caesarxuchao commented Nov 2, 2016

caesarxuchao commented Nov 2, 2016

k8s-github-robot commented Nov 2, 2016

smarterclayton commented Nov 1, 2016 •

edited

Loading