New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eliminate round-trip conversion of API objects in kubectl #3955

Open
yujuhong opened this Issue Jan 29, 2015 · 29 comments

Comments

Projects
None yet
@yujuhong
Contributor

yujuhong commented Jan 29, 2015

kubectl decodes a json file and encodes it again before sending out the request to the API server, i.e.,
versioned json-> versioned api object -> internal api object -> versioned api object -> versioned json
The API server then decodes the versioned json and converts it to the internal API object.

It is debatable whether kubectl needs to perform the round-trip conversion, since the API server would perform the similar process immediately after receiving the request. Can we simplify this?

@bgrant0607 @smarterclayton

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Jan 29, 2015

Contributor

Right now, the mutation we are doing on create/update is to set the namespace of the object, and during update we set the resource version. Ideally, we'd be able to do this opaquely, and with less conversions (perhaps by having a runtime.Object that supported deferred decoding by working a bit like runtime.Unknown). Right now we don't assume that it's safe to mutate the JSON directly because we'd have to write code for each object type that says what sort of mutation we need for metadata (we can read it from the scheme via the name conversion rules, but it's a bit tricky because we don't define what fields are metadata). The assumption that an apiversion can have rules defined on it is invalid (because eventually core resources will split and migrate at different times, so they may have different rules).

Once we nuke v1beta1/2, it would be a good time to go through the code and simplify a bit to say that there is a fast path for certain known object types (core resources) which can bypass direct conversion and do mutation directly on the JSON, and the rest can use the slower pass for mutation.

----- Original Message -----

kubectl decodes a json file and encodes it again before sending out the
request to the API server, i.e.,
versioned json-> versioned api object -> internal api object -> versioned api object -> versioned json
The API server then decodes the versioned json and converts it to the
internal API object.

It is debatable whether kubectl needs to perform the round-trip conversion,
since the API server would perform the similar process immediately after
receiving the request. Can we simplify this?

@bgrant0607 @smarterclayton


Reply to this email directly or view it on GitHub:
#3955

Contributor

smarterclayton commented Jan 29, 2015

Right now, the mutation we are doing on create/update is to set the namespace of the object, and during update we set the resource version. Ideally, we'd be able to do this opaquely, and with less conversions (perhaps by having a runtime.Object that supported deferred decoding by working a bit like runtime.Unknown). Right now we don't assume that it's safe to mutate the JSON directly because we'd have to write code for each object type that says what sort of mutation we need for metadata (we can read it from the scheme via the name conversion rules, but it's a bit tricky because we don't define what fields are metadata). The assumption that an apiversion can have rules defined on it is invalid (because eventually core resources will split and migrate at different times, so they may have different rules).

Once we nuke v1beta1/2, it would be a good time to go through the code and simplify a bit to say that there is a fast path for certain known object types (core resources) which can bypass direct conversion and do mutation directly on the JSON, and the rest can use the slower pass for mutation.

----- Original Message -----

kubectl decodes a json file and encodes it again before sending out the
request to the API server, i.e.,
versioned json-> versioned api object -> internal api object -> versioned api object -> versioned json
The API server then decodes the versioned json and converts it to the
internal API object.

It is debatable whether kubectl needs to perform the round-trip conversion,
since the API server would perform the similar process immediately after
receiving the request. Can we simplify this?

@bgrant0607 @smarterclayton


Reply to this email directly or view it on GitHub:
#3955

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607

bgrant0607 Jan 31, 2015

Member

Some observations:

  • So long as defaults are set by kubectl, we'll need to capture user intent first in order to facilitate merging an updated configuration with the live desired state.
  • Obvious but worth stating: apiserver can't rely on kubectl to apply defaults.
    1. The API has other clients.
    2. We need to retain the ability to add new fields without changing the api version.
Member

bgrant0607 commented Jan 31, 2015

Some observations:

  • So long as defaults are set by kubectl, we'll need to capture user intent first in order to facilitate merging an updated configuration with the live desired state.
  • Obvious but worth stating: apiserver can't rely on kubectl to apply defaults.
    1. The API has other clients.
    2. We need to retain the ability to add new fields without changing the api version.
@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin

thockin Jan 31, 2015

Member

Yeah, its dubious that clients should even be able to see internal structs
and conversion. The obvious exception is validation, with the caveat that
it could drift.

