1170142 - replace util.retry in build/tools with an import of "redo" into the vendor library

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[[e-mail] (:bhearsum)]

Redo is a standalone module at this point, and its RoR is not build/tools, so we shouldn't treat it that way. It should be easy to do this if we rip out all contents from utils/retry.py and replace it with imports of redo to avoid breaking existing usage. The retry.py script in buildfarm/utils.py might be trickier.

Comment 1

[[e-mail] (:bhearsum)]

bug 1173343 would be a bit easier if we had this, so I'm going to grab it.

Comment 2

[[e-mail] (:bhearsum)]

Attachment: redo-tools-1.diff

This is pretty much just copying in redo to the vendor library and replacing util/retry.py with reimports of redo. I'm not sure if all consumers of redo are going to have added lib/python as a sitedir, so I'm doing that as well to make sure we don't have any issues with PYTHONPATH.

Unfortunately, redo and the build/tools version were more out of date than I thought - most notably, the build/tools version didn't have the refactor that put the core logic into "retrier", so diffing the old util/retry.py with redo's __init__.py isn't terribly helpful. All of the tests still pass, though, and this diff shows that the only changes to them are:
* Adjust mocking of retry
* Add jitter=0 to a bunch of stuff (upstream patch)

So, I think this is relative safe, though I still plan to give the sheriffs a heads up when I land...it's hard to be 100% certain about things like this.

Comment 3

[Rail Aliiev [:rail]]

Comment on attachment 8617939 [details] [diff] [review]
redo-tools-1.diff

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

::: lib/python/mozilla_buildtools/test/test_util_retry.py
@@ +54,5 @@
> +        self.sleep_patcher = mock.patch('time.sleep')
> +        self.sleep_patcher.start()
> +
> +    def tearDown(self):
> +        self.sleep_patcher.stop()

Do we still need to keep the tests here? Shouldn't they go to the library? OR do you want to keep them separately to ensure the API is stable?

Comment 4

[[e-mail] (:bhearsum)]

(In reply to Rail Aliiev [:rail] from comment #3)
> Comment on attachment 8617939 [details] [diff] [review]
> redo-tools-1.diff
> 
> Review of attachment 8617939 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/python/mozilla_buildtools/test/test_util_retry.py
> @@ +54,5 @@
> > +        self.sleep_patcher = mock.patch('time.sleep')
> > +        self.sleep_patcher.start()
> > +
> > +    def tearDown(self):
> > +        self.sleep_patcher.stop()
> 
> Do we still need to keep the tests here? Shouldn't they go to the library?
> OR do you want to keep them separately to ensure the API is stable?

I think we can remove them later...I kept them for now to help me be sure about the API, like you said. Hopefully we can just get rid of util.retry completely down the road.

Comment 5

[[e-mail] (:bhearsum)]

Comment on attachment 8617939 [details] [diff] [review]
redo-tools-1.diff

This looked like it was working fine in production, but Travis found some test failures like:
   Traceback (most recent call last):

    lib/python/mozilla_buildtools/test/test_util_hg.py line 353 in testShareReset

      mercurial(self.repodir, self.wc, shareBase=shareBase)

    lib/python/util/hg.py line 571 in mercurial

      mirrors=mirrors, bundles=bundles, autoPurge=False)

    lib/python/util/hg.py line 531 in mercurial

      mirrors=mirrors)

    lib/python/util/hg.py line 392 in pull

      sleeptime = _ * RETRY_EXTRA_WAIT_SCALE

   TypeError: unsupported operand type(s) for *: 'NoneType' and 'int'

I backed out as a precaution until I can fix them.

Comment 6

[[e-mail] (:bhearsum)]

Looks like redo and the build/tools version were more inconsistent than I thought. The build/tools version of retrier yield's the sleeptime, the redo version does not. Grr.

Comment 7

[[e-mail] (:bhearsum)]

Attachment: import new version of redo with synced up retrier behaviour

Turns out the difference was that this build/tools patch: https://github.com/mozilla/build-tools/commit/da487e6ee8ac0028b435de9eadfd6650bd4c9227#diff-758ab347493e90b609675250012a9183 was not fully ported to redo: https://github.com/bhearsum/redo/commit/ef77b6792fa7bca6190eff63324a2488f892a064

I fixed that and we should be good now. Thankfully, I don't think this caused any problems in production - we only would've hit it if we actually retried for something.

Comment 8

[Rail Aliiev [:rail]]

Comment on attachment 8620941 [details] [diff] [review]
import new version of redo with synced up retrier behaviour

lgtm, let's try again! :)

Comment 9

[[e-mail] (:bhearsum)]

Looks like this is working well in production.