748698 - Support new attributes for update.xml in Balrog

Status: RESOLVED FIXED

(Release Engineering Graveyard :: Applications: Balrog (backend), defect, P2)

Description

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

Bug 459972 added support for several attributes in update.xml which have been supported since Firefox 4/Gecko 2. eg appVersion and displayVersion replacing extensionVersion and version, actions, notifications etc. Note that AUS2 doesn't support all the functionality of the client application.

I'm intending this bug be for figuring out the blob and createXML() changes, and probably a separate one for the API submissions for releases (still in progress). We need support for this for betas/release for Fx4 or later so we can suppress the What's New page after the update is applied.

Comment 1

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

This is in progress at 
 https://github.com/nthomas-mozilla/balrog/compare/master...newer-attributes

Comment 2

[[e-mail] (:bhearsum)]

Nick, I did a review for your current patch over in https://github.com/mozilla/balrog/pull/14. In retrospect, I probably should've attached it here instead....

Unless you like the idea and are willing to take it on as part of this, you can ignore/defer the stuff about moving schema stuff into blob.py. I think the only thing I'd really like to see addressed is the killing of fakePartials if possible...

Comment 3

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

I've been spelunking the bugs and code to figure out if we can deprecate fakePartials.

We stopped generating fake partials in bug 514040, which leads to bug 538533 where an initial application fix landed, and bug 576939 where a roll up was backported to 1.9.2. So I think
* Firefox 3.6.9 and later, and everything 4.0a1 onwards correctly handle an error if only a complete is offered and that fails
* Firefox 3.5.x and older didn't get those fixes
* We kept generating fake partial snippets for 3.5 and 3.6 (aka 1.9.1 and 1.9.2) branches until they were EOL
* We currently offer 
 * 3.6.13 through 28 --> 12.0, using the older attributes and a fake partial
 * older 3.6.x get offered 3.6.28, using the older attributes and a fake partial
 * 3.5.x get 3.6.18, using the older attributes and a fake partial

So how about I drop fake partials from ReleaseBlobV2 but leave it there for V1 ?

Comment 4

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #3)
> I've been spelunking the bugs and code to figure out if we can deprecate
> fakePartials.
> 
> We stopped generating fake partials in bug 514040, which leads to bug 538533
> where an initial application fix landed, and bug 576939 where a roll up was
> backported to 1.9.2. So I think
> * Firefox 3.6.9 and later, and everything 4.0a1 onwards correctly handle an
> error if only a complete is offered and that fails
> * Firefox 3.5.x and older didn't get those fixes
> * We kept generating fake partial snippets for 3.5 and 3.6 (aka 1.9.1 and
> 1.9.2) branches until they were EOL
> * We currently offer 
>  * 3.6.13 through 28 --> 12.0, using the older attributes and a fake partial
>  * older 3.6.x get offered 3.6.28, using the older attributes and a fake
> partial
>  * 3.5.x get 3.6.18, using the older attributes and a fake partial
> 
> So how about I drop fake partials from ReleaseBlobV2 but leave it there for
> V1 ?

Sounds good to me. As an aside, we'll have to figure out what we want to do in terms of rules for Really Old releases. We'll have the ability to do 3.6* -> 12.0 if we want, or even 3.[56]*.

Comment 5

[[e-mail] (:bhearsum)]

mass component change

Comment 6

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

I've got patches mostly done for this, progress at
 https://github.com/nthomas-mozilla/balrog/compare/master...newer-attributes
 https://github.com/nthomas-mozilla/build-tools/compare/master...newer-attributes

Aiming to have them finished tomorrow.

Comment 7

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

Attachment: [tools] Submit schema version 2

Add support for sending the new version parameters in the version 2 schema, and include schema_version in the request too. At some point the property names could be rationalized on the buildbot jobs so that they're not the version 1 names.

If you have an existing blob and send a different schema version you get a 
  Caught HTTPError: Couldn't update release: New release blob is invalid.
This could be changed to be more informative. To deploy this change we'll need to first modify the rules for nightly/aurora to use the latest dated blob, and get IT to modify or remove the -latest blobs.

Comment 8

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

Attachment: [balrog] Create schema version 2

This is fairly similar to what you gave feedback on a while back, but a few changes. Notes on things in a rough order of the patch:
* cull out of date README
* fix up mounting on newer vagrant versions (part of bug 919270)
* add the now mandatory schema_version to existing snippet tests
* add snippet test for newly supported attributes
* jump to blob.py
 * schema_version is required in the first level of blob if in format, add support for ignoring this when recursing
 * createBlob() handles creation of blobs and parsing into json now, it requires schema_version to be set in the input. Returning ValueError() when schema_version isn't set introduces confusion with JSON decoding issue, ideas welcome.
