825136 - Extra --tries passed with DownloadFile

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Justin Wood (:Callek)]

So philor passed me https://tbpl.mozilla.org/php/getParsedLog.php?id=18304871&tree=Mozilla-Inbound suspecting that the retrying nature of DownloadFile was broken (and adding tries multiple times)

Code inspection based on that indeed does show it is broken (slightly) for this use-case. I'll explain below and then offer a possible solution:

We call DownloadFile() step for talos_from_code.py
http://mxr.mozilla.org/build/source/buildbotcustom/process/factory.py#5650
in this case explicitly passing in some wget args as an array.

We then in DownloadFile()'s constructor add to said array http://mxr.mozilla.org/build/source/buildbotcustom/steps/misc.py#173 the tries.

Subsequent calls to that constructor will end up appending to the array repeatedly (as would be teh case in a reconfig) rather than starting with the initial arg we pass in.

The solution, I suggest, is to use an array-splice to simply copy it, ala:
174             self.wget_args = wget_args[:]

Comment 1

[Edmund Wong (:ewong)]

Attachment: Clean up the self.wgets array before using it.

Comment 2

[[e-mail] (:bhearsum)]

Comment on attachment 696222 [details] [diff] [review]
Clean up the self.wgets array before using it.

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

"clean" isn't really an accurate description of what's happening here, but this patch looks good. The [:] slice returns a full copy of the list, so you don't end up with the shared reference problem.

Comment 3

[Chris Cooper [:coop] (he/him)]

(In reply to Justin Wood (:Callek) from comment #0) 
> Subsequent calls to that constructor will end up appending to the array
> repeatedly (as would be teh case in a reconfig) rather than starting with
> the initial arg we pass in.

FWIW, we've had this problem before in places where we iterate over args, leading to some comically long command lines. Any value in auditing more widely for this type of behavior?

Comment 4

[Justin Wood (:Callek)]

So, this patch is indeed correct and what we want, but fixes things for a different reason than I thought when I filed.

The issue is that, self.addFactoryArguments(...) happens at the end of __init__, buildbot uses those args to know how to init the step. The code *was* assigning a ref to what wget_args was to self.wget_args then modifying it, and then assigning wget_args to the addFactory...

This patch makes self.wget_args a *copy* (not a ref) to the passed-in-arg wget_args, and we still set addFactoryArguments based on the passed-in-arg, so this makes us safe.

Comment 5

[Edmund Wong (:ewong)]

Pushed to buildbotcustoms:

http://hg.mozilla.org/build/buildbotcustom/rev/275bd5879941

Comment 6

[hwine]

in production

Comment 7

[Edmund Wong (:ewong)]

I'm looking at https://tbpl.mozilla.org/php/getParsedLog.php?id=18530275&tree=Mozilla-Inbound&full=1 . 

It looks like it's fixed.