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

Versioned clients #4874

Closed
lavalamp opened this Issue Feb 26, 2015 · 38 comments

Comments

@lavalamp
Member

lavalamp commented Feb 26, 2015

Right now, pkg/client auto-converts everything to our internal, unversioned format. This is convenient for kubernetes components, but not so convenient for any 3rd party users. (The whole point of the versioning system is so that we can make changes to the unversioned structs without affecting downstream users.)

So we should maintain versioned clients.

It is possible that this is as easy as copying pkg/client and replacing the "pkg/api" imports with api "pkg/api/v1beta1". If so, we should do this soon. If it turns out to be much more complex than that, we can defer this change until after 1.0, I think.

When we deprecate an old client version, we should be nice and provide people a list of gofmt rewrite rules which will switch them to the next client version.

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607

bgrant0607 Feb 26, 2015

Member

See #3955 and #1451 (comment).

Clients generating specific objects, such as kubectl run-container, should target a specific API version (or multiple specific versions).

Generic clients shouldn't be versioned. They should only get the schema via swagger.

Member

bgrant0607 commented Feb 26, 2015

See #3955 and #1451 (comment).

Clients generating specific objects, such as kubectl run-container, should target a specific API version (or multiple specific versions).

Generic clients shouldn't be versioned. They should only get the schema via swagger.

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Apr 1, 2015

Member

On the timing of when to fix this. Goal is we want pkg/client users to work with versioned api types instead of unversioned types. So at some point we'll want to alias api to api/v1beta3 (in e.g. test code).

When we have v1beta3 as default but before we've accumulated many changes, then the diff between internal types and v1beta3 types should be minimized. ...so now would be a good time to think about doing this, actually.

Member

lavalamp commented Apr 1, 2015

On the timing of when to fix this. Goal is we want pkg/client users to work with versioned api types instead of unversioned types. So at some point we'll want to alias api to api/v1beta3 (in e.g. test code).

When we have v1beta3 as default but before we've accumulated many changes, then the diff between internal types and v1beta3 types should be minimized. ...so now would be a good time to think about doing this, actually.

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607
Member

bgrant0607 commented Apr 1, 2015

cc @vishh

@vishh

This comment has been minimized.

Show comment
Hide comment
@vishh

vishh Apr 1, 2015

Member

@lavalamp: My understanding was that we are are doing this before v1.0.

On Wed, Apr 1, 2015 at 10:24 AM, Brian Grant notifications@github.com
wrote:

cc @vishh https://github.com/vishh


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

Member

vishh commented Apr 1, 2015

@lavalamp: My understanding was that we are are doing this before v1.0.

On Wed, Apr 1, 2015 at 10:24 AM, Brian Grant notifications@github.com
wrote:

cc @vishh https://github.com/vishh


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

@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin

thockin Apr 10, 2015

Member

I want to argue for a prio boost on this. If we go 1.0 and we leave internal structs exposed, we're as good as frozen.

We should either

a) hide all the internal structs and have purely versioned client libs (how do we get DeltaFIFO and cache and controller framework etc for versioned?)
b) use swagger codegen to generate Go libs
c) not have a Go client library at all

Almost all of our own components would be better off with versioned interfaces, but I think all the work that has been done on client libs and controller framework has to apply.

I propose P2, enhancement, v1.0 milestone. Needs an owner ASAP.

Member

thockin commented Apr 10, 2015

I want to argue for a prio boost on this. If we go 1.0 and we leave internal structs exposed, we're as good as frozen.

We should either

a) hide all the internal structs and have purely versioned client libs (how do we get DeltaFIFO and cache and controller framework etc for versioned?)
b) use swagger codegen to generate Go libs
c) not have a Go client library at all

Almost all of our own components would be better off with versioned interfaces, but I think all the work that has been done on client libs and controller framework has to apply.

I propose P2, enhancement, v1.0 milestone. Needs an owner ASAP.

@satnam6502

This comment has been minimized.

Show comment
Hide comment
@satnam6502

satnam6502 Apr 10, 2015

Contributor

Casting aside the fact the either is not a ternary operator, if nobody else want to take this on then I'd be happy to help esp. if we follow Daniel's proposal to have versioned Go client libraries but address the issues in (a). I think Brain said there were some technicalities to do with (b) w.r.t. encoding for a few fields but perhaps these could be overcome somehow. (c) would be a shame.

Contributor

satnam6502 commented Apr 10, 2015

