Add a server side exporter parameter and impl. by brendandburns · Pull Request #17483 · kubernetes/kubernetes

brendandburns · 2015-11-19T07:11:27Z

This is mostly an import of the existing openshift code

Ref: #13038

@smarterclayton @bgrant0607

k8s-github-robot · 2015-11-19T07:20:37Z

Labelling this PR as size/L

bgrant0607 · 2015-11-19T07:37:26Z

Thanks, we do need export, and I do think it should be in the server.

cc @liggitt @derekwaynecarr re. whether this can be extended to work for Openshift

cc @lavalamp to see whether he has suggestions

I would prefer to put the individual export methods into each resource's strategy implementation. As we've found with the namespace deleter, centralizing the code like this makes it likely that it will be overlooked when adding new resources.

I think a subresource isn't needed, so the query parameter is probably fine.

Will take a closer look tomorrow.

cc @janetkuo

k8s-bot · 2015-11-19T07:41:46Z

GCE e2e test build/test passed for commit 2014d975b3ad3c57fb58ccec7752812222faaeb1.

smarterclayton · 2015-11-19T14:57:25Z

pkg/apiserver/apiserver.go

If we do this here clients can't tell what methods support exporting. Servers outside the Kube code base also can't provide an export behavior. the pattern we use for optional behavior like this is to support another interface on the storage object which calls to the strategy. I would prefer to test for the interface in apiinstaller and pass it down to the get handler, and also add the parameter to the go-restful description there.

Swagger supports specifying query parameters, though we don't use it consistently.

One could view this as a specialized form of field selection.

Probably every resource that supports GET should support export, even if it's a no-op.

If we did leave this as a query param, we'd want to handle it similarly to other query params. See DeleteOptions and ListOptions as examples.

If we did leave this as a query param, we'd want to handle it similarly to other query params.
+1

bgrant0607 · 2015-11-19T23:01:23Z

Somewhat related: #17333

Eventually we may want to export just what the user configured, both in terms of fields and resources, and if a user did a bulk export, they won't want to export resources (e.g., pods) created by other resources (e.g., RCs).

OTOH, if they were using kubectl apply already, they probably don't need export.

bgrant0607 · 2015-11-19T23:03:25Z

pkg/apiserver/apiserver.go

Why do we need exact? What does it mean?

This is copied from openshift, exact means "preserve things that are cluster dependent, like namespace"

bgrant0607 · 2015-11-19T23:08:47Z

One thing I'd like to use this for is edit: export, edit, then patch. @janetkuo Do you think that would work?

The benefit is that it would remove cruft like the uid, creationTimestamp, status, etc.

brendandburns · 2015-11-20T00:15:08Z

@smarterclayton what you suggest is tricky to do given the way the framework is put together. The best you could do is either pass an Exporter to GetResourceWithOptions/GetResource or augment the Getter and GetterWithOptions interfaces to include an Export method.

I think another way to achieve what you are asking for @smarterclayton is to allow upstream users to inject their own Exporter, that seems to accomplish what you need.

@bgrant0607 there is already a GetterWithOptions but those options are an opaque runtime.Object so we would have to add an ExportOptions object from the query, but we would have to plumb it in addition to the existing Getter/GetterWithOptions interfaces (see above).

So then my concrete proposal would be to make

type GetterWithOptions interface {
  Get(ctx api.Context, name string, opts runtime.Object, export ExportOptions)
}

type Getter interface {
  Get(ctx api.Context, name string, export ExportOptions)
}

and then drive that down to a concrete implementation in the Strategy.

Does that work for both people?

I would very much like to enable:

kubectl --context=cluster-a export all | kubectl --context=cluster-b create -f -

to clone one cluster into another

smarterclayton · 2015-11-20T01:20:16Z

I think export is different from the intent of getterwithoptions. The
latter is intended to support non-uniform subresources. We could simply
dictate that the presence of export / exact triggers the Export interface,
which does get and then passes that through the strategy filter. I did not
originally intent for getter with options and normal get to be used on base
resources (only virtual ones). Getterwithoptions is an escape hatch rather
than something like watch (export to me is a lot like watch)

On Nov 19, 2015, at 7:15 PM, Brendan Burns notifications@github.com wrote:

@smarterclayton https://github.com/smarterclayton what you suggest is
tricky to do given the way the framework is put together. The best you
could do is either pass an Exporter to GetResourceWithOptions/GetResource
or augment the Getter and GetterWithOptions interfaces to include an Export
method.

