745289 - balrog request matching code doesn't cope well with missing locales

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[[e-mail] (:bhearsum)]

I noticed this today while debugging something else. Rail and I dove into it, and discovered this traceback:
DEBUG:auslib.AUS:AUS.identifyRequest: Trying to match request to Firefox-mozilla-central-nightly-20120321031151
ERROR:auslib.client.base:Exception on /update/3/Firefox/14.0a1/20120411030716/Linux_x86_64-gcc3/fa/nightly/Linux 3.0.0-12-generic (GTK 2.24.6)/default/default/update.xml [GET]
Traceback (most recent call last):
  File "/usr/lib/python2.6/site-packages/flask/app.py", line 1292, in wsgi_app
    response = self.full_dispatch_request()
  File "/usr/lib/python2.6/site-packages/flask/app.py", line 1062, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/usr/lib/python2.6/site-packages/flask/app.py", line 1060, in full_dispatch_request
    rv = self.dispatch_request()
  File "/usr/lib/python2.6/site-packages/flask/app.py", line 1047, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/usr/lib/python2.6/site-packages/flask/views.py", line 56, in view
    return self.dispatch_request(*args, **kwargs)
  File "/usr/lib/python2.6/site-packages/flask/views.py", line 112, in dispatch_request
    return meth(*args, **kwargs)
  File "/data/www/aus4-dev.allizom.org/auslib/client/views/client.py", line 51, in get
    query = self.getQueryFromURL(queryVersion, url)
  File "/data/www/aus4-dev.allizom.org/auslib/client/views/client.py", line 42, in getQueryFromURL
    query['name'] = AUS.identifyRequest(query)
  File "/data/www/aus4-dev.allizom.org/auslib/AUS.py", line 58, in identifyRequest
    releaseBuildID = release['data'].getBuildID(buildTarget, locale)
  File "/data/www/aus4-dev.allizom.org/auslib/blob.py", line 121, in getBuildID
    return self['platforms'][platform]['buildID']
KeyError: 'buildID'


I dumped that blob from the database, and found that it was missing data for most locales -- I think this day is around the time when we had a whole bunch of l10n builds fail due to out of sync hg mirrors. So, the root of this issue appears to be that the code that deals with matching incoming builds to blobs doesn't cope well with missing data. We should make that better!

Comment 1

[[e-mail] (:bhearsum)]

Attachment: don't return buildID unless locale exists; catch errors

After digging into this a bit more I noticed that the getBuildID method of the blob tells lies sometimes....in particular, it will always return a buildID when the platform exists, even if the locale doesn't. I changed it to raise a KeyError when the locale doesn't exist. To fix the actual problem this bug is about, identifyRequest needed to catch any KeyError's raised by getBuildID, and skip the release in those cases. Additionally, I had to update a bunch of the rule tests to cope.

Comment 2

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

Comment on attachment 614889 [details] [diff] [review]
don't return buildID unless locale exists; catch errors

>diff --git a/aus-data-snapshots/version-globbing/Firefox-3.6.10-build1.json b/aus-data-snapshots/version-globbing/Firefox-3.6.10-build1.json

You could remove Linux and WinNT from all the json in this dir, since there aren't any snippets for them.

>diff --git a/auslib/blob.py b/auslib/blob.py
>     def getBuildID(self, platform, locale):
>+        platform = self.getResolvedPlatform(platform)

There are also some problems if the platform isn't in the blobs. We should probably revisit error/exception handling for all the core logic code at some point.

>diff --git a/auslib/test/test_AUS.py b/auslib/test/test_AUS.py
>+    # Make sure identifyRequest gracefully handles missing locales, and dosen't

typo: dosen't

>diff --git a/auslib/test/test_blob.py b/auslib/test/test_blob.py
>+    def testGetBuildIDMissingLocale(self):
>+        blob = ReleaseBlobV1(platforms=dict(c=dict(locales=dict(d=dict(buildID=9)))))

Aside, is there any advantage to using the dict style vs something ?
 {'c': {'locales': {'d': {'buildID': 9}}}}
Personally I find this more readable.

>     # XXX: should we support the locale overriding the platform? this should probably be invalid

We could if we can think of a situation that would require it. I'd probably prefer to handle it with extra rules and an extra release, unless we need a large number of rules.

Comment 3

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #2)
> Comment on attachment 614889 [details] [diff] [review]
> don't return buildID unless locale exists; catch errors
> 
> >diff --git a/aus-data-snapshots/version-globbing/Firefox-3.6.10-build1.json b/aus-data-snapshots/version-globbing/Firefox-3.6.10-build1.json
> 
> You could remove Linux and WinNT from all the json in this dir, since there
> aren't any snippets for them.
> 
> >diff --git a/auslib/blob.py b/auslib/blob.py
> >     def getBuildID(self, platform, locale):
> >+        platform = self.getResolvedPlatform(platform)
> 
> There are also some problems if the platform isn't in the blobs. We should
> probably revisit error/exception handling for all the core logic code at
> some point.

Hmmm, I think that ends up being OK, because it's another KeyError that will get caught in identifyRequest. Or are you seeing something different? It might be worthwhile checking and raising a KeyError with a better message though, like we're doing for locale in getBuildID.

> >diff --git a/auslib/test/test_blob.py b/auslib/test/test_blob.py
> >+    def testGetBuildIDMissingLocale(self):
> >+        blob = ReleaseBlobV1(platforms=dict(c=dict(locales=dict(d=dict(buildID=9)))))
> 
> Aside, is there any advantage to using the dict style vs something ?
>  {'c': {'locales': {'d': {'buildID': 9}}}}
> Personally I find this more readable.

I don't have a big preference, but we are using the dict() style most other places. I can change it if you want.

> >     # XXX: should we support the locale overriding the platform? this should probably be invalid
> 
> We could if we can think of a situation that would require it. I'd probably
> prefer to handle it with extra rules and an extra release, unless we need a
> large number of rules.

OK, it sounds like there's no reason to implement it now?

Comment 4

[[e-mail] (:bhearsum)]

Comment on attachment 614889 [details] [diff] [review]
don't return buildID unless locale exists; catch errors

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

Landed, with the unnecessary parts of the json removed and the typo fixed. Jenkins run is here: https://jenkins.mozilla.org/job/Balrog/79/