Casting aside the fact the either is not a ternary operator, if nobody else want to take this on then I'd be happy to help esp. if we follow Daniel's proposal to have versioned Go client libraries but address the issues in (a). I think Brain said there were some technicalities to do with (b) w.r.t. encoding for a few fields but perhaps these could be overcome somehow. (c) would be a shame.

@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin

thockin Apr 10, 2015

Member

One option is to get an intern to work on (b) but it might be too little
too late

On Fri, Apr 10, 2015 at 12:04 AM, Satnam Singh notifications@github.com
wrote:

Casting aside the fact the either is not a ternary operator, if nobody
else want to take this on then I'd be happy to help esp. if follow Daniel's
proposal to have versioned Go client libraries but address the issues in
(a). I think Brain said there were some technicalities to do with (b)
w.r.t. encoding for a few fields but perhaps these could be overcome
somehow. (c) would be a shame.


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

Member

thockin commented Apr 10, 2015

One option is to get an intern to work on (b) but it might be too little
too late

On Fri, Apr 10, 2015 at 12:04 AM, Satnam Singh notifications@github.com
wrote:

Casting aside the fact the either is not a ternary operator, if nobody
else want to take this on then I'd be happy to help esp. if follow Daniel's
proposal to have versioned Go client libraries but address the issues in
(a). I think Brain said there were some technicalities to do with (b)
w.r.t. encoding for a few fields but perhaps these could be overcome
somehow. (c) would be a shame.


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

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607

bgrant0607 Apr 10, 2015

Member

Swagger codegen for Go doesn't yet exist. And it doesn't yet work for any language for our API AFAIK.

There are no intrinsic blockers to codegen that I'm aware of. Some of the fields that require custom marshaling/unmarshaling would need custom client code, but that doesn't seem like a big deal. I don't think most clients would need complicated types, such as Resource Quantity. Users could just provide the strings.

If we have time, this would be great to do sooner rather than later. The problem will get bigger over time. I'm not sure there is a hard deadline, though.

Member

bgrant0607 commented Apr 10, 2015

Swagger codegen for Go doesn't yet exist. And it doesn't yet work for any language for our API AFAIK.

There are no intrinsic blockers to codegen that I'm aware of. Some of the fields that require custom marshaling/unmarshaling would need custom client code, but that doesn't seem like a big deal. I don't think most clients would need complicated types, such as Resource Quantity. Users could just provide the strings.

If we have time, this would be great to do sooner rather than later. The problem will get bigger over time. I'm not sure there is a hard deadline, though.

@satnam6502

This comment has been minimized.

Show comment
Hide comment
@satnam6502

satnam6502 Apr 10, 2015

Contributor

Indeed swagger codegen for Go seems to be an open issue: swagger-api/swagger-codegen#325

@bgrant0607 : let me be sure about what you mean. You mean "write a program that parses the swagger spec and spits out a Go client library?" Or do you mean something else?

Contributor

satnam6502 commented Apr 10, 2015

Indeed swagger codegen for Go seems to be an open issue: swagger-api/swagger-codegen#325

@bgrant0607 : let me be sure about what you mean. You mean "write a program that parses the swagger spec and spits out a Go client library?" Or do you mean something else?

@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin

thockin Apr 10, 2015

Member

My concern is that, based on our own experiences and the evolution of the
client-side caches and watch helper libs, the swagger generated client API
will be insufficient.

On Fri, Apr 10, 2015 at 9:23 AM, Satnam Singh notifications@github.com
wrote:

Indeed swagger codegen for Go seems to be an open issue:
swagger-api/swagger-codegen#325
swagger-api/swagger-codegen#325

@bgrant0607 https://github.com/bgrant0607 : let me be sure about what
you mean. You mean "write a program that parses the swagger spec and spits
out a Go client library?" Or do you mean something else?


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

Member

thockin commented Apr 10, 2015

My concern is that, based on our own experiences and the evolution of the
client-side caches and watch helper libs, the swagger generated client API
will be insufficient.

On Fri, Apr 10, 2015 at 9:23 AM, Satnam Singh notifications@github.com
wrote:

Indeed swagger codegen for Go seems to be an open issue:
swagger-api/swagger-codegen#325
swagger-api/swagger-codegen#325

@bgrant0607 https://github.com/bgrant0607 : let me be sure about what
you mean. You mean "write a program that parses the swagger spec and spits
out a Go client library?" Or do you mean something else?


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

@davidopp

This comment has been minimized.

Show comment
Hide comment
@davidopp

