816216 - Add retry parameter to devicemanager commands

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Andrew Halberstadt [:ahal]]

In particular dmADB.pushFile() and dmADB.pushDir() will sometimes hit an adb error where adb will hang and the command will timeout. We have a whole bunch of places throughout the B2G automation code to get around this by retrying the push if it times out.
e.g: http://hg.mozilla.org/mozilla-central/file/6c23f41b0747/testing/marionette/client/marionette/emulator.py#l392

I would be happy if this got added to just pushFile and pushDir, but it seems like a retry option might be good to add to several of the other commands in both implementations as well.

Comment 1

[William Lachance (:wlach)]

Note that dmSUT already does retry automatically several times in case of error, with the exception of pullFile. This is even configurable by modifying the internal "retrylimit" variable. See for example:

https://github.com/mozilla/mozbase/blob/master/mozdevice/mozdevice/devicemanagerSUT.py#L113

For dmADB, I agree this would be a good idea.

Comment 2

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: implement retry functionality in _checkCmd

Like this? I tried to follow dmSUT's pattern.

Comment 3

[Andrew Halberstadt [:ahal]]

Comment on attachment 689704 [details] [diff] [review]
implement retry functionality in _checkCmd

Review of attachment 689704 [details] [diff] [review]:
-----------------------------------------------------------------

I guess it depends how much work you want to do :p. On one hand I'd say this is fine because it's how dmSUT works, on the other I can't say I'm a fan of how dmSUT works. We found the larger the file you are pushing, the more likely it is to fail. For that reason I'd rather see a retries parameter that you can pass in per function that defaults to 1 (or maybe that defaults to self.retrylimit). Just personal preference, but doing it this way is quicker and easier so I'd be fine with it for now.

I'll f+ and leave it up to you

Comment 4

[William Lachance (:wlach)]

