933161 - Balrog shouldn't serve updates to older builds

Status: RESOLVED FIXED

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

Description

[Ioana Chiorean]

Device: Nexus 4 (Android 4.2) & Galaxy R (Android 2.3.4) 
Aurora 27.0a2 from 10/30

Steps to Reproduce:

1. Install Aurora 27.0a2 from 10/30
2. Do some browsing 
3. After a few second/minutes you will get a software update notification 
4. Apply update

Actual Results:  
- Aurora 27.0a2 from 10/30 updates to Aurora 26.0a2 from 10/28

Expected Results: 
- While in process of Aurora SignOff updates on his channel should be block

Comment 1

[Aaron Train [:aaronmt]]

https://aus4.mozilla.org/update/4/Fennec/27.0a2/20131031004003/Android_arm-eabi-gcc3/en-US/aurora/4.3/default/default/27.0a2/update.xml

<update type="minor" version="26.0a2" extensionVersion="26.0a2" buildID="20131028004004">
<patch type="complete" URL="http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/2013/10/2013-10-28-00-40-04-mozilla-aurora-android/fennec-26.0a2.multi.android-arm.apk" hashFunction="sha512" hashValue="4255fcf07abda8fb2fc9125b78afb7de94be3c93a0fc37ae1ff57d49e9b24af3bab0c0901a5ba291419c57c55cbcd866c3308ba416977ec93f611813ca2c1db3" size="30777203"/>
</update>

Comment 2

[Rail Aliiev [:rail]]

Hmm, I though that the updater code checks and prevents downgrades...

Comment 3

[Rail Aliiev [:rail]]

FTR, Aurora updates are frozen (not disabled completely) in bug 931832. See also https://bugzilla.mozilla.org/show_bug.cgi?id=931832#c8

Comment 4

[[e-mail] (:bhearsum)]

Hey Ioana, thanks for reporting this. This is happening because of our switch to the new update server (bug 927364). Unlike the old server, it will still serve you an update even if the build you're on is newer than the build it thinks you should get. We thought this wouldn't be an issue because the Desktop updater will refuse the update anyways, but it seems we didn't think about mobile.

I don't think this is a _super_ critical issue, as it only affects new installs that happen between turning off Aurora updates and turning the back on. These users will get one update that sends them back to the point of shut off (in this case, 10/28), and then they'll stay stuck there.

There's nothing we can do immediately about this, but I think we should fix this before the next merge.

We already serve a null update if the incoming release is the same as one we'd serve (https://github.com/mozilla/balrog/blob/58eda7b667b1229ce0643bcedea0919637918962/auslib/AUS.py#L107), maybe this is as simple as "if updateQuery['buildID'] >= release['buildID']: return None"?

Any thoughts Nick?

Comment 5

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

In the old server the buildID is required to increase for all channel's which are defined as nightly-style (see findPatch()'s calls to isNewerBuild() in http://mxr.mozilla.org/mozilla/source/webtools/aus/xml/inc/patch.class.php). We should bring that over at least, but I don't think there are any cases where we would want to downgrade non-nightly channels (and as you say the desktop updater will refuse to anyway) so we can do it globally like you suggest.

Comment 7

[[e-mail] (:bhearsum)]

Nightly Android got into a similar situation described in comment #0 today. The build failed to submit to Balrog because certain properties weren't set:
https://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/2014/01/2014-01-17-03-02-07-mozilla-central-android/mozilla-central-android-nightly-bm64-build1-build1.txt.gz

Comment 8

[[e-mail] (:bhearsum)]

Attachment: older-balrog-balrog.diff

This mostly works...but it breaks the major update test, because 3.6.27 has a larger buildid than 10.0.2. I guess this works in aus2/3 because we don't have the checks for non-Nightly channels.

My current thinking is that we should check version for newness, and then only check the buildid if the version is same. What do you think?

Comment 9

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

