Adding dynamic client #20351

krousey · 2016-01-29T22:29:37Z

Attempt to provide a dynamic API client that can access metadata common to API object and lists, and perform common operations on them.

k8s-github-robot · 2016-01-29T22:36:56Z

Labelling this PR as size/XL

krousey · 2016-01-29T22:37:01Z

pkg/client/typed/dynamic/client.go

+	result := new(List)
+	err := rc.namespace(rc.cl.Get()).
+		Resource(rc.resource.Name).
+		VersionedParams(&opts, api.Scheme). // What to do about VersionedParams converter?


Is it ok that we use api.Scheme for conversion here? I think the whole idea behind a dynamic client is not needing to know anything (like how to convert) about the underlying structure.

Yeah, I think it's appropriate to take the standard ListOptions here. But maybe from the v1 package and not the unversioned api package?

This should be using a generic parametercodec created specifically for the "common API" things. It also assumes that the endpoint supports this version of list params - ultimately that means that the client has a certain "level" of list options that the server may not match.

Do you mean api.ParameterCodec? I've run into a problem with that. It fails to recognize the ListOptions belonging to an arbitrary GroupVersion if the GroupVersion isn't registered. This means the conversion step fails and results in an error.

I was thinking of just writing a parameter codec that doesn't do group conversion and just calls queryparams.Convert. Is this a bad idea?

I think there is a generic.ParameterCodec that only needs to know about all of the possible list options a server could support. To make progress you could have a big TODO in there "deal with conversion". I think dealing with conversion will require the discovery doc to describe what the common schema is so you can prime generic.ParameterCodec. For now "best effort' is fine.

I'm having trouble finding the ParameterCodec you mention. The only EncodeParameters implementation is on runtime.parameterCodec and the only call to runtime.NewParameterCodec is in pkg/api/register.

There is no generic codec yet - just giving it that name for convenience
here. dynamic.ParameterCodec

On Wed, Feb 3, 2016 at 1:33 PM, krousey notifications@github.com wrote:

In pkg/client/typed/dynamic/client.go
#20351 (comment)
:

+// namespace applies a namespace to the request if the configured
+// resource is a namespaced resource. Otherwise, it just returns the
+// passed in request.
+func (rc *ResourceClient) namespace(req *client.Request) *client.Request {

if rc.resource.Namespaced {

return req.Namespace(rc.ns)

}

return req
+}

