1069512 - deal with tracebacks for unsupported version numbers

Status: RESOLVED FIXED

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

Description

[[e-mail] (:bhearsum)]

We're getting some valid looking requests from 2.0.0.20 (!) users on Windows 98 (!!!). Eg:
https://aus3.mozilla.org/update/2/Firefox/2.0.0.20/2008121709/WINNT_x86-msvc/de/beta/Windows_98%204.10/update.xml

And they cause tracebacks like:


ValueError: invalid version number '2.0.0.20'

Stacktrace (most recent call last):

  File "flask/app.py", line 1060, in full_dispatch_request
    rv = self.dispatch_request()
  File "flask/app.py", line 1047, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "flask/views.py", line 56, in view
    return self.dispatch_request(*args, **kwargs)
  File "flask/views.py", line 112, in dispatch_request
    return meth(*args, **kwargs)
  File "auslib/web/views/client.py", line 35, in get
    release, update_type = AUS.evaluateRules(query)
  File "auslib/AUS.py", line 53, in evaluateRules
    fallbackChannel=getFallbackChannel(updateQuery['channel'])
  File "auslib/db.py", line 744, in getRulesMatchingQuery
    if not self._versionMatchesRule(rule['version'], updateQuery['version']):
  File "auslib/db.py", line 671, in _versionMatchesRule
    return version_compare(queryVersion, ruleVersion)
  File "auslib/util/comparison.py", line 38, in version_compare
    value = MozillaVersion(value)
  File "distutils/version.py", line 40, in __init__
    self.parse(vstring)
  File "distutils/version.py", line 107, in parse
    raise ValueError, "invalid version number '%s'" % vstring

Comment 1

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

Hopefully just updating https://github.com/mozilla/balrog/blob/master/auslib/util/versions.py#L9

Comment 2

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

Attachment: [balrog] Squash middle .0 in x.y.0.z

StrictVersion doesn't support versions longer than x.y.z, so this is a vaguely hacky workaround for our historic use cases. ie update paths of
  1.5.0.x --> 1.5.0.12 ---> 2.0.0.6
  2.0.0.x --> 2.0.0.20 ---> 3.0.15
We'd have a rule with version of 1.5.12 mapped to Firefox-2.0.0.6, and 2.0.20 to Firefox-3.0.15.

We could duplicate & modify StrictVersion.parse() too, if you prefer.

Comment 3

[[e-mail] (:bhearsum)]

Comment on attachment 8506807 [details] [diff] [review]
[balrog] Squash middle .0 in x.y.0.z

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

The bulk of StrictVersion is parse(), so if we end up duplicating it we're almost maintaining our own version class entirely.

It sucks a bit that we need to look for "0" instead of a more generic number there, but I would still call this solution elegant rather than hacky =). I can't think of any 4 part version number where the 3rd part has been significant, so ignoring that part seems like the right thing.

Comment 4

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

We could write 
 version_re = re.compile(r'^(\d+) \. (\d+) ((?:\. \d)? \. (\d+ ))? ([a-zA-Z]+(\d+))?$', re.VERBOSE)
to modify w.x.y.z to w.x.z. That seems like a bigger footgun for the future if we don't need to cover any cases which aren't w.x.0.z right now.

Comment 5

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

Attachment: [balrog] Handle old version separately

Rather than write one regexp to handle all cases, this leaves the existing one alone except for renaming the class. If we fail to parse the version with that, then try the old style with any digit in third position, otherwise call for mama.

Comment 6

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

Attachment: [balrog] Handle old version separately, with clearer regexp

As for v2, but trims the regexp down for clarity.

Comment 7

[[e-mail] (:bhearsum)]

Comment on attachment 8507576 [details] [diff] [review]
[balrog] Handle old version separately, with clearer regexp

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

Ah, very good idea.

Comment 8

[[github robot]]

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

https://github.com/mozilla/balrog/commit/3f8776be8ca4e372075ed89be8e872d3569e6682
Bug 1069512, deal with tracebacks for old style w.x.y.z versions, r=bhearsum

Comment 9

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

Verified in errormill that we've squashed all the errors for 2.0.0.20 errors. Still getting one for 3.7a4webm.

Comment 10

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

Ben, I'm not sure it's worth handling this one old, anomalous, version, but I don't really want to leave exceptions in errormill either. Any thoughts ?

Comment 11

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #10)
> Ben, I'm not sure it's worth handling this one old, anomalous, version, but
> I don't really want to leave exceptions in errormill either. Any thoughts ?

It's not the end of the world if we have just this one sitting in there. Perhaps we could add a finally "except" block in the MozillaVersion function that raises a BadDataError instead of ValueError? Or would that mask other potentially useful failures to parse version?

I also wonder if getting a newer version of Sentry would give us more ways to filter or sort data on the interface in general. It has tags already, but no way of adding custom ones.

Comment 12

[[e-mail] (:bhearsum)]

Been thinking about this more and it seems like we shouldn't be optimizing around zero exceptions on Sentry. I agree that it's not worth fixing the version regex for this edge case. If Sentry (or some better system) was able to let us tag or otherwise mark exceptions as "acceptable" (aka "stfu about this forever"), we'd be done here. Perhaps we should close this bug and live with these appearing on Sentry until we switch to something more flexible?

Comment 13

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

Fine by me.