I think another way to achieve what you are asking for @smarterclayton
https://github.com/smarterclayton is to allow upstream users to inject
their own Exporter, that seems to accomplish what you need.

@bgrant0607 https://github.com/bgrant0607 there is already a
GetterWithOptions but those options are an opaque runtime.Object so we
would have to add an ExportOptions object from the query, but we would have
to plumb it in addition to the existing Getter/GetterWithOptions interfaces
(see above).

So then my concrete proposal would be to make

type GetterWithOptions interface {
Get(ctx api.Context, name string, opts runtime.Object, export ExportOptions)
}
type Getter interface {
Get(ctx api.Context, name string, export ExportOptions)
}

and then drive that down to a concrete implementation in the Strategy.

Does that work for both people?

I would very much like to enable:

kubectl --context=cluster-a export all | kubctl --context=cluster-b create -f -

to clone one cluster into another

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

brendandburns · 2015-11-20T17:28:52Z

So to be concrete, what you are suggesting is that in GetResource we cast the Getter to an Exporter if the export options are present on the request? That is do-able. Alternately, we could force the GetResource handler to take an Exporter as a parameter and have it be nil if export isn't supported.

Let me know which one you prefer.

smarterclayton · 2015-11-20T21:58:23Z

The latter - sorry for the delay, in meetings.

On Fri, Nov 20, 2015 at 12:29 PM, Brendan Burns notifications@github.com
wrote:

So to be concrete, what you are suggesting is that in GetResource we cast
the Getter to an Exporter if the export options are present on the
request? That is do-able. Alternately, we could force the GetResource
handler to take an Exporter as a parameter and have it be nil if export
isn't supported.

Let me know which one you prefer.

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

bgrant0607 · 2015-11-20T23:26:59Z

cc @lavalamp

lavalamp · 2015-11-21T00:11:55Z

pkg/api/export/exporter.go

I would strongly prefer to see something added to the strategy (like the list, get, create, etc behaviors). You can even add a default to the generic one and get most of this for free, it looks like.

k8s-bot · 2015-11-21T05:41:05Z

GCE e2e build/test failed for commit 4bf228835229f65458b8d0872f8a9101e66e72dd.

brendandburns · 2015-11-21T05:41:57Z

Moved over to the strategy based approach suggested by @smarterclayton, @lavalamp and others. please take another look.

--brendan

k8s-bot · 2015-11-21T06:11:14Z

GCE e2e build/test failed for commit 09681562d70b0165279e335c9c3171f48e851d9b.

k8s-bot · 2015-11-21T06:14:34Z

GCE e2e build/test failed for commit 44d948dcf15e642a40e9285849b41e7142a9bbae.

brendandburns · 2015-11-23T21:53:04Z

ping @smarterclayton

@k8s-bot please test this

smarterclayton · 2015-11-23T22:10:16Z

pkg/apiserver/resthandler.go

Why do we need this check? Would delegate it to the strategy implementor.

Why force every strategy to put the same boilerplate at the top of each export method?

You don't need to do it in the strategy. The etcd generic store can do it and call the strategy to do the rest. I think it'd make more sense there.

smarterclayton · 2015-11-23T22:13:58Z

A few comments, also needs a test in apiserver_test, and a strategy test for each type.

k8s-bot · 2015-12-16T22:55:48Z

GCE e2e test build/test passed for commit 060836c4570deacaf98322a0095a6134375fb9de.

k8s-bot · 2015-12-16T22:59:03Z

GCE e2e test build/test passed for commit fb55ab4c8280b8d2f6d79f79db20e9361aff696e.

k8s-github-robot · 2015-12-16T23:15:13Z

Labelling this PR as size/XL

k8s-bot · 2015-12-16T23:17:18Z

GCE e2e test build/test passed for commit 8cfe4a72a202e16cb125c3ab5ec9f8aea0d42e6f.

k8s-bot · 2015-12-16T23:18:32Z

GCE e2e test build/test passed for commit dc8f1683ae4081f0a3c0594f84d499ebc99beba7.

k8s-bot · 2015-12-16T23:56:39Z

GCE e2e test build/test passed for commit 2efcccf.

brendandburns · 2015-12-16T23:57:46Z

@k8s-bot unit test this please

