Client package proposal #15730

lavalamp · 2015-10-15T20:51:16Z

I'm accumulating everything I've learned from talking with people about the client into a single proposal to make sure everyone is on the same page.

@smarterclayton @krousey @caesarxuchao

k8s-bot · 2015-10-15T21:05:04Z

GCE e2e build/test failed for commit 50ca09edc2bb31dcb9bbda0ff7bb924fab7e0b29.

k8s-github-robot · 2015-10-15T21:05:24Z

Labelling this PR as size/L

caesarxuchao · 2015-10-15T21:05:24Z

docs/proposals/client-package-structure.md

+  Extensions() ext_v1beta1.Client
+  Net() net_v1beta1.Client
+
+  GroupVersion(group, version string) generic.Client


What is this for?

Nevermind, I see it in next paragraph.

wojtek-t · 2015-10-16T14:38:33Z

/sub

derekwaynecarr · 2015-10-16T14:54:39Z

docs/proposals/client-package-structure.md

+somewhere. It consumes a Config object with options appropriate for this.
+(That's most of the current client.Config structure.)
+
+Third parties (e.g., open shift) may have their own implementation.


OpenShift

abstain from commenting the same through doc

I figured someone would correct me, I just didn't know what to--I will fix. ;)

I don't think we want Openshift or other clients to have their own implementation. I believe transport is generic to "Kube-like" APIs - standard auth, security, behavior, and other patterns. A kube like API should be able to use the Kube like transport

OK-- I think I interpreted your comments in the previous discussion at the wrong level of meta. See if you agree now.

derekwaynecarr · 2015-10-16T15:06:09Z

@lavalamp - I am interested in also defining a client pattern that lets me take an APIResource and perform generic ObjectMeta oriented LIST, DELETE(item) request/response.

use case is for namespace controller to allow generic deletion pattern:

find all namespaced resources via discovery client
for each namespaced resource, do a LIST(resource), DELETE(item) flow

so this client would know how to build URL paths given arbitrary APIResource definition.

thoughts?

lavalamp · 2015-10-16T16:35:31Z

@derekwaynecarr I absolutely think we should support that, and I think that falls well within the scope of what I've imaginatively called the generic client.

We can also extend the things like runtime.Unknown/Unstructured to provide metadata manipulation abilities for arbitrary (api-conforming) objects.

krousey · 2015-10-16T17:24:02Z

docs/proposals/client-package-structure.md

+for executing discovery commands. The group and version may be overridable with
+a function call.
+
+RESTClients may have semantic information built in. For example, if the


Couldn't this be accomplished at the transport layer? Currently you can provide your own transport or even provide something that wraps a transport. I imagine this RESTClient could just consume a transport that does this?

It's possible, but I'm imagining a different RESTClient per group/version, with all of them sharing a common Transport. We do want to cache & reuse transports if possible, on account of http connection reuse.

It should be relatively simple to wrap a shared transport for different clients. HTTPWrappersForConfig does this already.

As Clayton points out, we should keep as much semantic info in the Codec as possible.

lavalamp · 2015-10-16T21:41:39Z

Thanks for the comments everyone-- I will update accordingly, perhaps on Monday.

smarterclayton · 2015-10-16T22:06:02Z

I'll try and take a look before Monday.

On Oct 16, 2015, at 5:41 PM, Daniel Smith notifications@github.com wrote:

Thanks for the comments everyone-- I will update accordingly, perhaps on
Monday.

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

smarterclayton · 2015-10-18T23:01:13Z

docs/proposals/client-package-structure.md

+appropriate query parameter when you call the "LabelSelector()" method.
+
+3rd parties such as open shift may provide their own implementation, but it
+should conform to the same interface as the standard RESTClient.


Again, any go code implementing a Kube like API should be able to reuse the standard impl. No "hacks" specific to a certain API group should be in this layer

smarterclayton · 2015-11-04T18:33:00Z

docs/proposals/client-package-structure.md

+  below).
+* `GroupName()` returns the generated client that this section is about.
+* `NamespaceSpecifier()` may take a namespace parameter or nothing.
+* `Action` is one of Create/Get/Update/Delete/Watch, or appropriate actions


We need to settle on the pattern for sub resources. You don't have to do it here, but I'd like the current state called out as "insufficiently defined"

caesarxuchao · 2015-11-16T23:03:20Z

Will kubectl use a pre-built clientset, or shall it build its clientset at runtime according to the versions specified in kubeconfig?

bgrant0607 · 2015-11-17T22:02:47Z

docs/proposals/client-package-structure.md

+
+# Client: layering and package structure
+
+## Desired layers


TOC and/or TL;DR of the proposed layers / modules.

k8s-bot · 2015-11-19T19:07:49Z

GCE e2e test build/test passed for commit 6f8fcbfacc9a5015ba92e42feeb8eab5f20bf336.

smarterclayton · 2015-11-22T19:18:46Z

One thought I had today - we really should manage and version the conversions and API objects outside of the core Kube package somehow.

Having go code that can take a v1beta3 object and convert it to v3 (even if it's lossy to reverse) is generally useful. Being able to keep old conversions over a long lifetime seems useful. It would also make it easy to control lifecycle of API external versions.

lavalamp · 2015-12-02T22:48:02Z

OK, I pushed a new revision.

@caesarxuchao

Will kubectl use a pre-built clientset, or shall it build its clientset at runtime according to the versions specified in kubeconfig?

kubectl should use the dynamic client as much as possible, and for things that it has special knowledge of, it should use a pre-built clientset.

@smarterclayton

we really should manage and version the conversions and API objects outside of the core Kube package somehow.

Agree, but outside the scope of this doc...

One concrete proposal that I think fits into a lot of our general problems:

We need to build, demonstrate, and test against an "out-of-tree" api group for client, scaler, and general logic like autoscaler
When we make changes to the client library, we should ensure that keeps working (without being able to refactor that code)
We need to use that to prove, test, and verify the discovery, generic references across types, and api groups problems.

I added lines for the first two in various places.

lavalamp · 2015-12-02T22:48:42Z

@smarterclayton if you're happy with these changes I'll squash and we can get this in :)

