Add requests timeouts option #244

cben · 2017-04-24T09:32:30Z

I have a use case for extending the http timeout (a big openshift cluster whose /oapi/v1/images takes >2min to compute before it starts sending data).
This PR adds new options to control the 2 timeouts rest-client exposes.

I wasn't sure if the options should also affect watches, which currently have infinite timeout. Watch data arrives at unpredictable intervals, I think just adding ability to timeout would not make a useful API, so decided to punt and wait for a use case.
Backward compatible: Works with rest-client 1.8 and 2.0 (extended travis matrix to test both).
Default open timeout will depend on ruby version, which is exactly the previous behavior (added tests), users that want ruby-independent behavior can simply specify :open timeout.

cben · 2017-04-24T10:02:10Z

Opened rest-client/rest-client#591 but I don't think outcome there affects decisions here.
[deleted next comment, was intended there, sorry for noise]

simon3z · 2017-04-25T07:29:07Z

@cben please fix travis.

moolitayer · 2017-04-25T14:47:53Z

README.md

@@ -148,7 +152,9 @@ client = Kubeclient::Client.new(

 You can find information about tokens in [this guide](http://kubernetes.io/docs/user-guide/accessing-the-cluster/) and in [this reference](http://kubernetes.io/docs/admin/authentication/).

-You can also use kubeclient with non-blocking sockets such as Celluloid::IO, see [here](https://github.com/httprb/http/wiki/Parallel-requests-with-Celluloid%3A%3AIO)
+### Non-blocking IO


moolitayer · 2017-04-25T14:55:53Z

lib/kubeclient/common.rb

+      # These do NOT affect watch, watching never times out.
+      connect: 60,
+      read: 60
+    }


moolitayer · 2017-04-25T15:00:33Z

@cben I don't think this forces us to bump major, from your description it sounds like we were inconsistent (between ruby versions) and you are fixing that.

We both should test all the verbs thoroughly.

cc @grosser (regarding #204)

simon3z · 2017-04-26T10:44:21Z

@cben on the BZ (https://bugzilla.redhat.com/show_bug.cgi?id=1440950) you mentioned that you are considering to reduce this to something easier. Are you working on that now? Any idea on when it will be ready?

cben · 2017-04-26T12:27:45Z

found way to support older rest-client.
I'll switch to setting only the timeouts user gives, and will document default open timeout changed in ruby 2.3. Users that require consistent behavior ≤2.2 can just always pass :open.

hope to push it today, maybe tomorrow it test gives me trouble.

cben · 2017-04-27T12:21:31Z

@simon3z @moolitayer Reworked, PTAL. Now completely compatible, with tests for that. Updated PR description.

moolitayer · 2017-04-27T12:35:47Z

.Gemfile-rest-client-1.8.rb

@@ -0,0 +1,11 @@
+# Allow travis to test rest-client 1.x.


Why are we doing this? is our version not available?

I'm testing both, see .travis.yml.
I'm all for bumping to rest-client >= 2.0 and dropping these at some later point.

Just to double-check the matrix works:
https://travis-ci.org/abonas/kubeclient/jobs/226372922 run with Using rest-client 1.8.0
(using .Gemfile-rest-client-1.8.rb)
https://travis-ci.org/abonas/kubeclient/jobs/226372920 run with Using rest-client 2.0.2
(using Gemfile uses gemspec, which doesn't specify rest-client version, so we get latest)

@cben Can you create a pr to bump rest-client ? we should discuss that desperately
(I think we should do it, rest client < 2.0 has not seen updates in over two years)

moolitayer · 2017-04-27T13:15:51Z

README.md

+### Timeouts
+
+Watching never times out.
+One-off actions like `.get_*`, `.delete_*` have a timeout, can be changed:


s/have a timeout, can be changed/have a configurable timeout/ ?

👍 adopted "have a configurable timeout".

simon3z · 2017-04-27T13:22:53Z

@cben I don't think .Gemfile-rest-client-1.8.rb should be an hidden file. Any problem to leave it visible?

cben · 2017-04-27T13:28:13Z

I thought it's noise for local development (like .travis.yml). No problem, done.

moolitayer · 2017-04-27T13:31:13Z

lib/kubeclient/common.rb

@@ -416,6 +428,18 @@ def api
      JSON.parse(response)
    end

+    def self.restclient_read_timeout_option


private?

do you mind putting this method next to our exiting validate_X methods?

What happens if we pass in both options?

couldn't private because used by test too.
but yeah, this actually exposes it, not nice :-(

use send to bypass private in test?

move to separate class (Kubeclient::RestClientCompatibility)?

👍 for send in test

can't make private at all, as it's a class method called from normal method.
(or I'd need send in main code too)
I want the methods probing to get cached once, not per instance.
could make it a const but that feels even more public...

What happens if we pass in both options?

Interesting, you mean:

open_timeout: @timeouts[:open], read_timeout: @timeouts[:read], timeout: @timeouts[:read]

Yes, that works AFAICT! I've attempted something similar but not exactly this.

However, while our code would look simpler what actually happens will be harder to reason about.
It'll also be harder to test, due to rest-client/rest-client#591 I discovered while trying. I even tried testing it upstream in rest-client itself but gave up (rest-client/rest-client#595).

=> Bottom line: I'd prefer the method probing.

Yeah me too actually + we can remove when we bump rest-client

BTW make sure other parameters weren't renamed as well

moolitayer · 2017-04-27T13:34:11Z

lib/kubeclient/common.rb

@@ -42,14 +48,15 @@ module ClientMixin
    attr_reader :headers
    attr_reader :discovered

-    def initialize_client(
+    def initialize_client( # rubocop:disable Metrics/ParameterLists


I guess we can live with that. We should make travis warn and not fail on rubocop at some point

see rubocop's PR description.
I believe 8 positional f(a, b, c, d, e, f, g, h) would be insane,
but f(a, b, c, d, options) is not so bad, and f(a, b, c, d, x:, y:, z:, w:) is strictly cleaner than unpacking options inside the function, shouldn't be penalized.

AFAICT rubocop can't make this distinction, but this function is only place >5 total.

moolitayer · 2017-04-27T13:40:29Z

lib/kubeclient/common.rb

@@ -63,6 +70,9 @@ def initialize_client(
      @ssl_options = ssl_options
      @auth_options = auth_options
      @socket_options = socket_options
+      # Allow passing partial timeouts hash, without unspecified
+      # @timeouts[:foo] == nil resulting in infinite timeout.


This comment isn't telling me much. Do what you feel

With other options, you either get FOO_DEFAULTS or replace it entirely.
If you pass less keys than FOO_DEFAULTS had, it's treated as nil, which is generally buggy, in this case would silently give infinite timeout which I felt too subtle, so broke the pattern.

I'll send separate PR to do same with other options (and drop this comment).

👍 for code.
not sure we should merge all of the other defaults - lets discuss there

why would @timeouts[:foo] == nil result in infinite timeout?

nil represents infinite timeout to RestClient & Net::HTTP

why would an unexpected key be considered? why is merge solving it?

if you pass just one, timeouts:{open: 10} or timeouts: {read: 20}. merge ensures @timeouts always has both.

moolitayer

Looking good, a couple of comments

cben · 2017-04-27T15:12:33Z

PTAL.
Placated rubocop wrt. Gemfile-rest-client-1.8.rb.
Don't see a way to make restclient_read_timeout_option private (that I wouldn't have to explain to rubocop ;-)

cben · 2017-04-27T15:57:34Z

@moolitayer @simon3z responded to all points, believe this is as good as I can make it.

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.

Doesn't affect watch, only rest-client based functionality.

cben · 2017-04-28T07:36:35Z

Found rubocop knob to not penalize keyword args, dropped rubocop:disable 🎉
Removed dot from .Gemfile-rest-client-1.8.rb in one more place in .travis.yml.
Re-squashed commits cleanly.

simon3z · 2017-04-30T21:45:23Z

@cben since for master we're planning another major version, please clean this up (in another PR) requiring the rest-client gem and removing the backward compatibility. Thanks.

Needs newer kubeclient with ManageIQ/kubeclient#244 or backported ManageIQ/kubeclient#246

timeouts options recently added in ManageIQ/kubeclient#244, Kubeclient 2.4.0.

moolitayer reviewed Apr 25, 2017

View reviewed changes

lib/kubeclient/common.rb Outdated

# These do NOT affect watch, watching never times out.

connect: 60,

read: 60

}

Copy link

Collaborator

moolitayer Apr 25, 2017

Choose a reason for hiding this comment

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

freeze

cben mentioned this pull request Apr 27, 2017

Add test for :timeout + :open_timeout combo behaving same as <2.0 rest-client/rest-client#595

Closed

cben force-pushed the timeout branch 2 times, most recently from f0e691f to d021c26 Compare April 27, 2017 12:09

moolitayer reviewed Apr 27, 2017

View reviewed changes

cben force-pushed the timeout branch from 2a874d9 to 22f465b Compare April 27, 2017 13:20

cben force-pushed the timeout branch from 22f465b to a271c3c Compare April 27, 2017 13:27

moolitayer reviewed Apr 27, 2017

View reviewed changes

moolitayer approved these changes Apr 27, 2017

View reviewed changes

cben force-pushed the timeout branch from 0a756ff to 085d2ab Compare April 28, 2017 07:24

cben added 3 commits April 28, 2017 10:31

Test both rest-client 1.8 and latest

15cd763

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.

ParameterLists rubocop: don't penalize keyword args

98c43fc

Add timeouts option

13c8126

Doesn't affect watch, only rest-client based functionality.

cben force-pushed the timeout branch from 085d2ab to 13c8126 Compare April 28, 2017 07:34

simon3z changed the title ~~Add timeouts option, bump rest-client ~> 2.0.0~~ Add requests timeouts option Apr 28, 2017

simon3z mentioned this pull request Apr 28, 2017

[v2.x] Add requests timeouts option #246

Merged

simon3z merged commit 2e98117 into ManageIQ:master Apr 30, 2017

cben added a commit to cben/manageiq-providers-kubernetes that referenced this pull request May 1, 2017

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

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-openshift that referenced this pull request May 1, 2017

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-kubernetes that referenced this pull request May 1, 2017

kubernetes_connect: add timeout settings

4b2c7ea

Needs newer kubeclient with ManageIQ/kubeclient#244 or backported ManageIQ/kubeclient#246

This was referenced May 1, 2017

kubernetes_connect: add timeout settings ManageIQ/manageiq-providers-kubernetes#10

Merged

openshift_connect: use kubernetes timeout settings ManageIQ/manageiq-providers-openshift#8

Closed

cben added a commit to cben/manageiq-providers-kubernetes that referenced this pull request May 1, 2017

kubernetes_connect: add timeout settings

98e0473

Needs newer kubeclient with ManageIQ/kubeclient#244 or backported ManageIQ/kubeclient#246

cben added a commit to cben/the-ultimate-guide-to-ruby-timeouts that referenced this pull request May 16, 2017

Add kubeclient

6c61857

timeouts options recently added in ManageIQ/kubeclient#244, Kubeclient 2.4.0.

cben mentioned this pull request May 16, 2017

Add Kubeclient ankane/the-ultimate-guide-to-ruby-timeouts#16

Merged

cben mentioned this pull request May 16, 2017

#read_timeout, #open_timeout always return effective values rest-client/rest-client#601

Open

cben mentioned this pull request Mar 4, 2018

modify open_timeout #163

Closed

Add requests timeouts option #244

Add requests timeouts option #244

Conversation

cben commented Apr 24, 2017 • edited Loading

cben commented Apr 24, 2017 • edited Loading

simon3z commented Apr 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented Apr 25, 2017

simon3z commented Apr 26, 2017

cben commented Apr 26, 2017 • edited Loading

cben commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cben Apr 27, 2017 • edited Loading

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

simon3z commented Apr 27, 2017

cben commented Apr 27, 2017 via email

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

cben Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

cben Apr 27, 2017 • edited Loading

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

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

moolitayer left a comment

Choose a reason for hiding this comment

cben commented Apr 27, 2017

cben commented Apr 27, 2017

cben commented Apr 28, 2017

simon3z commented Apr 30, 2017

cben commented Apr 24, 2017 •

edited

Loading

cben commented Apr 24, 2017 •

edited

Loading

cben commented Apr 26, 2017 •

edited

Loading

cben Apr 27, 2017 •

edited

Loading

cben Apr 27, 2017 •

edited

Loading

cben Apr 27, 2017 •

edited

Loading