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

Set timeout for inventory refresh calls #14245

Merged
merged 1 commit into from
Mar 10, 2017

Conversation

masayag
Copy link
Contributor

@masayag masayag commented Mar 9, 2017

The settings.yml contains timeout only for RHV service calls. However
when services are invoked as part of the refresh process, no timeout
is provided for these calls, letting the Net::HTTP 60 seconds default
timeout to be the affective timeout.

The 60 seconds default timeout is too short, therefore the default
timeout for inventory service calls to RHV is set to 1 hour.

http://bugzilla.redhat.com/1430722

The settings.yml contains timeout only for RHV service calls. However
when services are invoked as part of the refresh process, no timeout
is provided for these calls, letting the Net::HTTP 60 seconds default
timeout to be the affective timeout.

The 60 seconds default timeout is too short, therefore the default
timeout for inventory service calls to RHV is set to 1 hour.

http://bugzilla.redhat.com/1430722
@miq-bot
Copy link
Member

miq-bot commented Mar 9, 2017

Checked commit masayag@5565e5a with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks good. 👍

@masayag
Copy link
Contributor Author

masayag commented Mar 9, 2017

@miq-bot add_label blocker, euwe/yes, providers/rhevm

@masayag
Copy link
Contributor Author

masayag commented Mar 9, 2017

@miq-bot assign @borod108

@durandom
Copy link
Member

durandom commented Mar 9, 2017

Wow, nasty bug @masayag

here this option gets set

@durandom
Copy link
Member

durandom commented Mar 9, 2017

@oourfali wdyt regarding docs? I think new options should also have new docs :)

@borod108
Copy link

borod108 commented Mar 9, 2017

LGTM.

Copy link
Member

@durandom durandom left a comment

Choose a reason for hiding this comment

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

@miq-bot assign @agrare

@agrare pls merge

@agrare
Copy link
Member

agrare commented Mar 9, 2017

@masayag is this timeout per API call? 1 hour seems like a very long time for an API call

@masayag
Copy link
Contributor Author

masayag commented Mar 9, 2017

@agrare This timeout is per API call, same timeout is set for the :service (which is an explicit call for the API not as part of the refresh).

We hit that on a setup which had a slow network, where a request for all of the vms took over 10 minutes.
I wouldn't expect to hit timeout in any part of the refresh process. I assume that after the first timeout the PartialRefreshException will be thrown and the refresh process will be terminated due to that timeout.

@agrare
Copy link
Member

agrare commented Mar 10, 2017

Makes sense to have the same timeout as the other service, but if you need a 1hr timeout per API call I think we're going to have bigger problems haha

@agrare agrare merged commit 0a7255b into ManageIQ:master Mar 10, 2017
@agrare agrare self-assigned this Mar 10, 2017
@agrare agrare added the bug label Mar 10, 2017
@agrare agrare added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 10, 2017
simaishi pushed a commit that referenced this pull request Mar 15, 2017
@simaishi
Copy link
Contributor

Euwe backport details:

$ git log -1
commit 2b87bd1158d4148e6ed69642b1a63bbed37bcb32
Author: Adam Grare <[email protected]>
Date:   Fri Mar 10 09:21:49 2017 -0500

    Merge pull request #14245 from masayag/rhv_inventory_timeout
    
    Set timeout for inventory refresh calls
    (cherry picked from commit 0a7255b7ae6e3e94925173931466078b3da09e61)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1431620

@masayag masayag deleted the rhv_inventory_timeout branch June 27, 2018 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants