make a simple dynamic client that is easy to use #62913

deads2k · 2018-04-20T15:55:39Z

The dynamic client has annoyed me for the last time! The existing one takes arguments at odd levels, requires lots of information to instantiate, does some weird pool thing, and uses unusual types. This creates an interface like this:

type DynamicInterface interface {
	ClusterResource(resource schema.GroupVersionResource) DynamicResourceInterface
	NamespacedResource(resource schema.GroupVersionResource, namespace string) DynamicResourceInterface
}

type DynamicResourceInterface interface {
	Create(obj *unstructured.Unstructured) (*unstructured.Unstructured, error)
	Update(obj *unstructured.Unstructured) (*unstructured.Unstructured, error)
	UpdateStatus(obj *unstructured.Unstructured) (*unstructured.Unstructured, error)
	Delete(name string, options *metav1.DeleteOptions) error
	DeleteCollection(options *metav1.DeleteOptions, listOptions metav1.ListOptions) error
	Get(name string, options metav1.GetOptions) (*unstructured.Unstructured, error)
	List(opts metav1.ListOptions) (*unstructured.UnstructuredList, error)
	Watch(opts metav1.ListOptions) (watch.Interface, error)
	Patch(name string, pt types.PatchType, data []byte, subresources ...string) (*unstructured.Unstructured, error)
}

You create it from just a rest.Config, no mapper, no path resolving func, no trying to set up codecs ahead of time, no unnecessary pool. It just works.

I updated the namespace controller to use it and I updated the existing dynamic client to leverage it so that I get all their tests for "free".

@kubernetes/sig-api-machinery-pr-reviews
@liggitt @smarterclayton @bparees @sttts @ironcladlou I know each of us has struggled with the dynamic client in our time.
@lavalamp @caesarxuchao This is vastly simplifying. I'm eager to drop the old ClientPool. client-go will technically have another incompatible semver this release. I'm up for changing it in tree.

client-go developers: the new dynamic client is easier to use and the old is deprecated, you must switch.

liggitt · 2018-04-20T16:05:58Z

staging/src/k8s.io/client-go/dynamic/simple.go

