kubernetes_connect: add timeout settings #10

cben · 2017-05-01T11:54:57Z

Ability to constrol kubeclient timeouts from settings.yml

openshift half: ~~ManageIQ/manageiq-providers-openshift#8~~ unnecessary on master given ManageIQ/manageiq-providers-openshift#7, but will add it in backports.

Tested:

Exercised in rails console and refresh, events, SSA in manageiq, confirmed low enough values cause timeouts.
core manageiq tests passed locally with this, but I don't think they cover anything relevant.

https://bugzilla.redhat.com/show_bug.cgi?id=1440950
@miq-bot add-label euwe/yes, fine/yes (actually we plan a 5.7.1 hotfix but I suppose we also want normal backports, so customer uprading will not lose hotfix functionality?)

@moolitayer @agrare Please review.

miq-bot · 2017-05-01T11:55:10Z

@cben Cannot apply the following labels because they are not recognized: fine/yes (actually we plan a 5.7.1 hotfix but i suppose we also want normal backports, so customer uprading will not lose hotfix functionality?)

cben · 2017-05-01T12:04:05Z

https://bugzilla.redhat.com/show_bug.cgi?id=1440950
@miq-bot add-label euwe/yes, fine/yes

moolitayer · 2017-05-01T12:22:27Z

@cben will our gem dependency in euwe bring in the new gem automatically or will we need to upgrade it?

agrare

LGTM
@cben once the gem is release put a PR in to https://github.com/ManageIQ/manageiq-gems-pending to change https://github.com/ManageIQ/manageiq-gems-pending/blob/6ddfbb62b1178d54bd16b020d171c679bd602c5a/manageiq-gems-pending.gemspec#L42 so it will pick up the new gem.

cben · 2017-05-01T12:56:06Z

we have strict =2.3.0 on euwe, fine and master, will need a gems-pending PR and backport it.

agrare · 2017-05-08T13:00:51Z

@cben any luck getting a new gem released?

simon3z · 2017-05-10T10:02:27Z

@cben any luck getting a new gem released?

@cben @agrare gem is released here: https://rubygems.org/gems/kubeclient/versions/2.4.0

simon3z · 2017-05-10T15:14:36Z

config/settings.yml

@@ -3,6 +3,8 @@
  :ems_kubernetes:
    :event_handling:
      :event_groups:
+    :open_timeout: 60.seconds
+    :read_timeout: 60.seconds


@cben @agrare wasn't the default 2 minutes? Are we shortening this timeout by default?

The default read_timeout in ruby / restclient was always 60. (open_timeout was infinite in ruby 2.2 but afaict euwe appliance was already ruby 2.3)

I was never able to account for the "2 minutes" number.

"60 + 60 = 120" is not a plausible explanation, doesn't take a minute to establish tcp + tls.

Curl showed server takes >2min but that doesn't mean anything, client may timeout after 1min.

IIRC the source for us believing we timeout at 2 minutes is log lines ~2min apart. But we don't have per-request log lines, we have something like "start of refresh – error = 2min"... there are many requests before images.

Darn. Customer log strongly suggests timeout was 2min.
It contains >70 timeouts, and times are very stable: 9–12sec from first connect (/api) to second connect (/oapi) then 126–128sec to timeout.
This agrees perfrect with per-request timing in VCR: all /api requests total 11sec, /oapi requests without images total 7sec.

Gonna simulate/reproduce a slow server and measure actual timeout before & after patch...