davidopp Apr 23, 2015

Member

/subscribe

Member

davidopp commented Apr 23, 2015

/subscribe

@satnam6502

This comment has been minimized.

Show comment
Hide comment
@satnam6502

satnam6502 Apr 23, 2015

Contributor

If this is something that needs attention soon thenI am happy to direct some energy in this direction, although that may not please Tim since I am supposed to be learning about networking.

Contributor

satnam6502 commented Apr 23, 2015

If this is something that needs attention soon thenI am happy to direct some energy in this direction, although that may not please Tim since I am supposed to be learning about networking.

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607

bgrant0607 May 4, 2015

Member

Package factoring to consider: #7584.

Member

bgrant0607 commented May 4, 2015

Package factoring to consider: #7584.

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Jul 14, 2015

Member

OK. Time to do this. The motivation is so that people can import pkg/client/v1 and have code that will continue to build against head without changes as long as v1 is supported.

Proposed factoring:

  • pkg/client/config
    • This has everything to do with certs, setup, etc.
  • pkg/client/rest
    • RESTClient moves here
  • pkg/client/unversioned
    • the current client.Client struct and interfaces move here.
  • pkg/client/v1
    • A copy of unversioned, but using pkg/api/v1 instead of pkg/api

I propose doing this in a few steps.

  1. Copy pkg/client to pkg/client/v1 AND pkg/client/unversioned.
    1. Change the import lines in pkg/client/v1 to go to pkg/api/v1.
  2. Change references in the rest of the codebase to the appropriate client.
    1. I know @bgrant0607 would love it if nothing ended up referencing pkg/client/unversioned. I wouldn't prioritize this at the moment. It's still a net improvement in that it will be clearly marked what uses v1 and what doesn't.
  3. Extract common code into pkg/client/rest & pkg/client/config
    1. This can happen in parallel with step 2.
  4. Remove all client like things from pkg/client once 2 is finished.
Member

lavalamp commented Jul 14, 2015

OK. Time to do this. The motivation is so that people can import pkg/client/v1 and have code that will continue to build against head without changes as long as v1 is supported.

Proposed factoring:

  • pkg/client/config
    • This has everything to do with certs, setup, etc.
  • pkg/client/rest
    • RESTClient moves here
  • pkg/client/unversioned
    • the current client.Client struct and interfaces move here.
  • pkg/client/v1
    • A copy of unversioned, but using pkg/api/v1 instead of pkg/api

I propose doing this in a few steps.

  1. Copy pkg/client to pkg/client/v1 AND pkg/client/unversioned.
    1. Change the import lines in pkg/client/v1 to go to pkg/api/v1.
  2. Change references in the rest of the codebase to the appropriate client.
    1. I know @bgrant0607 would love it if nothing ended up referencing pkg/client/unversioned. I wouldn't prioritize this at the moment. It's still a net improvement in that it will be clearly marked what uses v1 and what doesn't.
  3. Extract common code into pkg/client/rest & pkg/client/config
    1. This can happen in parallel with step 2.
  4. Remove all client like things from pkg/client once 2 is finished.
@davidopp

This comment has been minimized.

Show comment
Hide comment
@davidopp

davidopp Jul 14, 2015

Member

On Tue, Jul 14, 2015 at 2:48 PM, Daniel Smith notifications@github.com
wrote:

OK. Time to do this. The motivation is so that people can import
pkg/client/v1 and have code that will continue to build against head
without changes as long as v1 is supported.

Proposed factoring:

  • pkg/client/config
    • This has everything to do with certs, setup, etc.
      • pkg/client/rest
    • RESTClient moves here
      • pkg/client/unversioned
    • the current client.Client struct and interfaces move here.
      • pkg/client/v1
    • A copy of unversioned, but using pkg/api/v1 instead of pkg/api

Meaning you'll "lock down" this last one (it will reflect the unversioned
we have today, while pkg/client/unversioned will be the one that's always
up-to-date)?

I propose doing this in a few steps.

  1. Copy pkg/client to pkg/client/v1 AND pkg/client/unversioned.
    1. Change the import lines in pkg/client/v1 to go to pkg/api/v1.
      1. Change references in the rest of the codebase to the appropriate
        client.
    2. I know @bgrant0607 https://github.com/bgrant0607 would love it
      if nothing ended up referencing pkg/client/unversioned. I wouldn't
      prioritize this at the moment. It's still a net improvement in that it will
      be clearly marked what uses v1 and what doesn't.
      1. Extract common code into pkg/client/rest & pkg/client/config
    3. This can happen in parallel with step 2.


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