* auslib/AUS.py
 * I've deferred refactoring (eg moving snippet and xml creation onto the blobs and removing the intermediate updateData object), but I'm more convinced than ever it needs doing
 * git's diff in expandRelease() makes it out to be worse than it is. There's quite a bit of indentation change so try a 'diff -w'
 * also adds support for licenceUrl in both schemas
* forms.py - JSONBlobFileField._set_default appears to be unused (or we have a test gap)
* releases.py: createRelease() creaks a bit here in the new try/except - it's trying to handle to many cases and we should unwind it as we discussed a while ago
* a lot of setting schema version to 1 in existing tests
* test-rules.py support and improvements

Comment 9

[[e-mail] (:bhearsum)]

Comment on attachment 8346476 [details] [diff] [review]
[balrog] Create schema version 2

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

Looks good to me!

(In reply to Nick Thomas [:nthomas] from comment #8)
>  * createBlob() handles creation of blobs and parsing into json now, it
> requires schema_version to be set in the input. Returning ValueError() when
> schema_version isn't set introduces confusion with JSON decoding issue,
> ideas welcome.

I think it's OK to leave as is. In both cases we're talking about bad input from the user, which we can't do anything about in code -- so distinguishing between the two cases doesn't seem terribly important. One way to distinguish would be to introduce a BlobValidationError (or something similar) which subclasses ValueError. Then you could do a catchall with ValueError or catch the more specific validation error.

> * auslib/AUS.py
>  * I've deferred refactoring (eg moving snippet and xml creation onto the
> blobs and removing the intermediate updateData object), but I'm more
> convinced than ever it needs doing

+100000. That's on me at some point, I think.

::: auslib/blob.py
@@ +22,5 @@
>      if not hasattr(blob, 'keys') or not callable(blob.keys):
>          return False
> +    # If the blob format has a schema_version then that's a mandatory int
> +    if topLevel and 'schema_version' in format_:
> +        if 'schema_version' not in blob or not isinstance(blob['schema_version'], int):

What do you think of maintaining a list of valid schema versions and checking that too? I suppose it makes no practical difference when createBlob() raises a ValueError already....

Comment 10

[[e-mail] (:bhearsum)]

Comment on attachment 8346475 [details] [diff] [review]
[tools] Submit schema version 2

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

::: lib/python/balrog/submitter/cli.py
@@ +50,5 @@
> +            data['extv'] = extVersion
> +        elif schemaVersion == 2:
> +            data['appVersion'] = appVersion
> +            data['platformVersion'] = extVersion
> +            data['displayVersion'] = appVersion

Seeing this makes me wonder if we should be passing these more explicitly...I guess app and platform version aren't going to be different for most things though.

I wonder how well these will work for b2g? Not a blocker, just a thought. We can solve it later.

Comment 11

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

I've merged recent changes, https://github.com/nthomas-mozilla/balrog/compare/master...newer-attributes is up to date. There are some minor tweaks due to other bugs which will need review. 

Also did some testing of build submission. If we lock the rules to the most recent dated blobs, then remove the latest blobs, and use the tools patch it submit a new dated blob it works fine. 

I did discover a logging issue, this shows up in Apache's error log
[Fri Mar 07 05:20:35 2014] [error] Traceback (most recent call last):
[Fri Mar 07 05:20:35 2014] [error]   File "/usr/lib64/python2.6/logging/__init__.py", line 776, in emit
[Fri Mar 07 05:20:35 2014] [error]     msg = self.format(record)
[Fri Mar 07 05:20:35 2014] [error]   File "/usr/lib64/python2.6/logging/__init__.py", line 654, in format
[Fri Mar 07 05:20:35 2014] [error]     return fmt.format(record)
[Fri Mar 07 05:20:35 2014] [error]   File "/usr/lib64/python2.6/logging/__init__.py", line 439, in format
[Fri Mar 07 05:20:35 2014] [error]     s = self._fmt % record.__dict__
[Fri Mar 07 05:20:35 2014] [error] KeyError: 'requestid'

This happens several times even on successful POSTs. You get more if you don't delete the latest blob and blob.isValid() fails during the POST (which also points to a missing check on schema_version changing).

