1187962 - Balrog submitter should retry smarter

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P1)

Description

[Rail Aliiev [:rail]]

We submit 2 PUT requests for every nightly builds. If the second one fails, we start from the first one. This adds unnecessary entries into the history table. We should retry 2 requests separately.

Comment 1

[Rail Aliiev [:rail]]

Attachment: idempotent_submitter-tools.diff

This is an untested patch. It relies on dictionary comparison, which surprisingly works. I can test the patch by backing a custom funsize-balrog-submitter docker image with this patch applied.

One thing that bothers me is the test case, which uses mock's side_effects, what is a sign that the code could be refactored. I'm not sure if it's worth to make a big change for a simple if/else case...

Comment 2

[[e-mail] (:bhearsum)]

Comment on attachment 8639470 [details] [diff] [review]
idempotent_submitter-tools.diff

Per IRC, I think it would be nicer to add retry calls around each update_build method. This gives you better granularity (retries will only happen for very specific failures), and avoids the need to check for existing data. It probably simplifies your test cases a lot too.

Might want to add retries around the get_data calls as well.

Comment 3

[Rail Aliiev [:rail]]

Comment on attachment 8639470 [details] [diff] [review]
idempotent_submitter-tools.diff

Thanks, I'll update the patch

Comment 4

[Rail Aliiev [:rail]]

Attachment: redo.diff

Comment 5

[[e-mail] (:bhearsum)]

Comment on attachment 8639650 [details] [diff] [review]
redo.diff

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

::: lib/python/balrog/submitter/cli.py
@@ +299,5 @@
> +                                                           data)),
> +                alias=json.dumps(alias),
> +                schemaVersion=schemaVersion, data_version=data_version)
> +
> +        retry(update_dated, sleeptime=10)

Do you want to be more specific about how you retry? retrier would let you deal with specific http return codes: http://mxr.mozilla.org/build/source/tools/buildfarm/utils/repository_manifest.py#122

It's not going to be a huge problem if you just retry for anything, so up to you.

Comment 6

[Rail Aliiev [:rail]]

Comment on attachment 8639650 [details] [diff] [review]
redo.diff

https://hg.mozilla.org/build/tools/rev/346d313d1180

I pushed it without any changes. I may customize the list of exceptions in the future, when we have more data on common errors happening here.

Comment 7

[Rail Aliiev [:rail]]

Probably I should work on the first patch to make sure we don't submit blobs if they are already there.

Comment 8

[Rail Aliiev [:rail]]

Attachment: idempotent_submission-tools.diff

Here we go. This one checks both dated and latest with moar tests.

Comment 9

[[e-mail] (:bhearsum)]

Comment on attachment 8643339 [details] [diff] [review]
idempotent_submission-tools.diff

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

Looks fine. The only thing I could say is that it might be useful to add a test where both dated and latest are up to date already.

Comment 10

[Rail Aliiev [:rail]]

(In reply to Ben Hearsum [:bhearsum] from comment #9)
> Comment on attachment 8643339 [details] [diff] [review]
> idempotent_submission-tools.diff
> 
> Review of attachment 8643339 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine. The only thing I could say is that it might be useful to add a
> test where both dated and latest are up to date already.

The last one (test_same_latest_data()) covers that:

  self.assertEqual(update_build.call_count, 0)

Comment 11

[Rail Aliiev [:rail]]

Comment on attachment 8643339 [details] [diff] [review]
idempotent_submission-tools.diff

https://hg.mozilla.org/build/tools/rev/224af2cb8bcc

Comment 12

[Rail Aliiev [:rail]]

Attachment: idempotent_submission-tools-1.diff

A small followup for the previous one. This handles the cases where we have multiple partials and we should compare subsets of the blob.

Comment 13

[Rail Aliiev [:rail]]

Comment on attachment 8643759 [details] [diff] [review]
idempotent_submission-tools-1.diff

https://hg.mozilla.org/build/tools/rev/6bcab61fa67d