879813 - support version 2 & 4 url schemas in balrog

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[[e-mail] (:bhearsum)]

Nick and I talked about this a lot yesterday and decided that we could support these without changing the schema of the database or fixing bug 855833 in general.

Specifically, version 2 is a subset of version 3, so we can use the existing rules and just ignore "distribution" and "distribution version" parts of them.

Version 4 is a superset, but the added field has never been used - it's always matched %VERSION%. We can use existing rules for this too, and simply ignore the last part of the URL.

Comment 1

[[e-mail] (:bhearsum)]

https://github.com/bhearsum/balrog/compare/mozilla:master...schema-2-4 seems to do the trick. It took a suspiciously short amount of time...not putting up for review yet for fear that I've missed something terribly important. Probably needs some polish/comments too.

Comment 2

[[e-mail] (:bhearsum)]

Attachment: support schema versions 2 & 4

This is pretty much the bare minimum we need to do to support them. In addition to the unit tests I also did local testing with a db dump. I was able to get to the point of getting an exception about hashFunction when querying http://localhost:8000/update/4/Fennec/21.0a1/20130126143652/Android_arm-eabi-gcc3/en-US/nightly/4.1.1/default/default/21.0a1/update.xml. (That's normal when hashFunction isn't set in the blob.)

Couple of other small fixes in here:
* Fix tests not to depend on ordered lists that come from a 'select foo from bar' query.. I just upgraded my OS, and it seems that these are not guaranteed to be the same in all situations.
* Fix AUS-server.py to work with existing or nonexisting dbs.

I'm happy to change impl if you're not happy with this, so don't hold back!

Comment 3

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

Comment on attachment 759387 [details] [diff] [review]
support schema versions 2 & 4

r- for the testVersion2GetIgnoresRuleWithDistribution question, otherwise a few nits but fine.