=> It takes 2 minutes on euwe with old kubeclient too. Nothing changed here.
The reason turned out to be that ruby's Net::HTTP unconditionally retries requests that are supposed to be idempotent (e.g. GET, DELETE but not POST)
[ankane/the-ultimate-guide-to-ruby-timeouts#8, https://bugs.ruby-lang.org/issues/10674]

@simon3z ready for merge.

simon3z · 2017-05-11T15:53:45Z

@cben let me know when this is ready.

miq-bot · 2017-05-11T15:58:57Z

This pull request is not mergeable. Please rebase and repush.

simon3z · 2017-05-15T08:24:35Z

config/settings.yml

 :http_proxy:
  :kubernetes:
    :host:
    :password:
    :port:
    :user:
-:container_scanning:
-  :scanning_job_timeout: 20.minutes


@cben really? 😮 I guess you needed a more careful rebase 😊

whoa thanks fixed.
need to revise my mergetool config, meld without --auto-merge is error prone...

Relies on kubeclient 2.4 bumped in ManageIQ/manageiq-gems-pending#156.

miq-bot · 2017-05-15T09:18:38Z

Checked commit cben@e74b251 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

ManageIQ/manageiq-providers-kubernetes#10 from cben/kubeclient-timeout kubernetes_connect: add timeout settings (cherry picked from merge commit ManageIQ/manageiq-providers-kubernetes@1ee90b5) openshift_connect: use kubernetes timeout settings (cherry picked from unmerged ManageIQ/manageiq-providers-openshift#8 - unnecessary on master but required in backports) Requires kubeclient >= 2.4.0

bump kubeclient ~> 2.4.0 (ported from manageiq-gems-pending.gemspec to gems/pending/Gemfile) - Merge ManageIQ/manageiq-providers-kubernetes#10 kubernetes_connect: add timeout settings (cherry picked from merge commit ManageIQ/manageiq-providers-kubernetes@1ee90b5) - openshift_connect: use kubernetes timeout settings (cherry picked from unmerged ManageIQ/manageiq-providers-openshift#8 - unnecessary on master but required in backports)

simaishi · 2017-05-22T18:55:43Z

@cben Marking as euwe/conflict for now. Please remove the conflict label when you have Euwe PR. Thanks!

cben · 2017-05-22T19:51:13Z

@miq-bot remove-label euwe/conflict
ManageIQ/manageiq#15188

simaishi · 2017-05-24T17:02:53Z

Backported to Euwe via ManageIQ/manageiq#15188

simaishi · 2017-06-08T15:52:18Z

Backported to Fine via ManageIQ/manageiq#15090

miq-bot added euwe/yes wip labels May 1, 2017

cben mentioned this pull request May 1, 2017

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

Closed

3 tasks

miq-bot added the fine/yes label May 1, 2017

cben force-pushed the kubeclient-timeout branch from 4b2c7ea to 98e0473 Compare May 1, 2017 12:06

moolitayer approved these changes May 1, 2017

View reviewed changes

agrare approved these changes May 1, 2017

View reviewed changes

cben mentioned this pull request May 1, 2017

[WIP] Settings for providers ManageIQ/manageiq#10944

Merged

cben mentioned this pull request May 10, 2017

bump kubeclient ~> 2.4.0 ManageIQ/manageiq-gems-pending#156

Merged

cben changed the title ~~[WIP] kubernetes_connect: add timeout settings~~ kubernetes_connect: add timeout settings May 10, 2017

miq-bot removed the wip label May 10, 2017

cben closed this May 10, 2017

cben reopened this May 10, 2017

cben mentioned this pull request May 10, 2017

configurable timeout for SSA #14

Merged

simon3z reviewed May 10, 2017

View reviewed changes

miq-bot added the unmergeable label May 11, 2017

cben force-pushed the kubeclient-timeout branch from 98e0473 to 1a66bb3 Compare May 14, 2017 12:02

miq-bot removed the unmergeable label May 14, 2017

simon3z suggested changes May 15, 2017

View reviewed changes

kubernetes_connect: add timeout settings

e74b251

Relies on kubeclient 2.4 bumped in ManageIQ/manageiq-gems-pending#156.

cben force-pushed the kubeclient-timeout branch from 1a66bb3 to e74b251 Compare May 15, 2017 09:14

simon3z merged commit 1ee90b5 into ManageIQ:master May 15, 2017

cben mentioned this pull request May 15, 2017

[FINE] kubernetes_connect, openshift_connect: add timeout settings ManageIQ/manageiq#15090

Merged

simaishi added the euwe/conflict label May 22, 2017

cben mentioned this pull request May 22, 2017

[EUWE] kubernetes_connect, openshift_connect: add timeout settings ManageIQ/manageiq#15188

Merged

miq-bot removed the euwe/conflict label May 22, 2017

simaishi added euwe/backported and removed euwe/yes labels May 24, 2017

simaishi added fine/backported and removed fine/yes labels Jun 8, 2017

moolitayer added this to the Sprint 61 Ending May 22, 2017 milestone Aug 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubernetes_connect: add timeout settings #10

kubernetes_connect: add timeout settings #10

cben commented May 1, 2017 •

edited

Loading

miq-bot commented May 1, 2017

cben commented May 1, 2017

moolitayer commented May 1, 2017

agrare left a comment

cben commented May 1, 2017

agrare commented May 8, 2017

simon3z commented May 10, 2017

simon3z May 10, 2017

cben May 11, 2017

cben May 11, 2017 •

edited

Loading

cben May 14, 2017 •

edited

Loading

simon3z commented May 11, 2017

miq-bot commented May 11, 2017

simon3z May 15, 2017

cben May 15, 2017

miq-bot commented May 15, 2017

simaishi commented May 22, 2017

cben commented May 22, 2017

simaishi commented May 24, 2017

simaishi commented Jun 8, 2017

kubernetes_connect: add timeout settings #10

kubernetes_connect: add timeout settings #10

Conversation

cben commented May 1, 2017 • edited Loading

miq-bot commented May 1, 2017

cben commented May 1, 2017

moolitayer commented May 1, 2017

agrare left a comment

Choose a reason for hiding this comment

cben commented May 1, 2017

agrare commented May 8, 2017

simon3z commented May 10, 2017

simon3z May 10, 2017

Choose a reason for hiding this comment

cben May 11, 2017

Choose a reason for hiding this comment

cben May 11, 2017 • edited Loading

Choose a reason for hiding this comment

cben May 14, 2017 • edited Loading

Choose a reason for hiding this comment

simon3z commented May 11, 2017

miq-bot commented May 11, 2017

simon3z May 15, 2017

Choose a reason for hiding this comment

cben May 15, 2017

Choose a reason for hiding this comment

miq-bot commented May 15, 2017

simaishi commented May 22, 2017

cben commented May 22, 2017

simaishi commented May 24, 2017

simaishi commented Jun 8, 2017

cben commented May 1, 2017 •

edited

Loading

cben May 11, 2017 •

edited

Loading

cben May 14, 2017 •

edited

Loading