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

Proposal: Use context throughout kubernetes #815

Closed
pnasrat opened this Issue Aug 7, 2014 · 15 comments

Comments

Projects
None yet
@pnasrat

pnasrat commented Aug 7, 2014

In an ideal world we need a way to seamlessly carry trace, debugging, cancellation and other data across boundaries - whether that be process, goroutine or RPC.

The code.google.com/p/go.net/context package provides wiring to do this.
References http://blog.golang.org/context

I'd like to wire this into kubernetes, but want blessing before going off and doing it. If you think this would be of value let me know and I'll send a PR.

@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Aug 7, 2014

I'm definitely intrigued. Some questions:

Can you expand on how you imagine this working inside k8s?

Does this context continue to work if it is serialized into storage and then de-serialized N-minutes later?

Many of the patterns in Kubernetes are of the form:
Handle HTTP request
Write to persistent store
Some other process reads from persistent store
Process sends a new HTTP request

Enabling tracing through that whole thing would be super cool! Is that what you were imagining? Can you stack multiple contexts together?

Thanks in advance!
--brendan

@lavalamp

This comment has been minimized.

Member

lavalamp commented Aug 7, 2014

This is probably a good idea, but it will add complexity, so we should perhaps decide on the scope. I'm curious what @smarterclayton thinks. We've independently added some support for operations and logging associated with a request, so it's not 100% stuff we don't have.

@brendandburns, I am not so sure that we should consider, say, a replication controller's actions as part of the request that added the replication controller to the system. That sounds like an auth/identity/ownership issue, not an RPC tracability issue.

@smarterclayton

This comment has been minimized.

Contributor

smarterclayton commented Aug 7, 2014

A very cool concept, but since many of the transitions occur across HTTP boundaries (with more planned), how would that context propagate? I had imagined that as things like identity propagation became relevant (and potentially projects) there is a domain model object like a "context" (who am i, which resource scope am I in) that would filter through various calls. I could see that associating that with this sort of context also makes sense.

HTTP request trace-ability through any nested calls would be very desirable though.

@smarterclayton

This comment has been minimized.

Contributor

smarterclayton commented Sep 27, 2014

@derekwaynecarr added the first part of this. I'll probably plumb the context through the client. Once we have both of those this can be closed for more specific needs (cancellation, traceability, etc)

@thockin

This comment has been minimized.

Member

thockin commented Oct 7, 2014

Status?

@derekwaynecarr

This comment has been minimized.

Member

derekwaynecarr commented Oct 7, 2014

@thockin - I added it to client and server. There are some parts of server not yet completed - specifically minion registry, but I think that is being handled in a separate PR.

@smarterclayton

This comment has been minimized.

Contributor

smarterclayton commented Oct 7, 2014

Also, we aren't conveying a request identifier for traceability through all client calls, which I think in some respects this shou

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

@thockin - I added it to client and server. There are some parts of server
not yet completed - specifically minion registry, but I think that is being
handled in a separate PR.


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

@bgrant0607

This comment has been minimized.

Member

bgrant0607 commented Dec 3, 2014

@derekwaynecarr Is this finished yet? If not, who is working on the final bits?

@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Aug 30, 2016

go 1.7 introduces context into the HTTP libs as well as the httptrace library, we should re-consider this in light of those additions.

https://golang.org/doc/go1.7#context

@smarterclayton

This comment has been minimized.

Contributor

smarterclayton commented Aug 30, 2016

I agree, we should wire context (as is) into more places. Storage did it for the etcd3 work.

I don't know that we've resolved the "acting-as" wrinkles with controllers in a coherent way where the context would be related to the end user.

@bgrant0607 bgrant0607 removed the help-wanted label Aug 30, 2016

@bgrant0607

This comment has been minimized.

Member

bgrant0607 commented Mar 9, 2017

There's also OpenTracing.

http://opentracing.io/

@bgrant0607 bgrant0607 added the triaged label Mar 9, 2017

@ash2k

This comment has been minimized.

Member

ash2k commented Apr 13, 2017

It would be useful to add context to all client methods. Also a lot of places use stopCh <-chan struct{} and context could be used there instead.

@odeke-em

This comment has been minimized.

odeke-em commented Jul 21, 2017

How's it going here?

import (
   "context"
)

"context" is now mainstream for Go1.7, Go1.8 and soon Go1.9.

@fejta-bot

This comment has been minimized.

fejta-bot commented Jan 1, 2018

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

This comment has been minimized.

fejta-bot commented Jan 31, 2018

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

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 rotten
/remove-lifecycle stale

k8s-merge-robot added a commit that referenced this issue Feb 7, 2018

Merge pull request #59287 from cheftako/cloud-context-level
Automatic merge from submit-queue (batch tested with PRs 59441, 58264, 59287, 59396, 59439). 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>.

Add context to all relevant cloud APIs

**What this PR does / why we need it**:

This adds context to all the relevant cloud provider interface signatures.
Callers of those APIs are currently satisfied using context.TODO().
There will be follow on PRs to push the context through the stack.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #815

**Special notes for your reviewer**:
For an idea of the full scope of this change please look at PR #58532.

**Release note**:
```release-note
Implementers of the cloud provider interface will note the addition of a context to this interface. Trivial code modification will be necessary for a cloud provider to continue to compile.
```

dims pushed a commit to dims/kubernetes that referenced this issue Feb 8, 2018

Merge pull request kubernetes#59287 from cheftako/cloud-context-level
Automatic merge from submit-queue (batch tested with PRs 59441, 58264, 59287, 59396, 59439). 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>.

Add context to all relevant cloud APIs

**What this PR does / why we need it**:

This adds context to all the relevant cloud provider interface signatures.
Callers of those APIs are currently satisfied using context.TODO().
There will be follow on PRs to push the context through the stack.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes kubernetes#815

**Special notes for your reviewer**:
For an idea of the full scope of this change please look at PR kubernetes#58532.

**Release note**:
```release-note
Implementers of the cloud provider interface will note the addition of a context to this interface. Trivial code modification will be necessary for a cloud provider to continue to compile.
```

k8s-merge-robot added a commit that referenced this issue May 19, 2018

Merge pull request #60012 from atlassian/dial-with-context
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

k8s-publishing-bot added a commit to kubernetes/apimachinery that referenced this issue May 19, 2018

Merge pull request #60012 from atlassian/dial-with-context
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-publishing-bot added a commit to kubernetes/client-go that referenced this issue May 19, 2018

Merge pull request #60012 from atlassian/dial-with-context
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-publishing-bot added a commit to kubernetes/apiserver that referenced this issue May 19, 2018

Merge pull request #60012 from atlassian/dial-with-context
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-publishing-bot added a commit to kubernetes/kube-aggregator that referenced this issue May 19, 2018

Merge pull request #60012 from atlassian/dial-with-context
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment