1142872 - Extend mozharness l10n support for build promotions

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Massimo Gervasini [:massimo]]

We need to extend the mozharness l10n to support build promotions.

At the moment, l10n repacks with mozharness use "make wget-en-US" to download the en-US binary from: [1]

The script should be able to do repacks from a specific en-US binary too e.g: [2]

[1] http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/
[2] http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/firefox-39.0a1.en-US.linux-x86_64.complete.mar

Comment 1

[Massimo Gervasini [:massimo]]

Attachment: [mozharness] Bug 1142872 - add --en-us-binary-url option in desktop_l10n.py.patch

This patch adds the --en-us-binary-url option from the command line. 
If the value of this option is a know installer (tar.bz2, dmg, zip), the binary is downloaded using _retry_download_file(). In any other cases, we let make wget-en-us figure out what to do.

Comment 2

[Jordan Lund (:jlund)]

Comment on attachment 8578126 [details] [diff] [review]
[mozharness] Bug 1142872 - add --en-us-binary-url option in desktop_l10n.py.patch

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

ship it

Comment 3

[Massimo Gervasini [:massimo]]

Comment on attachment 8578126 [details] [diff] [review]
[mozharness] Bug 1142872 - add --en-us-binary-url option in desktop_l10n.py.patch

Thanks Jordan!

https://hg.mozilla.org/build/mozharness/rev/116e748b985b

Comment 4

[Jordan Lund (:jlund)]

mozharness production tag moved to: https://hg.mozilla.org/build/mozharness/rev/production

Comment 5

[Massimo Gervasini [:massimo]]

Attachment: [mozharness] Bug 1142872 - make upload requires ZIP_IN env variable when passing the en-us-binary-url parameter.patch

When we specify an installer url, we need to set the ZIP_IN env variable so make unpack can find the file we have just downloaded.

Comment 6

[Massimo Gervasini [:massimo]]

Comment on attachment 8582340 [details] [diff] [review]
[mozharness] Bug 1142872 - make upload requires ZIP_IN env variable when passing the en-us-binary-url parameter.patch

On hold for now. Partial updates are not created using the provided --en-us-binary-url value.

Comment 7

[Massimo Gervasini [:massimo]]

Attachment: [mozharness] Bug 1142872 - make upload requires ZIP_IN env variable when passing the en-us-binary-url parameter.patch

Changes in this version:
* renamed optional parameter to en-us-installer-url (was en-us-binary-url)
* now repacks work properly when the --en-us-installer-url option is passed to the script
* fixed installer check 

This version requires two mach configure && make wget-en-us calls, there's room for optimizations (such as caching the local binary and avoid to download it twice)

test results with --en-us-installer-url [1] and without [2]

[1] http://dev-master2.bb.releng.use1.mozilla.com:8914/builders/WINNT%205.2%20ash%20nightly%20l10n%201%2F10/builds/119/steps/run_script/logs/stdio
[2] http://dev-master2.bb.releng.use1.mozilla.com:8914/builders/WINNT%205.2%20ash%20nightly%20l10n%201%2F10/builds/118/steps/run_script/logs/stdio

Comment 8

[Massimo Gervasini [:massimo]]

Comment on attachment 8583292 [details] [diff] [review]
[mozharness] Bug 1142872 - make upload requires ZIP_IN env variable when passing the en-us-binary-url parameter.patch

Thanks Chris!

https://hg.mozilla.org/build/mozharness/rev/abc8a082305a

Comment 9

[Rail Aliiev [:rail]]

Attachment: l10n_kill_dead_code-mozharness-1.diff

Before we proceed with release promotion and funsize, it'd be great to kill dead code here.

I partially tested this "on my laptop". It generates repacks and completes properly, tries to ssh to stage to get previous MARs, but fails, of course.

I would like to "test it in production" using the following scenario:
* land the code after all nightly repacks are done.
* retrigger a chunk on win32 to see how it works
* leave or backout depending on the result

The patch:
* removes unused enable_partials (this is controlled by mozconfigs now)
* removes unused previous_mar_url
* removes unused "--keystore" (copy/paste from android repacks)
* removes a bunch of unused methods (MAR generation is controlled by the in-tree code now)
* renames misleading summarize() to _map()
* removes some unused steps that are in-tree now (mkdir MAR dirs, rm MAR dirs, rm pgc files, etc)

What can possibly go wrong?! :)

Comment 10

[Rail Aliiev [:rail]]

FTR, the script looks almost ready for build promotion:

`--locale xy --locale xz ...` can be used to specify exact list of locales

We may still need to change how the `make upload` step works.

Comment 11

[Michael Shal [:mshal]]

Comment on attachment 8633478 [details] [diff] [review]
l10n_kill_dead_code-mozharness-1.diff

LGTM, and seems to work in staging as far as I can tell.

Comment 12

[Rail Aliiev [:rail]]

Comment on attachment 8633478 [details] [diff] [review]
l10n_kill_dead_code-mozharness-1.diff

remote:   https://hg.mozilla.org/build/mozharness/rev/da004b78c19a
remote:   https://hg.mozilla.org/build/mozharness/rev/94d72bb2ba9b

Comment 13

[Rail Aliiev [:rail]]

It worked without any issues on linux and window. I'll check all platforms tomorrow.

Comment 14

[Rail Aliiev [:rail]]

(In reply to Rail Aliiev [:rail] from comment #13)
> It worked without any issues on linux and window. I'll check all platforms
> tomorrow.

So far everything looks good \\o//

Comment 15

[[e-mail] (:bhearsum)]

Attachment: move back to l10n chunks; set locales property

This patch should get us most of the way to using the new l10n script. It switches back to chunkification, and sets the "locales" property with locale+changeset combinations.

It's also got some commented out code for setting en_us_binary_url, but I'm not quite sure what to do there yet. The best thing I can come up with is what's in the comment -- calculate the artifact URL in release runner, and pass it in. This would require release runner to know the task ids of all of the en-US builds, and build the artifact names based on the usual formula of product, version, platform. Release runner _should_ be able to get the taskids by looking up tasks in the index, eg: https://tools.taskcluster.net/index/#buildbot.revisions.00008b664c22.mozilla-inbound/buildbot.revisions.00008b664c22.mozilla-inbound.win32. However, because these tasks are currently created by Mozharness, they won't exist until the en-US builds complete. When scheduling is moved into tree this might be easier.

There's also the complication of what to do for multiple runs for the same task, and multiple tasks for the same revision+branch+platform. For the former, I think it makes sense just to use the latest run. For the latter, I don't know what's best. It's probably a pretty rare case, so maybe taking the newest task id makes the most sense?

Comment 16

[Rail Aliiev [:rail]]

Comment on attachment 8655494 [details] [review]
move back to l10n chunks; set locales property

I thought that I already reviewed it. :)

Comment 17

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

Comment on attachment 8655494 [details] [review]
move back to l10n chunks; set locales property

Sorry for the delay in reviewing, this looks fine to me. I'm not sure what to say about en_us_binary_url. Did you consider doing it at runtime *inside* the l10n job, grabbing the most recent en-US run/artifact ? Alternatively the l10n part of graph could be extended once en-US build finishes, although I'm not sure if that works very well if we rebuild en-US.

There's also the filename part to consider. I don't recall if we decided if we were going to run beetmover (and hence create files in the candidates bucket with pretty names) before or after l10n kicked off. Does it help if we say l10n second ? Perhaps then we just have to teach release-runner how files get pretty named, and that'd be stable over rebuilds. Bonus points if release-runner and beetmover import the same code.

Comment 18

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #17)
> Comment on attachment 8655494 [details] [review]
> move back to l10n chunks; set locales property
> 
> Sorry for the delay in reviewing, this looks fine to me. I'm not sure what
> to say about en_us_binary_url. Did you consider doing it at runtime *inside*
> the l10n job, grabbing the most recent en-US run/artifact ? Alternatively
> the l10n part of graph could be extended once en-US build finishes, although
> I'm not sure if that works very well if we rebuild en-US.

I hadn't really considered doing it in the l10n job. I guess the advantage to that is that you don't need to know en-US task ids when you're creating the graph (because l10n jobs will look them up in the index). It seems nicer to do the look-up ahead of time, but looking it at runtime is pretty much the same as what we do now...

I'm not sure about extending the graph once the en-US build finishes. It's certainly possible from a Taskcluster standpoint, but we'd have to rework releasetasks to be able to generate partial forms of the graph, and release runner would have to have special run modes and triggers to make sure it only generates/submits the appropriate parts. Unless I misunderstood you're suggestion?

> There's also the filename part to consider. I don't recall if we decided if
> we were going to run beetmover (and hence create files in the candidates
> bucket with pretty names) before or after l10n kicked off.

IIRC we've been talking about doing it in parallel, per https://bug1178315.bmoattachments.org/attachment.cgi?id=8628561.