Member

davidopp commented Jul 14, 2015

On Tue, Jul 14, 2015 at 2:48 PM, Daniel Smith notifications@github.com
wrote:

OK. Time to do this. The motivation is so that people can import
pkg/client/v1 and have code that will continue to build against head
without changes as long as v1 is supported.

Proposed factoring:

  • pkg/client/config
    • This has everything to do with certs, setup, etc.
      • pkg/client/rest
    • RESTClient moves here
      • pkg/client/unversioned
    • the current client.Client struct and interfaces move here.
      • pkg/client/v1
    • A copy of unversioned, but using pkg/api/v1 instead of pkg/api

Meaning you'll "lock down" this last one (it will reflect the unversioned
we have today, while pkg/client/unversioned will be the one that's always
up-to-date)?

I propose doing this in a few steps.

  1. Copy pkg/client to pkg/client/v1 AND pkg/client/unversioned.
    1. Change the import lines in pkg/client/v1 to go to pkg/api/v1.
      1. Change references in the rest of the codebase to the appropriate
        client.
    2. I know @bgrant0607 https://github.com/bgrant0607 would love it
      if nothing ended up referencing pkg/client/unversioned. I wouldn't
      prioritize this at the moment. It's still a net improvement in that it will
      be clearly marked what uses v1 and what doesn't.
      1. Extract common code into pkg/client/rest & pkg/client/config
    3. This can happen in parallel with step 2.


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

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Jul 14, 2015

Member

Meaning you'll "lock down" this last one

Well, sort of-- I'll lock it down by changing the import line from "...pkg/api" to api "...pkg/api/v1.

There's no way to lock it down without doing this.

Member

lavalamp commented Jul 14, 2015

Meaning you'll "lock down" this last one

Well, sort of-- I'll lock it down by changing the import line from "...pkg/api" to api "...pkg/api/v1.

There's no way to lock it down without doing this.

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Jul 14, 2015

Member

I'm working on a PR to do step 1 above. Then I'll try and draft some folks to help me do 2 & 3 in parallel.

Member

lavalamp commented Jul 14, 2015

I'm working on a PR to do step 1 above. Then I'll try and draft some folks to help me do 2 & 3 in parallel.

@davidopp

This comment has been minimized.

Show comment
Hide comment
@davidopp

davidopp Jul 14, 2015

Member

Sorry, I accidentally replied by email which I think munged my question. Just trying to understand the difference between pkg/client/unversioned and pkg/client/v1

When we make changes to types.go, we'll make them in the former dir but not the latter, yes?

Member

davidopp commented Jul 14, 2015

Sorry, I accidentally replied by email which I think munged my question. Just trying to understand the difference between pkg/client/unversioned and pkg/client/v1

When we make changes to types.go, we'll make them in the former dir but not the latter, yes?

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Jul 14, 2015

Member

types.go is inside the pkg/api/ tree; I'm not copying that. But yes, there should be no backwards-incompatible changes inside pkg/api/v1 as of a month or so ago.

Member

lavalamp commented Jul 14, 2015

types.go is inside the pkg/api/ tree; I'm not copying that. But yes, there should be no backwards-incompatible changes inside pkg/api/v1 as of a month or so ago.

@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin

thockin Jul 14, 2015

Member

I still think we should discuss what our client library is. Fat client
libs have a long history of coming back to bite people in the butt, but
"just a bunch of structs" is also problematic. If we want to offer a
client library outside of our own codebase, we should start from the place
of deciding what feature set we want to offer to clients - not what structs
we can move around to suck less.

We're conflating two different things in pkg/api/v1 - it's simultaneously a
client lib (so make it reasonably friendly to use) and it's the thing we
use to decode JSON, including whatever tricks we pull to make it unmarshal
the way we want.

On Tue, Jul 14, 2015 at 3:42 PM, Daniel Smith notifications@github.com
wrote:

types.go is inside the pkg/api/ tree; I'm not copying that. But yes, there
should be no backwards-incompatible changes inside pkg/api/v1 as of a month
or so ago.


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

Member

thockin commented Jul 14, 2015

I still think we should discuss what our client library is. Fat client
libs have a long history of coming back to bite people in the butt, but
"just a bunch of structs" is also problematic. If we want to offer a
client library outside of our own codebase, we should start from the place
of deciding what feature set we want to offer to clients - not what structs
we can move around to suck less.

