Closed Bug 933161 Opened 7 years ago Closed 7 years ago

Balrog shouldn't serve updates to older builds

Categories

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

ARM
Android
defect
Not set
major

Tracking

(firefox27 wontfix, firefox28 wontfix, firefox29 affected)

RESOLVED FIXED
Tracking Status
firefox27 --- wontfix
firefox28 --- wontfix
firefox29 --- affected

People

(Reporter: ioana.chiorean, Assigned: bhearsum)

References

Details

Attachments

(1 file, 3 obsolete files)

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
Component: General → Releases
Product: Firefox for Android → Release Engineering
QA Contact: rail
Version: Firefox 27 → other
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>
Hmm, I though that the updater code checks and prevents downgrades...
FTR, Aurora updates are frozen (not disabled completely) in bug 931832. See also https://bugzilla.mozilla.org/show_bug.cgi?id=931832#c8
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?
Component: Releases → Balrog: Backend
Flags: needinfo?(nthomas)
QA Contact: rail → bhearsum
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.
Flags: needinfo?(nthomas)
Summary: Aurora 27.oa2 10/30 updates to Aurora 26.0a2 10/28 → Balrog shouldn't serve updates to older builds
Duplicate of this bug: 686491
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
Attached patch older-balrog-balrog.diff (obsolete) — Splinter Review
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?
Attachment #8363754 - Flags: feedback?(nthomas)
Assignee: nobody → bhearsum
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 ?
Attachment #8363754 - Flags: feedback?(nthomas) → feedback+
Still seeing this for Aurora 29.oa2 (02/05/14) receiving update to Aurora 28.0a2 (02/03/14)
(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?
Flags: needinfo?(ioana.chiorean)
Flags: needinfo?(ioana.chiorean)
Attached patch check version then buildid (obsolete) — Splinter Review
(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!).
Attachment #8363754 - Attachment is obsolete: true
Attachment #8370767 - Flags: review?(nthomas)
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.
Attachment #8370767 - Flags: review?(nthomas) → review+
(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?
Flags: needinfo?(nthomas)
(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
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.
Attachment #8370767 - Attachment is obsolete: true
Attachment #8372252 - Flags: review?(nthomas)
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.
Attachment #8372252 - Flags: review?(nthomas) → review+
StrictVersion is fine by me, the b2g proposal to use w.x.y.z went away.
Flags: needinfo?(nthomas)
Attachment #8372252 - Attachment is obsolete: true
Attachment #8375496 - Flags: review?(nthomas)
Comment on attachment 8375496 [details] [diff] [review]
add test for version decrease

\o/
Attachment #8375496 - Flags: review?(nthomas) → review+
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 on attachment 8375496 [details] [diff] [review]
add test for version decrease

Landed, will ride with the next push to production - probably sometime next week.
Attachment #8375496 - Flags: checked-in+
Depends on: 976081
Deployed to prod.
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.