+		url = append(url, name)
+
+		// subresources only work on things with names
+		if len(c.subresource) > 0 {


this case should error, not construct a different URL... if I try to construct a request to delete a pod binding subresource and accidentally use an empty name var, I should error, not delete all pods in the namespace.

this case should error, not construct a different URL... if I try to construct a request to delete a pod binding subresource and accidentally use an empty name var, I should error, not delete all pods in the namespace.

Fixed in callers with proper error. If someone uses an empty name, I put a panic below as a backstop, but its a programmer error.

smarterclayton · 2018-04-20T17:21:22Z

+1 this is what dynamic client should look like to consumers.

deads2k · 2018-04-20T22:40:30Z

/retest

deads2k · 2018-04-21T17:49:01Z

note to self, gc sets incorrect namespace for cluster scoped resources here:

kubernetes/pkg/controller/garbagecollector/garbagecollector.go

Line 298 in 550c015

    
           owner, err = client.Resource(resource, item.identity.Namespace).Get(reference.Name, metav1.GetOptions{})

deads2k · 2018-04-23T13:59:33Z

/retest

deads2k · 2018-04-23T14:40:15Z

/retest

deads2k · 2018-04-23T16:33:50Z

/retest

kops seems to be having difficulty

deads2k · 2018-04-25T12:55:33Z

comments addressed.

sttts · 2018-04-25T13:11:21Z

lgtm

k8s-github-robot · 2018-04-25T15:09:50Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-04-25T15:50:08Z

Automatic merge from submit-queue (batch tested with PRs 63137, 62913). If you want to cherry-pick this change to another branch, please follow the instructions here.

lavalamp · 2018-04-26T20:06:03Z

/assign @yliaog @roycaihw

lavalamp · 2018-04-26T20:06:17Z

/assign @yliaog

yliaog · 2018-04-26T21:51:27Z

staging/src/k8s.io/client-go/dynamic/simple.go

+		opts = &metav1.DeleteOptions{}
+	}
+	if opts == nil {
+		opts = &metav1.DeleteOptions{}


duplicates?

yliaog · 2018-04-26T22:45:31Z

if old ClientPool is dropped, what's the plan to enable sharing the REST clients?

liggitt · 2018-04-26T23:56:26Z

if old ClientPool is dropped, what's the plan to enable sharing the REST clients?

the previous implementation required constructing a new client per group/version, which led to the need for a client pool.

with the implementation in this PR, a single restClient inside a single dynamic client can be used for requests to any Group/Version/Resource, so there's no need for a client pool.

kubernetes#62913 switched from using a client pool, where each groupVersionResource got its own rest client, to a single client. This increases the QPS to account for increased requests using a single rest client rate limiter.

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Bump QPS on namespace controller #62913 switched from using a client pool, where each groupVersionResource got its own rest client, to a single client. This increases the QPS to account for increased requests using a single rest client rate limiter. Fixes #63240 ```release-note NONE ```

roycaihw · 2018-05-08T17:19:17Z

staging/src/k8s.io/client-go/dynamic/simple.go

+
+func (c *dynamicResourceClient) Create(obj *unstructured.Unstructured) (*unstructured.Unstructured, error) {
+	if len(c.subresource) > 0 {
+		return nil, fmt.Errorf("create not supported for subresources")


We support subresource creation for pods/binding, pods/eviction and some deployments/rollback.

kubernetes#62913 switched from using a client pool, where each groupVersionResource got its own rest client, to a single client. This increases the QPS to account for increased requests using a single rest client rate limiter.

k8s-ci-robot requested review from caesarxuchao, dashpole, derekwaynecarr, enisoc, mqliang, resouer, saad-ali, soltysh, timothysc and zmerlynn April 20, 2018 15:55

liggitt reviewed Apr 20, 2018

View reviewed changes

deads2k force-pushed the client-04-dynamic branch from fd07417 to cd8d8d2 Compare April 20, 2018 16:31

timothysc removed their request for review April 20, 2018 16:45

deads2k force-pushed the client-04-dynamic branch 2 times, most recently from 7ee2ac9 to d6cb8fb Compare April 20, 2018 16:56

deads2k force-pushed the client-04-dynamic branch 2 times, most recently from 5d8535a to cfead73 Compare April 20, 2018 20:52

deads2k force-pushed the client-04-dynamic branch 2 times, most recently from 14b417b to c1a3d7a Compare April 23, 2018 13:22

deads2k force-pushed the client-04-dynamic branch 2 times, most recently from d030237 to 3632037 Compare April 25, 2018 12:55

deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 25, 2018

k8s-github-robot merged commit 5d7569d into kubernetes:master Apr 25, 2018

hzxuzhonghu mentioned this pull request Apr 26, 2018

We should use simple dynamic client everywhere #63182

Closed

k8s-ci-robot assigned roycaihw and yliaog Apr 26, 2018

yliaog reviewed Apr 26, 2018

View reviewed changes

Random-Liu mentioned this pull request Apr 27, 2018

Node E2E Test timeout #63240

Closed

liggitt mentioned this pull request Apr 27, 2018

Bump QPS on namespace controller #63251

Merged

roycaihw reviewed May 8, 2018

View reviewed changes

marpaia mentioned this pull request Jun 29, 2018

Update Kubernetes and Client-Go for 1.11.0 / 8.0.0 vmware-tanzu/velero#634

Merged

deads2k deleted the client-04-dynamic branch July 3, 2018 18:03

jessesuen mentioned this pull request Jul 4, 2018

Update to client-go 8.0 to use new dynamic client argoproj/argo-cd#353

Closed

tamalsaha mentioned this pull request Jul 10, 2018

Remove dependency to deprecated-dynamic client kmodules/client-go#196

Merged

wenjiaswe mentioned this pull request Sep 12, 2018

Rewrite finalURLTemplate used only for metrics because of dynamic client change #68530

Merged

hausdorff mentioned this pull request Nov 16, 2018

Refactor client/pool into a single struct pulumi/pulumi-kubernetes#281

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make a simple dynamic client that is easy to use #62913

make a simple dynamic client that is easy to use #62913

deads2k commented Apr 20, 2018

liggitt Apr 20, 2018 •

edited

Loading

deads2k Apr 20, 2018

smarterclayton commented Apr 20, 2018

deads2k commented Apr 20, 2018

deads2k commented Apr 21, 2018

deads2k commented Apr 23, 2018

deads2k commented Apr 23, 2018

deads2k commented Apr 23, 2018

deads2k commented Apr 25, 2018

sttts commented Apr 25, 2018

k8s-github-robot commented Apr 25, 2018

k8s-github-robot commented Apr 25, 2018

lavalamp commented Apr 26, 2018

lavalamp commented Apr 26, 2018

yliaog Apr 26, 2018

yliaog commented Apr 26, 2018

liggitt commented Apr 26, 2018

roycaihw May 8, 2018

make a simple dynamic client that is easy to use #62913

make a simple dynamic client that is easy to use #62913

Conversation

deads2k commented Apr 20, 2018

liggitt Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

deads2k Apr 20, 2018

Choose a reason for hiding this comment

smarterclayton commented Apr 20, 2018

deads2k commented Apr 20, 2018

deads2k commented Apr 21, 2018

deads2k commented Apr 23, 2018

deads2k commented Apr 23, 2018

deads2k commented Apr 23, 2018

deads2k commented Apr 25, 2018

sttts commented Apr 25, 2018

k8s-github-robot commented Apr 25, 2018

k8s-github-robot commented Apr 25, 2018

lavalamp commented Apr 26, 2018

lavalamp commented Apr 26, 2018

yliaog Apr 26, 2018

Choose a reason for hiding this comment

yliaog commented Apr 26, 2018

liggitt commented Apr 26, 2018

roycaihw May 8, 2018

Choose a reason for hiding this comment

liggitt Apr 20, 2018 •

edited

Loading