We're conflating two different things in pkg/api/v1 - it's simultaneously a
client lib (so make it reasonably friendly to use) and it's the thing we
use to decode JSON, including whatever tricks we pull to make it unmarshal
the way we want.

On Tue, Jul 14, 2015 at 3:42 PM, Daniel Smith notifications@github.com
wrote:

types.go is inside the pkg/api/ tree; I'm not copying that. But yes, there
should be no backwards-incompatible changes inside pkg/api/v1 as of a month
or so ago.


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

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Jul 15, 2015

Member

I've started on #11280 and already found a number of places where we're not versioned as completely as we should be.

We're conflating two different things in pkg/api/v1

We should move conversion/registration functions out of v1; this solves that problem.

If we want to offer a client library outside of our own codebase, we should start from the place of deciding what feature set we want to offer to clients - not what structs we can move around to suck less.

I have a much more modest goal of just letting people write code against our v1 api, using our current client interface.

I want to make the addition of a versioned client now, offering our existing interface. That lets us give developers something we can promise to "keep working". We can fix things over time so that the thing we provide requires fewer and fewer dependencies (ideally, it would not depend on pkg/api!). But the important thing now is to get that interface up; we haven't yet drifted from v1 yet, so we can do this with minimum pain.

Otherwise folks will have to make changes in their code every time they fetch k8s from head.

Member

lavalamp commented Jul 15, 2015

I've started on #11280 and already found a number of places where we're not versioned as completely as we should be.

We're conflating two different things in pkg/api/v1

We should move conversion/registration functions out of v1; this solves that problem.

If we want to offer a client library outside of our own codebase, we should start from the place of deciding what feature set we want to offer to clients - not what structs we can move around to suck less.

I have a much more modest goal of just letting people write code against our v1 api, using our current client interface.

I want to make the addition of a versioned client now, offering our existing interface. That lets us give developers something we can promise to "keep working". We can fix things over time so that the thing we provide requires fewer and fewer dependencies (ideally, it would not depend on pkg/api!). But the important thing now is to get that interface up; we haven't yet drifted from v1 yet, so we can do this with minimum pain.

Otherwise folks will have to make changes in their code every time they fetch k8s from head.

@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin

thockin Jul 15, 2015

Member

It's hard to disagree with the goal, but I am very anxious abouyt exposing
to users the same struct that we use for unmarshalling.

As an example, I'd like to make ServicePort.NodePort into a pointer so we
don't always print it when it is 0. From an API point of view that's a
transparent change - no client will see it. After you make this change
we're more or less supporting the v1 structs as a published API forever.

I'd much rather say that officially we DO NOT HAVE A CLIENT LIBRARY YET.
People might link to our libs but they are not a client lib.

On Tue, Jul 14, 2015 at 6:09 PM, Daniel Smith notifications@github.com
wrote:

I've started on #11280
#11280 and
already found a number of places where we're not versioned as completely as
we should be.

We're conflating two different things in pkg/api/v1

We should move conversion/registration functions out of v1; this solves
that problem.

If we want to offer a client library outside of our own codebase, we
should start from the place of deciding what feature set we want to offer
to clients - not what structs we can move around to suck less.

I have a much more modest goal of just letting people write code against
our v1 api, using our current client interface.

I want to make the addition of a versioned client now, offering our
existing interface. That lets us give developers something we can promise
to "keep working". We can fix things over time so that the thing we provide
requires fewer and fewer dependencies (ideally, it would not depend on
pkg/api!). But the important thing now is to get that interface up; we
haven't yet drifted from v1 yet, so we can do this with minimum pain.

Otherwise folks will have to make changes in their code every time they
fetch k8s from head.


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

Member

thockin commented Jul 15, 2015

It's hard to disagree with the goal, but I am very anxious abouyt exposing
to users the same struct that we use for unmarshalling.

As an example, I'd like to make ServicePort.NodePort into a pointer so we
don't always print it when it is 0. From an API point of view that's a
transparent change - no client will see it. After you make this change
we're more or less supporting the v1 structs as a published API forever.

I'd much rather say that officially we DO NOT HAVE A CLIENT LIBRARY YET.
People might link to our libs but they are not a client lib.

On Tue, Jul 14, 2015 at 6:09 PM, Daniel Smith notifications@github.com
wrote:

I've started on #11280
#11280 and
already found a number of places where we're not versioned as completely as
we should be.

We're conflating two different things in pkg/api/v1