>diff --git a/auslib/test/web/test_client.py b/auslib/test/web/test_client.py
>+        AUS.releases.t.insert().execute(name='c', product='c', version='c', data_version=1, data="""
>+{
>+    "name": "c",
>+    "schema_version": 1,
>+    "appv": "c",
>+    "extv": "c",
>+    "hashFunction": "sha512",
>+    "platforms": {
>+        "p": {
>+            "buildID": 1,
>+            "locales": {
>+                "l": {
>+                    "complete": {
>+                        "filesize": 1,
>+                        "from": "*",
>+                        "hashValue": "1",
>+                        "fileUrl": "a"

Would be worth setting filesize, hashValue, fileUrl to different values from the b release, to avoid accidental test passes.

>+    def testVersion2Get(self):

Lets rename testGet() to testVersion3Get() for consistency.

>+    def testVersion2GetIgnoresRuleWithDistribution(self):

What sense do you mean 'Ignores' here ? The test is ensuring release c is returned, rather than no update. Did you mean to add an else block in auslib/db.py
to make sure distribution and distVersion are undefined for update/2/ ? That probably makes sense from business logic pov.


>+    def testVersion4Get(self):
>+        ret = self.client.get('/update/3/b/b/1/p/l/a/a/a/a/b/update.xml')

The url should be update/4 instead of update/3.

>diff --git a/auslib/web/base.py b/auslib/web/base.py
>+app.add_url_rule('/update/<int:queryVersion>/<product>/<version>/<buildID>/<buildTarget>/<locale>/<channel>/<osVersion>/update.xml', view_func=ClientRequestView.as_view('clientrequest'))
> app.add_url_rule('/update/<int:queryVersion>/<product>/<version>/<buildID>/<buildTarget>/<locale>/<channel>/<osVersion>/<distribution>/<distVersion>/update.xml', view_func=ClientRequestView.as_view('clientrequest'))
>+app.add_url_rule('/update/<int:queryVersion>/<product>/<version>/<buildID>/<buildTarget>/<locale>/<channel>/<osVersion>/<distribution>/<distVersion>/<platformVersion>/update.xml', 
view_func=ClientRequestView.as_view('clientrequest'))

I see the utility in automatically parsing queryVersion here, but in some ways it would be nicer to be explicit. Could we add comments to identify the value of queryVersion for each rule ?

Comment 4

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #3)
> >+                        "fileUrl": "a"
> 
> Would be worth setting filesize, hashValue, fileUrl to different values from
> the b release, to avoid accidental test passes.

> >+    def testVersion2Get(self):
> 
> Lets rename testGet() to testVersion3Get() for consistency.

Will do.


> >+    def testVersion2GetIgnoresRuleWithDistribution(self):
> 
> What sense do you mean 'Ignores' here ? The test is ensuring release c is
> returned, rather than no update.

Yeah, this is poorly named...it should probably be called "Version2GetIgnoresDistributionInRule"

> Did you mean to add an else block in
> auslib/db.py
> to make sure distribution and distVersion are undefined for update/2/ ? That
> probably makes sense from business logic pov.

Hm. I thought we'd decided that version 2 queries would ignore distribution values when evaluating rules. I guess we get a little bit more flexibility if we do it this way though...so that seems like a good idea.

> 
> >+    def testVersion4Get(self):
> >+        ret = self.client.get('/update/3/b/b/1/p/l/a/a/a/a/b/update.xml')
> 
> The url should be update/4 instead of update/3.

D'oh.

> >diff --git a/auslib/web/base.py b/auslib/web/base.py
> >+app.add_url_rule('/update/<int:queryVersion>/<product>/<version>/<buildID>/<buildTarget>/<locale>/<channel>/<osVersion>/update.xml', view_func=ClientRequestView.as_view('clientrequest'))
> > app.add_url_rule('/update/<int:queryVersion>/<product>/<version>/<buildID>/<buildTarget>/<locale>/<channel>/<osVersion>/<distribution>/<distVersion>/update.xml', view_func=ClientRequestView.as_view('clientrequest'))
> >+app.add_url_rule('/update/<int:queryVersion>/<product>/<version>/<buildID>/<buildTarget>/<locale>/<channel>/<osVersion>/<distribution>/<distVersion>/<platformVersion>/update.xml', 
> view_func=ClientRequestView.as_view('clientrequest'))
> 
> I see the utility in automatically parsing queryVersion here, but in some
> ways it would be nicer to be explicit. Could we add comments to identify the
> value of queryVersion for each rule ?

Previously, I thought that queryVersion wouldn't get passed to ClientRequestView if specified explicitly but I just did some digging, and it looks like you can set defaults per rule. I agree that it's nicer to be explicit, so I'll make that change.

Comment 5

[[e-mail] (:bhearsum)]

Attachment: address review comments

Assuming we're on the same page about how to handle version 2 queries, I think this should address everything...

Comment 6

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

Comment on attachment 759817 [details] [diff] [review]
address review comments

The interdiff looks good to me.

Comment 7

[[github robot]]

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

https://github.com/mozilla/balrog/commit/6373821ed14e99e065336c14da0c3188e70def6c
bug 879813: support version 2 & 4 url schemas in balrog. r=nthomas

Comment 8

[[e-mail] (:bhearsum)]

This looks to be working in production. Eg
https://aus4-dev.allizom.org/update/4/Fennec/21.0a1/20130126143652/Android_arm-eabi-gcc3/en-US/nightly/4.1.1/default/default/21.0a1/update.xml matches https://aus4-admin-dev.allizom.org/releases/Firefox-mozilla-central-nightly-latest/data

https://aus4-dev.allizom.org/update/2/Firefox/2.0.0.13pre/2008030203/WINNT_x86-msvc/en-US/nightly/Windows_NT%205.1/update.xml?force=1 vs https://aus3.mozilla.org/update/2/Firefox/2.0.0.13pre/2008030203/WINNT_x86-msvc/en-US/nightly/Windows_NT%205.1/update.xml?force=1

The latter has some differences in the attributes it serves. Nick, is that going to get dealt with in bug 748698, or should I file a new bug?

Comment 9

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

That's kinda fun. We don't want to serve the new attributes to anything that can't handle it. It might be this just works when we import some of the old cruft in the AUS2 snippets, I'd have to dig into bug 459972 and it's parents on the client side. Definitely has a bearing on bug 748698.

Comment 10

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

Oops, didn't notice you had the nightly channel there. That'd require some logic in Balrog instead, although we've long given up on any users on builds that ancient. At the time, the delay in implementing bug 459972 meant that we had well and truly given everyone a shot at getting a build that groks the new attributes.