From 06f49f7ca79989084058a31b6a6ac751ddf30064 Mon Sep 17 00:00:00 2001 From: Chao Xu Date: Sat, 28 May 2016 15:10:25 -0700 Subject: [PATCH] Let the dynamic client take a customized parameter codec for List, Watch, and DeleteCollection. Let the gc's ListWatcher use api.ParameterCodec. Fixes 25890. --- pkg/client/typed/dynamic/client.go | 52 ++++++++++++++++--- .../garbagecollector/garbagecollector.go | 50 +++++++++++------- .../garbagecollector/garbagecollector_test.go | 28 +++++++++- test/integration/garbage_collector_test.go | 3 -- 4 files changed, 102 insertions(+), 31 deletions(-) diff --git a/pkg/client/typed/dynamic/client.go b/pkg/client/typed/dynamic/client.go index 96700ac944a..3d0d7b6b663 100644 --- a/pkg/client/typed/dynamic/client.go +++ b/pkg/client/typed/dynamic/client.go @@ -42,6 +42,11 @@ type Client struct { cl *restclient.RESTClient } +type ClientWithParameterCodec struct { + client *Client + parameterCodec runtime.ParameterCodec +} + // NewClient returns a new client based on the passed in config. The // codec is ignored, as the dynamic client uses it's own codec. func NewClient(conf *restclient.Config) (*Client, error) { @@ -79,9 +84,9 @@ func NewClient(conf *restclient.Config) (*Client, error) { return &Client{cl: cl}, nil } -// Resource returns an API interface to the specified resource for -// this client's group and version. If resource is not a namespaced -// resource, then namespace is ignored. +// Resource returns an API interface to the specified resource for this client's +// group and version. If resource is not a namespaced resource, then namespace +// is ignored. func (c *Client) Resource(resource *unversioned.APIResource, namespace string) *ResourceClient { return &ResourceClient{ cl: c.cl, @@ -90,17 +95,42 @@ func (c *Client) Resource(resource *unversioned.APIResource, namespace string) * } } +// ParameterCodec wraps a parameterCodec around the Client. +func (c *Client) ParameterCodec(parameterCodec runtime.ParameterCodec) *ClientWithParameterCodec { + return &ClientWithParameterCodec{ + client: c, + parameterCodec: parameterCodec, + } +} + +// Resource returns an API interface to the specified resource for this client's +// group and version. If resource is not a namespaced resource, then namespace +// is ignored. The ResourceClient inherits the parameter codec of c. +func (c *ClientWithParameterCodec) Resource(resource *unversioned.APIResource, namespace string) *ResourceClient { + return &ResourceClient{ + cl: c.client.cl, + resource: resource, + ns: namespace, + parameterCodec: c.parameterCodec, + } +} + // ResourceClient is an API interface to a specific resource under a // dynamic client. type ResourceClient struct { - cl *restclient.RESTClient - resource *unversioned.APIResource - ns string + cl *restclient.RESTClient + resource *unversioned.APIResource + ns string + parameterCodec runtime.ParameterCodec } // List returns a list of objects for this resource. func (rc *ResourceClient) List(opts runtime.Object) (*runtime.UnstructuredList, error) { result := new(runtime.UnstructuredList) + parameterEncoder := rc.parameterCodec + if parameterEncoder == nil { + parameterEncoder = defaultParameterEncoder + } err := rc.cl.Get(). NamespaceIfScoped(rc.ns, rc.resource.Namespaced). Resource(rc.resource.Name). @@ -135,6 +165,10 @@ func (rc *ResourceClient) Delete(name string, opts *v1.DeleteOptions) error { // DeleteCollection deletes a collection of objects. func (rc *ResourceClient) DeleteCollection(deleteOptions *v1.DeleteOptions, listOptions runtime.Object) error { + parameterEncoder := rc.parameterCodec + if parameterEncoder == nil { + parameterEncoder = defaultParameterEncoder + } return rc.cl.Delete(). NamespaceIfScoped(rc.ns, rc.resource.Namespaced). Resource(rc.resource.Name). @@ -174,6 +208,10 @@ func (rc *ResourceClient) Update(obj *runtime.Unstructured) (*runtime.Unstructur // Watch returns a watch.Interface that watches the resource. func (rc *ResourceClient) Watch(opts runtime.Object) (watch.Interface, error) { + parameterEncoder := rc.parameterCodec + if parameterEncoder == nil { + parameterEncoder = defaultParameterEncoder + } return rc.cl.Get(). Prefix("watch"). NamespaceIfScoped(rc.ns, rc.resource.Namespaced). @@ -231,4 +269,4 @@ func (parameterCodec) DecodeParameters(parameters url.Values, from unversioned.G return errors.New("DecodeParameters not implemented on dynamic parameterCodec") } -var parameterEncoder runtime.ParameterCodec = parameterCodec{} +var defaultParameterEncoder runtime.ParameterCodec = parameterCodec{} diff --git a/pkg/controller/garbagecollector/garbagecollector.go b/pkg/controller/garbagecollector/garbagecollector.go index 91d1b228a23..23aae3fb3aa 100644 --- a/pkg/controller/garbagecollector/garbagecollector.go +++ b/pkg/controller/garbagecollector/garbagecollector.go @@ -433,6 +433,35 @@ type GarbageCollector struct { propagator *Propagator } +// TODO: make special List and Watch function that removes fields other than +// ObjectMeta. +func gcListWatcher(client *dynamic.Client, resource unversioned.GroupVersionResource) *cache.ListWatch { + return &cache.ListWatch{ + ListFunc: func(options api.ListOptions) (runtime.Object, 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. + // 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) + }, + 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) + }, + } +} + func monitorFor(p *Propagator, clientPool dynamic.ClientPool, resource unversioned.GroupVersionResource) (monitor, error) { // TODO: consider store in one storage. glog.V(6).Infof("create storage for resource %s", resource) @@ -442,26 +471,7 @@ func monitorFor(p *Propagator, clientPool dynamic.ClientPool, resource unversion return monitor, err } monitor.store, monitor.controller = framework.NewInformer( - // TODO: make special List and Watch function that removes fields other - // than ObjectMeta. - &cache.ListWatch{ - ListFunc: func(options api.ListOptions) (runtime.Object, 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} - return client.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} - return client.Resource(&apiResource, api.NamespaceAll).Watch(&options) - }, - }, + gcListWatcher(client, resource), nil, ResourceResyncTime, framework.ResourceEventHandlerFuncs{ diff --git a/pkg/controller/garbagecollector/garbagecollector_test.go b/pkg/controller/garbagecollector/garbagecollector_test.go index 6c044a90c60..617054439d3 100644 --- a/pkg/controller/garbagecollector/garbagecollector_test.go +++ b/pkg/controller/garbagecollector/garbagecollector_test.go @@ -52,6 +52,7 @@ func TestNewGarbageCollector(t *testing.T) { type fakeAction struct { method string path string + query string } // String returns method=path to aid in testing @@ -78,7 +79,7 @@ func (f *fakeActionHandler) ServeHTTP(response http.ResponseWriter, request *htt f.lock.Lock() defer f.lock.Unlock() - f.actions = append(f.actions, fakeAction{method: request.Method, path: request.URL.Path}) + f.actions = append(f.actions, fakeAction{method: request.Method, path: request.URL.Path, query: request.URL.RawQuery}) fakeResponse, ok := f.response[request.Method+request.URL.Path] if !ok { fakeResponse.statusCode = 200 @@ -317,3 +318,28 @@ func TestDependentsRace(t *testing.T) { } }() } + +// test the list and watch functions correctly converts the ListOptions +func TestGCListWatcher(t *testing.T) { + testHandler := &fakeActionHandler{} + srv, clientConfig := testServerAndClientConfig(testHandler.ServeHTTP) + defer srv.Close() + clientPool := dynamic.NewClientPool(clientConfig, dynamic.LegacyAPIPathResolverFunc) + podResource := unversioned.GroupVersionResource{Version: "v1", Resource: "pods"} + client, err := clientPool.ClientForGroupVersion(podResource.GroupVersion()) + if err != nil { + t.Fatal(err) + } + lw := gcListWatcher(client, podResource) + lw.Watch(api.ListOptions{ResourceVersion: "1"}) + lw.List(api.ListOptions{ResourceVersion: "1"}) + if e, a := 2, len(testHandler.actions); e != a { + t.Errorf("expect %d requests, got %d", e, a) + } + if e, a := "resourceVersion=1", testHandler.actions[0].query; e != a { + t.Errorf("expect %s, got %s", e, a) + } + if e, a := "resourceVersion=1", testHandler.actions[1].query; e != a { + t.Errorf("expect %s, got %s", e, a) + } +} diff --git a/test/integration/garbage_collector_test.go b/test/integration/garbage_collector_test.go index e0326641303..0c183ae2ffd 100644 --- a/test/integration/garbage_collector_test.go +++ b/test/integration/garbage_collector_test.go @@ -149,9 +149,6 @@ func setup(t *testing.T) (*garbagecollector.GarbageCollector, clientset.Interfac // This test simulates the cascading deletion. func TestCascadingDeletion(t *testing.T) { - // TODO: Figure out what's going on with this test! - t.Log("This test is failing too much-- lavalamp removed it to stop the submit queue bleeding") - return gc, clientSet := setup(t) oldEnableGarbageCollector := registry.EnableGarbageCollector registry.EnableGarbageCollector = true