1255411 - Optimize update verify process

Status: RESOLVED FIXED

(Release Engineering :: Release Automation, defect)

Description

[Rail Aliiev [:rail]]

Update verify tests take a lot of time. Probably there is no reason to test all versions of firefox in the world.

Comment 1

[Rail Aliiev [:rail]]

Attachment: MozReview Request: Bug 1255411 - Optimize update verify process: skip some full checks. r=nthomas

Review commit: https://reviewboard.mozilla.org/r/39215/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39215/

Comment 2

[Rail Aliiev [:rail]]

I aggressively trimmed the config. Is it too radical?

Comment 3

[Nick Thomas [:nthomas] (UTC+12)]

Kinda torn about this. On one hand we can't test windows because of the SHA1 watershed, and that's where the vast majority of the users are; GTK3 removes older linux, and we may have a watershed for mac at some point if 10.6-10.8 are desupported. And yet we have this long tail of users on old versions and it's desirable to make sure they can update. We may need a test suite that looks at update.xml for static paths. or something. I'll think on it.

Comment 4

[Rail Aliiev [:rail]]

Comment on attachment 8728971 [details]
MozReview Request: Bug 1255411 - Optimize update verify process: skip some full checks. r=nthomas

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39215/diff/1-2/

Comment 5

[Rail Aliiev [:rail]]

This is another, "fair" approach. Limit the update verify to some number (30 by default in release promotion). With tests!

Comment 6

[Rail Aliiev [:rail]]

The interdiff looks a bit weird...

Comment 7

[Rail Aliiev [:rail]]

Attachment: MozReview Request: Bug 1255411 - Pass limit

Review commit: https://reviewboard.mozilla.org/r/40677/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40677/

Comment 8

[Rail Aliiev [:rail]]

Comment on attachment 8728971 [details]
MozReview Request: Bug 1255411 - Optimize update verify process: skip some full checks. r=nthomas

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39215/diff/2-3/

Comment 9

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Nick Thomas [:nthomas] from comment #3)
> Kinda torn about this. On one hand we can't test windows because of the SHA1
> watershed, and that's where the vast majority of the users are; GTK3 removes
> older linux, and we may have a watershed for mac at some point if 10.6-10.8
> are desupported. And yet we have this long tail of users on old versions and
> it's desirable to make sure they can update. We may need a test suite that
> looks at update.xml for static paths. or something. I'll think on it.

In this week's channel meeting I suggested that we might want to set a watershed periodically to ensure that older instances are updating with known good update paths while limiting the number of update tests that we have to run. I don't have details worked out just thinking how we can support what we need to update wise while limiting the work that we do for little benefit.

Comment 10

[Nick Thomas [:nthomas] (UTC+12)]