If validation is applied to versioned structs, then clients can stay
versioned. To get validation we will need per-version defaulting.
Luckily....
On Jan 30, 2015 7:39 PM, "Brian Grant" notifications@github.com wrote:

Some observations:

  • So long as defaults are set by kubectl, we'll need to capture user
    intent first in order to facilitate merging an updated configuration with
    the live desired state.
  • Obvious but worth stating: apiserver can't rely on kubectl to apply
    defaults.
    1. The API has other clients.
    2. We need to retain the ability to add new fields without changing
      the api version.

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

Member

thockin commented Jan 31, 2015

Yeah, its dubious that clients should even be able to see internal structs
and conversion. The obvious exception is validation, with the caveat that
it could drift.

If validation is applied to versioned structs, then clients can stay
versioned. To get validation we will need per-version defaulting.
Luckily....
On Jan 30, 2015 7:39 PM, "Brian Grant" notifications@github.com wrote:

Some observations:

  • So long as defaults are set by kubectl, we'll need to capture user
    intent first in order to facilitate merging an updated configuration with
    the live desired state.
  • Obvious but worth stating: apiserver can't rely on kubectl to apply
    defaults.
    1. The API has other clients.
    2. We need to retain the ability to add new fields without changing
      the api version.

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

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607

bgrant0607 Jan 31, 2015

Member

We should use (potentially cached) swagger for basic (optional) client-side validation. That approach will work with newer releases, new versions, and new objects (plugins, etc.).

Detailed validation that can't be expressed via swagger can be performed by the server.

Member

bgrant0607 commented Jan 31, 2015

We should use (potentially cached) swagger for basic (optional) client-side validation. That approach will work with newer releases, new versions, and new objects (plugins, etc.).

Detailed validation that can't be expressed via swagger can be performed by the server.

@derekwaynecarr

This comment has been minimized.

Show comment
Hide comment
@derekwaynecarr

derekwaynecarr Jan 31, 2015

Member

Some of the work I did with RESTMapper last week could be used in the future where we introspect swagger and build a RESTMapper from it on the client.

Sent from my iPhone

On Jan 31, 2015, at 1:38 AM, Brian Grant notifications@github.com wrote:

We should use (potentially cached) swagger for basic (optional) client-side validation. That approach will work with newer releases, new versions, and new objects (plugins, etc.).

Detailed validation that can't be expressed via swagger can be performed by the server.


Reply to this email directly or view it on GitHub.

Member

derekwaynecarr commented Jan 31, 2015

Some of the work I did with RESTMapper last week could be used in the future where we introspect swagger and build a RESTMapper from it on the client.

Sent from my iPhone

On Jan 31, 2015, at 1:38 AM, Brian Grant notifications@github.com wrote:

We should use (potentially cached) swagger for basic (optional) client-side validation. That approach will work with newer releases, new versions, and new objects (plugins, etc.).

Detailed validation that can't be expressed via swagger can be performed by the server.


Reply to this email directly or view it on GitHub.

@yujuhong

This comment has been minimized.

Show comment
Hide comment
@yujuhong

yujuhong Feb 3, 2015

Contributor

Agreed that clients should not have to perform the conversion. What's the main purpose of client-side validation?

Contributor

yujuhong commented Feb 3, 2015

Agreed that clients should not have to perform the conversion. What's the main purpose of client-side validation?

@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin

thockin Feb 3, 2015

Member

Client-side validation can be lower-latency.

On Tue, Feb 3, 2015 at 11:58 AM, Yu-Ju Hong notifications@github.com
wrote:

Agreed that clients should not have to perform the conversion. What's the
main purpose of client-side validation?

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

Member

thockin commented Feb 3, 2015

Client-side validation can be lower-latency.

On Tue, Feb 3, 2015 at 11:58 AM, Yu-Ju Hong notifications@github.com
wrote:

Agreed that clients should not have to perform the conversion. What's the
main purpose of client-side validation?

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

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Feb 3, 2015

Contributor

