Proposal: Allowing context to be used with client-go #1166

maleck13 · 2017-10-09T15:40:46Z

This is a first pass at a set of changes aimed at allowing consumers of client-go to leverage functionality offered by the context package in the std lib. It is in response to the following issue:
kubernetes/kubernetes#46503

To the reviewers:
This is the first proposal I have done for Kubernetes so any feedback and guidance would be appreciated, I am not really sure of the process after creating the proposal.

jbeda · 2017-10-09T16:32:10Z

It would be best for someone from API Machinery to comment here.

@kubernetes/sig-api-machinery-pr-reviews

sttts · 2017-10-11T08:51:17Z

contributors/design-proposals/api-machinery/client-go-context.md

+
+// WithContext allows you to set a context that will be used by the underlying http request
+func (c *pods) WithContext(ctx context.Context) PodInterface {
+  	c.ctx = ctx


WithContext should not mutate, but has to create a shallow copy first.

Could you explain a little more, do you mean before the assignment on line 60 happens? My understanding is ctx is being passed by value into this method and so is a shallow copy or am I misunderstanding?

I think I meant a shallow copy of c *pods.

sttts · 2017-10-11T08:54:31Z

contributors/design-proposals/api-machinery/client-go-context.md

+
+**Concerns**
+
+This is very clearly a breaking change that would likely require extensive changes to both tests and implementation code throughout the impacted code bases.


I don't care much about everything in k8s.io/kubernetes. It's a one-time cleanup to adapt using context.ToDo().

Passing a context through where it makes sense would be a bigger change, but that's not dependent on which of your solution we choose.

It's also a breaking change for our published client-go. We actually had similar changes in 1.6 with GetOptions and ListOptions. It might be tedious for our 3rdparty users to fix their code. But at least the breakage and fix is obvious and easy to repair.

Agree that is it is relatively easy to fix. Also clearly announces context support in the client. With the none breaking change, a consumer would need to discover that context support was now available.

On the other hand, this approach would guarantee that we update all call sites. The other we could silently miss them.

silently missing call site updates seems bad

I'd be more concerned with third-parties scrambling to fix their build and misusing context without. If we silent'y failed back to context.ToDo() people could add context on their own initiative.

ggaaooppeenngg · 2017-11-01T03:48:28Z

Any further work?

maleck13 · 2017-11-02T11:39:52Z

@ggaaooppeenngg I have not put any additional work in yet. Not entirely sure what the next steps would be

smarterclayton · 2017-11-05T19:10:38Z

This seems pretty clear. I think the breaking change is the correct long term behavior (consistent with the intended use of the context package). However, it will impact a wide swath of the ecosystem, so we would need to be very careful about communicating the changes and their impact, as well as ensure that concerns raised by the community are addressed. Are other clients in the Go ecosystem actively moving towards context in its "intended" form, or is it a slower process? I admit to not seeing a large uptake, but haven't done a real survey.

…

On Thu, Nov 2, 2017 at 7:39 AM, Craig Brookes ***@***.***> wrote: @ggaaooppeenngg <https://github.com/ggaaooppeenngg> I have not put any additional work in yet. Not entirely sure what the next steps would be — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#1166 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p1T_jdncyC2ngJlbS165uiwLMbTJks5syaoKgaJpZM4Pyrq4> .

maleck13 · 2017-11-06T08:54:32Z

@sttts You mentioned a similar change when adding the GetOptions and ListOptions are you aware of how this change was communicated?

@smarterclayton pkgs in the stdlib are making use of the context pkg, but I am also unfamiliar with the wider uptake in community pkgs.

sttts · 2017-11-06T11:04:00Z

You mentioned a similar change when adding the GetOptions and ListOptions are you aware of how this change was communicated?

As most of our breaking changes not very well https://github.com/kubernetes/client-go/blob/master/CHANGELOG.md#v300. But at least it's pretty obvious for every 3rdparty what changed and how to fix it. I worry more about changes which have a less clear local fix. Contexts will belong to the former.

maleck13 · 2017-11-06T15:16:49Z

Ok so is the next step here then to do an actual implementation or is there more that needs doing on the proposal?

maleck13 · 2017-12-11T09:03:14Z

I am hoping to start on an implementation of this early in the new year after the holiday period.

maleck13 · 2017-12-11T15:51:26Z

@sttts should we consider adding ctx support to the client-gen tool also?

sttts · 2017-12-11T19:38:13Z

should we consider adding ctx support to the client-gen tool also?

We have to, don't we?

maleck13 · 2017-12-12T09:42:32Z

@sttts Yes of course we do, ignore the previous comment

maleck13 · 2018-01-18T11:54:35Z

started looking at the implementation for this, new to the client generator so it may take a little while

lavalamp · 2018-01-18T17:12:57Z

cc @yliaog

maleck13 · 2018-01-22T18:15:16Z

@sttts I believe I have the changes in the gen tools done. I would like to create a WIP pr to get feedback, unless there is a reason not to, I will create the WIP pr in the morning.

sttts · 2018-01-23T09:43:29Z

I will create the WIP pr in the morning.

Sure, assign it to me.

@sttts

Automatic merge from submit-queue (batch tested with PRs 60012, 63692, 63977, 63960, 64008). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Use Dial with context **What this PR does / why we need it**: `net/http/Transport.Dial` field is deprecated: ```go // DialContext specifies the dial function for creating unencrypted TCP connections. // If DialContext is nil (and the deprecated Dial below is also nil), // then the transport dials using package net. DialContext func(ctx context.Context, network, addr string) (net.Conn, error) // Dial specifies the dial function for creating unencrypted TCP connections. // // Deprecated: Use DialContext instead, which allows the transport // to cancel dials as soon as they are no longer needed. // If both are set, DialContext takes priority. Dial func(network, addr string) (net.Conn, error) ``` This PR switches all `Dial` usages to `DialContext`. Fixes #63455. **Special notes for your reviewer**: Also related: #59287 #58532 #815 kubernetes/community#1166 #58677 #57932 **Release note**: ```release-note HTTP transport now uses `context.Context` to cancel dial operations. k8s.io/client-go/transport/Config struct has been updated to accept a function with a `context.Context` parameter. This is a breaking change if you use this field in your code. ``` /sig api-machinery /kind enhancement /cc @sttts

@sttts

Automatic merge from submit-queue (batch tested with PRs 60012, 63692, 63977, 63960, 64008). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Use Dial with context **What this PR does / why we need it**: `net/http/Transport.Dial` field is deprecated: ```go // DialContext specifies the dial function for creating unencrypted TCP connections. // If DialContext is nil (and the deprecated Dial below is also nil), // then the transport dials using package net. DialContext func(ctx context.Context, network, addr string) (net.Conn, error) // Dial specifies the dial function for creating unencrypted TCP connections. // // Deprecated: Use DialContext instead, which allows the transport // to cancel dials as soon as they are no longer needed. // If both are set, DialContext takes priority. Dial func(network, addr string) (net.Conn, error) ``` This PR switches all `Dial` usages to `DialContext`. Fixes #63455. **Special notes for your reviewer**: Also related: kubernetes/kubernetes#59287 kubernetes/kubernetes#58532 kubernetes/kubernetes#815 kubernetes/community#1166 kubernetes/kubernetes#58677 kubernetes/kubernetes#57932 **Release note**: ```release-note HTTP transport now uses `context.Context` to cancel dial operations. k8s.io/client-go/transport/Config struct has been updated to accept a function with a `context.Context` parameter. This is a breaking change if you use this field in your code. ``` /sig api-machinery /kind enhancement /cc @sttts Kubernetes-commit: ddf551c24b7d88454f8332ce6855e53281440958

@sttts

Automatic merge from submit-queue (batch tested with PRs 60012, 63692, 63977, 63960, 64008). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Use Dial with context **What this PR does / why we need it**: `net/http/Transport.Dial` field is deprecated: ```go // DialContext specifies the dial function for creating unencrypted TCP connections. // If DialContext is nil (and the deprecated Dial below is also nil), // then the transport dials using package net. DialContext func(ctx context.Context, network, addr string) (net.Conn, error) // Dial specifies the dial function for creating unencrypted TCP connections. // // Deprecated: Use DialContext instead, which allows the transport // to cancel dials as soon as they are no longer needed. // If both are set, DialContext takes priority. Dial func(network, addr string) (net.Conn, error) ``` This PR switches all `Dial` usages to `DialContext`. Fixes #63455. **Special notes for your reviewer**: Also related: kubernetes/kubernetes#59287 kubernetes/kubernetes#58532 kubernetes/kubernetes#815 kubernetes/community#1166 kubernetes/kubernetes#58677 kubernetes/kubernetes#57932 **Release note**: ```release-note HTTP transport now uses `context.Context` to cancel dial operations. k8s.io/client-go/transport/Config struct has been updated to accept a function with a `context.Context` parameter. This is a breaking change if you use this field in your code. ``` /sig api-machinery /kind enhancement /cc @sttts Kubernetes-commit: ddf551c24b7d88454f8332ce6855e53281440958

@sttts

Automatic merge from submit-queue (batch tested with PRs 60012, 63692, 63977, 63960, 64008). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Use Dial with context **What this PR does / why we need it**: `net/http/Transport.Dial` field is deprecated: ```go // DialContext specifies the dial function for creating unencrypted TCP connections. // If DialContext is nil (and the deprecated Dial below is also nil), // then the transport dials using package net. DialContext func(ctx context.Context, network, addr string) (net.Conn, error) // Dial specifies the dial function for creating unencrypted TCP connections. // // Deprecated: Use DialContext instead, which allows the transport // to cancel dials as soon as they are no longer needed. // If both are set, DialContext takes priority. Dial func(network, addr string) (net.Conn, error) ``` This PR switches all `Dial` usages to `DialContext`. Fixes #63455. **Special notes for your reviewer**: Also related: kubernetes/kubernetes#59287 kubernetes/kubernetes#58532 kubernetes/kubernetes#815 kubernetes/community#1166 kubernetes/kubernetes#58677 kubernetes/kubernetes#57932 **Release note**: ```release-note HTTP transport now uses `context.Context` to cancel dial operations. k8s.io/client-go/transport/Config struct has been updated to accept a function with a `context.Context` parameter. This is a breaking change if you use this field in your code. ``` /sig api-machinery /kind enhancement /cc @sttts Kubernetes-commit: ddf551c24b7d88454f8332ce6855e53281440958

@sttts

Automatic merge from submit-queue (batch tested with PRs 60012, 63692, 63977, 63960, 64008). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Use Dial with context **What this PR does / why we need it**: `net/http/Transport.Dial` field is deprecated: ```go // DialContext specifies the dial function for creating unencrypted TCP connections. // If DialContext is nil (and the deprecated Dial below is also nil), // then the transport dials using package net. DialContext func(ctx context.Context, network, addr string) (net.Conn, error) // Dial specifies the dial function for creating unencrypted TCP connections. // // Deprecated: Use DialContext instead, which allows the transport // to cancel dials as soon as they are no longer needed. // If both are set, DialContext takes priority. Dial func(network, addr string) (net.Conn, error) ``` This PR switches all `Dial` usages to `DialContext`. Fixes #63455. **Special notes for your reviewer**: Also related: kubernetes/kubernetes#59287 kubernetes/kubernetes#58532 kubernetes/kubernetes#815 kubernetes/community#1166 kubernetes/kubernetes#58677 kubernetes/kubernetes#57932 **Release note**: ```release-note HTTP transport now uses `context.Context` to cancel dial operations. k8s.io/client-go/transport/Config struct has been updated to accept a function with a `context.Context` parameter. This is a breaking change if you use this field in your code. ``` /sig api-machinery /kind enhancement /cc @sttts Kubernetes-commit: ddf551c24b7d88454f8332ce6855e53281440958

fejta-bot · 2018-05-29T13:31:05Z

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.

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

smarterclayton · 2018-05-30T14:15:18Z

/lifecycle frozen

smarterclayton · 2018-05-30T14:16:12Z

/remove-lifecycle stale

cceckman · 2018-07-09T18:28:37Z

Gentle ping. Is this waiting on anything? (Attention from reviewers?)

sttts · 2018-07-10T08:42:17Z

We decided in the sig-api-machinery meeting to go forward with a hard one-time signature change. PRs are in the works right now.

liggitt · 2018-08-09T19:42:01Z

contributors/design-proposals/api-machinery/client-go-context.md

+```
+
+
+HTTP Tracing and cancellation are also likely useful when using ```informers``` and ```listers```, as these also use the client interfaces backwards compatibility can be maintained by adding a new set of constructors for adding a


cancellation via deadline or timeout in a context would not work well with a single context provided to a long-lived lister/informer. similarly, trace ids are typically per request, not long-lived. I don't think a single context object for a lister/informer is what we want.

liggitt · 2018-08-09T19:43:12Z

I'm in favor of this, other than the lister/informer bits

rlguarino · 2018-08-16T17:43:11Z

contributors/design-proposals/api-machinery/client-go-context.md

+		Resource("pods").
+		Name(name).
+		VersionedParams(&options, scheme.ParameterCodec).
+		Context(ctx).


is this line meant to be: Context(c.ctx) ?

rlguarino · 2018-08-16T17:45:16Z

contributors/design-proposals/api-machinery/client-go-context.md

+		Resource("pods").
+		Name(name).
+		VersionedParams(&options, scheme.ParameterCodec).
+		Context(c.ctx).


Is this line meant to be: Context(ctx)?

rlguarino · 2018-08-16T17:49:51Z

contributors/design-proposals/api-machinery/client-go-context.md

+
+These change would impact on every API. Although the changes required would be many, the simplest approach to maintaining current behavior would be to pass ```context.TODO()```.
+
+For the ```listers``` and ```informers``` the change outlined above (adding new constructors) would still allow a context to be passed through to client functions doing the work.


Would it make more sense in the context of listers and informers to specify a context constructor function?

Some sample use cases for context like tracing and deadlines, won't make much sense to set once per informer or lister, but would make more sense to create a new one every time its needed.

Trace id are commonly set per request and the only thing a deadline set at the construction of the informer or lister would do is limit the time your program can get resources from the api.

rlguarino · 2018-08-16T17:52:04Z

contributors/design-proposals/api-machinery/client-go-context.md

+
+**Concerns**
+
+This is very clearly a breaking change that would likely require extensive changes to both tests and implementation code throughout the impacted code bases.


I'd be more concerned with third-parties scrambling to fix their build and misusing context without. If we silent'y failed back to context.ToDo() people could add context on their own initiative.

aanm · 2018-10-07T19:31:27Z

We decided in the sig-api-machinery meeting to go forward with a hard one-time signature change. PRs are in the works right now.

@sttts Any news with regarding this?

hypnoce · 2019-06-05T09:25:49Z

This proposal would be a great help for my use case. Currently writing a small service that calls the kubernetes API server. To workaround the problem, I have to create a new kubernetes client with a custom transport every time there is an incoming request.
While it's possible to optimize memory allocation by only creating the client for the needed api group (corev1, extensionv1...), the code still allocates a bunch of resource.

liggitt · 2019-11-06T04:23:17Z

This should move to https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery in KEP form to make progress.

gkarthiks · 2020-01-31T17:52:30Z

hi @maleck13 Since we have a KEP process, can we please merge this soon or close this and create a KEP?

liggitt · 2020-02-03T02:18:46Z

Moved to kubernetes/enhancements#1503

liggitt · 2020-02-03T02:18:56Z

/close

k8s-ci-robot · 2020-02-03T02:18:58Z

@liggitt: Closed this PR.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sttts

Automatic merge from submit-queue (batch tested with PRs 60012, 63692, 63977, 63960, 64008). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Use Dial with context **What this PR does / why we need it**: `net/http/Transport.Dial` field is deprecated: ```go // DialContext specifies the dial function for creating unencrypted TCP connections. // If DialContext is nil (and the deprecated Dial below is also nil), // then the transport dials using package net. DialContext func(ctx context.Context, network, addr string) (net.Conn, error) // Dial specifies the dial function for creating unencrypted TCP connections. // // Deprecated: Use DialContext instead, which allows the transport // to cancel dials as soon as they are no longer needed. // If both are set, DialContext takes priority. Dial func(network, addr string) (net.Conn, error) ``` This PR switches all `Dial` usages to `DialContext`. Fixes #63455. **Special notes for your reviewer**: Also related: kubernetes/kubernetes#59287 kubernetes/kubernetes#58532 kubernetes/kubernetes#815 kubernetes/community#1166 kubernetes/kubernetes#58677 kubernetes/kubernetes#57932 **Release note**: ```release-note HTTP transport now uses `context.Context` to cancel dial operations. k8s.io/client-go/transport/Config struct has been updated to accept a function with a `context.Context` parameter. This is a breaking change if you use this field in your code. ``` /sig api-machinery /kind enhancement /cc @sttts Kubernetes-commit: ddf551c24b7d88454f8332ce6855e53281440958

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 9, 2017

maleck13 mentioned this pull request Oct 9, 2017

It should be possible to inject Context into client-go API call kubernetes/kubernetes#46503

Closed

k8s-github-robot assigned dchen1107 and jbeda Oct 9, 2017

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Oct 9, 2017

jbeda removed their assignment Oct 9, 2017

sttts reviewed Oct 11, 2017

View reviewed changes

smarterclayton mentioned this pull request Nov 26, 2017

Add RequestTimeoutQuery to configure API timeout kubernetes/kubernetes#56153

Closed

maleck13 mentioned this pull request Dec 15, 2017

Proposed README aerogear/proposals#1

Merged

maleck13 changed the title ~~Proposal: First pass at allowing context to be used with client-go~~ Proposal: Allowing context to be used with client-go Jan 5, 2018

lavalamp unassigned dchen1107 Jan 18, 2018

maleck13 mentioned this pull request Jan 23, 2018

WIP initial work adding ctx to client-gen and informer-gen kubernetes/kubernetes#58677

Closed

k8s-github-robot added the kind/design Categorizes issue or PR as related to design. label Feb 6, 2018

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 29, 2018

k8s-ci-robot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label May 30, 2018

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 30, 2018

liggitt reviewed Aug 9, 2018

View reviewed changes

rlguarino reviewed Aug 16, 2018

View reviewed changes

varshavaradarajan mentioned this pull request Jul 17, 2019

Allow users to configure timeout while building kubernetes client digitalocean/clusterlint#45

Merged

liggitt mentioned this pull request Aug 13, 2019

WIP: client-go: wire context.Context kubernetes/kubernetes#78108

Closed

liggitt mentioned this pull request Sep 25, 2019

Propagate context to remote authorize/authenticate webhook calls kubernetes/kubernetes#83064

Merged

liggitt mentioned this pull request Nov 6, 2019

Can't pass CreateOptions / UpdateOptions kubernetes/client-go#635

Closed

mikedanese mentioned this pull request Jan 17, 2020

context in client-go kubernetes/kubernetes#87299

Merged

k8s-ci-robot closed this Feb 3, 2020


		Concerns

		This is very clearly a breaking change that would likely require extensive changes to both tests and implementation code throughout the impacted code bases.

		```


		HTTP Tracing and cancellation are also likely useful when using ```informers``` and ```listers```, as these also use the client interfaces backwards compatibility can be maintained by adding a new set of constructors for adding a


		These change would impact on every API. Although the changes required would be many, the simplest approach to maintaining current behavior would be to pass ```context.TODO()```.

		For the ```listers``` and ```informers``` the change outlined above (adding new constructors) would still allow a context to be passed through to client functions doing the work.

Proposal: Allowing context to be used with client-go #1166

Proposal: Allowing context to be used with client-go #1166

Conversation

maleck13 commented Oct 9, 2017

jbeda commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggaaooppeenngg commented Nov 1, 2017

maleck13 commented Nov 2, 2017

smarterclayton commented Nov 5, 2017 via email

maleck13 commented Nov 6, 2017

sttts commented Nov 6, 2017

maleck13 commented Nov 6, 2017

maleck13 commented Dec 11, 2017

maleck13 commented Dec 11, 2017

sttts commented Dec 11, 2017

maleck13 commented Dec 12, 2017

maleck13 commented Jan 18, 2018

lavalamp commented Jan 18, 2018

maleck13 commented Jan 22, 2018

sttts commented Jan 23, 2018

fejta-bot commented May 29, 2018

smarterclayton commented May 30, 2018 via email

smarterclayton commented May 30, 2018

cceckman commented Jul 9, 2018

sttts commented Jul 10, 2018

Choose a reason for hiding this comment

liggitt commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aanm commented Oct 7, 2018

hypnoce commented Jun 5, 2019

liggitt commented Nov 6, 2019

gkarthiks commented Jan 31, 2020

liggitt commented Feb 3, 2020

liggitt commented Feb 3, 2020

k8s-ci-robot commented Feb 3, 2020