Skip to content
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 Timeout field to REST client (1.4, 1.5) config #10

Conversation

juanvallejo
Copy link
Contributor

Fixes test panics in k8s PR:
kubernetes/kubernetes#33958
and OpenShift PR: openshift/origin#11104

The linked Kubernetes PR adds a "Timeout" field to the rest client in
order to implement global http request timeouts for server requests.

@soltysh @fabianofranz

Fixes test panics in k8s PR:
kubernetes/kubernetes#33958
and OpenShift PR: openshift/origin#11104

The linked Kubernetes PR adds a "Timeout" field to the rest client in
order to implement global http request timeouts for server requests.
@lavalamp
Copy link
Member

lavalamp commented Oct 7, 2016

Code changes should be made in the main kubernetes repo. This repo is published from there by a bot. Sorry, I know this is terribly inconvenient-- I will try and get a message to appear when people open PRs.

@lavalamp lavalamp closed this Oct 7, 2016
@juanvallejo
Copy link
Contributor Author

@lavalamp Thanks for the info, will do. When I attempted to update client-go/1.5/rest/config.go in k8s, it would only allow me to stage and commit a file in k8s.io/kubernetes/staging/src... is this normal?

@soltysh
Copy link
Contributor

soltysh commented Oct 7, 2016

@juanvallejo sorry for misinformation, I didn't know either :(

@lavalamp
Copy link
Member

lavalamp commented Oct 7, 2016

The /staging directory is still not the source. You need to change the actual source and run staging/copy.sh. You can look at copy.sh to see what gets copied into the package you're trying to change.

@juanvallejo
Copy link
Contributor Author

@soltysh completely fine, I'm just happy to see different kinds of test failures on the PR now :)

@juanvallejo
Copy link
Contributor Author

@lavalamp I tried running staging/src/k8s.io/client-go/copy.sh, and it generates several untracked files as and changes a lot more files than I would expect in ./staging/src, is this expected behavior from running the script? Link to pastebin of modified and untracked files, thanks

@lavalamp
Copy link
Member

lavalamp commented Oct 7, 2016

It probably hasn't been run for a few days. @caesarxuchao does that look
right?

On Fri, Oct 7, 2016 at 4:21 PM, Juan Vallejo notifications@github.com
wrote:

@lavalamp https://github.com/lavalamp I tried running staging/src/
k8s.io/client-go/copy.sh, and it generates several untracked files as and
changes a lot more files than I would expect in ./staging/src, is this
expected behavior from running the script? Link to pastebin of modified
and untracked files http://pastebin.com/dWrXWmwk, thanks


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

@caesarxuchao
Copy link
Member

kubernetes/kubernetes#31994 is merged recently, but we didn't run copy.sh after that, so most diffs are caused by that.

Note that #31994 is a breaking change, if we are going to follow semver, we'll need to bump client-go to v2.0.0. We need to make a decision for #9 soon.

@lavalamp
Copy link
Member

lavalamp commented Oct 7, 2016

Gotcha. hrm.

On Fri, Oct 7, 2016 at 4:40 PM, Chao Xu notifications@github.com wrote:

kubernetes/kubernetes#31994
kubernetes/kubernetes#31994 is merged recently,
but we didn't run copy.sh after that, so most diffs are caused by that.

Note that #31994 is a breaking change, if we are going to follow semver,
we'll need to bump client-go to v2.0.0. We need to make a decision for #9
#9 soon.


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

k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this pull request Oct 15, 2016
…t-flag

Automatic merge from submit-queue

Add global timeout flag

**Release note**:
```release-note
Add a new global option "--request-timeout" to the `kubectl` client
```

UPSTREAM: kubernetes/client-go#10

This patch adds a global timeout flag (viewable with `kubectl -h`) with
a default value of `0s` (meaning no timeout).

The timeout value is added to the default http client, so that zero
values and default behavior are enforced by the client.

Adding a global timeout ensures that user-made scripts won't hang for an
indefinite amount of time while performing remote calls (right now, remote
calls are re-tried up to 10 times when each attempt fails, however, there is
no option to set a timeout in order to prevent any of these 10 attempts from
hanging indefinitely).

**Example**
```
$ kubectl get pods # no timeout flag set - default to 0s (which means no
timeout)
NAME                      READY     STATUS    RESTARTS   AGE
docker-registry-1-h7etw   1/1       Running   1          2h
router-1-uv0f9            1/1       Running   1          2h

$ kubectl get pods --request-timeout=0 # zero means no timeout no timeout flag set
NAME                      READY     STATUS    RESTARTS   AGE
docker-registry-1-h7etw   1/1       Running   1          2h
router-1-uv0f9            1/1       Running   1          2h

$kubectl get pods --request-timeout=1ms
Unable to connect to the server: net/http: request canceled while
waiting for connection (Client.Timeout exceeded while awaiting headers)
```
sttts pushed a commit to sttts/client-go that referenced this pull request Dec 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants