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

Add context to all relevant cloud APIs #59287

Merged
merged 1 commit into from Feb 7, 2018

Conversation

@cheftako
Member

cheftako commented Feb 2, 2018

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:

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.
@lavalamp

This comment has been minimized.

Member

lavalamp commented Feb 3, 2018

OK, I can get behind this version :)

I was going to ask for another reviewer, but after looking at it I am going to just approve and LGTM this to keep it out of rebase hell. It should be zero impact and then folks can wire up their areas in parallel. I don't think there's anything in this that should be controversial and the context.TODO() makes it really clear where future attention is needed.

/approve
/lgtm

@lavalamp

This comment has been minimized.

Member

lavalamp commented Feb 3, 2018

/hold

@lavalamp

This comment has been minimized.

Member

lavalamp commented Feb 3, 2018

Remove the hold when you feel it's sufficiently publicized.

@dims

This comment has been minimized.

Member

dims commented Feb 3, 2018

so, really quick feedback without looking deeper. i was planning to maintain https://github.com/dims/openstack-cloud-controller-manager that would support both 1.9 and 1.10 and sync my tree from the changes in the main k/k repo. this will totally break my flow ... 2 cents.

@lavalamp

This comment has been minimized.

Member

lavalamp commented Feb 3, 2018

I don't think we have made promises about the stability of the cloud provider interface. The change here is very mechanical, it should be as easy to maintain in a patch as one could hope. Is there any reasonable alternative?

@dims

This comment has been minimized.

Member

dims commented Feb 3, 2018

+1 in principle. Let's please broadcast and merge early next week (max 1 rebase!)

@dims

This comment has been minimized.

Member

dims commented Feb 3, 2018

/approve

@cheftako

This comment has been minimized.

Member

cheftako commented Feb 5, 2018

So the 1.9 to 1.10 support issue is something I would love to hear from the out of tree cloud providers on. Any input @wlan0? In tree the changes I make in this PR should be sufficient for now. (Later we may need the cloud providers to start honoring the context restrictions like timeout but that is still distant at this point) I would think the out of tree providers already branching based on K8S version so @justinsb suggested branching plan would already be taken care of. They would still need to fix there head/1.10 branch after this fix but if the branch already exists older releases should be fine. That having been said I'm not supporting an out of tree cloud provider yet, so I would appreciate input from anyone supporting an out of tree cloud provider.

Add context to all relevant cloud APIs
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.
For an idea of the full scope of this change please look at PR #58532.
@cheftako

This comment has been minimized.

Member

cheftako commented Feb 6, 2018

/remove hold

@cheftako

This comment has been minimized.

Member

cheftako commented Feb 6, 2018

/test pull-kubernetes-bazel-test

@lavalamp

This comment has been minimized.

Member

lavalamp commented Feb 6, 2018

/hold remove

@lavalamp

This comment has been minimized.

Member

lavalamp commented Feb 6, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 6, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 6, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheftako, dims, lavalamp

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@lavalamp

This comment has been minimized.

Member

lavalamp commented Feb 6, 2018

/remove-hold
/hold-remove

@lavalamp

This comment has been minimized.

Member

lavalamp commented Feb 6, 2018

/unhold

@lavalamp

This comment has been minimized.

Member

lavalamp commented Feb 6, 2018

@fejta the above few comments might be a good usability study for the hold command :)

@fejta-bot

This comment has been minimized.

fejta-bot commented Feb 7, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@cheftako

This comment has been minimized.

Member

cheftako commented Feb 7, 2018

/test pull-kubernetes-bazel-test

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 7, 2018

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 here.

@k8s-merge-robot k8s-merge-robot merged commit e5b6026 into kubernetes:master Feb 7, 2018

13 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation cheftako authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

dims pushed a commit to dims/kubernetes that referenced this pull request 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.
```

@ash2k ash2k referenced this pull request Feb 17, 2018

Merged

Use Dial with context #60012

k8s-merge-robot added a commit that referenced this pull request 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 pull request 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 pull request 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 pull request 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 pull request 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