> Does it help if
> we say l10n second ? Perhaps then we just have to teach release-runner how
> files get pretty named, and that'd be stable over rebuilds. Bonus points if
> release-runner and beetmover import the same code.

I think you're saying that if we do it after en-US beetmoving that we won't need the tasks ids, because we'll be able to pull from the candidates directory instead? If so, that would help some, since we wouldn't need to know task ids anymore. One downside to this is that you add the en-US antivirus+beetmoving to the critical path...

Comment 19

[[e-mail] (:bhearsum)]

Comment on attachment 8655494 [details] [review]
move back to l10n chunks; set locales property

I merged this pull request, and will follow-up with something for the en-US binary URL part.

Comment 20

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum (:bhearsum) from comment #18)
> (In reply to Nick Thomas [:nthomas] from comment #17)
> I'm not sure about extending the graph once the en-US build finishes. It's
> certainly possible from a Taskcluster standpoint, but we'd have to rework
> releasetasks to be able to generate partial forms of the graph, and release
> runner would have to have special run modes and triggers to make sure it
> only generates/submits the appropriate parts. Unless I misunderstood you're
> suggestion?

I may have misunderstood this part. I took it to mean, "submit some form of the graph, potentially ahead of the en-US builds finishing, and only submit the l10n part after they finish". However, I realized now that it may not make sense to submit anything ahead of the en-US builds finishing, since nothing can start. So perhaps you simply meant "don't allow release promotion to be started until en-US builds are finished"?

Comment 21

[[e-mail] (:bhearsum)]

I fiddled with the Taskcluster client a bit today to try and figure out how to get the artifacts we need. It turns out that the Index has a findArtifactFromTask method that will do the taskid detection for you, eg:
resp = index.findArtifactFromTask("buildbot.revisions.00008b664c22.mozilla-inbound.win32", "public/build/firefox-42.0a1.en-US.win32.installer.exe")

So regardless of where we do it, it should be pretty simple to do. That method seems to have a bug in it right now though, as it never returns. I filed bug 1203176 on this.

Comment 22

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

(In reply to Ben Hearsum (:bhearsum) from comment #20)
> I may have misunderstood this part. I took it to mean, "submit some form of
> the graph, potentially ahead of the en-US builds finishing, and only submit
> the l10n part after they finish". However, I realized now that it may not
> make sense to submit anything ahead of the en-US builds finishing, since
> nothing can start. So perhaps you simply meant "don't allow release
> promotion to be started until en-US builds are finished"?

You grokked it right the first time but raise a good point. Until we schedule the en-US builds in TC we probably can't create the rest of the graph.

Comment 23

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

... until all the en-US builds are done.

Comment 24

[[e-mail] (:bhearsum)]

Attachment: update release runner to set+verify en_us_binary_url

This patch should get everything that we need to generate release graphs w/ l10n enabled set. It looks up taskIds in the Index, and then constructs URLs based on them. I feel like there should be a better way to construct these URLs, but I think the expected use case is to use the taskcluster library to grab them by taskId or something...that doesn't really work for us here because we're passing around URLs, not taskIds.

It also does a couple of other things:
* Refactor a bit to fully segregate graph kwarg generation code and kwarg validation code. This should make it a bit easier to add more checks as we go along. I think this could still be improved more, but it's a start.
* Enabled updates if partials are set (should've been done awhile ago)

Comment 25

[[e-mail] (:bhearsum)]

Attachment: set en_us_binary_url in l10n graph

Comment 26

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

Comment on attachment 8663103 [details] [diff] [review]
update release runner to set+verify en_us_binary_url

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

::: buildfarm/release/release-runner.py
@@ +152,5 @@
> +
> +
> +# TODO: Replace this with simple names
> +def get_l10n_platforms(platforms, release, branch, index):
> +    l10n_platforms = {}

I wondered about the naming of this now that it's a dict of en-US binary urls. Otherwise looks good.

Comment 27

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

Comment on attachment 8663103 [details] [diff] [review]
update release runner to set+verify en_us_binary_url

>diff --git a/buildfarm/release/release-runner.py b/buildfarm/release/release-runner.py
...
>+def get_l10n_platforms(platforms, release, branch, index):
....
>+        l10n_platforms[platform] = "https://queue.taskcluster.net/v1/task/{}/artifacts/{}".format(task["taskId"], filename)

Oh, the graph generator is expecting l10n_platforms[platform]['en_us_binary_url'] ?

Comment 28

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #26)
> Comment on attachment 8663103 [details] [diff] [review]
> update release runner to set+verify en_us_binary_url
> 
> Review of attachment 8663103 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: buildfarm/release/release-runner.py
> @@ +152,5 @@
> > +
> > +
> > +# TODO: Replace this with simple names
> > +def get_l10n_platforms(platforms, release, branch, index):
> > +    l10n_platforms = {}
> 
> I wondered about the naming of this now that it's a dict of en-US binary
> urls. Otherwise looks good.

Yeah, I wasn't entirely sure what to do with this. The name "l10n_platforms" still matches the keys (which may differ from the list of enUS_platforms), but it surely doesn't represent the full l10n data structure anymore. Perhaps it's better to smush this together with l10n_changesets and call it l10n_platform_info or something like that?

(In reply to Nick Thomas [:nthomas] from comment #27)
> Comment on attachment 8663103 [details] [diff] [review]
> update release runner to set+verify en_us_binary_url
> 
> >diff --git a/buildfarm/release/release-runner.py b/buildfarm/release/release-runner.py
> ...
> >+def get_l10n_platforms(platforms, release, branch, index):
> ....
> >+        l10n_platforms[platform] = "https://queue.taskcluster.net/v1/task/{}/artifacts/{}".format(task["taskId"], filename)
> 
> Oh, the graph generator is expecting
> l10n_platforms[platform]['en_us_binary_url'] ?

I think so...it iterates over l10n_platforms with "for platform, platform_info in l10n_platforms.iteritems()", and then plucks out "platform_info['en_us_binary_url']".

Comment 29

[Rail Aliiev [:rail]]

Comment on attachment 8663103 [details] [diff] [review]
update release runner to set+verify en_us_binary_url

We can merge the l10n_platforms data structure with l10n changesets, probably as a followup.

Comment 30

[Rail Aliiev [:rail]]

Comment on attachment 8663104 [details] [review]
set en_us_binary_url in l10n graph

lgtm

Comment 31

[[e-mail] (:bhearsum)]

(In reply to Rail Aliiev [:rail] from comment #29)
> Comment on attachment 8663103 [details] [diff] [review]
> update release runner to set+verify en_us_binary_url
> 
> We can merge the l10n_platforms data structure with l10n changesets,
> probably as a followup.

I'd actually rather do it now, to be honest :). It doesn't seem like it should be that hard...just needs a decision.

Comment 32

[Rail Aliiev [:rail]]

Let's do it then! :)

Comment 33

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

Comment on attachment 8663104 [details] [review]
set en_us_binary_url in l10n graph

Combining them could make sense, but you are better placed to know. I'll r+ this so you have the option either way.

Comment 34

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #27)
> Comment on attachment 8663103 [details] [diff] [review]
> update release runner to set+verify en_us_binary_url
> 
> >diff --git a/buildfarm/release/release-runner.py b/buildfarm/release/release-runner.py
> ...
> >+def get_l10n_platforms(platforms, release, branch, index):
> ....
> >+        l10n_platforms[platform] = "https://queue.taskcluster.net/v1/task/{}/artifacts/{}".format(task["taskId"], filename)
> 
> Oh, the graph generator is expecting
> l10n_platforms[platform]['en_us_binary_url'] ?

Oh, I see what you were getting at now...they don't agree on the data structure!

Comment 35

[[e-mail] (:bhearsum)]

Attachment: switch to unified l10n config in release runner

This patch switches to a unified l10n config, including putting chunks in a platform specific spot. I updated releasetasks to match, so I wouldn't mind another look at https://github.com/mozilla/releasetasks/pull/38 too.

Comment 36

[Rail Aliiev [:rail]]

Comment on attachment 8665068 [details] [diff] [review]
switch to unified l10n config in release runner

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

I don't want to block you on the comments below. Feel free either address them on landing.

::: buildfarm/release/release-runner.py
@@ +137,5 @@
> +        task = index.findTask("buildbot.revisions.{}.{}.{}".format(release["mozillaRevision"], branch, platform))
> +
> +        # TODO: Replace this with simple names
> +        if platform.startswith("win"):
> +            binary_fmt = "public/build/{}-{}.en-US.{}.installer.exe"

maybe instead of using positional args, we should use something like {product}, {platform}, etc? At least it would be easier to read it here, since the interpolation happens way below.

@@ +149,5 @@
> +        l10n_platforms[platform]["en_us_binary_url"] = url
> +
> +        # TODO: deal with platform-specific locales
> +        for l in l10n_changesets:
> +            l10n_platforms[platform]["locales"].append(l)

The following would look more readable imo:

l10n_platforms[platform]["locales"] = l10n_changesets.keys()

This probably deserves a separate function, something like platform_locales(l10n_changesets, platform), which can be unit tested.

Comment 37

[[e-mail] (:bhearsum)]

Attachment: improve string formatting and l10n platform generation

I think this addresses your comments? I'm not sure how to unit test things in here though...

Comment 38

[Rail Aliiev [:rail]]

Comment on attachment 8665428 [details] [diff] [review]
improve string formatting and l10n platform generation

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

LGTM. We can add tests whenever we implement get_platform_locales() for reals.

Comment 39

[[e-mail] (:bhearsum)]

Rail and I attempted a release today. We had to work through numerous issues, some in the releasetasks code and some in release runner, but eventually got a graph generated that included l10n: https://tools.taskcluster.net/task-graph-inspector/#W0N7eMMcSWqouTA_W-T4Nw/PT_fAT9HRaWOfak9iWGyTQ/1

l10n still doesn't work, beacuse we have no buildbot builders - I filed bug 1208180 to get those added. I'll also be adding follow-up patches here with the local changes we needed to make to get the graph generated.

Comment 40

[[e-mail] (:bhearsum)]

Attachment: spattering of fixes found in dev

This fixes a bunch of things:
* taskId vs. taskid typo when accessing the returned data from index.findTask()
* Set l10n_chunks for each platform
* Pass version/buildNumber to the graph generator
* Fix kwarg passing to validate_graph_kwargs.

Comment 41

[[e-mail] (:bhearsum)]

Attachment: fix graph validation issues found in dev

Comment 42

[[e-mail] (:bhearsum)]

Attachment: require chunkify for release runner

This is an indirect dependency, because releasetasks requires it.

Comment 43

[[e-mail] (:bhearsum)]

Attachment: iterate over correct platforms

I realized a silly mistake - release runner is iterating over en-US platforms rather than l10n platforms, oops.

Comment 44

[[e-mail] (:bhearsum)]

While working on bug 1208180 I tried to run this script by hand in release mode...I think it still needs a couple of enhancements, in particular it's not picking up the locales or en_us_binary_url property. Eg, I ran:
python desktop_l10n.py --branch-config single_locale/date.py --platform-config single_locale/linux64.py --environment-config single_locale/production.py --balrog-config balrog/staging.py --no-clobber

And this happened:
13:57:51     INFO - Reading from file /home/bhearsum/tmp/rel/gecko/testing/mozharness/scripts/build/date/browser/locales/all-locales


I don't see "read-buildbot-config" anywhere in the actions...I think the stuff that deals with properties in this script is only for spitting them out at the end.

Comment 45

[[e-mail] (:bhearsum)]

Attachment: add more releaserunner deps

Discovered more necessary dependencies!

Comment 46

[[e-mail] (:bhearsum)]

Comment on attachment 8668096 [details] [diff] [review]
add more releaserunner deps

isn't ready, need to instal libffi-devel too

Comment 47

[[e-mail] (:bhearsum)]

Attachment: add new deps + libffi dep

Comment 48

[Rail Aliiev [:rail]]

Comment on attachment 8668413 [details] [diff] [review]
add new deps + libffi dep

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

::: modules/releaserunner/manifests/init.pp
@@ +42,2 @@
>                  "decorator==3.4.0",
> +                "enum34==1.0.4.tar.gz",

Conditional r+, s/.tar.gz//

Otherwise lgtm.

Comment 49

[[e-mail] (:bhearsum)]

Attachment: use app version when building en-US version urls

I noticed this was wrong when I looked over the task definition for the graph we kicked yesterday. Since we're using CI builds, the inputs for the l10n jobs are named by appVersion, not version.

Comment 50

[[e-mail] (:bhearsum)]

Attachment: use getAppVersion

release runner needs tests :(. My previous patch was causing this traceback:
Traceback (most recent call last):
  File "release-runner.py", line 276, in main
    "l10n_config": get_l10n_config(release, branchConfig, branch, l10n_changesets, index),
  File "release-runner.py", line 153, in get_l10n_config
    version=release["appVersion"],
KeyError: 'appVersion'

Comment 51

[Rail Aliiev [:rail]]

Calling this done regardless of bug 1184091 (extra RPMs) which is not a hard blocker