Comment on attachment 8363754 [details] [diff] [review]
older-balrog-balrog.diff

(In reply to Ben Hearsum [:bhearsum] from comment #8)
> My current thinking is that we should check version for newness, and then
> only check the buildid if the version is same. What do you think?

I can't think of anything wrong if
  version can't decrease, buildID can't be equal or smaller
but there's that annoying jangly feeling in my head which usually means I missed something. Will try to shake it loose, but carry on in the meantime.

>diff --git a/auslib/AUS.py b/auslib/AUS.py

>+from auslib.blob import ReleaseBlobV1

Not actually using this new import ?

>diff --git a/auslib/test/test_AUS.py b/auslib/test/test_AUS.py
>         self.AUS.db.releases.t.insert().execute(name='b', product='b', version='b', data_version=1, data=json.dumps(self.relData['b']))
>+        self.AUS.db.rules.t.insert().execute(mapping='b', priority=100, backgroundRate=100, update_type='minor', data_version=1)

This addition looks a little odd. What do we need it for now ?

Comment 10

[Ioana Chiorean]

Still seeing this for Aurora 29.oa2 (02/05/14) receiving update to Aurora 28.0a2 (02/03/14)

Comment 11

[[e-mail] (:bhearsum)]

(In reply to Ioana Chiorean from comment #10)
> Still seeing this for Aurora 29.oa2 (02/05/14) receiving update to Aurora
> 28.0a2 (02/03/14)

Yep. I expect this to be fixed before the next cycle. Note that only Aurora and Nightly are affected by this, though. Beta and Release use the older update server, and aren't affected. Can you update the flags?

Comment 12

[[e-mail] (:bhearsum)]

Attachment: check version then buildid

(In reply to Nick Thomas [:nthomas] from comment #9)
> >diff --git a/auslib/test/test_AUS.py b/auslib/test/test_AUS.py
> >         self.AUS.db.releases.t.insert().execute(name='b', product='b', version='b', data_version=1, data=json.dumps(self.relData['b']))
> >+        self.AUS.db.rules.t.insert().execute(mapping='b', priority=100, backgroundRate=100, update_type='minor', data_version=1)
> 
> This addition looks a little odd. What do we need it for now ?

Whoops, this was from a previous attempt to write a test. I removed it.


This latest patch implements the version/buildid checks described in comment #8, and should fix this bug (unless you've found the source of your jangle!).

Comment 13

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

Comment on attachment 8370767 [details] [diff] [review]
check version then buildid

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

r+ with some changes.

::: auslib/AUS.py
@@ +112,5 @@
> +        buildTarget = updateQuery['buildTarget']
> +        locale = updateQuery['locale']
> +        releaseVersion = LooseVersion(release['data'].getExtv(buildTarget, locale))
> +        queryVersion = LooseVersion(updateQuery['version'])
> +        if releaseVersion < queryVersion:

I think my jangle was just a preference about the ordering of the comparisons. For some reason 
   if queryVersion > releaseVersion:
is easier to read. Since this is style I'll leave it up to you.

More of a worry is if we ever send versions different from X.Y.Z is:
  >>> LooseVersion('27.0b2') < LooseVersion('27.0')
  False
The documentation is unusually poor for Python, but StrictVersion looks like a better fit as it actually bothers to define the sorting order (see help(distutils.version.StrictVersion)). Doesn't support w.x.y.z though, did we see that suggested somewhere recently ?

@@ +113,5 @@
> +        locale = updateQuery['locale']
> +        releaseVersion = LooseVersion(release['data'].getExtv(buildTarget, locale))
> +        queryVersion = LooseVersion(updateQuery['version'])
> +        if releaseVersion < queryVersion:
> +            self.log.debug("Incoming query's version is greater than the matching rule's mapping's version.")

Nit, the message could be cleaner. Depending on the order chosen above:
 "Matching rule has older version than request, ignoring rule"
 "Request has higher version than matching rule, ignoring rule"

@@ +118,4 @@
>              return None, None
> +        elif releaseVersion == queryVersion:
> +            if updateQuery['buildID'] >= release['data'].getBuildID(updateQuery['buildTarget'], updateQuery['locale']):
> +                self.log.debug("Incoming query's buildid is greater than the matchin rule's mapping's buildid.")

Another nit on the string.

Comment 14

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #13)
> More of a worry is if we ever send versions different from X.Y.Z is:
>   >>> LooseVersion('27.0b2') < LooseVersion('27.0')
>   False

Oof. That's unfortunate. And we're already using LooseVersion in ReleaseUpdatesFactory....

> The documentation is unusually poor for Python, but StrictVersion looks like
> a better fit as it actually bothers to define the sorting order (see
> help(distutils.version.StrictVersion)).

Indeed. I think we can use this instead...

> Doesn't support w.x.y.z though, did
> we see that suggested somewhere recently ?

It was talked about for B2G IIRC. I don't think it will be too difficult to deal with if it comes to it. I managed to subclass StrictVersion to support 4 part versions in just a few minutes. Do you want to get ahead of the game and do that now, or should we stick with StrictVersion until we know we need support?

Comment 15

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #14)
> It was talked about for B2G IIRC. I don't think it will be too difficult to
> deal with if it comes to it. I managed to subclass StrictVersion to support
> 4 part versions in just a few minutes. Do you want to get ahead of the game
> and do that now, or should we stick with StrictVersion until we know we need
> support?

I pushed that code to braindump in case we need it in the future: https://hg.mozilla.org/build/braindump/rev/29047545e43e

Comment 16

[[e-mail] (:bhearsum)]

Attachment: use StrictVersion; address other comments

Here's a patch that uses StrictVersion. I'm equally happy to spend a bit more time to make ModeratelyStrictVersion better tested and used here if you think we should be proactive about it.

Comment 17

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

Comment on attachment 8372252 [details] [diff] [review]
use StrictVersion; address other comments

>diff --git a/auslib/test/web/test_client.py b/auslib/test/web/test_client.py
>     def testDontUpdateToYourself(self):
>-        ret = self.client.get('/update/3/b/1/2/p/l/a/a/a/a/update.xml')
>+        ret = self.client.get('/update/3/b/1.0/2/p/l/a/a/a/a/update.xml')
>+        self.assertEqual(ret.status_code, 200)
>+        self.assertEqual(ret.mimetype, 'text/xml')
>+        self.assertEqual(minidom.parseString(ret.data).getElementsByTagName('updates')[0].firstChild.nodeValue, '\n')
>+
>+    def testDontUpdateBackwards(self):
>+        ret = self.client.get('/update/3/b/1.0/5/p/l/a/a/a/a/update.xml')
>         self.assertEqual(ret.status_code, 200)
>         self.assertEqual(ret.mimetype, 'text/xml')
>         self.assertEqual(minidom.parseString(ret.data).getElementsByTagName('updates')[0].firstChild.nodeValue, '\n')

So we've got two buildid tests, but no version test. Lets add one for that and call this done.

Comment 18

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

StrictVersion is fine by me, the b2g proposal to use w.x.y.z went away.

Comment 19

[[e-mail] (:bhearsum)]

Attachment: add test for version decrease

Comment 20

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

Comment on attachment 8375496 [details] [diff] [review]
add test for version decrease

\o/

Comment 21

[[github robot]]

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

https://github.com/mozilla/balrog/commit/b0f9f909567f481289e59eaf706c07bb347a4924
bug 933161: Balrog shouldn't serve updates to older builds. r=nthomas

Comment 22

[[e-mail] (:bhearsum)]

Comment on attachment 8375496 [details] [diff] [review]
add test for version decrease

Landed, will ride with the next push to production - probably sometime next week.

Comment 23

[[e-mail] (:bhearsum)]

Deployed to prod.