We could do that if the UX of multiple updates is OK, and it would probably help developers to write migration code for data stores changing (don't need to support that really old format forever). Not sure how we go about adding a new watershed though. If it was just for testing efficiency we'd like a set of moving watersheds, spaced some # of releases back, eg current-N cycles, current-*2N cycles, where N could be something like 5 for release, and maybe smaller for beta. But that might be a bit confusing to keep track of, and also to script as the Balrog would need adjusting each cycle. 

So going the other way with fixed watersheds, what happens when we get close to a new watershed ? Is it OK to go old -> current-1 -> current ? Could get a bit messy with partials which we do want to test. We could let it run a bit long until we clear those, eg old -> current-(N+4) -> current-4 -> current. Still a bit of a pain while ship-it is making bad (old!) suggestions about which partials to generate (bug 1146863).

rail's proposal of going back 30 lines in the update-verify config is a bit like moving watersheds, only limited to testing.
It works out to about 17 releases or 39.0.3 right now; for beta it's also 17 and 44.0b1. (The config has one line when we have a partial; two lines for older releases where we do a full check for en-US/de/ru, and just an update query for the rest). It should save about half the run time on mac, assuming that counting the number of locales we do a full check for is a good way to estimate. We'd have the fix the sort order use buildID, as RC builds end up lower in the file than the betas they succeed. I'm coming round to this but let me throw another idea out there ...

What if we do this
* (existing) full checks for all versions we have partials for
* (modified) full checks for en-US/de/ru, but only go back N releases instead of a long way
* (modified) quick checks for everything else, but know the version and buildID that should be offered, and make sure the appVersion and buildID match in the update.xml

Comment 11

[Rail Aliiev [:rail]]

(In reply to Nick Thomas [:nthomas] from comment #10)
> What if we do this
> * (existing) full checks for all versions we have partials for
> * (modified) full checks for en-US/de/ru, but only go back N releases
> instead of a long way
> * (modified) quick checks for everything else, but know the version and
> buildID that should be offered, and make sure the appVersion and buildID
> match in the update.xml

I like this approach!

Comment 12

[Rail Aliiev [:rail]]

Attachment: uv_reduce.diff

I ran this modified script against linux and mac and this is the diff against the current config: https://gist.github.com/rail/849ef79f966587bdd78a

Comment 13

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8733337 [details] [diff] [review]
uv_reduce.diff

Sorry for the delay reviewing this.

>diff --git a/scripts/build-promotion/create-update-verify-config.py b/scripts/build-promotion/create-update-verify-config.py
>+    parser.add_argument("--max-full-checks", type=int, default=3)

I would prefer this to be a little bigger, enough that we make sure we test the previous cycle on beta. So the worst case seems to be X.0b9, we'll have b6-b8 as partials, then need to go back through b1-b5, the rc, and one more, so that's 7 ? Assuming we don't accelerate our beta tempo :-S

>         else:
>-            if len(this_full_check_locales) > 0:
>+            if len(this_full_check_locales) > 0 and \
>+                            full_checks <= args.max_full_checks:

The <= gives an off-by-one error, the gist has four quick checks in it.

>                 log.info("Generating full check configs for %s" % fromVersion)
>                 uvc.addRelease(release=appVersion, build_id=build_id,
>                                locales=this_full_check_locales,
>                                from_path=from_path,
>                                ftp_server_from=prev_archive_prefix,
>                                ftp_server_to=args.archive_prefix,
>                                mar_channel_IDs=mar_channel_IDs,
>                                platform=update_platform)
>+                full_checks += 1
>             # Quick test for other locales, no download
>             if len(quick_check_locales) > 0:
>                 log.info("Generating quick check configs for %s" % fromVersion)
>                 uvc.addRelease(release=appVersion, build_id=build_id,
>                                locales=quick_check_locales,
>                                platform=update_platform)

This else block is going to need further reworking to handle max_full_checks. In your gist, once we've gotten past the limit we've gone from a full test of the top locales (en-US, de, ru) to nothing.

Bonus points for teaching download_mars to verify version and buildID in the update.xml, otherwise we're just verifying we're serving an update rather the one we expect.

Comment 14

[Rail Aliiev [:rail]]

Here is another approach. Instead of using some pre-set max number, let's use decreasing density of full checks. In this approach I'm performing full checks only if the current full check index is a triangular number (0, 1, 3, 6, 10, 15, ...). This way we cover the whole population and reduce the sample size.

This is the new diff:
https://gist.github.com/rail/82e8c33a80f3da0625fa49b0a3432ea9

If you find the density not enough, I can look at some other approach.

Patch incoming.

Meantime I can look that download_mars to verify version and buildID.

Comment 15

[Rail Aliiev [:rail]]

Comment on attachment 8728971 [details]
MozReview Request: Bug 1255411 - Optimize update verify process: skip some full checks. r=nthomas

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39215/diff/3-4/

Comment 16

[Rail Aliiev [:rail]]

ooh, the reviewboard view looks a bit weird. Just look at the squashed diff.

Comment 17

[Rail Aliiev [:rail]]

Attachment: MozReview Request: Bug 1255411 - verify buildID, appVersion, displayVersion. r=nthomas

Review commit: https://reviewboard.mozilla.org/r/56630/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56630/

Comment 18

[Rail Aliiev [:rail]]

I ran the modified code locally against old and new update verify configs and it looked fine. I also faked to_build_id and got failures like this:

FAIL: expected buildID 20160526140251 does not match actual 20160526140250

Comment 19

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8728971 [details]
MozReview Request: Bug 1255411 - Optimize update verify process: skip some full checks. r=nthomas

https://reviewboard.mozilla.org/r/39215/#review54616

Nice! A really practical approach to dealing with a growing tail of old releases.

::: scripts/build-promotion/create-update-verify-config.py:127
(Diff revision 4)
>              locale='%locale%', signed=True, exclude_secondary=True
>          ).values()[0]
>          release_dir = makeReleasesDir(product_name, fromVersion, ftp_root='/')
>          from_path = "%s%s" % (release_dir, path_)
>  
>          # Exclude locales being full checked



::: scripts/build-promotion/create-update-verify-config.py:146
(Diff revision 4)
>                             ftp_server_from=prev_archive_prefix,
>                             ftp_server_to=args.archive_prefix,
>                             mar_channel_IDs=mar_channel_IDs,
>                             platform=update_platform)
>          else:
> -            if len(this_full_check_locales) > 0:
> +            if this_full_check_locales and is_triangualar(completes_only_index):

I'd like to see the is_triangular() be used near 127-132, where quick_check_locales and this_full_check_locales are defined for each old version. That way 'en-US de ru' can have the xml verified if they lost the draw on doing the full check. ie the gist for diff in the configs should have this kind of change https://gist.github.com/nthomas-mozilla/de4e5a3b4fbbf0a2e0317a29d51f5296. r+ if you do that.

Comment 20

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8758398 [details]
MozReview Request: Bug 1255411 - verify buildID, appVersion, displayVersion. r=nthomas

https://reviewboard.mozilla.org/r/56630/#review54618

lgtm!

Comment 21

[Rail Aliiev [:rail]]

https://reviewboard.mozilla.org/r/39215/#review54616

> I'd like to see the is_triangular() be used near 127-132, where quick_check_locales and this_full_check_locales are defined for each old version. That way 'en-US de ru' can have the xml verified if they lost the draw on doing the full check. ie the gist for diff in the configs should have this kind of change https://gist.github.com/nthomas-mozilla/de4e5a3b4fbbf0a2e0317a29d51f5296. r+ if you do that.

Oh! Very true! We want to run the quick check for all locales if we skip the full test. This is the new generated diff: https://gist.github.com/rail/76c890896bc0decbd34c39b59c4d7f3c

Comment 22

[Rail Aliiev [:rail]]

Addressed and pushed:

remote:   https://hg.mozilla.org/build/tools/rev/62421c0e40e0
remote:   https://hg.mozilla.org/build/tools/rev/2f8f8c7c1fc0
remote:   https://hg.mozilla.org/build/tools/rev/0f17bad9cd4c