We should move conversion/registration functions out of v1; this solves
that problem.

If we want to offer a client library outside of our own codebase, we
should start from the place of deciding what feature set we want to offer
to clients - not what structs we can move around to suck less.

I have a much more modest goal of just letting people write code against
our v1 api, using our current client interface.

I want to make the addition of a versioned client now, offering our
existing interface. That lets us give developers something we can promise
to "keep working". We can fix things over time so that the thing we provide
requires fewer and fewer dependencies (ideally, it would not depend on
pkg/api!). But the important thing now is to get that interface up; we
haven't yet drifted from v1 yet, so we can do this with minimum pain.

Otherwise folks will have to make changes in their code every time they
fetch k8s from head.


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

@krousey

This comment has been minimized.

Show comment
Hide comment
@krousey

krousey Jul 27, 2015

Member

As an example, I'd like to make ServicePort.NodePort into a pointer so we
don't always print it when it is 0. From an API point of view that's a
transparent change - no client will see it. After you make this change
we're more or less supporting the v1 structs as a published API forever.

Not to nitpick, but changing that to a pointer won't do what you want. It would just now let it print null instead of 0 when not initialized. You would have to add omitempty to the JSON field tag, and that would be an API change according to http://kubernetes.io/v1.0/docs/api-reference/definitions.html#_v1_serviceport

Member

krousey commented Jul 27, 2015

As an example, I'd like to make ServicePort.NodePort into a pointer so we
don't always print it when it is 0. From an API point of view that's a
transparent change - no client will see it. After you make this change
we're more or less supporting the v1 structs as a published API forever.

Not to nitpick, but changing that to a pointer won't do what you want. It would just now let it print null instead of 0 when not initialized. You would have to add omitempty to the JSON field tag, and that would be an API change according to http://kubernetes.io/v1.0/docs/api-reference/definitions.html#_v1_serviceport

@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin

thockin Jul 28, 2015

Member

In the realm of strictly breaking changes this seems minor. Is this likely
to cause any real problems?

On Mon, Jul 27, 2015 at 3:56 PM, krousey notifications@github.com wrote:

As an example, I'd like to make ServicePort.NodePort into a pointer so we
don't always print it when it is 0. From an API point of view that's a
transparent change - no client will see it. After you make this change
we're more or less supporting the v1 structs as a published API forever.

Not to nitpick, but changing that to a pointer won't do what you want. It
would just now let it print null instead of 0 when not initialized. You
would have to add omitempty to the JSON field tag, and that would be an
API change according to
http://kubernetes.io/v1.0/docs/api-reference/definitions.html#_v1_serviceport


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

Member

thockin commented Jul 28, 2015

In the realm of strictly breaking changes this seems minor. Is this likely
to cause any real problems?

On Mon, Jul 27, 2015 at 3:56 PM, krousey notifications@github.com wrote:

As an example, I'd like to make ServicePort.NodePort into a pointer so we
don't always print it when it is 0. From an API point of view that's a
transparent change - no client will see it. After you make this change
we're more or less supporting the v1 structs as a published API forever.

Not to nitpick, but changing that to a pointer won't do what you want. It
would just now let it print null instead of 0 when not initialized. You
would have to add omitempty to the JSON field tag, and that would be an
API change according to
http://kubernetes.io/v1.0/docs/api-reference/definitions.html#_v1_serviceport


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

@krousey

This comment has been minimized.

Show comment
Hide comment
@krousey

krousey Jul 28, 2015

Member

@thockin It depends on where you change it. If you do it in a way where a v1 client gets a message that doesn't have this field, any logic that depends on it being there (e.g. swagger spec based validation) will fail. I do not know how likely that is.

If you change it in a future version (say v1.1), you will just have an incompatibility, but v1 should still work. When clients upgrade our versioned client library from v1 to v1.1, they would see this field change and have to update the calling code.

Member

krousey commented Jul 28, 2015

@thockin It depends on where you change it. If you do it in a way where a v1 client gets a message that doesn't have this field, any logic that depends on it being there (e.g. swagger spec based validation) will fail. I do not know how likely that is.

If you change it in a future version (say v1.1), you will just have an incompatibility, but v1 should still work. When clients upgrade our versioned client library from v1 to v1.1, they would see this field change and have to update the calling code.

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607

bgrant0607 Sep 25, 2015

Member

What about:

  • experimental client
  • pkg/client/cache
  • pkg/client/metrics
  • pkg/client/record
  • pkg/kubectl/resource
  • #5660
  • #7584