caesarxuchao · 2015-12-02T23:10:01Z

kubectl should use the dynamic client as much as possible, and for things that it has special knowledge of, it should use a pre-built clientset.

My point is, if kubectl uses the pre-built clientset, then the version is decided at compile time, the APIVersion field in kubeconfig will be meaningless. Perhaps we should just remove that field from kubeconfig.

smarterclayton · 2015-12-02T23:17:41Z

I'm fine with changes, LGTM

smarterclayton · 2015-12-02T23:30:46Z

There can be multiple built in clientsets

On Dec 2, 2015, at 6:10 PM, Chao Xu notifications@github.com wrote:

kubectl should use the dynamic client as much as possible, and for things
that it has special knowledge of, it should use a pre-built clientset.

My point is, if kubectl uses the pre-built clientset, then the version is
decided at compile time, the APIVersion field in kubeconfig will be
meaningless. Perhaps we should just remove that field from kubeconfig.

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

caesarxuchao · 2015-12-02T23:46:47Z

There can be multiple built in clientsets

Sure, we can switch clientset based on the specified "group/version", but I imagine the code won't be elegant, because you need to translate "group/version" to the the release version of clientset somehow.

k8s-bot · 2015-12-02T23:50:35Z

GCE e2e build/test failed for commit a873007f4115e2343aa3ff101f9a8528269388ad.

smarterclayton · 2015-12-02T23:50:52Z

Why is that hard? We already have to have scheme registries in clients for
conversions.

On Dec 2, 2015, at 6:47 PM, Chao Xu notifications@github.com wrote:

There can be multiple built in clientsets

Sure, we can switch clientset based on the specified "group/version", but I
imagine the code won't be elegant, because you need to translate
"group/version" to the the release version of clientset somehow.

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

caesarxuchao · 2015-12-03T00:14:57Z

Sorry I don't follow. Let's look at a concrete example. (https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/portforward.go#L104-L109)

    client, err := f.Client()
    if err != nil {
        return err
    }
    pod, err := client.Pods(namespace).Get(podName)

In this proposal, the client should now be a pkg/client/clientset/release-X.Y#client, and the returned pod will be a vZ.pod. How are we going to rewrite this piece of code? I think we can't even switch among different clientsets, because the the clients in different sets are of different types.

lavalamp · 2015-12-09T05:16:08Z

Squashed. Let's merge and we can make changes later if needed.

k8s-bot · 2015-12-09T05:47:14Z

GCE e2e test build/test passed for commit dc5e1ea.

k8s-github-robot · 2015-12-09T17:12:52Z

Automatic merge from submit-queue

Auto commit by PR queue bot

lavalamp added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Oct 15, 2015

lavalamp assigned krousey Oct 15, 2015

googlebot added the cla: yes label Oct 15, 2015

k8s-github-robot added kind/design Categorizes issue or PR as related to design. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 15, 2015

caesarxuchao reviewed Oct 15, 2015
View reviewed changes

lavalamp added this to the v1.2 milestone Oct 16, 2015

derekwaynecarr reviewed Oct 16, 2015
View reviewed changes

krousey reviewed Oct 16, 2015
View reviewed changes

smarterclayton reviewed Oct 18, 2015
View reviewed changes

smarterclayton reviewed Nov 4, 2015
View reviewed changes

bgrant0607 reviewed Nov 17, 2015
View reviewed changes

caesarxuchao mentioned this pull request Nov 19, 2015

Generate client code using go2idl #17468

Closed

13 tasks

caesarxuchao mentioned this pull request Nov 25, 2015

output of client-gen (#17534) #17738

Closed

lavalamp force-pushed the client-doc branch from 6f8fcbf to a873007 Compare December 2, 2015 22:47

Create client package proposal

dc5e1ea

lavalamp force-pushed the client-doc branch from a873007 to dc5e1ea Compare December 9, 2015 05:15

wojtek-t added the e2e-not-required label Dec 9, 2015

smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 9, 2015

k8s-github-robot pushed a commit that referenced this pull request Dec 9, 2015

Merge pull request #15730 from lavalamp/client-doc

598e3b0

Auto commit by PR queue bot

k8s-github-robot merged commit 598e3b0 into kubernetes:master Dec 9, 2015

Client package proposal #15730

Client package proposal #15730

Conversation

lavalamp commented Oct 15, 2015

k8s-bot commented Oct 15, 2015

k8s-github-robot commented Oct 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t commented Oct 16, 2015

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

derekwaynecarr commented Oct 16, 2015

lavalamp commented Oct 16, 2015

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

lavalamp commented Oct 16, 2015

smarterclayton commented Oct 16, 2015

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 Nov 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-bot commented Nov 19, 2015

smarterclayton commented Nov 22, 2015

lavalamp commented Dec 2, 2015

lavalamp commented Dec 2, 2015

caesarxuchao commented Dec 2, 2015

smarterclayton commented Dec 2, 2015

smarterclayton commented Dec 2, 2015

caesarxuchao commented Dec 2, 2015

k8s-bot commented Dec 2, 2015

smarterclayton commented Dec 2, 2015

caesarxuchao commented Dec 3, 2015

lavalamp commented Dec 9, 2015

k8s-bot commented Dec 9, 2015

k8s-github-robot commented Dec 9, 2015