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.

Sign up for GitHub

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

Jump to bottom

[v2.x] Add requests timeouts option #246

Merged
merged 3 commits into from
Apr 30, 2017
Merged

[v2.x] Add requests timeouts option #246

merged 3 commits into from
Apr 30, 2017

Conversation

simon3z
Copy link
Collaborator

@simon3z simon3z commented Apr 28, 2017

This is a backport to 2.3.x of #244

Beni can you test this before we'll go ahead and merge #244. Thanks.

cc @cben @moolitayer

@simon3z simon3z changed the title [2.3] Add requests timeouts option [v2] Add requests timeouts option Apr 28, 2017
@simon3z simon3z changed the base branch from v2.3.x to v2.x April 28, 2017 12:22
cben added 2 commits April 28, 2017 14:30
@cben @simon3z
Test both rest-client 1.8 and latest
cecf873 
Until we can require 2.0 - next major release?

In theory our gemspec lets you combine any ancient
rest-client with kubeclient but not really,
our tests fail on 1.7.
@cben @simon3z
ParameterLists rubocop: don't penalize keyword args
991871e
@simon3z simon3z changed the title [v2] Add requests timeouts option [v2.x] Add requests timeouts option Apr 28, 2017
@cben @simon3z
Add timeouts option
539591f 
Doesn't affect watch, only rest-client based functionality.

Signed-off-by: Federico Simoncelli <[email protected]>
simon3z
simon3z commented Apr 28, 2017
View reviewed changes
test/test_kubeclient.rb

client = Kubeclient::Client.new('http://localhost:8080/api/')

exception = assert_raises(KubeException) { client.api }
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cben note that I changed this to KubeException from Kubeclient::HttpError

simon3z
simon3z commented Apr 28, 2017
View reviewed changes
lib/kubeclient/common.rb
elsif RestClient::Resource.instance_methods.include?(:timeout) # rest-client 1.x
:timeout
else
fail ArgumentError("RestClient doesn't support neither :read_timeout nor :timeout")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cben I changed raise to fail.

@cben
Copy link
Collaborator

cben commented Apr 30, 2017 •
edited
Loading

  • Tested kubeclient manually. On a slow (mobile) connection, setting either open: 0.5 or read: 0.2 causes timeouts, which having both at 1, 60, nil (infinite) or omitted (default) works.
  • Testing manageiq with this...
    • With high timeouts {open: 60, read: 300} => works fine — refresh (get_*), image scanning (using create_pod), and events watch.
    • Tweaked read timeout such that most operations succeed but /oapi get_images fails => refresh fails, image scanning fails (various errors, seems create_pod and delete_pod suceeded), events watch still worked (unrealibly, unless api discovery times out).

@simon3z simon3z merged commit a949abe into ManageIQ:v2.x Apr 30, 2017
cben added a commit to cben/manageiq-providers-kubernetes that referenced this pull request May 1, 2017
@cben
Add kubeclient timeout settings
444f59b 
Needs newer kubeclient with ManageIQ/kubeclient#244
or backported ManageIQ/kubeclient#246
cben added a commit to cben/manageiq-providers-kubernetes that referenced this pull request May 1, 2017
@cben
WIP DO NOT MERGE use ManageIQ/kubeclient#246
06e6a17
cben added a commit to cben/manageiq-providers-kubernetes that referenced this pull request May 1, 2017
@cben
kubernetes_connec: add timeout settings
85b47e6 
Needs newer kubeclient with ManageIQ/kubeclient#244
or backported ManageIQ/kubeclient#246
cben added a commit to cben/manageiq-providers-kubernetes that referenced this pull request May 1, 2017
@cben
WIP DO NOT MERGE use ManageIQ/kubeclient#246
610ea76
cben added a commit to cben/manageiq-providers-openshift that referenced this pull request May 1, 2017
@cben
openshift_connect: use kubernetes timeout settings
ffc6c98 
Needs newer kubeclient with ManageIQ/kubeclient#244
or backported ManageIQ/kubeclient#246
cben added a commit to cben/manageiq-providers-openshift that referenced this pull request May 1, 2017
@cben
WIP DO NOT MERGE use ManageIQ/kubeclient#246
52d7877
cben added a commit to cben/manageiq-providers-kubernetes that referenced this pull request May 1, 2017
@cben
kubernetes_connect: add timeout settings
4b2c7ea 
Needs newer kubeclient with ManageIQ/kubeclient#244
or backported ManageIQ/kubeclient#246
This was referenced May 1, 2017
Merged
Closed
cben added a commit to cben/manageiq-providers-kubernetes that referenced this pull request May 1, 2017
@cben
kubernetes_connect: add timeout settings
98e0473 
Needs newer kubeclient with ManageIQ/kubeclient#244
or backported ManageIQ/kubeclient#246
@agrare agrare mentioned this pull request May 10, 2017
Merged
@cben cben mentioned this pull request Mar 4, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cben cben cben approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@simon3z @cben