(In reply to Andrew Halberstadt [:ahal] from comment #3)
> Comment on attachment 689704 [details] [diff] [review]
> implement retry functionality in _checkCmd
> 
> Review of attachment 689704 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess it depends how much work you want to do :p. On one hand I'd say this
> is fine because it's how dmSUT works, on the other I can't say I'm a fan of
> how dmSUT works. We found the larger the file you are pushing, the more
> likely it is to fail. For that reason I'd rather see a retries parameter
> that you can pass in per function that defaults to 1 (or maybe that defaults
> to self.retrylimit). Just personal preference, but doing it this way is
> quicker and easier so I'd be fine with it for now.

I'd prefer if we didn't introduce any more confusing differences between dmADB and dmSUT. The patch just posted is fine, but adding a retries parameter to each function is something I'd want in both implementations.

Comment 5

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: Add retrylimit param to pushdir, pushfile, checkcmd and checkcmdas

:ahal, like this?
:wlach, I guess we could add the retrylimit kwarg in the abstract methods in dm.py?

Comment 6

[Andrew Halberstadt [:ahal]]

(In reply to William Lachance (:wlach) from comment #4)
> I'd prefer if we didn't introduce any more confusing differences between
> dmADB and dmSUT. The patch just posted is fine, but adding a retries
> parameter to each function is something I'd want in both implementations.

Agreed. Which is why I would be fine with just landing this as is for now (the other way would involve more changes). Sorry, should have made that more clear.

Mihnea, however if you are up to it, feel free to make the changes across the board

Comment 7

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Andrew Halberstadt [:ahal] from comment #6)
> Mihnea, however if you are up to it, feel free to make the changes across
> the board

Sure, so basically this means having the retrylimit kwarg in the method prototypes for pushFile, pushDir and the corresponding implementations of "checkCmd" (or however they are called in dmSUT). Something similar to my latest patch but replicated for dmSUT and reflected in the abstract dm, right?

Also, do you mind changing retrylimit to retry_limit? :)

Comment 8

[Andrew Halberstadt [:ahal]]

(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #7)
> Sure, so basically this means having the retrylimit kwarg in the method
> prototypes for pushFile, pushDir and the corresponding implementations of
> "checkCmd" (or however they are called in dmSUT). Something similar to my
> latest patch but replicated for dmSUT and reflected in the abstract dm,
> right?

Yep, sounds about right

> Also, do you mind changing retrylimit to retry_limit? :)

Please!

Comment 9

[William Lachance (:wlach)]

(In reply to Andrew Halberstadt [:ahal] from comment #8)

> > Also, do you mind changing retrylimit to retry_limit? :)
> 
> Please!

For things like this we should be using the style in the surrounding module, which would mean a variable named retryLimit (retrylimit is probably close enough).

Comment 10

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to William Lachance (:wlach) from comment #9)
> (In reply to Andrew Halberstadt [:ahal] from comment #8)
> 
> > > Also, do you mind changing retrylimit to retry_limit? :)
> > 
> > Please!
> 
> For things like this we should be using the style in the surrounding module,
> which would mean a variable named retryLimit (retrylimit is probably close
> enough).

You are right, I forgot about the Java-ness of dm.

Comment 11

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: add retryLimit to dm, dmADB and dmSUT in pushFile and pushDir

Comment 12

[Andrew Halberstadt [:ahal]]

Comment on attachment 689801 [details] [diff] [review]
add retryLimit to dm, dmADB and dmSUT in pushFile and pushDir

Review of attachment 689801 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I don't think 'self' will be defined in the method signature though. I can change to r+ with that fixed.

::: mozdevice/mozdevice/devicemanagerADB.py
@@ +164,4 @@
>      def _disconnectRemoteADB(self):
>          self._checkCmd(["disconnect", self.host + ":" + str(self.port)])
>  
> +    def pushFile(self, localname, destname, retryLimit=self.retryLimit):

Pretty sure this won't work. You'll need to do something like 'retryLimit = retryLimit or self.retryLimit'

@@ +735,5 @@
> +                proc.kill()
> +                retries += 1
> +                continue
> +            return ret_code
> +        raise DMError("Timeout exceeded for _checkCmd call for %d retries." % retries)

nit: s/call for/call after

Comment 13

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: add retryLimit to dm, dmADB and dmSUT in pushFile and pushDir

Fixed

Comment 14

[Andrew Halberstadt [:ahal]]

Comment on attachment 689809 [details] [diff] [review]
add retryLimit to dm, dmADB and dmSUT in pushFile and pushDir

Review of attachment 689809 [details] [diff] [review]:
-----------------------------------------------------------------

Cool, thanks!

Comment 15

[Andrew Halberstadt [:ahal]]

I filed bug 819482 to remove the code we had in the B2G automation to do this

Comment 16

[Mihnea Dobrescu-Balaur (:mihneadb)]

I'm not familiar with the current policy on where the dm code should be merged first (mozbase@github vs m-c). Will you take care of the merge?

Comment 17

[Andrew Halberstadt [:ahal]]

Sure. I want to run it through try first to be safe. But normally we just land in github and it will get synced to m-c whenever.

Comment 18

[Andrew Halberstadt [:ahal]]

try: https://tbpl.mozilla.org/?tree=Try&rev=e6fa566d858b

Comment 19

[Andrew Halberstadt [:ahal]]

Looks like we missed a parameter: https://tbpl.mozilla.org/php/getParsedLog.php?id=17722618&tree=Try

Comment 20

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: add retryLimit to dm, dmADB and dmSUT in pushFile and pushDir

oops, fixed that.

Comment 21

[Andrew Halberstadt [:ahal]]

Pushed to try again: https://hg.mozilla.org/try/rev/48bf2fa5b665

Comment 22

[Andrew Halberstadt [:ahal]]

https://github.com/mozilla/mozbase/commit/93701293b5fa5ea538faec9d7f3ba6abcdf4ac48

I think we can wait for the next mozbase -> m-c, no need to cherry pick this.