k8s-github-robot · 2015-12-17T03:51:05Z

Labelling this PR as size/XXL

k8s-bot · 2015-12-17T04:18:03Z

GCE e2e test build/test passed for commit a60069b.

…into kubectl5

k8s-github-robot · 2015-12-17T04:29:23Z

Labelling this PR as size/XS

k8s-bot · 2015-12-17T04:52:58Z

GCE e2e test build/test passed for commit 884c163.

k8s-github-robot · 2015-12-17T05:25:53Z

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

k8s-bot · 2015-12-17T05:57:18Z

GCE e2e test build/test passed for commit 884c163.

k8s-github-robot · 2015-12-17T05:57:33Z

Automatic merge from submit-queue

googlebot added the cla: yes label Nov 19, 2015

k8s-github-robot assigned bgrant0607 Nov 19, 2015

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 19, 2015

bgrant0607 added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Nov 19, 2015

smarterclayton reviewed Nov 19, 2015
View reviewed changes

bgrant0607 reviewed Nov 19, 2015
View reviewed changes

lavalamp reviewed Nov 21, 2015
View reviewed changes

brendandburns force-pushed the kubectl5 branch from 2014d97 to 4bf2288 Compare November 21, 2015 05:37

brendandburns force-pushed the kubectl5 branch from 4bf2288 to 0968156 Compare November 21, 2015 05:41

brendandburns force-pushed the kubectl5 branch from 0968156 to 44d948d Compare November 21, 2015 05:44

smarterclayton reviewed Nov 23, 2015
View reviewed changes

brendandburns force-pushed the kubectl5 branch from 44d948d to 32b0825 Compare December 1, 2015 05:55

brendandburns force-pushed the kubectl5 branch 2 times, most recently from dc8f168 to 8cfe4a7 Compare December 16, 2015 22:31

Add a server side export facility

2efcccf

brendandburns force-pushed the kubectl5 branch from 8cfe4a7 to 2efcccf Compare December 16, 2015 23:01

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 16, 2015

Add a server side export facility

a60069b

brendandburns force-pushed the kubectl5 branch from 2efcccf to a60069b Compare December 17, 2015 03:42

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 17, 2015

Merge branch 'kubectl5' of https://github.com/brendandburns/kubernetes …

884c163

…into kubectl5

k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 17, 2015

bgrant0607 mentioned this pull request Apr 19, 2016

Kubectl needs export and import commands #21582

Closed

0xmichalis mentioned this pull request Apr 27, 2016

Implement export functionality server-side for all resources #24855

Closed

3 tasks

soltysh mentioned this pull request Mar 8, 2017

Proposal: Alternate API representations for resources kubernetes/community#123

Merged

Conversation

brendandburns commented Nov 19, 2015

Uh oh!

k8s-github-robot commented Nov 19, 2015

Uh oh!

bgrant0607 commented Nov 19, 2015

Uh oh!

k8s-bot commented Nov 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgrant0607 commented Nov 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgrant0607 commented Nov 19, 2015

Uh oh!

brendandburns commented Nov 20, 2015

Uh oh!

smarterclayton commented Nov 20, 2015

Uh oh!

brendandburns commented Nov 20, 2015

Uh oh!

smarterclayton commented Nov 20, 2015

Uh oh!

bgrant0607 commented Nov 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-bot commented Nov 21, 2015

Uh oh!

brendandburns commented Nov 21, 2015

Uh oh!

k8s-bot commented Nov 21, 2015

Uh oh!

k8s-bot commented Nov 21, 2015

Uh oh!

brendandburns commented Nov 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Nov 23, 2015

Uh oh!

k8s-bot commented Dec 16, 2015

Uh oh!

k8s-bot commented Dec 16, 2015

Uh oh!

k8s-github-robot commented Dec 16, 2015

Uh oh!

k8s-bot commented Dec 16, 2015

Uh oh!

k8s-bot commented Dec 16, 2015

Uh oh!

k8s-bot commented Dec 16, 2015

Uh oh!

brendandburns commented Dec 16, 2015

Uh oh!

k8s-github-robot commented Dec 17, 2015

Uh oh!

k8s-bot commented Dec 17, 2015

Uh oh!

k8s-github-robot commented Dec 17, 2015

Uh oh!

k8s-bot commented Dec 17, 2015

Uh oh!

k8s-github-robot commented Dec 17, 2015