Clients can be about validation, which increases user frustration (go to web UI, set something, go to client, can't set it, hate product forever). I would argue client validation is always optional and as much as possible comes from source of truth (schema exposed a la swagger, which is a reasonable compromise).

----- Original Message -----

Client-side validation can be lower-latency.

On Tue, Feb 3, 2015 at 11:58 AM, Yu-Ju Hong notifications@github.com
wrote:

Agreed that clients should not have to perform the conversion. What's the
main purpose of client-side validation?

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


Reply to this email directly or view it on GitHub:
#3955 (comment)

Contributor

smarterclayton commented Feb 3, 2015

Clients can be about validation, which increases user frustration (go to web UI, set something, go to client, can't set it, hate product forever). I would argue client validation is always optional and as much as possible comes from source of truth (schema exposed a la swagger, which is a reasonable compromise).

----- Original Message -----

Client-side validation can be lower-latency.

On Tue, Feb 3, 2015 at 11:58 AM, Yu-Ju Hong notifications@github.com
wrote:

Agreed that clients should not have to perform the conversion. What's the
main purpose of client-side validation?

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


Reply to this email directly or view it on GitHub:
#3955 (comment)

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Feb 3, 2015

Contributor

"can be very wrong"

----- Original Message -----

Clients can be about validation, which increases user frustration (go to web
UI, set something, go to client, can't set it, hate product forever). I
would argue client validation is always optional and as much as possible
comes from source of truth (schema exposed a la swagger, which is a
reasonable compromise).

----- Original Message -----

Client-side validation can be lower-latency.

On Tue, Feb 3, 2015 at 11:58 AM, Yu-Ju Hong notifications@github.com
wrote:

Agreed that clients should not have to perform the conversion. What's the
main purpose of client-side validation?

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


Reply to this email directly or view it on GitHub:
#3955 (comment)

Contributor

smarterclayton commented Feb 3, 2015

"can be very wrong"

----- Original Message -----

Clients can be about validation, which increases user frustration (go to web
UI, set something, go to client, can't set it, hate product forever). I
would argue client validation is always optional and as much as possible
comes from source of truth (schema exposed a la swagger, which is a
reasonable compromise).

----- Original Message -----

Client-side validation can be lower-latency.

On Tue, Feb 3, 2015 at 11:58 AM, Yu-Ju Hong notifications@github.com
wrote:

Agreed that clients should not have to perform the conversion. What's the
main purpose of client-side validation?

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


Reply to this email directly or view it on GitHub:
#3955 (comment)

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607

bgrant0607 Feb 4, 2015

Member

Agreed that we want the source of truth to come from the server.

Member

bgrant0607 commented Feb 4, 2015

Agreed that we want the source of truth to come from the server.

@bgrant0607 bgrant0607 changed the title from Assess whether round-trip conversion of API objects is necessary in kubectl to Eliminate round-trip conversion of API objects in kubectl Feb 4, 2015

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607

bgrant0607 Mar 19, 2015

Member

Found that the test for template-based printing in kubectl (resource_printer_test.go) failed when I removed the json tags on api/types.go for #3933.

Member

bgrant0607 commented Mar 19, 2015

Found that the test for template-based printing in kubectl (resource_printer_test.go) failed when I removed the json tags on api/types.go for #3933.

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607
Member

bgrant0607 commented Apr 2, 2015

cc @vishh

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607

bgrant0607 Jun 18, 2015

Member

Another wrinkle -- version negotiation: #9762

Member

bgrant0607 commented Jun 18, 2015

Another wrinkle -- version negotiation: #9762

@krousey

This comment has been minimized.

Show comment
Hide comment
@krousey

krousey May 6, 2016

Member

My plan to attack this is to decode things to runtime.Unstructured and use the dynamic client for most operations. Here's a quick guess at commands that can be switched to decoding to runtime.Unstructured

  • annotate
  • apply
  • create
  • delete
  • edit
  • get
  • label
  • replace

I plan on adding a method to cmdutil.Factory to get dynamic clients, and the command by command, switching the decoder and changing use of Helper to dynamic client.

Member

krousey commented May 6, 2016

My plan to attack this is to decode things to runtime.Unstructured and use the dynamic client for most operations. Here's a quick guess at commands that can be switched to decoding to runtime.Unstructured

  • annotate
  • apply
  • create
  • delete
  • edit
  • get
  • label
  • replace

I plan on adding a method to cmdutil.Factory to get dynamic clients, and the command by command, switching the decoder and changing use of Helper to dynamic client.

@krousey

This comment has been minimized.

Show comment
Hide comment
@krousey

krousey May 6, 2016

Member

cc @kubernetes/sig-api-machinery @kubernetes/rh-ux

Member

krousey commented May 6, 2016

cc @kubernetes/sig-api-machinery @kubernetes/rh-ux

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton May 6, 2016

Contributor

Sounds reasonable to me. I'm currently concerned that there is use of the
third party decoder in certain methods - we need to put that under the
decoder returned by the factory in some code paths, and you may hit that.

On Fri, May 6, 2016 at 12:56 PM, krousey notifications@github.com wrote:

My plan to attack this is to decode things to runtime.Unstructured and
use the dynamic client for most operations. Here's a quick guess at
commands that can be switched to decoding to runtime.Unstructured

  • annotate
  • apply
  • create
  • delete
  • edit
  • get
  • label
  • replace

I plan on adding a method to cmdutil.Factory to get dynamic clients, and
the command by command, switching the decoder and changing use of Helper
to dynamic client.


You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
#3955 (comment)

Contributor

smarterclayton commented May 6, 2016

Sounds reasonable to me. I'm currently concerned that there is use of the
third party decoder in certain methods - we need to put that under the
decoder returned by the factory in some code paths, and you may hit that.

On Fri, May 6, 2016 at 12:56 PM, krousey notifications@github.com wrote:

My plan to attack this is to decode things to runtime.Unstructured and
use the dynamic client for most operations. Here's a quick guess at
commands that can be switched to decoding to runtime.Unstructured

  • annotate
  • apply
  • create
  • delete
  • edit
  • get
  • label
  • replace

I plan on adding a method to cmdutil.Factory to get dynamic clients, and
the command by command, switching the decoder and changing use of Helper
to dynamic client.


You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
#3955 (comment)

@krousey

This comment has been minimized.

Show comment
Hide comment
@krousey

krousey May 6, 2016

Member

@smarterclayton Just took a quick peek at the third party codec. From what it looks like, the dynamic client would exhibit the exact same file->wire behavior just with different internal representation. I'll be sure to cc on PRs and maybe you can point out what you mean.

Member

krousey commented May 6, 2016

@smarterclayton Just took a quick peek at the third party codec. From what it looks like, the dynamic client would exhibit the exact same file->wire behavior just with different internal representation. I'll be sure to cc on PRs and maybe you can point out what you mean.

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton May 6, 2016

Contributor

All I meant was that no code outside of cmdutil.Factory() needs to be aware
of ThirdPartyCodec. Right now, it's littered through the codebase next to
the codec we pass through - the original intent of the RESTMapper and Codec
and ObjectTyper interfaces is that they're supposed to hide those details
from consumers in the code.

On Fri, May 6, 2016 at 1:23 PM, krousey notifications@github.com wrote:

@smarterclayton https://github.com/smarterclayton Just took a quick
peek at the third party codec. From what it looks like, the dynamic client
would exhibit the exact same file->wire behavior just with different
internal representation. I'll be sure to cc on PRs and maybe you can point
out what you mean.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#3955 (comment)

Contributor

smarterclayton commented May 6, 2016

All I meant was that no code outside of cmdutil.Factory() needs to be aware
of ThirdPartyCodec. Right now, it's littered through the codebase next to
the codec we pass through - the original intent of the RESTMapper and Codec
and ObjectTyper interfaces is that they're supposed to hide those details
from consumers in the code.

On Fri, May 6, 2016 at 1:23 PM, krousey notifications@github.com wrote:

@smarterclayton https://github.com/smarterclayton Just took a quick
peek at the third party codec. From what it looks like, the dynamic client
would exhibit the exact same file->wire behavior just with different
internal representation. I'll be sure to cc on PRs and maybe you can point
out what you mean.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#3955 (comment)

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp May 26, 2016

Member

This didn't make the date. :'(

Member

lavalamp commented May 26, 2016

This didn't make the date. :'(

@lavalamp lavalamp modified the milestones: v1.4, v1.3 May 26, 2016

@janetkuo janetkuo added the team/ux label Jun 10, 2016

juanvallejo pushed a commit to juanvallejo/kubernetes that referenced this issue Aug 17, 2016

Merge pull request #30250 from krousey/kctl_dynamic
Automatic merge from submit-queue

Change kubectl create to use dynamic client

kubernetes#16764 kubernetes#3955

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**:
<!--  Steps to write your release note:
1. Use the release-note-* labels to set the release note state (if you have access) 
2. Enter your extended release note in the below block; leaving it blank means using the PR title as the release note. If no release note is required, just write `NONE`. 
-->
```release-note
kubectl will no longer do client-side defaulting on create and replace.
```
@krousey

This comment has been minimized.

Show comment
Hide comment
@krousey

krousey Aug 25, 2016

Member

With my most recent change, kubectl replace and create no longer do round trip conversions on the client side. I believe that covers most of the bugs we had with client-side conversion/defaulting.

Member

krousey commented Aug 25, 2016

With my most recent change, kubectl replace and create no longer do round trip conversions on the client side. I believe that covers most of the bugs we had with client-side conversion/defaulting.

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Aug 25, 2016

Contributor
Contributor

smarterclayton commented Aug 25, 2016

@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin

thockin Aug 25, 2016

Member

w00t! that's huge.

On Thu, Aug 25, 2016 at 8:52 AM, Clayton Coleman notifications@github.com
wrote:

Phenomenal


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#3955 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVNxqTOM2P94CECuae7ERDH7ZuU11ks5qjbo2gaJpZM4DZOOC
.

Member

thockin commented Aug 25, 2016

w00t! that's huge.

On Thu, Aug 25, 2016 at 8:52 AM, Clayton Coleman notifications@github.com
wrote:

Phenomenal


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#3955 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVNxqTOM2P94CECuae7ERDH7ZuU11ks5qjbo2gaJpZM4DZOOC
.

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Aug 25, 2016

Member

@krousey is there more to do or can we close this issue?

Member

lavalamp commented Aug 25, 2016

@krousey is there more to do or can we close this issue?

@krousey

This comment has been minimized.

Show comment
Hide comment
@krousey

krousey Aug 25, 2016

Member

@lavalamp Because of the way we do patching (relies on data being of the same version and also needs versioned struct tags for merge strategies), anything that computes a strategic merge patch still does client side conversion. I may have missed a few, but that's apply, edit, label, and annotate. I think label and annotate don't have any danger of introducing fields that have defaulting. apply and edit could potentially introduce new fields to an object that has some defaulted internals. It is possible, but I think most fields are defaulted before clients that apply or edit see the object.

We'd have to fix how we compute patches client-side (and/or handle them server-side) to change that. I don't know if we want to attempt that, and if we do, whether this is the place to track that effort.

Member

krousey commented Aug 25, 2016

@lavalamp Because of the way we do patching (relies on data being of the same version and also needs versioned struct tags for merge strategies), anything that computes a strategic merge patch still does client side conversion. I may have missed a few, but that's apply, edit, label, and annotate. I think label and annotate don't have any danger of introducing fields that have defaulting. apply and edit could potentially introduce new fields to an object that has some defaulted internals. It is possible, but I think most fields are defaulted before clients that apply or edit see the object.

We'd have to fix how we compute patches client-side (and/or handle them server-side) to change that. I don't know if we want to attempt that, and if we do, whether this is the place to track that effort.

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Aug 26, 2016

Contributor
Contributor

smarterclayton commented Aug 26, 2016

@lavalamp lavalamp removed this from the v1.4 milestone Aug 29, 2016

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Aug 29, 2016

Member

Kicking this out of the milestone since we're not planning on doing more at the moment.

Member

lavalamp commented Aug 29, 2016

Kicking this out of the milestone since we're not planning on doing more at the moment.

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Jun 1, 2017

Contributor

This was mostly completed in 1.6. Some specific commands still use internal version.

Contributor

smarterclayton commented Jun 1, 2017

This was mostly completed in 1.6. Some specific commands still use internal version.

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Dec 25, 2017

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

Prevent issues from auto-closing with an /lifecycle frozen comment.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle stale

fejta-bot commented Dec 25, 2017

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

Prevent issues from auto-closing with an /lifecycle frozen comment.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle stale

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607

bgrant0607 Jan 22, 2018

Member

/remove-lifecycle stale
/lifecycle frozen

Member

bgrant0607 commented Jan 22, 2018

/remove-lifecycle stale
/lifecycle frozen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment