1312560 - deal with unparsable versions

Status: RESOLVED FIXED

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

Description

[[e-mail] (:bhearsum)]

We're getting a lot of tracebacks due to versions that can't be parsed. For example: 39.0.1a, 45, 41a1pre, 40.0.29.10, 2.0.0.25pre.

Here's some links to Sentry:
https://sentry.prod.mozaws.net/operations/prod-public/issues/360266/
https://sentry.prod.mozaws.net/operations/prod-public/issues/360281/
https://sentry.prod.mozaws.net/operations/prod-public/issues/360296/
https://sentry.prod.mozaws.net/operations/prod-public/issues/360319/
https://sentry.prod.mozaws.net/operations/prod-public/issues/360914/

All of the tracebacks end up coming from ModernMozillaVersion or AncientMozillaVersion after they hand off to distutils. Eg:
ValueError: invalid version number '2.0.0.25pre'
  File "flask/app.py", line 1475, in full_dispatch_request
    rv = self.dispatch_request()
  File "flask/app.py", line 1461, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "flask/views.py", line 84, in view
    return self.dispatch_request(*args, **kwargs)
  File "flask/views.py", line 149, in dispatch_request
    return meth(*args, **kwargs)
  File "auslib/web/views/client.py", line 55, in get
    release, update_type = AUS.evaluateRules(query)
  File "auslib/AUS.py", line 95, in evaluateRules
    if not blob.shouldServeUpdate(updateQuery):
  File "auslib/blobs/apprelease.py", line 163, in shouldServeUpdate
    queryVersion = MozillaVersion(updateQuery['version'])
  File "auslib/util/versions.py", line 30, in MozillaVersion
    return AncientMozillaVersion(version)
  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


...but they may come from different parts of the app. The one above comes from AppReleaseBlob.shouldServeUpdate, but others come from Rules.getRulesMatchingQuery.


We should probably be raising BadDataError when the version can't be parsed, which will still let things abort when something tries to use it, but will avoid sending it to Sentry (which has a lot of overhead).

Comment 1

[dpaks]

Ben, are the sentry links viewable by a guest?

Comment 2

[dpaks]

(In reply to Ben Hearsum (:bhearsum) from comment #0)
> We're getting a lot of tracebacks due to versions that can't be parsed. For
> example: 39.0.1a, 45, 41a1pre, 40.0.29.10, 2.0.0.25pre.
> 
> Here's some links to Sentry:
> https://sentry.prod.mozaws.net/operations/prod-public/issues/360266/
> https://sentry.prod.mozaws.net/operations/prod-public/issues/360281/
> https://sentry.prod.mozaws.net/operations/prod-public/issues/360296/
> https://sentry.prod.mozaws.net/operations/prod-public/issues/360319/
> https://sentry.prod.mozaws.net/operations/prod-public/issues/360914/
> 
> All of the tracebacks end up coming from ModernMozillaVersion or
> AncientMozillaVersion after they hand off to distutils. Eg:
> ValueError: invalid version number '2.0.0.25pre'
>   File "flask/app.py", line 1475, in full_dispatch_request
>     rv = self.dispatch_request()
>   File "flask/app.py", line 1461, in dispatch_request
>     return self.view_functions[rule.endpoint](**req.view_args)
>   File "flask/views.py", line 84, in view
>     return self.dispatch_request(*args, **kwargs)
>   File "flask/views.py", line 149, in dispatch_request
>     return meth(*args, **kwargs)
>   File "auslib/web/views/client.py", line 55, in get
>     release, update_type = AUS.evaluateRules(query)
>   File "auslib/AUS.py", line 95, in evaluateRules
>     if not blob.shouldServeUpdate(updateQuery):
>   File "auslib/blobs/apprelease.py", line 163, in shouldServeUpdate
>     queryVersion = MozillaVersion(updateQuery['version'])
>   File "auslib/util/versions.py", line 30, in MozillaVersion
>     return AncientMozillaVersion(version)
>   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
> 
> 
> ...but they may come from different parts of the app. The one above comes
> from AppReleaseBlob.shouldServeUpdate, but others come from
> Rules.getRulesMatchingQuery.
>
Ben, correct me if I am wrong. 
1) The plan I have in my mind is to raise a BadDataError with a meaningful message instead of https://github.com/mozilla/balrog/blob/master/auslib/util/versions.py#L32.

2) As we already modified https://github.com/mozilla/balrog/blob/master/auslib/web/base.py to raise the errors to client side and given it doesn't capture BadDataErrors for Sentry, we don't have to do anything w.r.t the following right? 
> 
> We should probably be raising BadDataError when the version can't be parsed,
> which will still let things abort when something tries to use it, but will
> avoid sending it to Sentry (which has a lot of overhead).

Comment 3

[[e-mail] (:bhearsum)]

(In reply to dpaks from comment #2)
> (In reply to Ben Hearsum (:bhearsum) from comment #0)
> > We're getting a lot of tracebacks due to versions that can't be parsed. For
> > example: 39.0.1a, 45, 41a1pre, 40.0.29.10, 2.0.0.25pre.
> > 
> > Here's some links to Sentry:
> > https://sentry.prod.mozaws.net/operations/prod-public/issues/360266/
> > https://sentry.prod.mozaws.net/operations/prod-public/issues/360281/
> > https://sentry.prod.mozaws.net/operations/prod-public/issues/360296/
> > https://sentry.prod.mozaws.net/operations/prod-public/issues/360319/
> > https://sentry.prod.mozaws.net/operations/prod-public/issues/360914/
> > 
> > All of the tracebacks end up coming from ModernMozillaVersion or
> > AncientMozillaVersion after they hand off to distutils. Eg:
> > ValueError: invalid version number '2.0.0.25pre'
> >   File "flask/app.py", line 1475, in full_dispatch_request
> >     rv = self.dispatch_request()
> >   File "flask/app.py", line 1461, in dispatch_request
> >     return self.view_functions[rule.endpoint](**req.view_args)
> >   File "flask/views.py", line 84, in view
> >     return self.dispatch_request(*args, **kwargs)
> >   File "flask/views.py", line 149, in dispatch_request
> >     return meth(*args, **kwargs)
> >   File "auslib/web/views/client.py", line 55, in get
> >     release, update_type = AUS.evaluateRules(query)
> >   File "auslib/AUS.py", line 95, in evaluateRules
> >     if not blob.shouldServeUpdate(updateQuery):
> >   File "auslib/blobs/apprelease.py", line 163, in shouldServeUpdate
> >     queryVersion = MozillaVersion(updateQuery['version'])
> >   File "auslib/util/versions.py", line 30, in MozillaVersion
> >     return AncientMozillaVersion(version)
> >   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
> > 
> > 
> > ...but they may come from different parts of the app. The one above comes
> > from AppReleaseBlob.shouldServeUpdate, but others come from
> > Rules.getRulesMatchingQuery.
> >
> Ben, correct me if I am wrong. 
> 1) The plan I have in my mind is to raise a BadDataError with a meaningful
> message instead of
> https://github.com/mozilla/balrog/blob/master/auslib/util/versions.py#L32.

That sounds like a good plan to me.

> 2) As we already modified
> https://github.com/mozilla/balrog/blob/master/auslib/web/base.py to raise
> the errors to client side and given it doesn't capture BadDataErrors for
> Sentry, we don't have to do anything w.r.t the following right? 

That's right - auslib/web/base.py already does the correct thing with BadDataError, so there shouldn't be anything else to do there. Generally, when we have things like this were clients are sending weird things in the requests that we can't handle, we try to eat the exceptions they cause by re-raising as BadDataError.

Comment 4

[[github robot]]

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

https://github.com/mozilla/balrog/commit/530e5c46cc7e676e6889f2c50b79bed06f282940
bug 1312560: handling unparsable versions as BadDataError (#199). r=bhearsum

Comment 5

[[e-mail] (:bhearsum)]

This is in production. Thanks dpaks!