Closed Bug 1069512 Opened 7 years ago Closed 7 years ago

deal with tracebacks for unsupported version numbers

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bhearsum, Assigned: nthomas)

References

Details

Attachments

(1 file, 2 obsolete files)

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
Hopefully just updating https://github.com/mozilla/balrog/blob/master/auslib/util/versions.py#L9
Assignee: nobody → nthomas
Priority: -- → P2
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.
Attachment #8506807 - Flags: review?(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.
Attachment #8506807 - Flags: review?(bhearsum) → review+
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.
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.
Attachment #8507574 - Flags: review?(bhearsum)
As for v2, but trims the regexp down for clarity.
Attachment #8507574 - Attachment is obsolete: true
Attachment #8507574 - Flags: review?(bhearsum)
Attachment #8507576 - Flags: review?(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.
Attachment #8507576 - Flags: review?(bhearsum) → review+
Attachment #8506807 - Attachment is obsolete: true
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
Depends on: 1086565
Verified in errormill that we've squashed all the errors for 2.0.0.20 errors. Still getting one for 3.7a4webm.
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 ?
(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.
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?
Fine by me.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Product: Release Engineering → Release Engineering Graveyard
You need to log in before you can comment on or make changes to this bug.