739959 - submit release builds to balrog automatically

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[[e-mail] (:bhearsum)]

Rail, I think you already said you'd be working on this. Reassign if that's wrong.

Comment 1

[[e-mail] (:bhearsum)]

I think this is easier now that the PatcherConfig class exists: https://github.com/mozilla/build-tools/blob/master/lib/python/release/updates/patcher.py#L28

Comment 2

[[e-mail] (:bhearsum)]

Looks like I'll be taking a look at this.

Comment 3

[[e-mail] (:bhearsum)]

So this is going to be a bit more work now that we have multiple partials. The blob schema doesn't support having more than one 'partial' per platform+locale yet. I filed bug 797033 to fix that.

Comment 4

[[e-mail] (:bhearsum)]

So there's a couple of things we need to do here at different points of the release automation. They are:
1) Create the initial release blob at the very start of the release. We should be able to insert the following data into it at this time: name, version, extv, appv, product, schema_version, detailsUrl, fileUrls, ftpFilenames, bouncerProducts, hashFunction as well as setting up each platform with OS_BOUNCER and OS_FTP (or an alias, where we have them). This will likely end up being a new builder.

2) Submit per-build data to Balrog as part of en-US builds and l10n repacks. This will be very similar to what we already do for nightlies. This part depends on bug 797033.

3) After "deliverables ready" fires, another new builder will need to enable the release on at least the betatest channel. We could enable it on releasetest too, but it may make more sense to wait until we know the files are actually available. We might need

Comment 5

[[e-mail] (:bhearsum)]

Starting to work on this at https://github.com/bhearsum/tools/compare/mozilla:master...balrog-release

Comment 6

[[e-mail] (:bhearsum)]

Won't be getting back to this for awhile.

Comment 7

[[e-mail] (:bhearsum)]

Nick and I chatted, here's a refreshed list of things we need to do for this bug:
* The ability to change top-level blob data.
* The ability to query existing rules (so we can find the rule id that we'll want to change).
* The ability to manipulate an existing rule (at the very least, to adjust its Mapping).
* Multiple partial support in AUS backend+blob.
* Script to submit top-level blob data + manipulate test channel rule.
* Script to submit build/repack data.

Not all of this are necessary for initial dogfooding, but they're required to ship.

Comment 8

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #7)
> Nick and I chatted, here's a refreshed list of things we need to do for this
> bug:
> * The ability to change top-level blob data.

This was already done in bug 739960, I don't know I thought this wasn't possibly already...

> * The ability to query existing rules (so we can find the rule id that we'll
> want to change).
> * The ability to manipulate an existing rule (at the very least, to adjust
> its Mapping).

These are bug 721233.

> * Multiple partial support in AUS backend+blob.

bug 797033.

> * Script to submit top-level blob data + manipulate test channel rule.
> * Script to submit build/repack data.

These are the parts we're doing here.

Comment 9

[[e-mail] (:bhearsum)]

Attachment: support release blob submission in balrog submitter

I did a test of this with a manually generated buildprops. It looks like we're going to need to make sure we put mar checksum/size/etc in properties for release builds, which is probably some re-arrangement of that code in MercurialBuildFactory.

This is only one part of what we need to serve release builds in a basic way - we still need to submit a ton of top level data, including fileUrls.

Comment 10

[[e-mail] (:bhearsum)]

Attachment: pass balrog information to release build factory

Comment 11

[[e-mail] (:bhearsum)]

Comment on attachment 8336243 [details] [diff] [review]
support release blob submission in balrog submitter

Rail and I talked about this a bit IRL. He pointed out that this isn't going to work for locales, because they build in chunks. For those, we probably don't want to go through the balrog-submitter.py interface at all - we should use ReleaseSubmitter directly. To make this possible, we should move all the buildbot property parsing stuff out of the api and into the balrog-submitter.py script itself.

Comment 12

[Rail Aliiev [:rail]]

Comment on attachment 8336243 [details] [diff] [review]
support release blob submission in balrog submitter

Conditional r- :)m because tt won't work for locales (l10n repacks). We can refactor the script so it accepts all needed data and (maybe) fallbacks to buildbot properties.

Comment 13

[[e-mail] (:bhearsum)]

Attachment: refactored balrog submitter api

This moves all of the buildprops stuff to the script. I tested the release part, which worked fine, but testing nightly is harder because they delete their build dirs at the end...I'll probably have to run my own build to do that.

Comment 14

[[e-mail] (:bhearsum)]

Comment on attachment 8336368 [details] [diff] [review]
refactored balrog submitter api

I tested this and it submits the same data as before.

Comment 15

[Rail Aliiev [:rail]]

Comment on attachment 8336368 [details] [diff] [review]
refactored balrog submitter api

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

r+ with some nits:

::: lib/python/balrog/submitter/cli.py
@@ +27,5 @@
>      branch = None
>      build_target = None
>      appVersion = None
>      name = None
>      locale = None

Please remove all except build_type - they are unused (well, not set)

@@ +61,2 @@
>              data['partial'] = {
>                  'from': get_nightly_blob_name(self.appName, self.branch,

self.appName -> appName
self.branch -> branch

@@ +96,5 @@
> +
> +        appName = appName
> +        appVersion = appVersion
> +        version = version
> +        build_number = build_number

please remove 4 lines above

@@ +98,5 @@
> +        appVersion = appVersion
> +        version = version
> +        build_number = build_number
> +        name = get_release_blob_name(appName, version, build_number,
> +            self.dummy)

please insert moar spaces to align self.dummy with appName.

::: scripts/updates/balrog-submitter.py
@@ +36,5 @@
> +    fp = open(options.build_properties)
> +    bp = json.load(fp)
> +    fp.close()
> +    props = bp['properties']
> +    locale = props.get('locale', 'en-US')

Initially I though that we should add another command line option to explicitly set locale (for release repacks), but Ben mentioned that we are going to use the library not the script for those.

Comment 16

[[e-mail] (:bhearsum)]

Attachment: fix review comments

Comment 17

[Rail Aliiev [:rail]]

Comment on attachment 8337765 [details] [diff] [review]
fix review comments

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

A nit:

::: lib/python/balrog/submitter/cli.py
@@ +90,5 @@
> +
> +        appName = appName
> +        appVersion = appVersion
> +        version = version
> +        build_number = build_number

Aren't the 4 lines above redundant? Can you remove them in this case?

Comment 18

[[e-mail] (:bhearsum)]

(In reply to Rail Aliiev [:rail] from comment #17)
> Comment on attachment 8337765 [details] [diff] [review]
> fix review comments
> 
> Review of attachment 8337765 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A nit:
> 
> ::: lib/python/balrog/submitter/cli.py
> @@ +90,5 @@
> > +
> > +        appName = appName
> > +        appVersion = appVersion
> > +        version = version
> > +        build_number = build_number
> 
> Aren't the 4 lines above redundant? Can you remove them in this case?

Duh, yes...

Comment 19

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

Comment on attachment 8337765 [details] [diff] [review]
fix review comments

r+, assuming I grokked the bug comments properly and this would run as part of the en-US build, and support for multiple partials and locales is to follow later.

Comment 20

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #19)
> Comment on attachment 8337765 [details] [diff] [review]
> fix review comments
> 
> r+, assuming I grokked the bug comments properly and this would run as part
> of the en-US build, and support for multiple partials and locales is to
> follow later.

Yep, that's right. We won't even get any partials yet, in fact. I'll file follow-ups for all of these things once we've got the basics working.

Comment 21

[John Hopkins (:jhopkins)]

in production

Comment 22

[[e-mail] (:bhearsum)]

This didn't end up running because balrog submission in conditional on createSnippet (wtf?!), which is disabled for releases.

Comment 23

[[e-mail] (:bhearsum)]

Attachment: wip to set properties

One thing we need to do here is set the appropriate properties. After that's done we should be able to make balrog submitter run for release builds.

This patch will probably burn builds because I bet the addFilePropertiesSteps for packages/installers won't work for releases.

Don't have more time to look ta this at the moment.

Comment 24

[[e-mail] (:bhearsum)]

Attachment: set properties, call balrog submitter correctly

OK, I'm pretty sure I've got this worked out. It's an awful hack, or reasons described in a comment, but it seems to work. I also realized that we weren't passing -t release to the balrog submitter - which ended up with weird blob names.

I tested this with just a linux release build in staging. Based on what goes on here, and inspection of the dist/ dirs of windows/mac objdirs, I feel confident that this will work there too. Maybe that's just post-holiday confidence though - let me know if you want some additional testing before landing.

Comment 25

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

Comment on attachment 8355301 [details] [diff] [review]
set properties, call balrog submitter correctly

Most of this looks fine. As an alternative to the hack, would it help to do this sort of thing:

[[e-mail] ~]$ cd /builds/slave/rel-m-rel-osx64_bld-0000000000/build/obj-firefox/i386
[[e-mail] i386]$ MOZ_PKG_PRETTYNAMES=1 make -C browser/installer echo-variable-COMPLETE_MAR
update/mac/en-US/firefox-26.0.complete.mar
(I get a bit of extra output from make, a couple of warnings).

http://mxr.mozilla.org/mozilla-release/source/toolkit/mozapps/installer/packager.mk#773 has the default list of things uploaded.

Comment 26

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #25)
> Comment on attachment 8355301 [details] [diff] [review]
> set properties, call balrog submitter correctly
> 
> Most of this looks fine. As an alternative to the hack, would it help to do
> this sort of thing:
> 
> [[e-mail] ~]$ cd
> /builds/slave/rel-m-rel-osx64_bld-0000000000/build/obj-firefox/i386
> [[e-mail] i386]$
> MOZ_PKG_PRETTYNAMES=1 make -C browser/installer echo-variable-COMPLETE_MAR
> update/mac/en-US/firefox-26.0.complete.mar
> (I get a bit of extra output from make, a couple of warnings).

Unfortunately, this doesn't help :(. The problem is that addFilePropertiesSteps always gets called during packaging of the installer. This will fail for release builds because it sets "directory" to dist/ (https://github.com/mozilla/build-buildbotcustom/blob/master/process/factory.py#L1634), which is wrong for release builds. I tried fiddling with things to include the path in the packageFilename instead, but it looks like the find commands that get run don't like that.

I might be able to clean this up a bit if ReleaseBuildFactory continues to override addFilePropertiesSteps, but adjusts "directory" (but only for installers). I'm going to try to do this...the more I think about it the more uncomfortable I am with the possibility of things breaking if jsshell disappears.

Comment 27

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #26)
> (In reply to Nick Thomas [:nthomas] from comment #25)
> > Comment on attachment 8355301 [details] [diff] [review]
> > set properties, call balrog submitter correctly
> > 
> > Most of this looks fine. As an alternative to the hack, would it help to do
> > this sort of thing:
> > 
> > [[e-mail] ~]$ cd
> > /builds/slave/rel-m-rel-osx64_bld-0000000000/build/obj-firefox/i386
> > [[e-mail] i386]$
> > MOZ_PKG_PRETTYNAMES=1 make -C browser/installer echo-variable-COMPLETE_MAR
> > update/mac/en-US/firefox-26.0.complete.mar
> > (I get a bit of extra output from make, a couple of warnings).
> 
> Unfortunately, this doesn't help :(. The problem is that
> addFilePropertiesSteps always gets called during packaging of the installer.
> This will fail for release builds because it sets "directory" to dist/
> (https://github.com/mozilla/build-buildbotcustom/blob/master/process/factory.
> py#L1634), which is wrong for release builds. I tried fiddling with things
> to include the path in the packageFilename instead, but it looks like the
> find commands that get run don't like that.
> 
> I might be able to clean this up a bit if ReleaseBuildFactory continues to
> override addFilePropertiesSteps, but adjusts "directory" (but only for
> installers). I'm going to try to do this...the more I think about it the
> more uncomfortable I am with the possibility of things breaking if jsshell
> disappears.

Well, we can move past this, but then we'll start failing for Windows when we can't find the installer (https://github.com/mozilla/build-buildbotcustom/blob/master/process/factory.py#L1648), which isn't as easily fixable. I'm going to abandon ship on getting rid of the hack and just land what I have. This is all moving to mozharness soon anyways...

Comment 28

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #27)
> (In reply to Ben Hearsum [:bhearsum] from comment #26)
> > (In reply to Nick Thomas [:nthomas] from comment #25)
> > > Comment on attachment 8355301 [details] [diff] [review]
> > > set properties, call balrog submitter correctly
> > > 
> > > Most of this looks fine. As an alternative to the hack, would it help to do
> > > this sort of thing:
> > > 
> > > [[e-mail] ~]$ cd
> > > /builds/slave/rel-m-rel-osx64_bld-0000000000/build/obj-firefox/i386
> > > [[e-mail] i386]$
> > > MOZ_PKG_PRETTYNAMES=1 make -C browser/installer echo-variable-COMPLETE_MAR
> > > update/mac/en-US/firefox-26.0.complete.mar
> > > (I get a bit of extra output from make, a couple of warnings).
> > 
> > Unfortunately, this doesn't help :(. The problem is that
> > addFilePropertiesSteps always gets called during packaging of the installer.
> > This will fail for release builds because it sets "directory" to dist/
> > (https://github.com/mozilla/build-buildbotcustom/blob/master/process/factory.
> > py#L1634), which is wrong for release builds. I tried fiddling with things
> > to include the path in the packageFilename instead, but it looks like the
> > find commands that get run don't like that.
> > 
> > I might be able to clean this up a bit if ReleaseBuildFactory continues to
> > override addFilePropertiesSteps, but adjusts "directory" (but only for
> > installers). I'm going to try to do this...the more I think about it the
> > more uncomfortable I am with the possibility of things breaking if jsshell
> > disappears.
> 
> Well, we can move past this, but then we'll start failing for Windows when
> we can't find the installer
> (https://github.com/mozilla/build-buildbotcustom/blob/master/process/factory.
> py#L1648), which isn't as easily fixable. I'm going to abandon ship on
> getting rid of the hack and just land what I have. This is all moving to
> mozharness soon anyways...

Whoops, I missed the part where you didn't r+ this yet! What're your thoughts here?

Comment 29

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

Comment on attachment 8355301 [details] [diff] [review]
set properties, call balrog submitter correctly

To capture our IRC discussion - the suggestion to use the echo-variable calls would apply to nightly+release, and to installer/package/complete mar properties (ie more globally than I wrote originally, oops). I think that will avoid our current hackery to get filenames and the work around you needed, but we need to see if it works for Android too. r- on this for now, and we can come back to it if make doesn't work out.

Comment 30

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #29)
> Comment on attachment 8355301 [details] [diff] [review]
> set properties, call balrog submitter correctly
> 
> To capture our IRC discussion - the suggestion to use the echo-variable
> calls would apply to nightly+release, and to installer/package/complete mar
> properties (ie more globally than I wrote originally, oops). I think that
> will avoid our current hackery to get filenames and the work around you
> needed, but we need to see if it works for Android too. r- on this for now,
> and we can come back to it if make doesn't work out.

I tried to do this, and I think I'm past the point of it being worthwhile. The main problem is that we can't echo-variable-PACKAGE unless we're in a Makefile that includes packager.mk. The only ones that do are app-specific ones in $app/installer, l10n.mk (which can't be run on its own), and tools/update-packaging/Makefile.in (which isn't enabled for Android builds). In trying to use the app-specific ones I discovered that appName isn't even based to MercurialBuildFactory, and is even set incorrectly for Android builds in config.py. I think we only use it for Desktop l10n repacks these days.

I noticed that we grab "appName" from application.ini and tried to use that, but it turns out that this is actually the product name (ie, Firefox or Fennec), not the app name -- so it's useless in helping us figure out how to find the $app/installer directory we need to run make in.

We might be able to work around these things by getting appName set correctly in config.py (which means using "mobile/android" for those builds) and passing it into MercurialBuildFactory again -- but I'm not sure it's worthwhile, or that I won't hit some other roadblock down the rabbithole.

I think I just realized that this can be fixed by overriding getPackageFilename and returning False though...which is a much less hacky solution than the jsshell one...

Comment 31

[[e-mail] (:bhearsum)]

Attachment: don't set properties for packages/installers for releases

Alright, I think I did this without major hacks. The only difference between this and the previous patch is that we set packageFilename and installerFilename to None for releases, which skips the addFileProperties logic. Feeling a little silly for not seeing this option before!

I did a bunch of tests, results are here:
http://dev-master01.build.scl1.mozilla.com:8018/builders/OS%20X%2010.7%20mozilla-central%20nightly/builds/0
http://dev-master01.build.scl1.mozilla.com:8018/builders/release-mozilla-beta-linux64_build/builds/20
http://dev-master01.build.scl1.mozilla.com:8018/builders/Linux%20x86-64%20mozilla-central%20nightly/builds/1
http://dev-master01.build.scl1.mozilla.com:8018/builders/release-mozilla-beta-win32_build/builds/1
http://dev-master01.build.scl1.mozilla.com:8018/builders/Android%204.2%20x86%20mozilla-central%20nightly/builds/0
http://dev-master01.build.scl1.mozilla.com:8018/builders/release-mozilla-beta-win32_build/builds/2

(The last one is still running, and then there should be a Windows Nightly afterwards.)

The release blob that the builds submitted to is here;
https://aus4-admin-dev.allizom.org/releases/Firefox-27.0b2-build1/data

Everything looks fine, except that Nightly builds get rejected because they try to add links to dev-stage01, which isn't in the whitelist. I filed bug 957712 to get that changed on the dev instance.

Feel free to defer this until after all the test builds are done, I just wanted to get it posted because I'm out tomorrow.

Comment 32

[[e-mail] (:bhearsum)]

Attachment: actually build release installers ...

My previous broke on Windows because I put the new if condition in the wrong place...oops.

Comment 33

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

Comment on attachment 8358548 [details] [diff] [review]
actually build release installers ...

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

::: process/factory.py
@@ +1651,5 @@
>                  workdir='build/%s' % self.objdir,
>                  haltOnFailure=True
>              ))
> +            if installerFilename:
> +                self.addFilePropertiesSteps(filename='*.installer.exe',

Should pass filename=installerFilename here instead of hardcoding over your new function.

@@ +2607,4 @@
>          MercurialBuildFactory.__init__(self, env=env, **kwargs)
>  
> +    def getPackageFilename(self, platform, platform_variation):
> +        return False

Please add a comment that covers why this and getPackageFilename() return False.

Comment 34

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #33)
> Comment on attachment 8358548 [details] [diff] [review]
> actually build release installers ...
> 
> Review of attachment 8358548 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: process/factory.py
> @@ +1651,5 @@
> >                  workdir='build/%s' % self.objdir,
> >                  haltOnFailure=True
> >              ))
> > +            if installerFilename:
> > +                self.addFilePropertiesSteps(filename='*.installer.exe',
> 
> Should pass filename=installerFilename here instead of hardcoding over your
> new function.
> 
> @@ +2607,4 @@
> >          MercurialBuildFactory.__init__(self, env=env, **kwargs)
> >  
> > +    def getPackageFilename(self, platform, platform_variation):
> > +        return False
> 
> Please add a comment that covers why this and getPackageFilename() return
> False.

Will do on both accounts.

Comment 35

[[e-mail] (:bhearsum)]

Comment on attachment 8358548 [details] [diff] [review]
actually build release installers ...

Pushed in https://hg.mozilla.org/build/buildbotcustom/rev/9b78d5e97bbb. Hope to merge it before the release builds tomorrow.

Comment 36

[Rail Aliiev [:rail]]

In production

Comment 37

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

Attachment: [buildbotcustom] Skip setting properties on android relauto

The 27.0b5 android builds are failing with this:
bash -c 'find build/obj-firefox/dist -maxdepth 1 -type f -name False'
 in dir /builds/slave/rel-m-beta-and_bld-00000000000/. (timeout 1200 secs)

This is a new step compared to the last beta. This patch should fix it by skipping when packageName isn't defined.

We should also figure out a way to disable Balrog submissions for Android builds, or turn on mar files if we really do want them. But that can wait for later.

Comment 38

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

Comment on attachment 8359552 [details] [diff] [review]
[buildbotcustom] Skip setting properties on android relauto

default:  http://hg.mozilla.org/build/buildbotcustom/rev/59c19c961310
prod-0.8: http://hg.mozilla.org/build/buildbotcustom/rev/570d6d57ca83

Moved FENNEC_27_0b6_RELEASE, FENNEC_27_0b6_BUILD1 tags onto 570d6d57ca83. Reconfig'd build masters.

Comment 39

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #37)
> We should also figure out a way to disable Balrog submissions for Android
> builds, or turn on mar files if we really do want them. But that can wait
> for later.

We could use enableUpdatePackaging for this, or add a new one. I'm not too fussy. Do you have a preference?

Comment 40

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

I don't mind enableUpdatePackaging, but I assume it's on for nightly/aurora so that probably means some special case in release.py rather than changing the platform configs.

Comment 41

[[e-mail] (:bhearsum)]

Attachment: set balrog api root to none for fennec releases

Comment 42

[[e-mail] (:bhearsum)]

Attachment: allow for balrog_api_root to be overridden by release config

Based on my dump_master.py diff, this seems to work (6 hunks [3 platforms * 2 branches] that remove balrog both before and after running multil10n.py).

Comment 43

[[e-mail] (:bhearsum)]

Attachment: root blob stuff for releases

This might need a bit of polish, but it gets all the data there. Here's an example submission:
https://aus4-admin-dev.allizom.org/releases/Firefox-27.0b4-build1/data

I tried to submit a release build afterwards too, but I hit bug 961830.

Just f? for now because I'm pretty open to refactoring. It seems like some things could be moved elsewhere - fileUrls, ftpFilenames, and bouncerProducts in particular. I'm having trouble figuring out the right balance, so opinions are more than welcome!

Also, we currently put extv/appv in the locale specific sections in addition to putting it in the top level. Should I do something to make sure we don't get it in locale sections for releases?

Comment 44

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

Comment on attachment 8362662 [details] [diff] [review]
root blob stuff for releases

A lot or all of this patch is in the tree already, wrong patch file ?

Comment 45

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

(In reply to Ben Hearsum [:bhearsum] from comment #43)
> Also, we currently put extv/appv in the locale specific sections in addition
> to putting it in the top level. Should I do something to make sure we don't
> get it in locale sections for releases?

In an ideal world we'd only have it in the top-level of the blob, for brevity. Depends how the API code is though, might be something we can do better with after writing v2 of that.

Comment 46

[[e-mail] (:bhearsum)]

Attachment: root blob stuff for releases

Indeed, wrong patch...

Comment 47

[Rail Aliiev [:rail]]

Comment on attachment 8362938 [details] [diff] [review]
root blob stuff for releases

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

LGTM in overal. Some nits below.

::: lib/python/balrog/submitter/cli.py
@@ +71,5 @@
> +            data['platforms'][updatePlatforms[0]] = {
> +                'OS_BOUNCER': bouncerPlatform,
> +                'OS_FTP': ftpPlatform
> +            }
> +            for aliasedPlatform in updatePlatforms[1:]:

[1:] may fail for Linux, afaik. Probably you should have a check for the list length to guard this loop

::: scripts/updates/balrog-release-submitter.py
@@ +63,5 @@
> +
> +    for opt in ('build_properties', 'release_config', 'api_root', 'credentials_file', 'buildbot_configs'):
> +        if not getattr(options, opt):
> +            print >>sys.stderr, "Required option %s not present" % opt
> +            sys.exit(1)

I think we can switch to argparse here to get rid of boilerplate code.

Comment 48

[John Hopkins (:jhopkins)]

in production

Comment 49

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

Comment on attachment 8362938 [details] [diff] [review]
root blob stuff for releases

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

Looks good.

::: lib/python/balrog/submitter/cli.py
@@ +62,5 @@
> +        data['ftpFilenames']['complete'] = '%s-%s.complete.mar' % (product, version)
> +        data['ftpFilenames']['partial'] = '%s-%s-%s.partial.mar' % (product, previousVersion, version)
> +        data['bouncerProducts']['complete'] = '%s-%s-complete' % (product, version)
> +        data['bouncerProducts']['partial'] = '%s-%s-partial-%s' % (product, version, previousVersion)
> +

Multiple partial support to follow in bug 797033 or friend ?

::: lib/python/release/info.py
@@ +214,5 @@
> +
> +def getProductDetails(product, appVersion):
> +    url = 'https://www.mozilla.org'
> +    if product == 'thunderbird':
> +        url = 'https://www.mozillamessaging.com'

Might be worth asking Standard8 about this, I suspect it's legacy and we can just use mozilla.org with different product.

::: scripts/updates/balrog-release-submitter.py
@@ +9,5 @@
> +import site
> +import logging
> +import sys
> +
> +site.addsitedir(os.path.join(os.path.dirname(__file__), "../../lib/python"))

Nit, you've got mixed usage of path and os.path.

Comment 50

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #49)
> Comment on attachment 8362938 [details] [diff] [review]
> root blob stuff for releases
> 
> Review of attachment 8362938 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> ::: lib/python/balrog/submitter/cli.py
> @@ +62,5 @@
> > +        data['ftpFilenames']['complete'] = '%s-%s.complete.mar' % (product, version)
> > +        data['ftpFilenames']['partial'] = '%s-%s-%s.partial.mar' % (product, previousVersion, version)
> > +        data['bouncerProducts']['complete'] = '%s-%s-complete' % (product, version)
> > +        data['bouncerProducts']['partial'] = '%s-%s-partial-%s' % (product, version, previousVersion)
> > +
> 
> Multiple partial support to follow in bug 797033 or friend ?

Yep, that's the plan. I suspect we might need to support both on the server for a period of time (until we get clients/blobs upgraded), so hopefully we won't have to try to simultaneously land server+client changes.

Comment 51

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #49)
> Comment on attachment 8362938 [details] [diff] [review]
> root blob stuff for releases
> 
> Review of attachment 8362938 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> ::: lib/python/balrog/submitter/cli.py
> @@ +62,5 @@
> > +        data['ftpFilenames']['complete'] = '%s-%s.complete.mar' % (product, version)
> > +        data['ftpFilenames']['partial'] = '%s-%s-%s.partial.mar' % (product, previousVersion, version)
> > +        data['bouncerProducts']['complete'] = '%s-%s-complete' % (product, version)
> > +        data['bouncerProducts']['partial'] = '%s-%s-partial-%s' % (product, version, previousVersion)
> > +
> 
> Multiple partial support to follow in bug 797033 or friend ?
> 
> ::: lib/python/release/info.py
> @@ +214,5 @@
> > +
> > +def getProductDetails(product, appVersion):
> > +    url = 'https://www.mozilla.org'
> > +    if product == 'thunderbird':
> > +        url = 'https://www.mozillamessaging.com'
> 
> Might be worth asking Standard8 about this, I suspect it's legacy and we can
> just use mozilla.org with different product.

Mark, what say you? What's the canonical place for Thunderbird's product details these days? We're currently using things like http://live.mozillamessaging.com/thunderbird/releasenotes?locale=%locale%&platform=%platform%&version=%version%

Comment 52

[Mark Banner (:standard8)]

(In reply to Ben Hearsum [:bhearsum] from comment #51)
> (In reply to Nick Thomas [:nthomas] from comment #49)
> > > +def getProductDetails(product, appVersion):
> > > +    url = 'https://www.mozilla.org'
> > > +    if product == 'thunderbird':
> > > +        url = 'https://www.mozillamessaging.com'
> > 
> > Might be worth asking Standard8 about this, I suspect it's legacy and we can
> > just use mozilla.org with different product.
> 
> Mark, what say you? What's the canonical place for Thunderbird's product
> details these days? We're currently using things like
> http://live.mozillamessaging.com/thunderbird/
> releasenotes?locale=%locale%&platform=%platform%&version=%version%

Having a base of https://www.mozilla.org/ should be fine for this specific url (there's some live.momo.* urls that shouldn't be changed) - obviously the code then adds the locale and product, so we'll still get to the right place.

Comment 53

[[e-mail] (:bhearsum)]

(In reply to Mark Banner (:standard8) (away until 3 Mar) from comment #52)
> (In reply to Ben Hearsum [:bhearsum] from comment #51)
> > (In reply to Nick Thomas [:nthomas] from comment #49)
> > > > +def getProductDetails(product, appVersion):
> > > > +    url = 'https://www.mozilla.org'
> > > > +    if product == 'thunderbird':
> > > > +        url = 'https://www.mozillamessaging.com'
> > > 
> > > Might be worth asking Standard8 about this, I suspect it's legacy and we can
> > > just use mozilla.org with different product.
> > 
> > Mark, what say you? What's the canonical place for Thunderbird's product
> > details these days? We're currently using things like
> > http://live.mozillamessaging.com/thunderbird/
> > releasenotes?locale=%locale%&platform=%platform%&version=%version%
> 
> Having a base of https://www.mozilla.org/ should be fine for this specific
> url (there's some live.momo.* urls that shouldn't be changed) - obviously
> the code then adds the locale and product, so we'll still get to the right
> place.

OK, cool. I see that https://www.mozilla.org/en-US/thunderbird/24.3.0/releasenotes/ works (and I also discovered that I forgot to add the /releasenotes/ part, whoops).

Comment 54

[[e-mail] (:bhearsum)]

Attachment: balrog release pusher, fully implemented + review comments addressed

This patch should address the review comments, and implements the rule update part. I did a very basic test against aus4-admin-dev, but I'd like to do more testing after bug 976285 lands. Summary of the changes:
* Allow for updating of rules through Rule.update_rule().
* s/appName/productName/ in a bunch of places, because it was confusing me to have things like 'firefox' in variables called 'appName'.
* Fix details urls.
* Rename script to balrog-release-pusher.py, and run ReleaseCreator and ReleasePusher as part of it.

Incoming patch to have this run as part of ReleaseUpdatesFactory.

Also need a patch to set testChannels and testChannelRuleIds everywhere. I have to wait until we have rules first, though, so I've held off on that for now.

Comment 55

[[e-mail] (:bhearsum)]

Attachment: run release pusher as part of updates

Not much to say about this one. Flunk on failure is false for now because I'm certain will hit some issue or another after landing. Probably should stay this way until we switch over.

Comment 56

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

Comment on attachment 8382510 [details] [diff] [review]
balrog release pusher, fully implemented + review comments addressed

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

::: lib/python/balrog/submitter/cli.py
@@ +33,5 @@
> +
> +    def generate_data(self, appVersion, productName, version, buildNumber,
> +                      partialUpdates, updateChannels, stagingServer,
> +                      bouncerServer, enUSPlatforms):
> +        previousVersion = str(max(StrictVersion(v) for v in partialUpdates))

Would be worth a comment here that *multiple* partial update support is TODO.

Comment 57

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

Comment on attachment 8382511 [details] [diff] [review]
run release pusher as part of updates

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

::: process/release.py
@@ +1213,5 @@
>              promptWaitTime=releaseConfig.get(
>                  'promptWaitTime', None),
> +            balrog_api_root=balrog_api_root,
> +            balrog_credentials_file=branchConfig['balrog_credentials_file'],
> +            balrog_username=branchConfig['balrog_username'],

Any particular reason to use branchConfig only for credentials file and username, but allow the release config to override the api root ? All or nothing is fine AFAICT.

Comment 58

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #57)
> Comment on attachment 8382511 [details] [diff] [review]
> run release pusher as part of updates
> 
> Review of attachment 8382511 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: process/release.py
> @@ +1213,5 @@
> >              promptWaitTime=releaseConfig.get(
> >                  'promptWaitTime', None),
> > +            balrog_api_root=balrog_api_root,
> > +            balrog_credentials_file=branchConfig['balrog_credentials_file'],
> > +            balrog_username=branchConfig['balrog_username'],
> 
> Any particular reason to use branchConfig only for credentials file and
> username, but allow the release config to override the api root ? All or
> nothing is fine AFAICT.

Just because there's no use case for overriding the others at this time. I can add it, though.

Comment 59

[[e-mail] (:bhearsum)]

Did some testing with these patches today and they big update we do to add fileUrls, OS_FTP, etc. ends up clobbering the data that gets submitted by the en-US builder, oops.

Couple of possible workarounds:
1) Do all of the non-build specific blob updates before en-US builds. We skirt the issue this way, because the en-US builds and locales can already do incremental updates.
2) Try to intelligently merge the data on the server. I'm not sure if we can do this in a way that still makes it possible to remove parts of a blob. Eg, if you recursively call .update() on each dict, you can't do removals.

Comment 60

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #59)
> Did some testing with these patches today and they big update we do to add
> fileUrls, OS_FTP, etc. ends up clobbering the data that gets submitted by
> the en-US builder, oops.
> 
> Couple of possible workarounds:
> 1) Do all of the non-build specific blob updates before en-US builds. We
> skirt the issue this way, because the en-US builds and locales can already
> do incremental updates.
> 2) Try to intelligently merge the data on the server. I'm not sure if we can
> do this in a way that still makes it possible to remove parts of a blob. Eg,
> if you recursively call .update() on each dict, you can't do removals.

Option 3): retrieve current blob, update it locally, then post it back.

Comment 61

[[e-mail] (:bhearsum)]

Attachment: submit blob data + do betatest rule update

This fixes the aforementioned issue with the client doing the data merging. Here's a full summary of changes from the last patch
* Support data_version being passed into API classes. We need this to avoid the race condition where someone else updates the blob between us downloading the current release blob and POST'ing the new one back. Without this, we'd clobber their update silently. With it, we'll fail, which is arguably what we want.
* Send Accept-Encoding: application/json with requests to make sure we get back a parseable format. (There's a Balrog patch incoming that adds support for this to the /releases/:name endpoint.)
* Send hashType, because /releases/:name requires it. This is pretty silly, but necessary because of the kludge that is the Releases' endpoints right now.

I did some semi-manual runs of this against a Balrog instance on dev-master1. What I did was:
* Submit 28.0b8 linux64 build data (using the properties from the production 28.0b8 build directory)
* Run the updates builder on my dev buildbot master. Failed, because it was pointing at aus4-admin-dev. I tweaked the command and ran it by hand on the slave.
* Configured a 28.0b7 to point at my instance, updated to 28.0b8 successfully.
* Repeat the above for 28.0b9

You can browse around http://dev-master1.srv.releng.scl3.mozilla.com:9000/releases.html and http://dev-master1.srv.releng.scl3.mozilla.com:9000/rules.html if you want to poke at the data.

It's worth noting that we won't have any partial updates after this patch, because we don't set most of the partialMar* build properties as part of release builds. That seems fine to me, since we're going to look at multiple partial support pretty soon anyways.

Comment 62

[[e-mail] (:bhearsum)]

Attachment: support returning releases in json

This patch also includes a fix to make sure AUS.specialForceHosts is always defined, which I discovered can be problematic when running balrog-server.py.

Comment 63

[[e-mail] (:bhearsum)]

Attachment: allow releases to be returned as json

This patch also includes a fix to make sure AUS.specialForceHosts is always defined, which I discovered can be problematic when running balrog-server.py.

Comment 64

[[e-mail] (:bhearsum)]

Attachment: add necessary parts to release configs

I went through aus4-admin-dev and aus4-admin.m.o and added most of the rules we'll need:
* OS blocking rules for betatest, releasetest, beta.
* betatest & releasetest rules for Firefox/Thunderbird
* beta channel rule (dev only)

I'm holding off creating a beta channel rule in production until we're actually using it, because I'm feeling paranoid.

In doing this I also realized that we're eventually going to hit issues with OS blocking on betatest/releasetest, because those channels are shared by beta, release, and esr. Perhaps when we redo channel names we should ensure uniqueness there?

Comment 65

[Massimo Gervasini [:massimo]]

Comment on attachment 8388689 [details] [diff] [review]
allow releases to be returned as json

Hi Ben,

I am not sure about this line:

>+        if request.headers.get('Accept-Encoding') == 'application/json':

I did a tests: 

curl http://localhost:9000  -uuser:password --header "Accept-encoding:application/json, text/plain" ('Accept-Encoding' can be a list)

returns a html snippet not a json response because request.headers.get('Accept-Encoding') is "application/json, text/plain"

I suppose that we want always send a json response when 'application/json' is set, in this case I'd write something as:
 
          accept_encoding = request.headers.get('Accept-Encoding')
          if accept_encoding and 'application/json' in accept_encoding:

Comment 66

[[e-mail] (:bhearsum)]

(In reply to Massimo Gervasini [:mgerva] from comment #65)
> Comment on attachment 8388689 [details] [diff] [review]
> allow releases to be returned as json
> 
> Hi Ben,
> 
> I am not sure about this line:
> 
> >+        if request.headers.get('Accept-Encoding') == 'application/json':
> 
> I did a tests: 
> 
> curl http://localhost:9000  -uuser:password --header
> "Accept-encoding:application/json, text/plain" ('Accept-Encoding' can be a
> list)
> 
> returns a html snippet not a json response because
> request.headers.get('Accept-Encoding') is "application/json, text/plain"
> 
> I suppose that we want always send a json response when 'application/json'
> is set, in this case I'd write something as:
>  
>           accept_encoding = request.headers.get('Accept-Encoding')
>           if accept_encoding and 'application/json' in accept_encoding:

Hrm, I didn't realize that Accept-encoding could be plural. Thanks for catching that, I'll fix it up.

Comment 67

[[e-mail] (:bhearsum)]

Attachment: handle plural accept-encoding

Comment 68

[[github robot]]

Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/9ff17bdec0bbc504c02dd38d20a074b6e4c85c9f
bug 739959: submit release builds to balrog automatically - pay attention to accept-encoding for /releases/:name endpoint. r=mgerva

Comment 69

[[e-mail] (:bhearsum)]

Comment on attachment 8382511 [details] [diff] [review]
run release pusher as part of updates

I'm testing a new patch for this that makes all the balrog variables overridable.

Comment 70

[[e-mail] (:bhearsum)]

Attachment: allow more balrog overrides

With this patch, plus the other two up for review, I redid the tests from comment #61 against aus4-admin-dev. They worked fine, except that I had to poke the server because the domain whitelist for aus4-dev.allizom.org is wrong (bug 983385). You can have a look at the results on:
https://aus4-admin-dev.allizom.org/releases/Firefox-28.0b8-build1/data
https://aus4-admin-dev.allizom.org/releases/Firefox-28.0b9-build1/data
http://dev-master1.srv.releng.scl3.mozilla.com:8018/builders/release-mozilla-beta-updates/builds/27
http://dev-master1.srv.releng.scl3.mozilla.com:8018/builders/release-mozilla-beta-updates/builds/28

And if you go to the trouble of adjusting /data/www/aus4-dev.allizom.org/app/balrog.ini (and reloading httpd), can you verify that updates are served at:
https://aus4-dev.allizom.org/update/3/Firefox/28.0/20140224220227/Linux_x86_64-gcc3/en-US/betatest/Linux%203.11.0-17-generic%20%28GTK%202.24.20%29/default/default/update.xml?force=1
https://aus4-dev.allizom.org/update/3/Firefox/28.0/20140303165517/Linux_x86_64-gcc3/en-US/betatest/Linux%203.11.0-17-generic%20%28GTK%202.24.20%29/default/default/update.xml?force=1

...and similar URLs.

So, with the three remaining patches from this bug we should be able to start dogfooding betas on aus4.m.o!

Comment 71

[[e-mail] (:bhearsum)]

Unblocking on multiple partials, as we decided to do it afterwards...

Comment 72

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

Apologies for not getting to these reviews. I'm away a couple of days then it's priority #1.

Comment 73

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

Comment on attachment 8388670 [details] [diff] [review]
submit blob data + do betatest rule update

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

Looks good. Are you thinking of using SingleLocale() to add the release locale data ?

::: lib/python/balrog/submitter/cli.py
@@ +59,5 @@
> +
> +        data['ftpFilenames']['complete'] = '%s-%s.complete.mar' % (productName, version)
> +        data['ftpFilenames']['partial'] = '%s-%s-%s.partial.mar' % (productName, previousVersion, version)
> +        data['bouncerProducts']['complete'] = '%s-%s-complete' % (productName, version)
> +        data['bouncerProducts']['partial'] = '%s-%s-partial-%s' % (productName, version, previousVersion)

Nit, I'd prefer '%s-%s-Complete' and '%s-%s-Partial-%s' unless this was a deliberate change.

::: scripts/updates/balrog-release-pusher.py
@@ +63,5 @@
> +        logging_level = logging.DEBUG
> +    logging.basicConfig(stream=sys.stdout, level=logging_level,
> +                        format="%(message)s")
> +
> +    for opt in ('build_properties', 'release_config', 'api_root', 'credentials_file', 'buildbot_configs'):

username could be in this list too.

Comment 74

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

Comment on attachment 8390824 [details] [diff] [review]
allow more balrog overrides

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

::: process/release.py
@@ +1238,5 @@
>                  'slavebuilddir': normalizeName(builderPrefix('updates'), releaseConfig['productName']),
>                  'platform': platform,
>                  'branch': 'release-%s' % sourceRepoInfo['name'],
>                  'release_config': releaseConfigFile,
> +                'script_repo_revision': releaseTag,

At first this confused me because it looks like it's a ScriptFactory, but you're just using the same convention.

Comment 75

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

(In reply to Nick Thomas [:nthomas] from comment #73)
> Nit, I'd prefer '%s-%s-Complete' and '%s-%s-Partial-%s' unless this was a
> deliberate change.

Never mind this, bouncer has them set with the caps I like, but responds without caring about the case.

Comment 76

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

Comment on attachment 8388740 [details] [diff] [review]
add necessary parts to release configs

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

I had to resort to the likes of https://aus4-admin.mozilla.org/rules/26 to verify the IDs :-(

(In reply to Ben Hearsum [:bhearsum] from comment #64)
> In doing this I also realized that we're eventually going to hit issues with
> OS blocking on betatest/releasetest, because those channels are shared by
> beta, release, and esr. Perhaps when we redo channel names we should ensure
> uniqueness there?

We'll have to I think. Release builds on beta channel is another case where betatest/releasetest are overloaded. {beta,release,esr}-{local,cdn}test or something.

Comment 77

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #73)
> Comment on attachment 8388670 [details] [diff] [review]
> submit blob data + do betatest rule update
> 
> Review of attachment 8388670 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Are you thinking of using SingleLocale() to add the release
> locale data ?

Yeah. create-release-repacks.py has access to that, so I suspect we can just use it directly rather than calling out to balrog-submitter.py. I just filed bug 986487 on that -- this bug is already too big.

> > +
> > +        data['ftpFilenames']['complete'] = '%s-%s.complete.mar' % (productName, version)
> > +        data['ftpFilenames']['partial'] = '%s-%s-%s.partial.mar' % (productName, previousVersion, version)
> > +        data['bouncerProducts']['complete'] = '%s-%s-complete' % (productName, version)
> > +        data['bouncerProducts']['partial'] = '%s-%s-partial-%s' % (productName, version, previousVersion)
> 
> Nit, I'd prefer '%s-%s-Complete' and '%s-%s-Partial-%s' unless this was a
> deliberate change.

I know you took this back, but I'll make the change anyways. Consistency!

> ::: scripts/updates/balrog-release-pusher.py
> @@ +63,5 @@
> > +        logging_level = logging.DEBUG
> > +    logging.basicConfig(stream=sys.stdout, level=logging_level,
> > +                        format="%(message)s")
> > +
> > +    for opt in ('build_properties', 'release_config', 'api_root', 'credentials_file', 'buildbot_configs'):
> 
> username could be in this list too.

Will do.

Comment 78

[Armen [:armenzg]]

Live in production.

Comment 79

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #64)
> Created attachment 8388740 [details] [diff] [review]
> add necessary parts to release configs
> 
> I went through aus4-admin-dev and aus4-admin.m.o and added most of the rules
> we'll need:
> * OS blocking rules for betatest, releasetest, beta.
> * betatest & releasetest rules for Firefox/Thunderbird
> * beta channel rule (dev only)
> 
> I'm holding off creating a beta channel rule in production until we're
> actually using it, because I'm feeling paranoid.

I created the beta channel rule in production today in prep for 29.0b2 next week.

Comment 80

[[e-mail] (:bhearsum)]

This worked fine for 29.0b2. The blob is here: https://aus4-admin.mozilla.org/releases/Firefox-29.0b2-build1/data

And the test channels are already showing updates:
https://aus4.mozilla.org/update/3/Firefox/29.0/20140318013849/Linux_x86_64-gcc3/en-US/betatest/Linux%203.11.0-17-generic%20%28GTK%202.24.20%29/default/default/update.xml?force=1
https://aus4.mozilla.org/update/3/Firefox/29.0/20140318013849/Linux_x86_64-gcc3/en-US/releasetest/Linux%203.11.0-17-generic%20%28GTK%202.24.20%29/default/default/update.xml?force=1

Partials will be handled in bug 797033. Repacks will be handled in bug 986487.

Pushing a release will be done manually through the web interface the time being.

I think we're all done here, finally?