1128601 - Retry on http get errors in mozregression

Status: RESOLVED FIXED

(Testing :: mozregression, defect)

Description

[Julien Pagès (:parkouss)]

As stated in Bug 1088020 comment 9, it may happen (sometimes often!) that we have network related errors and that cause mozregression to crash.

We should attempt to retry on some well known errors (look at the comment link above). A good solution to do this seems to be the redo python package (https://pypi.python.org/pypi/redo). It looks great with some cool features like decorators, ability to sleep before new attempts and so on - perfect for network error retrying.

Comment 1

[Jalpreet Singh Nanda [:imjalpreet]]

Hey, I want to take this bug up. Can you please guide me where to start from?

Comment 2

[Julien Pagès (:parkouss)]

Yeah sure.

So you can start by looking at Bug 1888020, the last conversations are about that. The idea is that we have here:

https://github.com/mozilla/mozregression/blob/master/mozregression/utils.py#L160
https://github.com/mozilla/mozregression/blob/master/mozregression/build_data.py#L244
https://github.com/mozilla/mozregression/blob/master/mozregression/build_data.py#L321

For example (you cans search for 'get_http_session' in the code, that can help to find other places) where we need to retry in case of network errors. I looked at https://pypi.python.org/pypi/redo and it seems to me simple and perfect for the task, so I propose that we add it as dependency and use it to do the retry logic.

Is that OK for you ? If you're still interested, please tell me an I will assign you the bug. :)

Comment 3

[Jalpreet Singh Nanda [:imjalpreet]]

Yeah, I am interested in solving this bug. Sorry, for a late response. Actually, I had some university work. I would like this bug to be assigned to me.

Comment 4

[Julien Pagès (:parkouss)]

Great!

No problem at all for the late response.

Tell me if you have questions. :)

Comment 5

[Jalpreet Singh Nanda [:imjalpreet]]

Thanks. :) I will get back to you if I have any questions.

Comment 6

[Jalpreet Singh Nanda [:imjalpreet]]

Can you please explain what exactly I need to do in the bug? I have found out where all 'get_http_session' is being used and then do I have to check whether this call is successful or unsuccessful? and if this is unsuccessful, then I have to retry it? If I am wrong do correct me.

Comment 7

[Julien Pagès (:parkouss)]

Ok, then the get_http_session returns a session object, and we us the get method on this to download a url.

Sometime these "get()" calls fails, with some known errors:

- requests.exceptions.HTTPError
- requests.exceptions.ConnectionError

The idea is to retry (max three times I would say) when and only when these errors occurs in the get calls. A good way to do this is to use the redo module.

Look here fore example:

https://github.com/mozilla/mozregression/blob/master/mozregression/build_data.py#L339

I suspect that we could rewrite this like this with redo (not tested):

response = redo.retry(get_http_session().get, attempts=3, sleeptime=1, retry_exceptions=(HTTPError, ConnectionError), args=(chset_url,))

Of course, as I suspect that we will use the same arguments in many places, it could be good to create a retry_get function in utils.py that will provide our defaults arguments (sleeptime, attempt, etc).

Tell me if it is not clear for you. :)

Comment 8

[Jalpreet Singh Nanda [:imjalpreet]]

How can I test the changes?

Comment 9

[Julien Pagès (:parkouss)]

That may be a bit hard, but we could unit test this with mock I would say. You can launch the tests by following: https://wiki.mozilla.org/Auto-tools/Projects/Mozregression.

By the way, you don't need to NI me each time, I'm already registered on this bug and I got a mail when there are changes. :)

Comment 10

[Jalpreet Singh Nanda [:imjalpreet]]

I had a doubt that how do I add redo as a dependency and use it? Do I have to add the entire package files to the source folder or is there any other way?

Comment 11

[Julien Pagès (:parkouss)]

Yeah, there is an other way:

Add 'redo' in the setup.py file (install_requires list) : https://github.com/mozilla/mozregression/blob/master/setup.py#L27

add this line and issue again a "pip install -e ." in your mozregression folder - this will download the redo package. This is working thanks to pipy and pip: http://en.wikipedia.org/wiki/Pip_%28package_manager%29. You can serch more documentation on these over internet.

Also when users will download a new mozregression with this new dependency, that will be downloaded automatically for them. :)

Comment 12

[Jalpreet Singh Nanda [:imjalpreet]]

Ok, Thanks.

Comment 13

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Error Screenshot

I am getting this error in the following line of code: 
response = redo.retry(get_http_session().get, attempts=retry_get(1), sleeptime=retry_get(2), retry_exceptions=retry_get(3),args=(self.pushlog_url()));

Could you please tell what is the right approach?

Comment 14

[Julien Pagès (:parkouss)]

I think you misunderstood the purpose of the retry_get. I imagined just a wrapper for the retry function. Something like:

def retry_get(**kwargs):
    kwargs.setdefault('attempts', 3)
    kwargs.setdefault('sleeptime', 1)
    kwargs.setdefault('retry_exceptions', (HTTPError, ConnectionError))
    return redo.retry(get_http_session().get, **kwargs)

This will just allow to write:

response = retry_get(args=(chset_url,))

Or something like that, just to not copy paste things.
The attempts, sleeptime etc parameters will have the default so we won't have to repeat them everywhere.

I'm sorry, I may have been not clear on this. :) Thanks for beeing persistent!

Comment 15

[Jalpreet Singh Nanda [:imjalpreet]]

I have one doubt, are the args also a part of the **kwargs or we have to send it as a seperate argument to the redo.retry function?

Comment 16

[Julien Pagès (:parkouss)]

Yep, args are a part of the **kwargs.

Comment 17

[Jalpreet Singh Nanda [:imjalpreet]]

Can you please tell a way I can pass something like 'stream=True' in the args? I am unable to find a way to do so.

Comment 18

[Jalpreet Singh Nanda [:imjalpreet]]

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #17)
> Can you please tell a way I can pass something like 'stream=True' in the
> args? I am unable to find a way to do so.

This is being used in utils.py.

Comment 19

[Julien Pagès (:parkouss)]

yeah, you would have to do a call like:

retry_get(args=(url,), kwargs=dict(stream=True))

Note that this is another kwargs here, the one that would be passed directly in redo.retry.

But you may let this call (I think it is the one in download_url) without the retry logic actually. It would be a bit more complicated, and I'm not sure it is useful - so you can let this call as it is, without retry_get.

Comment 20

[Jalpreet Singh Nanda [:imjalpreet]]

So, should I use redo.retry or not? or should I leave it in it's original state?

Comment 21

[Julien Pagès (:parkouss)]

No this is not required in the utils.download_url function.

Comment 22

[Jalpreet Singh Nanda [:imjalpreet]]

I am ready with the patch should I submit it here or on github?

Comment 23

[Julien Pagès (:parkouss)]

Cool! Please do a pull request on gthub.

Comment 24

[Jalpreet Singh Nanda [:imjalpreet]]

I have sent the PR.

Comment 25

[Julien Pagès (:parkouss)]

Thanks! Sorry, I forgot to mention that you also need to ask for a review here. You need to click on the add an attachment link, fill the form like stated here (https://globau.wordpress.com/2013/10/21/github-pull-requests-and-bugzilla/), select "?" for the field review and copy paste my email in the text field (to ask me to review the PR).

Thanks Jalpreet

Comment 26

[Julien Pagès (:parkouss)]

Anyway I looked at the PR, this all looks good!

You just have to be compliant with our checker (that's style issue), You can see them here: https://travis-ci.org/mozilla/mozregression/builds/50596260

Also, it's preferable that you run that on you own. You can issue a:

pip install flake8
flake8 mozregression

to run the command locally. I will add that on the project page, it's missing.

So if you can fix the flake8 warnings, then squash your commits (http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html), do a git push -f (-f will be required because git history has changed) the request a review here, that would be awesome. :)

Comment 27

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Bug 1128601-Proposed_Patch

Comment 28

[Julien Pagès (:parkouss)]

Hey, thanks for this!

Unfortunately, this is not working (I made some comments on the github requests):

mozregression -g 2014-11-11 -b 2014-11-13
Retrieving valid builds from datetime.date(2014, 11, 11) generated an exception: get() takes exactly 2 arguments (4 given)

Also, looking at the patch, it seems to me now that the parameters to give to url_get are not really user friendly (the use of arg and kwargs is somewhere tricky and not really easy to read). Maybe it is better finally to just mimic the original get method (sorry, I changed my mind about it).

Could you make that change please ? I suppose that it will help for the retry_get calls, and the code will work as expected.

Thanks again, we're almost there. :)

Comment 29

[Julien Pagès (:parkouss)]

Comment on attachment 8564023 [details] [review]
Bug 1128601-Proposed_Patch

I remove the request flag for now as I asked Jalpreet to change the code.

Comment 30

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Bug 1128601-Proposed_Patch

Comment 31

[Julien Pagès (:parkouss)]

Hey Jalpreet,

I noted some things on the pull requested that I would like to see addressed before merging this. Also unless tests are passing, I don't think it works if you try the command:

mozregression -g 2014-11-11 -b 2014-11-13

Tests are passing because we mock the get calls to not issue real network things. That's tricky and may be done in a better way, but for now we have to be careful on that.

Can you ping me here when you have done the changes ? If you want, we can speak on the mozilla irc, on the #ateam channel. My nickname is parkouss.

Comment 32

[Julien Pagès (:parkouss)]

Since this bug is close to the end (thanks for that!), maybe you would be interested in another bug once this one is done ? There is bug 1132867 that may interest you if you like. :)

Comment 33

[Jalpreet Singh Nanda [:imjalpreet]]

Yeah, I am interested to take that bug!

Comment 34

[Julien Pagès (:parkouss)]

Excellent! Let's finish this one when you have time, then we'll focus on bug 1132867. I will assign it to you. :)

Comment 35

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Bug 1128601-Proposed_Patch

I have done the changes as suggested by you.

Comment 36

[Julien Pagès (:parkouss)]

Comment on attachment 8564294 [details] [review]
Bug 1128601-Proposed_Patch

Excellent, thank you!

Comment 37

[Julien Pagès (:parkouss)]

I tested this - seems all good to me - so I merged this in. :)