Comment 12

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #11)
> I've merged recent changes,
> https://github.com/nthomas-mozilla/balrog/compare/master...newer-attributes
> is up to date. There are some minor tweaks due to other bugs which will need
> review. 
> 
> Also did some testing of build submission. If we lock the rules to the most
> recent dated blobs, then remove the latest blobs, and use the tools patch it
> submit a new dated blob it works fine. 

This should be fine, though we'll need to ask IT to do the deletion directly for us - we don't have a way to do it through the admin interface or API yet.

> I did discover a logging issue, this shows up in Apache's error log
> [Fri Mar 07 05:20:35 2014] [error] Traceback (most recent call last):
> [Fri Mar 07 05:20:35 2014] [error]   File
> "/usr/lib64/python2.6/logging/__init__.py", line 776, in emit
> [Fri Mar 07 05:20:35 2014] [error]     msg = self.format(record)
> [Fri Mar 07 05:20:35 2014] [error]   File
> "/usr/lib64/python2.6/logging/__init__.py", line 654, in format
> [Fri Mar 07 05:20:35 2014] [error]     return fmt.format(record)
> [Fri Mar 07 05:20:35 2014] [error]   File
> "/usr/lib64/python2.6/logging/__init__.py", line 439, in format
> [Fri Mar 07 05:20:35 2014] [error]     s = self._fmt % record.__dict__
> [Fri Mar 07 05:20:35 2014] [error] KeyError: 'requestid'
>
> 
> This happens several times even on successful POSTs. You get more if you
> don't delete the latest blob and blob.isValid() fails during the POST (which
> also points to a missing check on schema_version changing).

I'm reading https://github.com/mozilla/balrog/blob/master/auslib/log.py#L15 and I can't think of a case where makeRecord will return without requestid being set in 'extra'....

Comment 13

[[e-mail] (:bhearsum)]

I looked over the patch and it still seems fine overall, just one thing of note:

getApplicationVersion seems weird....it returns extv for v1 blobs, but appv for v2 blobs? I remember there was churn on the different versions in update XML but I can't find the details anywhere...

Comment 14

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

In further testing, I found the change to CURRENT_SCHEMA_VERSION wasn't being picked up (in Vagrant, using the modified Balrog code but current tip of tools for submitting code). And also that the logging differs between python2.7 with admin.py on my mac vs python26 and admin.wsgi in Vagrant. eg add a log.debug() in auslib/admin/views/releases.py and it doesn't show up in the wsgi case. The import order is slightly different, but a quick play didn't resolve it.

(In reply to Ben Hearsum [:bhearsum] from comment #13)
> getApplicationVersion seems weird....it returns extv for v1 blobs, but appv
> for v2 blobs? I remember there was churn on the different versions in update
> XML but I can't find the details anywhere...

IIRC this is one of those historical things. For v1, extv was always the actual version, while appv might be some human friendly version for display in update dialog boxes. In v2, we have appVersion and displayVersion for that, so appVersion is the best choice to compare against the version coming from the query.

Comment 15

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

Attachment: [balrog] Create schema version 2, v2

Updated patch for Balrog backend. Interdiff highlights:
* vagrant fix already landed
* blob.getApplicationVersion() we discussed above, are my explanation and comments OK ?
* reverts CURRENT_SCHEMA_VERSION to 1, so that we can land the tools patch separately from balrog deployment. Once all is well we can bump it up again
* Some new tests in auslib/test/admin/views/test_releases.py. I didn't go so far as duplicating all the tests for both schemas, but we could. Some of these depend on CURRENT_SCHEMA_VERSION.

Comment 16

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

Attachment: [tools] Submit schema version 2, v2

Updated tools patch, highlights:
* send schema_version for all types of release submission (individual builds, ReleaseCreator). Tested with some mockups
* sneaky unrelated change to product names
* recover from bitrot (s/appName/productName mainly)

Comment 17

[[e-mail] (:bhearsum)]

Comment on attachment 8404612 [details] [diff] [review]
[balrog] Create schema version 2, v2

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

> * blob.getApplicationVersion() we discussed above, are my explanation and comments OK ?

Yup, seems fine. I have a tangentially related comment on the tools patch though.

Comment 18

[[e-mail] (:bhearsum)]

Comment on attachment 8404614 [details] [diff] [review]
[tools] Submit schema version 2, v2

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

::: lib/python/balrog/submitter/cli.py
@@ +52,5 @@
> +            data['extv'] = appVersion
> +        elif schemaVersion == 2:
> +            data['appVersion'] = appVersion
> +            data['platformVersion'] = appVersion
> +            data['displayVersion'] = appVersion

Hm. I know you're not adding it, but I'm not sure why version handling differs here vs. ReleaseSubmitter anymore :(. All these versions are utterly confusing...

@@ +192,5 @@
> +            data['extv'] = extVersion
> +        elif schemaVersion == 2:
> +            data['appVersion'] = appVersion
> +            data['platformVersion'] = extVersion
> +            data['displayVersion'] = appVersion

It looks like we might need to change this for Betas, as I'm seeing things like "27.0 Beta 4" for displayVersion in those snippets. That's clearly not a blocking issue though, that can be fixed elsewhere. Probably can deal with my previous comment there, too.

::: scripts/updates/balrog-release-pusher.py
@@ +54,5 @@
>      parser.add_option("-r", "--release-config", dest="release_config")
>      parser.add_option("-a", "--api-root", dest="api_root")
>      parser.add_option("-c", "--credentials-file", dest="credentials_file")
> +    parser.add_option("-s", "--schema", dest="schema_version",
> +                      help="blob schema version", type="int", default=2)

I was a bit surprised to see this default to 2 at first, but I think I understand now... The plan is:
* Land Balrog patch
* Lock any rules that point at a -latest blob to a v1 version.
* (Delete -latest blobs?)
* Land the tools patch, blobs will repopulate as v2
* Lock rules back to -latest blobs
* Bump default schema version on the Balrog backend

If I'm still confused about this we should probably jump on vidyo briefly

Comment 19

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

(In reply to Ben Hearsum [:bhearsum] from comment #18)
> ::: lib/python/balrog/submitter/cli.py
> @@ +52,5 @@
> > +            data['extv'] = appVersion
> > +        elif schemaVersion == 2:
> > +            data['appVersion'] = appVersion
> > +            data['platformVersion'] = appVersion
> > +            data['displayVersion'] = appVersion
> 
> Hm. I know you're not adding it, but I'm not sure why version handling
> differs here vs. ReleaseSubmitter anymore :(. All these versions are utterly
> confusing...

I agree it needs cleaning up at some point. In practice appVersion and platformVersion are the same; probably have been since the train model. But for completeness, we have:

Nightly/Aurora en-US & l10n:  call tools/scripts/updates/balrog-submitter.py -t nightly, aka NightlySubmitter. No extVersion property set in buildbot so we fall back to passing appVersion, and we use that for platformVersion.

Beta en-US: same as nightly, except -t release and uses ReleaseSubmitter, which also uses extVersion for platformVersion.

Beta l10n: tools/scripts/l10n/create-release-repacks.py uses ReleaseSubmitter, and sets extVersion to appVersion in the call to run().

Beta updates: tools/scripts/updates/balrog-release-pusher.py calls ReleaseCreator, which doesn't have an extVersion argument, so the hardcoding happens inside.

So they all set platformVersion using appVersion, one way or another. Whee!

> ::: scripts/updates/balrog-release-pusher.py
> > +    parser.add_option("-s", "--schema", dest="schema_version",
> > +                      help="blob schema version", type="int", default=2)
> 
> I was a bit surprised to see this default to 2 at first, but I think I
> understand now... The plan is:
> * Land Balrog patch
> * Lock any rules that point at a -latest blob to a v1 version.
> * (Delete -latest blobs?)
> * Land the tools patch, blobs will repopulate as v2
> * Lock rules back to -latest blobs
> * Bump default schema version on the Balrog backend

Yes, that's it.

Comment 20

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

Attachment: [tools] Submit schema version 2, v3

This fixes display version for betas, and leaves the appVersion/extVersion/platformVersion mess for another time.

Comment 21

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

FWIW, I didn't use getPrettyVersion in the nightly case because it doesn't make sense (to me) to draw attention to the use of a1 and a2 for Nightly and Aurora (eg "Firefox 36.0 Alpha 1").

Comment 22

[[github robot]]

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

https://github.com/mozilla/balrog/commit/f7c6a8daa648866d90fa299d500446ec35916fe4
Bug 748698, Support new attributes for update.xml in Balrog, r=bhearsum

Comment 23

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

Verified we're still serving schema 1 fine on aus4-dev.allizom.org. Will ask WebOps to deploy on Thursday.

Comment 24

[[e-mail] (:bhearsum)]

This is partially rolled out now. Tl;dr state is "we're waiting for nightlies to finish, then we'll repoint rules at the new -latest blobs".

Here's the full plan that we discussed in e-mail, and where we're at:
* Lock all the rules to dated blobs (done)
* Ask webops to remove %-nightly-latest and push the new code (done)
* Land new balrog client code (done)
* Trigger new nightlies - (done: https://tbpl.mozilla.org/?rev=b85b57f05fda)
* Move test channels back to -latest to do some verification
* Move live channels back to -latest

We ended up pushing the code live before all of the Firefox Aurora 32-bit Linux repacks finished -- they were taking ages, and I didn't want to delay this while wait for the last 10 or 20 of them. This means that most locaels won't be getting updates until tomorrow.

On the plus side, I had cturra remove the latest dated blob so the last few locales could successfully submit to Balrog. This got us instant verification of the client side patch, which you can see here:
https://aus4-admin.mozilla.org/releases/Firefox-mozilla-aurora-nightly-20140529004003/data
https://aus4.mozilla.org/update/3/Firefox/29.0a2/20140312004001/Linux_x86-gcc3/sr/aurora/default/default/default/update.xml

I resubmitted all of the en-US builds except 32-bit Linux, whose spot instance had already been terminated to minimize the damage of removing the blob.

Comment 25

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #24)
> * Trigger new nightlies - (done: https://tbpl.mozilla.org/?rev=b85b57f05fda)

The Mac build popped out. I moved the nightlytest channel back to -latest, and was able to update successfully. Same for Fennec Nightly.

All of the Fennec nightlies are done now, so I moved the "nightly" channel back to -latest. Firefox Desktop should probably wait until all of the en-US builds are done.

Comment 26

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

All desktop mozilla-central builds are out, confirmed mac/en-US and win/de working on nightlytest.
  --> back to Firefox-mozilla-central-nightly-latest on nightly

Test flame after creating a rule for nightlytest, that worked too.
  --> back to B2G-mozilla-central-nightly-latest on nightly

Comment 27

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #26)
> All desktop mozilla-central builds are out, confirmed mac/en-US and win/de
> working on nightlytest.
>   --> back to Firefox-mozilla-central-nightly-latest on nightly
> 
> Test flame after creating a rule for nightlytest, that worked too.
>   --> back to B2G-mozilla-central-nightly-latest on nightly

I just repointed the remaining rules back at their -latest blobs. The ones that didn't have a latest blob (eg, Cedar) I left pointing at their most recent dated blob. Should we point them back at mozilla-central's latest instead?

Also, QA filed bug 1018193. We should probably make some noise about it the next time we need to do similar to avoid confusion. All in all though, this landing went smoooooooooooth.

Comment 28

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

(In reply to Ben Hearsum [:bhearsum] from comment #27)
> I just repointed the remaining rules back at their -latest blobs.

Needed to do Thunderbird-comm-aurora-nightly-latest and Thunderbird-comm-central-nightly-latest, and Firefox-mozilla-esr24-nightly-latest, but otherwise looks good.

> The ones
> that didn't have a latest blob (eg, Cedar) I left pointing at their most
> recent dated blob. Should we point them back at mozilla-central's latest
> instead?

Maybe, if the channel-id on the files is ok for cedar builds. Either way we'd have to revisit the rule if those branches start doing nightlies again.
 
> Also, QA filed bug 1018193. We should probably make some noise about it the
> next time we need to do similar to avoid confusion. 

Fair call, had intended to blob but got missed.

Comment 29

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

I checked 30.0b9, which was submitted after we deployed this. It's pretty close to the same as on AUS2 but one issue to fix

detailsURL - needs fixing
old - https://www.mozilla.com/en-US/firefox/30.0/releasenotes/, which redirects to 
      https://www.mozilla.org/en-US/firefox/30.0beta/releasenotes/
new - https://www.mozilla.org/en-US/Firefox/30.0/releasenotes/, which is a 404. s/Firefox/firefox/ and it works. --> Bug 1019292 filed.

complete URL - capitalisation is ignored by bouncer so this is OK
old - http://download.mozilla.org/?product=firefox-30.0b9-complete&os=osx&lang=en-US
new - http://download.mozilla.org/?product=Firefox-30.0b9-Complete&os=osx&lang=en-US

hashFunction - lowercase preferred
old - SHA512
new - sha512

Comment 30

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

Belatedly cleaned up aus4-admin-dev.allizom.org of -latest blobs with schema:1, to avoid bustage in our staging.