Change kubectl create to use dynamic client #30250

krousey · 2016-08-08T23:03:37Z

This is a series of changes to allow kubectl create to use discovery-based REST mapping and dynamic clients.

cc @kubernetes/sig-api-machinery

Release note:

kubectl will no longer do client-side defaulting on create and replace.

This change is

smarterclayton · 2016-08-08T23:14:44Z

pkg/client/typed/discovery/restmapper.go

@@ -80,6 +80,8 @@ func NewRESTMapper(groupResources []*APIGroupResources, versionInterfaces meta.V
 				// TODO only do this if it supports listing
 				versionMapper.Add(gv.WithKind(resource.Kind+"List"), scope)
 			}
+			// TODO why is this type not in discovery (at least for "v1")
+			versionMapper.Add(gv.WithKind("List"), meta.RESTScopeRoot)


It's not a server resource? It's just a local object. Version mapper shouldn't be blocking this - the server should never have to know about Lists

Yea, I felt really wrong typing this in, but without it, kubectl create -f hack/testdata/list.yaml fails. I am certainly open to other suggestions.

wojtek-t · 2016-08-09T06:32:33Z

@smarterclayton @krousey - currently dynamic client is not using protobufs at all, so this may result in performance degradation. We should probably try to support protobufs in dynamic client too (based on some discovery?) WDYT?

deads2k · 2016-08-09T12:41:17Z

pkg/client/restclient/client.go

+		return nil, err
+	}
+
+	newRC := *c


This shallow copy worries me. We're still sharing the URL pointer, the client, BackoffManager func.

You're right, I'll change this to call back into NewRESTClient

smarterclayton · 2016-08-09T15:44:20Z

I think we can - part of my comments above would be correcting this code so that resource.Builder doesn't have to know about the details of all of that (it's hidden under Codec) which means that we can in the future use proto for those things. However priority for client is not to use protobuf for the near term.

krousey · 2016-08-10T16:09:08Z

@smarterclayton @wojtek-t

Regarding proto encoding for dynamic types, I don't see how you could do that without having proto definitions available, at which point... why use dynamic client?

smarterclayton · 2016-08-11T03:00:48Z

I can imagine interactions where you want to do reads using protobuf and then update an object via the dynamic client. I just don't want to let the interfaces get mixed up - Codec hides those details from RESTClient, not the other way around.

krousey · 2016-08-11T21:43:00Z

pkg/kubectl/resource/mapper.go

-		return nil, fmt.Errorf("unable to decode %q: %v [%v]", source, err, gvk)
-	}
+
+	obj, versioned := versions.Last(), versions.First()


@smarterclayton does this look better?

Yes, a million times.

krousey · 2016-08-11T21:44:51Z

@smarterclayton I've reworked this a good bit, and I think it's in a good spot for another pass.

smarterclayton · 2016-08-15T20:38:48Z

I think test-cmd is sufficient. Sure - makes it easier to review.

This will allow people to override the default parameter codec and still pass the resulting client with something that accepts *dynamic.Client.

krousey · 2016-08-15T22:27:11Z

Fun fact: all of our objects should be json.Marshal-able. Otherwise kubectl's annotation config saving is wrong. So you're going to see that in the next update to this PR.

smarterclayton · 2016-08-15T22:31:40Z

Definitely internal objects should fail / be horribly incompatible if
someone tries to marshal them. Did you find the apply internal version bug?

On Mon, Aug 15, 2016 at 6:27 PM, krousey notifications@github.com wrote:

Fun fact: all of our objects should be json.Marshal-able. Otherwise
kubectl's annotation config saving is wrong. So you're going to see that
in the next update to this PR.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30250 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p1exBpx7bE7SHD3IxtRGw-3IPLFvks5qgOfMgaJpZM4Jfj_0
.

krousey · 2016-08-15T22:36:02Z

@smarterclayton I don't think so. My change is only using a single runtime.Unstructured object. So info.Object == info.VersionedObject. And the unstructured types do not marshal to the appropriate JSON by default. There's also a bunch of comments in the apply code warning of the assumption.

I'm just adding marshal handlers to the Unstructured and UnstructuredList.

And also remove some duplicate code while I'm at it.

krousey · 2016-08-16T05:11:52Z

@smarterclayton I think I addressed the remaining issues you had (the moves in to meta and discovery). I added some json marshaling to unstructured types (runtime/types.go) for proper annotations. I also migrated the replace command. Please take a look when you can.

Also... is this release note worthy?

krousey · 2016-08-16T18:56:01Z

@smarterclayton one last pass?

k8s-github-robot · 2016-08-16T18:58:34Z

@krousey
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

k8s-github-robot · 2016-08-16T19:16:53Z

@krousey
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

smarterclayton · 2016-08-16T21:16:30Z

I think this is release note worthy, since it changes user facing behavior of create.

smarterclayton · 2016-08-16T21:17:08Z

Everything LGTM. Any other items you felt were worthy of an additional look?

krousey · 2016-08-16T21:18:47Z

@smarterclayton Nope, I think this is done. Thanks!

k8s-bot · 2016-08-17T21:08:00Z

GCE e2e build/test passed for commit b5235bc.

k8s-github-robot · 2016-08-17T21:48:55Z

Automatic merge from submit-queue

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

krousey added this to the v1.4 milestone Aug 8, 2016

krousey assigned deads2k Aug 8, 2016

googlebot added the cla: yes label Aug 8, 2016

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Aug 8, 2016

smarterclayton reviewed Aug 8, 2016
View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2016

deads2k reviewed Aug 9, 2016
View reviewed changes

deads2k assigned smarterclayton and unassigned deads2k Aug 10, 2016

krousey force-pushed the kctl_dynamic branch from 8622d5c to 4b68374 Compare August 11, 2016 21:32

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 11, 2016

krousey reviewed Aug 11, 2016
View reviewed changes

Kris added 3 commits August 15, 2016 14:18

Expose dynamic client's content config

d9ce524

Eliminate redundant dynamic client type

e5c2154

This will allow people to override the default parameter codec and still pass the resulting client with something that accepts *dynamic.Client.

Properly decode lists into VersionedObjects

69e0740

Kris added 7 commits August 15, 2016 22:07

Add JSON encoding handlers to unstructured objects

361f13d

Add generic "List" type for all versions

3999f07

Add discovery mapper and dynamic typer to kubectl

63a512f

Fix thirdparty codec to return GVK

d1aa3dd

kubectl/resource: Remove thirdparty special casing

24c441e

And also remove some duplicate code while I'm at it.

Make kubectl create use dynamic clients

7511412

Make kubectl replace use dynamic clients

b5235bc

krousey force-pushed the kctl_dynamic branch from 4b68374 to b5235bc Compare August 16, 2016 05:08

This was referenced Aug 16, 2016

Adding methods to kubectl factory to support dynamic objects #25546

Closed

Changing kubectl commands to not default and use dynamic client #25676

Closed

krousey added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Aug 16, 2016

smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 16, 2016

k8s-github-robot merged commit 4b5fd43 into kubernetes:master Aug 17, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change kubectl create to use dynamic client #30250

Change kubectl create to use dynamic client #30250

krousey commented Aug 8, 2016 •

edited

Loading

smarterclayton Aug 8, 2016

krousey Aug 9, 2016

wojtek-t commented Aug 9, 2016

deads2k Aug 9, 2016

krousey Aug 9, 2016

smarterclayton commented Aug 9, 2016

krousey commented Aug 10, 2016

smarterclayton commented Aug 11, 2016

krousey Aug 11, 2016

smarterclayton Aug 15, 2016

krousey commented Aug 11, 2016

smarterclayton commented Aug 15, 2016

krousey commented Aug 15, 2016

smarterclayton commented Aug 15, 2016

krousey commented Aug 15, 2016

krousey commented Aug 16, 2016

krousey commented Aug 16, 2016

k8s-github-robot commented Aug 16, 2016

k8s-github-robot commented Aug 16, 2016

smarterclayton commented Aug 16, 2016

smarterclayton commented Aug 16, 2016

krousey commented Aug 16, 2016

k8s-bot commented Aug 17, 2016

k8s-github-robot commented Aug 17, 2016

Change kubectl create to use dynamic client #30250

Change kubectl create to use dynamic client #30250

Conversation

krousey commented Aug 8, 2016 • edited Loading

smarterclayton Aug 8, 2016

Choose a reason for hiding this comment

krousey Aug 9, 2016

Choose a reason for hiding this comment

wojtek-t commented Aug 9, 2016

deads2k Aug 9, 2016

Choose a reason for hiding this comment

krousey Aug 9, 2016

Choose a reason for hiding this comment

smarterclayton commented Aug 9, 2016

krousey commented Aug 10, 2016

smarterclayton commented Aug 11, 2016

krousey Aug 11, 2016

Choose a reason for hiding this comment

smarterclayton Aug 15, 2016

Choose a reason for hiding this comment

krousey commented Aug 11, 2016

smarterclayton commented Aug 15, 2016

krousey commented Aug 15, 2016

smarterclayton commented Aug 15, 2016

krousey commented Aug 15, 2016

krousey commented Aug 16, 2016

krousey commented Aug 16, 2016

k8s-github-robot commented Aug 16, 2016

k8s-github-robot commented Aug 16, 2016

smarterclayton commented Aug 16, 2016

smarterclayton commented Aug 16, 2016

krousey commented Aug 16, 2016

k8s-bot commented Aug 17, 2016

k8s-github-robot commented Aug 17, 2016

krousey commented Aug 8, 2016 •

edited

Loading