+// List returns a list of objects for this resource.
+func (rc _ResourceClient) List(opts api.ListOptions) (_List, error) {

result := new(List)

err := rc.namespace(rc.cl.Get()).

Resource(rc.resource.Name).

VersionedParams(&opts, api.Scheme). // What to do about VersionedParams converter?

I'm having trouble finding the ParameterCodec you mention. The only
EncodeParameters implementation is on runtime.parameterCodec and the only
call to runtime.NewParameterCodec is in pkg/api/register.

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/20351/files#r51765129.

krousey · 2016-01-29T22:40:28Z

This is still a work in progress. I have more tests to write, as I'm not that confident in Watch yet.

There's a couple of open questions I left as comments in the code. I'll go back and comment on them directly. There's also a question about organization. The new types and codec are in this package. Is this OK? Is there a better place for them to live? It didn't make sense to register the codec, since it's not really associated to any version.

Open to all comments about how to do this better.

cc @lavalamp

cc @derekwaynecarr since you expressed interest in the SIG meeting
cc @smarterclayton To spot check how I implemented the codec considering that I don't want it to do any conversion. Did I appropriately ignore defaults and overrides?

krousey · 2016-01-29T22:40:50Z

@kubernetes/sig-api-machinery

lavalamp · 2016-01-29T22:51:53Z

pkg/client/typed/dynamic/client.go

+// codec is ignored, as the dynamic client uses it's own codec.
+func NewClient(conf *client.Config) (*Client, error) {
+	// avoid changing the original config
+	confCopy := *conf


Shallow copy is OK?

Precedent: https://github.com/kubernetes/kubernetes/blob/master/pkg/client/unversioned/discovery_client.go#L220

We should probably be careful on reviews of client config to ensure that holds true. Probably worth it to add a copy the next time we change client.config.

lavalamp · 2016-01-29T22:52:21Z

Looks good so far!

k8s-bot · 2016-01-29T23:01:29Z

GCE e2e test build/test passed for commit c6ffca566577bec731ee6d3b12fbb54b247e8e46.

smarterclayton · 2016-02-03T03:39:07Z

pkg/client/typed/dynamic/codec.go

+
+// JSONCodec is a runtime.Codec that attempts to dynamically decode
+// objects and lists metadata for generic consumption.
+type JSONCodec struct {


I think this is what UnstructuredCodec was supposed to do

Should I start working on UnstructuredObject, UnstructuredList, and moving some of this logic back into the UnstructuredJSONScheme? Or do you think this should remain separate?

The former sounds like a good idea to me.

Also I guess now that @smarterclayton's rename has gone in, this should be called a "Serializer" not a "Codec".

I would roll it into to UnstructuredJSONScheme as well - agree this is a Serializer (does not guarantee versioning).

krousey · 2016-02-08T23:08:34Z

pkg/client/typed/dynamic/client.go

+
+var pCodec runtime.ParameterCodec = parameterCodec{}
+
+func getObjectName(obj *runtime.Unstructured) (string, error) {


This is what I used instead of including metadata in the unstructured objects.

Leave a TODO that this needs to be centralized

On Status in decode, I'm surprised you need to do that - request.go should call decodeInto on your codec to check for status. Having a wrapper that lives in typed client and uses the returned GVK to transform specific kinds into known API types would probably be the short term recommendation. Another alternative is to create a scheme and import specifically unversioned status and the other common types (we need this anyway) and do a delegation call into the scheme.

krousey · 2016-02-08T23:16:13Z

@lavalamp @smarterclayton

I think I addressed the high level comments you both had. If you guys don't have any more high level ones, I'm going to add more tests and turn this in to a real review.

k8s-bot · 2016-02-08T23:52:19Z

GCE e2e test build/test passed for commit 7bf05ee99cd42391ee7fa715c6cf72b41e4599b6.

k8s-bot · 2016-02-08T23:55:13Z

GCE e2e test build/test passed for commit f852aedb1d2f5f05985b433f58f8370077fd8018.

smarterclayton · 2016-02-09T00:05:18Z

pkg/client/typed/dynamic/client.go

+	return errors.New("DecodeParameters not implemented on nilParameterCodec")
+}
+
+var pCodec runtime.ParameterCodec = parameterCodec{}


we generally don't use this naming style - it should be parameterCodec or parameterEncoder

derekwaynecarr · 2016-02-09T03:45:57Z

Can you add DeleteCollection?

On Friday, January 29, 2016, krousey notifications@github.com wrote:

Attempt to provide a dynamic API client that can access metadata common to

API object and lists, and perform common operations on them.

You can view, comment on, or merge this pull request online at:

#20351
Commit Summary

WIP Adding dynamic client

File Changes

A pkg/client/typed/dynamic/client.go
https://github.com/kubernetes/kubernetes/pull/20351/files#diff-0
(184)

A pkg/client/typed/dynamic/client_test.go
https://github.com/kubernetes/kubernetes/pull/20351/files#diff-1
(108)

A pkg/client/typed/dynamic/codec.go
https://github.com/kubernetes/kubernetes/pull/20351/files#diff-2
(159)

A pkg/client/typed/dynamic/codec_test.go
https://github.com/kubernetes/kubernetes/pull/20351/files#diff-3
(94)

Patch Links:

https://github.com/kubernetes/kubernetes/pull/20351.patch

https://github.com/kubernetes/kubernetes/pull/20351.diff

—
Reply to this email directly or view it on GitHub
#20351.

krousey · 2016-02-09T23:03:38Z

@derekwaynecarr From what I can tell, only events support DeleteCollection. What is the expected behavior if this were used with Pods or Services? I was trying to limit this to the common operations.

derekwaynecarr · 2016-02-09T23:41:57Z

DeleteCollection should work for all namespaced content except Services,
but that needs to be fixed.

See: #20581

On Tuesday, February 9, 2016, krousey notifications@github.com wrote:

@derekwaynecarr https://github.com/derekwaynecarr From what I can tell,
only events support DeleteCollection. What is the expected behavior if this
were used with Pods or Services? I was trying to limit this to the common
operations.

—
Reply to this email directly or view it on GitHub
#20351 (comment)
.

derekwaynecarr · 2016-02-09T23:43:33Z

If it's not supported, it's fine to return the error that comes back from
the API server. So for services, I would expect an error response, but for
all others, it should be fine.

On Tuesday, February 9, 2016, krousey notifications@github.com wrote:

@derekwaynecarr https://github.com/derekwaynecarr From what I can tell,
only events support DeleteCollection. What is the expected behavior if this
were used with Pods or Services? I was trying to limit this to the common
operations.

—
Reply to this email directly or view it on GitHub
#20351 (comment)
.

krousey · 2016-02-09T23:46:32Z

@derekwaynecarr I only see a client side implementation on events. Can I assume this to be a canonical was to call with with respect to query parameters, body, and HTTP Verb? That is a DELETE verb, list options in the query parameter, delete options in the body?

derekwaynecarr · 2016-02-09T23:51:48Z

All of the generated clients support the call.

https://github.com/kubernetes/kubernetes/blob/master/pkg/client/typed/generated/core/unversioned/pod.go#L36

Every registry that uses etcd generic for storage has the server side
support:

https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/generic/etcd/etcd.go#L444

Unfortunately services does not use that registry yet, but all others
should be good to go.

On Tuesday, February 9, 2016, krousey notifications@github.com wrote:

@derekwaynecarr https://github.com/derekwaynecarr I only see a client
side implementation on events. Can I assume this to be a canonical was to
call with with respect to query parameters, body, and HTTP Verb? That is a
DELETE verb, list options in the query parameter, delete options in the
body?

—
Reply to this email directly or view it on GitHub
#20351 (comment)
.

krousey · 2016-02-17T16:50:03Z

@derekwaynecarr It's possible, but I would like to understand why this is a problem. How does deferring the creation of the rest client help if the API the client produces is exactly the same?

derekwaynecarr · 2016-02-17T17:41:15Z

@krousey - I will push an example for the namespace controller using this for where it causes the problem.

derekwaynecarr · 2016-02-17T17:45:25Z

@krousey - here is a PR where I tried to start using this:

I want to be able to construct a dynamic client in the controllermanager.go here:
0fab678#diff-2aa9cfe6a01016c1679752b0fcfc9244R248

The issue is that at this point, it has no notion of the group version to use, so the call fails:

Failed to create client: GroupVersion is required when initializing a RESTClient

Later, I do some discovery, and know the set of resources I plan to delete, and will call delete collection here:
0fab678#diff-f86a8dd1c06d1f9578423999f8094cc7R138

At this point, I actually know api resource and preferred group version that I want to use, and so I would prefer the REST client is built in response to the Resource() call.

Make sense?

derekwaynecarr · 2016-02-17T17:46:46Z

I want to avoid passing a *client.Config into the namespace controller as it makes testing the controller really difficult versus passing in a mock client implementation.

derekwaynecarr · 2016-02-17T18:19:30Z

After discussion on Slack, we agreed to put an interface in front of dynamic.Client in a separate PR that will maintain a cache of clients per group version. The namespace controller will use that.

krousey · 2016-02-17T22:22:34Z

@k8s-bot e2e test this issue: #21257

k8s-bot · 2016-02-17T22:51:03Z

GCE e2e test build/test passed for commit 4c58302.

krmayankk · 2016-02-18T06:51:41Z

What does this client provide ? Is this different from unversion go client for kubernetes or is it a layer on top of it ? Does it remove the 70+ dependencies the earlier go client depends on ?

k8s-github-robot · 2016-02-18T12:22:44Z

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

k8s-bot · 2016-02-18T12:55:57Z

GCE e2e test build/test passed for commit 4c58302.

k8s-github-robot · 2016-02-18T13:21:06Z

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

k8s-bot · 2016-02-18T13:52:08Z

GCE e2e build/test failed for commit 4c58302.

krousey · 2016-02-18T16:56:43Z

@k8s-bot e2e test this issue: #21257

krmayankk · 2016-02-18T17:30:55Z

@derekwaynecarr What does this client provide ? Is this different from unversioned go client for kubernetes or is it a layer on top of it ? Does it remove the 70+ dependencies the earlier go client depends on ?

krousey · 2016-02-18T17:37:52Z

@krmayankk This client provides the ability to minimally interact with resources that weren't necessarily compiled in. The namespace controller (something @derekwaynecarr works on) has a use case to delete all resources under a namespace when the namespace is being deleted. This gives him a generic way to do that.

Another use case is to allow eventually allow kubectl to interact with 3rd party API extensions without having to compile them in.

Regarding dependencies, no. It might have few dependencies, but that was not an explicit goal for this PR. Also, this client isn't as fully featured as the others.

There are other efforts to reduce the dependencies (API restructuring, client generation...), but this is not one of them.

k8s-bot · 2016-02-18T17:58:48Z

GCE e2e build/test failed for commit 4c58302.

krousey · 2016-02-18T18:05:45Z

@k8s-bot e2e test this issue: #21138

lavalamp · 2016-02-18T18:16:43Z

@krmayankk we will work on reducing the import tree next. First we're
trying to get the client into the right shape at all...

On Thu, Feb 18, 2016 at 10:05 AM, krousey notifications@github.com wrote:

@k8s-bot https://github.com/k8s-bot e2e test this issue: #21138
#21138

—
Reply to this email directly or view it on GitHub
#20351 (comment)
.

k8s-bot · 2016-02-18T18:33:44Z

GCE e2e build/test failed for commit 4c58302.

krousey · 2016-02-18T18:43:35Z

@k8s-bot e2e test this issue: #21463

k8s-bot · 2016-02-18T19:17:06Z

GCE e2e test build/test passed for commit 4c58302.

k8s-github-robot · 2016-02-19T03:41:16Z

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

k8s-bot · 2016-02-19T04:11:21Z

GCE e2e build/test failed for commit 4c58302.

krousey · 2016-02-19T17:17:20Z

I'm adding the e2e-not-required label since e2e is flaky and this code in no way affects the e2e suite.

k8s-github-robot · 2016-02-19T18:00:54Z

Automatic merge from submit-queue

Auto commit by PR queue bot

krousey added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jan 29, 2016

krousey self-assigned this Jan 29, 2016

googlebot added the cla: yes label Jan 29, 2016

k8s-github-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 29, 2016

krousey reviewed Jan 29, 2016
View reviewed changes

lavalamp reviewed Jan 29, 2016
View reviewed changes

smarterclayton reviewed Feb 3, 2016
View reviewed changes

krousey mentioned this pull request Feb 8, 2016

Design/Implement "dynamic" client from #15730 #16764

Closed

krousey force-pushed the dynamic_client branch from c6ffca5 to 7bf05ee Compare February 8, 2016 23:06

krousey reviewed Feb 8, 2016
View reviewed changes

krousey force-pushed the dynamic_client branch from 7bf05ee to f852aed Compare February 8, 2016 23:15

smarterclayton reviewed Feb 9, 2016
View reviewed changes

krousey force-pushed the dynamic_client branch from f852aed to fb63234 Compare February 10, 2016 19:55

derekwaynecarr added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 17, 2016

derekwaynecarr mentioned this pull request Feb 17, 2016

Namespace deletion uses discovery + dynamic client #21400

Merged

krousey added the e2e-not-required label Feb 19, 2016

k8s-github-robot pushed a commit that referenced this pull request Feb 19, 2016

Merge pull request #20351 from krousey/dynamic_client

c3a962b

Auto commit by PR queue bot

k8s-github-robot merged commit c3a962b into kubernetes:master Feb 19, 2016


		var pCodec runtime.ParameterCodec = parameterCodec{}

		func getObjectName(obj *runtime.Unstructured) (string, error) {

Adding dynamic client #20351

Adding dynamic client #20351

Conversation

krousey commented Jan 29, 2016

k8s-github-robot commented Jan 29, 2016

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

krousey commented Jan 29, 2016

krousey commented Jan 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Jan 29, 2016

k8s-bot commented Jan 29, 2016

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

smarterclayton Feb 9, 2016 via email

Choose a reason for hiding this comment

krousey commented Feb 8, 2016

k8s-bot commented Feb 8, 2016

k8s-bot commented Feb 8, 2016

Choose a reason for hiding this comment

derekwaynecarr commented Feb 9, 2016

API object and lists, and perform common operations on them.

krousey commented Feb 9, 2016

derekwaynecarr commented Feb 9, 2016

derekwaynecarr commented Feb 9, 2016

krousey commented Feb 9, 2016

derekwaynecarr commented Feb 9, 2016

krousey commented Feb 17, 2016

derekwaynecarr commented Feb 17, 2016

derekwaynecarr commented Feb 17, 2016

derekwaynecarr commented Feb 17, 2016

derekwaynecarr commented Feb 17, 2016

krousey commented Feb 17, 2016

k8s-bot commented Feb 17, 2016

krmayankk commented Feb 18, 2016

k8s-github-robot commented Feb 18, 2016

k8s-bot commented Feb 18, 2016

k8s-github-robot commented Feb 18, 2016

k8s-bot commented Feb 18, 2016

krousey commented Feb 18, 2016

krmayankk commented Feb 18, 2016

krousey commented Feb 18, 2016

k8s-bot commented Feb 18, 2016

krousey commented Feb 18, 2016

lavalamp commented Feb 18, 2016

k8s-bot commented Feb 18, 2016

krousey commented Feb 18, 2016

k8s-bot commented Feb 18, 2016

k8s-github-robot commented Feb 19, 2016

k8s-bot commented Feb 19, 2016

krousey commented Feb 19, 2016

k8s-github-robot commented Feb 19, 2016