[GarbageCollector] let dynamic client handle non-registered ListOptions #27733

caesarxuchao · 2016-06-20T23:09:03Z

And register v1.ListOptions in the policy group.

This change is

caesarxuchao · 2016-06-20T23:12:57Z

pkg/client/typed/dynamic/client.go

@@ -270,3 +270,23 @@ func (parameterCodec) DecodeParameters(parameters url.Values, from unversioned.G
 }

 var defaultParameterEncoder runtime.ParameterCodec = parameterCodec{}
+
+type versionedParameterEncoderWithV1Fallback struct{}


Registering v1.ListOptions in the Policy group will fix #27622. But for thirdparty resources, we still need this versionedParameterEncoderWithV1Fallback codec.

For the record, API server always treats query parameters sent to a thirdparty resource endpoint as v1: https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/thirdpartyresourcedata/codec.go#L557

Add as comment: "But for thirdparty resources, we still need this versionedParameterEncoderWithV1Fallback codec. For the record, currently API server always treats query parameters sent to a thirdparty resource endpoint as v1"

Added to the comment on VersionedParameterEncoderWithV1Fallback

smarterclayton · 2016-06-21T01:06:18Z

pkg/apis/policy/v1alpha1/register.go

@@ -41,6 +42,7 @@ func addKnownTypes(scheme *runtime.Scheme) {
 	scheme.AddKnownTypes(SchemeGroupVersion,
 		&PodDisruptionBudget{},
 		&PodDisruptionBudgetList{},
+		&v1.ListOptions{},


What about DeleteOptions? And ExportOptions?

DeleteOptions makes sense. I'm not sure about ExportOptions. It's always treated as "v1": https://github.com/kubernetes/kubernetes/blob/master/pkg/apiserver/resthandler.go#L135, though that code might be wrong.

Added DeleteOptions but not the ExportOptions.

Wow, that code is wrong.

On Tue, Jun 21, 2016 at 12:18 AM, Chao Xu notifications@github.com wrote:

In pkg/apis/policy/v1alpha1/register.go
#27733 (comment)
:

@@ -41,6 +42,7 @@ func addKnownTypes(scheme *runtime.Scheme) {
scheme.AddKnownTypes(SchemeGroupVersion,
&PodDisruptionBudget{},
&PodDisruptionBudgetList{},

&v1.ListOptions{},

DeleteOptions makes sense. I'm not sure about ExportOptions. It's always
treated as "v1":
https://github.com/kubernetes/kubernetes/blob/master/pkg/apiserver/resthandler.go#L135,
though that code might be wrong.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/kubernetes/pull/27733/files/1c74f17935c2b2cd29db72eda4f7a271b9d3bf24#r67805956,
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p3syUQTAoe1yPQ_6qu4t5C1959qIks5qN2YNgaJpZM4I6N5r
.

ExportOptions is registered as unversioned type: https://github.com/kubernetes/kubernetes/blob/master/pkg/api/register.go#L108.

We should register it as an ordinary type. @smarterclayton Are you fine if I do that in another PR? Moving ExportOptions from unversioned/ to a api/ is an API change.

We can do it in another issue - it's no less broken then (everything in an
API is versioned, even if we haven't explicitly created a second versioned
yet). Said another way, unversioned is something we haven't correctly
versioned yet (a bug).

It's not really an API change - the schema accepted is a version, so it's
really just an internal error that each group version doesn't explicitly
say what schema of export options they support.

On Tue, Jun 21, 2016 at 4:16 PM, Chao Xu notifications@github.com wrote:

In pkg/apis/policy/v1alpha1/register.go
#27733 (comment)
:

@@ -41,6 +42,7 @@ func addKnownTypes(scheme *runtime.Scheme) {
scheme.AddKnownTypes(SchemeGroupVersion,
&PodDisruptionBudget{},
&PodDisruptionBudgetList{},

&v1.ListOptions{},

ExportOptions is registered as unversioned type:
https://github.com/kubernetes/kubernetes/blob/master/pkg/api/register.go#L108
.

We should register it as an ordinary type. @smarterclayton
https://github.com/smarterclayton Are you fine if I do that in another
PR? Moving ExportOptions from unversioned/ to a api/ is an API change.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/kubernetes/pull/27733/files/1c74f17935c2b2cd29db72eda4f7a271b9d3bf24#r67943081,
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p_9PwWxieo6bTCjo3x8TS_UAqHvEks5qOEaCgaJpZM4I6N5r
.

Tracked in #27886

caesarxuchao · 2016-06-21T16:54:16Z

@k8s-bot node e2e test this, issue #27779

lavalamp · 2016-06-22T17:32:12Z

pkg/controller/garbagecollector/garbagecollector.go

-			// TODO: api.ParameterCodec doesn't support thirdparty objects.
-			// We need a generic parameter codec.
-			return client.ParameterCodec(api.ParameterCodec).Resource(&apiResource, api.NamespaceAll).List(&options)
+			return client.ParameterCodec(dynamic.VersionedParameterEncoderWithV1Fallback).Resource(&apiResource, api.NamespaceAll).List(&options)


nit: line breaks make stuff like this easier to read.

lavalamp · 2016-06-22T17:33:43Z

LGTM-- please add the comment and apply the label yourself.

caesarxuchao · 2016-06-22T17:47:28Z

Comments addressed. Self applied the label. Thank you both for the review.

goltermann · 2016-06-22T20:16:02Z

@caesarxuchao looks like the build failed? Can you take a look?

FAIL k8s.io/kubernetes/pkg/api 24.789s

caesarxuchao · 2016-06-22T20:17:39Z

@goltermann thanks for the reminder!

register ListOptions for apis/policy

caesarxuchao · 2016-06-22T20:19:44Z

Registered v1.DeleteOptions in batch/v2alpha1, that should fix the test. Reapplying the lgtm.

k8s-bot · 2016-06-22T20:55:04Z

GCE e2e build/test passed for commit d9f0792.

k8s-github-robot · 2016-06-22T23:56:35Z

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

k8s-bot · 2016-06-23T00:36:06Z

GCE e2e build/test passed for commit d9f0792.

k8s-github-robot · 2016-06-23T00:36:16Z

Automatic merge from submit-queue

caesarxuchao added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jun 20, 2016

googlebot added the cla: yes label Jun 20, 2016

caesarxuchao reviewed Jun 20, 2016
View reviewed changes

k8s-github-robot assigned derekwaynecarr Jun 20, 2016

k8s-github-robot added kind/new-api size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 20, 2016

caesarxuchao assigned lavalamp and unassigned derekwaynecarr Jun 21, 2016

smarterclayton reviewed Jun 21, 2016
View reviewed changes

caesarxuchao force-pushed the gc-parametercodec branch from 1c74f17 to 7fe88cf Compare June 21, 2016 04:40

lavalamp reviewed Jun 22, 2016
View reviewed changes

lavalamp added this to the v1.3 milestone Jun 22, 2016

lavalamp added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 22, 2016

caesarxuchao mentioned this pull request Jun 22, 2016

Move ExportOptions from unversioned package to api package #27886

Closed

caesarxuchao force-pushed the gc-parametercodec branch from 7fe88cf to 94dfd07 Compare June 22, 2016 17:46

caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 22, 2016

let dynamic client handle non-registered ListOptions;

d9f0792

register ListOptions for apis/policy

caesarxuchao force-pushed the gc-parametercodec branch from 94dfd07 to d9f0792 Compare June 22, 2016 20:19

caesarxuchao added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 22, 2016

k8s-github-robot merged commit d363759 into kubernetes:master Jun 23, 2016

caesarxuchao changed the title ~~let dynamic client handle non-registered ListOptions~~ [GarbageCollector] let dynamic client handle non-registered ListOptions Aug 15, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GarbageCollector] let dynamic client handle non-registered ListOptions #27733

[GarbageCollector] let dynamic client handle non-registered ListOptions #27733

caesarxuchao commented Jun 20, 2016 •

edited by k8s-oncall

Loading

caesarxuchao Jun 20, 2016

lavalamp Jun 22, 2016

caesarxuchao Jun 22, 2016

smarterclayton Jun 21, 2016

caesarxuchao Jun 21, 2016

caesarxuchao Jun 21, 2016

smarterclayton Jun 21, 2016

caesarxuchao Jun 21, 2016

smarterclayton Jun 21, 2016

caesarxuchao Jun 22, 2016

caesarxuchao commented Jun 21, 2016

lavalamp Jun 22, 2016

caesarxuchao Jun 22, 2016

lavalamp commented Jun 22, 2016

caesarxuchao commented Jun 22, 2016

goltermann commented Jun 22, 2016

caesarxuchao commented Jun 22, 2016

caesarxuchao commented Jun 22, 2016

k8s-bot commented Jun 22, 2016

k8s-github-robot commented Jun 22, 2016

k8s-bot commented Jun 23, 2016

k8s-github-robot commented Jun 23, 2016

[GarbageCollector] let dynamic client handle non-registered ListOptions #27733

[GarbageCollector] let dynamic client handle non-registered ListOptions #27733

Conversation

caesarxuchao commented Jun 20, 2016 • edited by k8s-oncall Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented Jun 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Jun 22, 2016

caesarxuchao commented Jun 22, 2016

goltermann commented Jun 22, 2016

caesarxuchao commented Jun 22, 2016

caesarxuchao commented Jun 22, 2016

k8s-bot commented Jun 22, 2016

k8s-github-robot commented Jun 22, 2016

k8s-bot commented Jun 23, 2016

k8s-github-robot commented Jun 23, 2016

caesarxuchao commented Jun 20, 2016 •

edited by k8s-oncall

Loading