Member

bgrant0607 commented Sep 25, 2015

What about:

  • experimental client
  • pkg/client/cache
  • pkg/client/metrics
  • pkg/client/record
  • pkg/kubectl/resource
  • #5660
  • #7584
@krousey

This comment has been minimized.

Show comment
Hide comment
@krousey

krousey Sep 28, 2015

Member

The above WIP PR #13098 is a mess. To directly address those issues, I would
like to put RESTClient and Request into their own package that has no
dependencies on a version or group. Currently request.go has some custom
field selector filtering logic that depends on the version that I need to
understand. I think this would still leave it dependent on api.Status, but
we don't change that across versions/groups. I would also like to change it to
take the codec on each request instead of being embedded in the client. This
could allow multiple groups/versions to use the same RESTClient.

I would also like to move the VersionInterface and functionality into this
package as it is also independent of groups/versions.

There are some things which are in this package that I'm not sure belong in
the API client, but I don't know where I would put them. I'm open to
suggestions on:

  • KubeletClient
  • ContainerInfoGetter
  • flags.go

With those changes, the code left in the actual versioned clients should be
mostly boiler plate calls into this new package with the appropriate codecs
and type assertions on the return values.

Member

krousey commented Sep 28, 2015

The above WIP PR #13098 is a mess. To directly address those issues, I would
like to put RESTClient and Request into their own package that has no
dependencies on a version or group. Currently request.go has some custom
field selector filtering logic that depends on the version that I need to
understand. I think this would still leave it dependent on api.Status, but
we don't change that across versions/groups. I would also like to change it to
take the codec on each request instead of being embedded in the client. This
could allow multiple groups/versions to use the same RESTClient.

I would also like to move the VersionInterface and functionality into this
package as it is also independent of groups/versions.

There are some things which are in this package that I'm not sure belong in
the API client, but I don't know where I would put them. I'm open to
suggestions on:

  • KubeletClient
  • ContainerInfoGetter
  • flags.go

With those changes, the code left in the actual versioned clients should be
mostly boiler plate calls into this new package with the appropriate codecs
and type assertions on the return values.

@krousey

This comment has been minimized.

Show comment
Hide comment
@krousey

krousey Sep 28, 2015

Member

@bgrant0607

My opinions on your concerns...

  • experimental client:
    Should be it's own package separate from the versioned client.
  • pkg/client/cache:
    We probably need to version the top level interface of this package as well.
    Seems ripe for internal packages + code generation.
  • pkg/client/metrics:
    I don't see a problem with this one. Is it version dependent in some way
    that I don't see?
  • pkg/client/record:
    What is the use case for this? Is it something a customer needs? Or
    something we need internally? It might be ok to use runtime.Object
    instead of concrete events for this.
  • pkg/kubectl/resource:
    Still looking in to this one...
  • #5660 and #7584:
    These goals are a way off. I think it would require a huge cleanup with our
    many "schemes" and "codecs." I think the biggest step would be to completely
    separate encoding/decoding from conversion. The only ones that should need
    conversion are the API server and special utilities for upgrading various
    resources. Another option is to just abandon the schemes and codecs in the
    client code and roll our own simple ones. That with a types only package
    from the API could make this achievable.
Member

krousey commented Sep 28, 2015

@bgrant0607

My opinions on your concerns...

  • experimental client:
    Should be it's own package separate from the versioned client.
  • pkg/client/cache:
    We probably need to version the top level interface of this package as well.
    Seems ripe for internal packages + code generation.
  • pkg/client/metrics:
    I don't see a problem with this one. Is it version dependent in some way
    that I don't see?
  • pkg/client/record:
    What is the use case for this? Is it something a customer needs? Or
    something we need internally? It might be ok to use runtime.Object
    instead of concrete events for this.
  • pkg/kubectl/resource:
    Still looking in to this one...
  • #5660 and #7584:
    These goals are a way off. I think it would require a huge cleanup with our
    many "schemes" and "codecs." I think the biggest step would be to completely
    separate encoding/decoding from conversion. The only ones that should need
    conversion are the API server and special utilities for upgrading various
    resources. Another option is to just abandon the schemes and codecs in the
    client code and roll our own simple ones. That with a types only package
    from the API could make this achievable.
@krousey

This comment has been minimized.

Show comment
Hide comment
@krousey
Member

krousey commented Sep 28, 2015

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Sep 29, 2015

Member

