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.

Sign up for GitHub

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

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

Merged
merged 1 commit into from
Jun 23, 2016
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions pkg/api/serialization_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,20 @@ var nonRoundTrippableTypes = sets.NewString(
"WatchEvent",
)

var commonKinds = []string{"ListOptions", "DeleteOptions"}

// verify all external group/versions have the common kinds like the ListOptions, DeleteOptions are registered.
func TestCommonKindsRegistered(t *testing.T) {
for _, kind := range commonKinds {
for _, group := range testapi.Groups {
gv := group.GroupVersion()
if _, err := api.Scheme.New(gv.WithKind(kind)); err != nil {
t.Error(err)
}
}
}
}

var nonInternalRoundTrippableTypes = sets.NewString("List", "ListOptions", "ExportOptions")
var nonRoundTrippableTypesByVersion = map[string][]string{}

Expand Down
1 change: 1 addition & 0 deletions pkg/apis/apps/v1alpha1/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ func addKnownTypes(scheme *runtime.Scheme) {
&PetSet{},
&PetSetList{},
&v1.ListOptions{},
&v1.DeleteOptions{},
)
versionedwatch.AddToGroupVersion(scheme, SchemeGroupVersion)
}
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/autoscaling/v1/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ func addKnownTypes(scheme *runtime.Scheme) {
&HorizontalPodAutoscalerList{},
&Scale{},
&v1.ListOptions{},
&v1.DeleteOptions{},
)
versionedwatch.AddToGroupVersion(scheme, SchemeGroupVersion)
}
1 change: 1 addition & 0 deletions pkg/apis/batch/v1/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ func addKnownTypes(scheme *runtime.Scheme) {
&Job{},
&JobList{},
&v1.ListOptions{},
&v1.DeleteOptions{},
)
versionedwatch.AddToGroupVersion(scheme, SchemeGroupVersion)
}
1 change: 1 addition & 0 deletions pkg/apis/batch/v2alpha1/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ func addKnownTypes(scheme *runtime.Scheme) {
&ScheduledJob{},
&ScheduledJobList{},
&v1.ListOptions{},
&v1.DeleteOptions{},
)
versionedwatch.AddToGroupVersion(scheme, SchemeGroupVersion)
}
3 changes: 3 additions & 0 deletions pkg/apis/policy/v1alpha1/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package v1alpha1

import (
"k8s.io/kubernetes/pkg/api/unversioned"
"k8s.io/kubernetes/pkg/api/v1"
"k8s.io/kubernetes/pkg/runtime"
versionedwatch "k8s.io/kubernetes/pkg/watch/versioned"
)
Expand All @@ -41,6 +42,8 @@ func addKnownTypes(scheme *runtime.Scheme) {
scheme.AddKnownTypes(SchemeGroupVersion,
&PodDisruptionBudget{},
&PodDisruptionBudgetList{},
&v1.ListOptions{},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about DeleteOptions? And ExportOptions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added DeleteOptions but not the ExportOptions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracked in #27886

&v1.DeleteOptions{},
)
// Add the watch version that applies
versionedwatch.AddToGroupVersion(scheme, SchemeGroupVersion)
Expand Down
23 changes: 23 additions & 0 deletions pkg/client/typed/dynamic/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,3 +270,26 @@ func (parameterCodec) DecodeParameters(parameters url.Values, from unversioned.G
}

var defaultParameterEncoder runtime.ParameterCodec = parameterCodec{}

type versionedParameterEncoderWithV1Fallback struct{}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to the comment on VersionedParameterEncoderWithV1Fallback


func (versionedParameterEncoderWithV1Fallback) EncodeParameters(obj runtime.Object, to unversioned.GroupVersion) (url.Values, error) {
ret, err := api.ParameterCodec.EncodeParameters(obj, to)
if err != nil && runtime.IsNotRegisteredError(err) {
// fallback to v1
return api.ParameterCodec.EncodeParameters(obj, v1.SchemeGroupVersion)
}
return ret, err
}

func (versionedParameterEncoderWithV1Fallback) DecodeParameters(parameters url.Values, from unversioned.GroupVersion, into runtime.Object) error {
return errors.New("DecodeParameters not implemented on versionedParameterEncoderWithV1Fallback")
}

// VersionedParameterEncoderWithV1Fallback is useful for encoding query
// parameters for thirdparty resources. It tries to convert object to the
// specified version before converting it to query parameters, and falls back to
// converting to v1 if the object is not registered in the specified version.
// For the record, currently API server always treats query parameters sent to a
// thirdparty resource endpoint as v1.
var VersionedParameterEncoderWithV1Fallback runtime.ParameterCodec = versionedParameterEncoderWithV1Fallback{}
14 changes: 6 additions & 8 deletions pkg/controller/garbagecollector/garbagecollector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,21 +443,19 @@ func gcListWatcher(client *dynamic.Client, resource unversioned.GroupVersionReso
// namespaces if it's namespace scoped, so leave
// APIResource.Namespaced as false is all right.
apiResource := unversioned.APIResource{Name: resource.Resource}
// The default parameter codec used by the dynamic client cannot
// encode api.ListOptions.
// 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)
},
WatchFunc: func(options api.ListOptions) (watch.Interface, error) {
// APIResource.Kind is not used by the dynamic client, so
// leave it empty. We want to list this resource in all
// namespaces if it's namespace scoped, so leave
// APIResource.Namespaced as false is all right.
apiResource := unversioned.APIResource{Name: resource.Resource}
// The default parameter codec used by the dynamic client cannot
// encode api.ListOptions.
return client.ParameterCodec(api.ParameterCodec).Resource(&apiResource, api.NamespaceAll).Watch(&options)
return client.ParameterCodec(dynamic.VersionedParameterEncoderWithV1Fallback).
Resource(&apiResource, api.NamespaceAll).
Watch(&options)
},
}
}
Expand Down