I think pkg/client/cache is and should remain version agnostic. It's a general caching layer and should work with runtime.Objects.

Currently request.go has some custom field selector filtering logic that depends on the version that I need to understand.

Should be moved into the high level client, or a generic high-ish level client (like kubectl's builder)

I would also like to change it to take the codec on each request instead of being embedded in the client. This could allow multiple groups/versions to use the same RESTClient.

I actually think we should leave the codec in the RESTClient. There should be an additional layer under RESTClient, the http connection/transport/cert layer; RESTClients that actually point at the same server can re-use that layer. This should help @smarterclayton with openshift integration.

Other misc thoughts:

  • RESTClient needs to become an interface instead of a struct.
  • If we have 10 groups with 3 versions each, it becomes infeasible to offer every combination as a typed client. Therefore, we must do one or both of:
    • Offer curated/tested versioned clients, where we select the versions for each group.
    • Switch our deliverable to be a versioned client generator, such that you specify the group/versions you wish and a high level client will be generated for you.
Member

lavalamp commented Sep 29, 2015

I think pkg/client/cache is and should remain version agnostic. It's a general caching layer and should work with runtime.Objects.

Currently request.go has some custom field selector filtering logic that depends on the version that I need to understand.

Should be moved into the high level client, or a generic high-ish level client (like kubectl's builder)

I would also like to change it to take the codec on each request instead of being embedded in the client. This could allow multiple groups/versions to use the same RESTClient.

I actually think we should leave the codec in the RESTClient. There should be an additional layer under RESTClient, the http connection/transport/cert layer; RESTClients that actually point at the same server can re-use that layer. This should help @smarterclayton with openshift integration.

Other misc thoughts:

  • RESTClient needs to become an interface instead of a struct.
  • If we have 10 groups with 3 versions each, it becomes infeasible to offer every combination as a typed client. Therefore, we must do one or both of:
    • Offer curated/tested versioned clients, where we select the versions for each group.
    • Switch our deliverable to be a versioned client generator, such that you specify the group/versions you wish and a high level client will be generated for you.
@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Oct 20, 2015

Member

Explicit proposal in #15730

Member

lavalamp commented Oct 20, 2015

Explicit proposal in #15730

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Feb 24, 2016

Member

I think if #21408 merges, we can punt the rest of this to the next release.

Member

lavalamp commented Feb 24, 2016

I think if #21408 merges, we can punt the rest of this to the next release.

@goltermann

This comment has been minimized.

Show comment
Hide comment
@goltermann

goltermann Mar 5, 2016

Contributor

#21408 has merged, time to punt?

Contributor

goltermann commented Mar 5, 2016

#21408 has merged, time to punt?

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607

bgrant0607 Jul 14, 2016

Member

@lavalamp @krousey What remains to be done here?

Member

bgrant0607 commented Jul 14, 2016

@lavalamp @krousey What remains to be done here?

@caesarxuchao

This comment has been minimized.

Show comment
Hide comment
@caesarxuchao

caesarxuchao Jul 14, 2016

Member

Clientset is our versioned client. The next step is extract the client lib into its own repo, then vendor it in the main repo, and convert pkg/controllers and pkg/kubelet to use the versioned client.

AFAIK, kubectl should use the dynamic client, @krousey is working on it and have more insight.

Member

caesarxuchao commented Jul 14, 2016

Clientset is our versioned client. The next step is extract the client lib into its own repo, then vendor it in the main repo, and convert pkg/controllers and pkg/kubelet to use the versioned client.

AFAIK, kubectl should use the dynamic client, @krousey is working on it and have more insight.

@ae6rt

This comment has been minimized.

Show comment
Hide comment
@ae6rt

ae6rt Jul 14, 2016

Contributor

@caesarxuchao Is there a tracking issue for "next step is extract the client lib into its own repo"?

Contributor

ae6rt commented Jul 14, 2016

@caesarxuchao Is there a tracking issue for "next step is extract the client lib into its own repo"?

@caesarxuchao

This comment has been minimized.

Show comment
Hide comment
@caesarxuchao
Member

caesarxuchao commented Jul 14, 2016

@ae6rt, it's #28559

@krousey

This comment has been minimized.

Show comment
Hide comment
@krousey

krousey Oct 27, 2016

Member

Closing in favor of #29934 as it represents the remaining tasks and is owned by the person actively working on it.

Member

krousey commented Oct 27, 2016

Closing in favor of #29934 as it represents the remaining tasks and is owned by the person actively working on it.

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