945179 - Match on a list of locales, rather than exact only

Status: RESOLVED FIXED

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

Description

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

I think I'm going to need to give a list of locales, eg
  ab,cd-EF,gh
for bug 657789, and don't want to write a rule for every locale.

Comment 1

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

Attachment: First cut

I'm interested in what you think of this, as I can't say all the consequences are clear to me yet.

Comment 2

[[e-mail] (:bhearsum)]

Comment on attachment 8340976 [details] [diff] [review]
First cut

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

This looks sensible to me. Might be worth running compare-update-requests.py against aus4.m.o either locally or after you push this to dev.

We now have 4 different methods like this (version, channel, osVersion, locale) that are all very similar. It sets off my refactor alert...but these all of different business logic, so maybe it's probably fine how it is.

Comment 4

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

Haven't got to this, being honest and unassigning.

Comment 5

[[e-mail] (:bhearsum)]

I want this to make bug 1134471 easier, I can probably drive it to completion....

Comment 6

[[e-mail] (:bhearsum)]

Attachment: increase locale column size

I discovered that the column wasn't wide enough when I tried out your patch in vagrant. This should increase it to 200 + adjust the form verification to accept that.

With this applied, I was able to create these rules:
mysql> select mapping, product, channel, locale from rules where channel="release";
+---------------------+-------------+---------+----------------+
| mapping             | product     | channel | locale         |
+---------------------+-------------+---------+----------------+
| Firefox-31.0-build2 | Firefox     | release | NULL           |
| Firefox-31.0-build1 | Firefox     | release | de,ru,zh-TW,zu |
+---------------------+-------------+---------+----------------+


And I get responses like:
(balrog)➜  balrog git:(localeslist) curl http://balrog.mozilla.dev:9000/update/3/Firefox/25.0/20120222174716/WINNT_x86-msvc/de/release/default/default/default/update.xml
<?xml version="1.0"?>
<updates>
    <update type="minor" displayVersion="31.0" appVersion="31.0" platformVersion="31.0" buildID="20140714151536" detailsURL="https://www.mozilla.org/de/firefox/31.0/releasenotes/">
        <patch type="complete" URL="http://download.mozilla.org/?product=Firefox-31.0-Complete&amp;os=win&amp;lang=de" hashFunction="sha512" hashValue="e472c908919f1e6a72ef05ba59b1b18dcdeae2c832c595d10a1c6812d5616ae2d05bbeb11ac587a9990166fc558f09ebedf7e9930cee3459848bb18d9ef22b67" size="39203770"/>
    </update>
</updates>%
(balrog)➜  balrog git:(localeslist) curl http://balrog.mozilla.dev:9000/update/3/Firefox/25.0/20120222174716/WINNT_x86-msvc/en-US/release/default/default/default/update.xml
<?xml version="1.0"?>
<updates>
    <update type="minor" displayVersion="31.0" appVersion="31.0" platformVersion="31.0" buildID="20140716183446" detailsURL="https://www.mozilla.org/en-US/firefox/31.0/releasenotes/">
        <patch type="complete" URL="http://download.mozilla.org/?product=Firefox-31.0-Complete&amp;os=win&amp;lang=en-US" hashFunction="sha512" hashValue="cd568f5155888e478bb38329292fc341759cd6d799ecb1f03c66d2b4c2e6b1aaa33eaf0322d97249b961e460e41dabb968effc7a3f1c539ee303aee9427eb696" size="39484922"/>
    </update>
</updates>%

Comment 7

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

Comment on attachment 8576835 [details] [diff] [review]
increase locale column size

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

::: auslib/db.py
@@ +707,5 @@
>  
> +    def _localeMatchesRule(self, ruleLocales, queryLocales):
> +        """Decides if a comma seperated list of locales in a rule matches an
> +        update request"""
> +        return self._matchesList(ruleLocales, queryLocales)

Nit, just the singular queryLocale instead of the plural.

@@ -713,5 @@
>             fallback, fallbackChannel should match the channel from the query."""
>          where=[
>              ((self.product==updateQuery['product']) | (self.product==None)) &
>              ((self.buildTarget==updateQuery['buildTarget']) | (self.buildTarget==None)) &
> -            ((self.locale==updateQuery['locale']) | (self.locale==None)) &

These queries get cheaper every time we add more functionality!

::: auslib/migrate/versions/007_longer_locales.py
@@ +7,5 @@
> +
> +def downgrade(migrate_engine):
> +    metadata = MetaData(bind=migrate_engine)
> +    Table('rules', metadata, autoload=True).c.locale.alter(type=String(10))
> +    Table('rules_history', metadata, autoload=True).c.locale.alter(type=String(10))

Do these work OK if you have longer strings in the DB, truncate or error ? Or is it only meant to work for short-term rollback where no rules have been added ?

Comment 8

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #7)
> @@ -713,5 @@
> >             fallback, fallbackChannel should match the channel from the query."""
> >          where=[
> >              ((self.product==updateQuery['product']) | (self.product==None)) &
> >              ((self.buildTarget==updateQuery['buildTarget']) | (self.buildTarget==None)) &
> > -            ((self.locale==updateQuery['locale']) | (self.locale==None)) &
> 
> These queries get cheaper every time we add more functionality!

:). If they do end up become a bottleneck or too expensive rule caching would help a lot. Some of them can probably be implemented in SQL if we try hard enough too.

> ::: auslib/migrate/versions/007_longer_locales.py
> @@ +7,5 @@
> > +
> > +def downgrade(migrate_engine):
> > +    metadata = MetaData(bind=migrate_engine)
> > +    Table('rules', metadata, autoload=True).c.locale.alter(type=String(10))
> > +    Table('rules_history', metadata, autoload=True).c.locale.alter(type=String(10))
> 
> Do these work OK if you have longer strings in the DB, truncate or error ?
> Or is it only meant to work for short-term rollback where no rules have been
> added ?

Looks like we truncate:
/usr/lib64/python2.6/site-packages/sqlalchemy/engine/default.py:325: Warning: Data truncated for column 'locale' at row 45
  cursor.execute(statement, parameters)

It really is just for short term rollback, but it would be nice if we could not silently truncate data. This is similar to bug 702271.

Comment 9

[[e-mail] (:bhearsum)]

Attachment: fix arg naming, don't truncate!

SQLAlchemy doesn't make it terribly easy to change the sql mode to something more restrictive. The underlying mysql driver supports setting it, but SQLAlchemy doesn't know how to forward to it along. I found this solution, which seems to work:
sqlalchemy.exc.DataError: (DataError) (1406, "Data too long for column 'locale' at row 45") '\nALTER TABLE rules CHANGE COLUMN locale locale VARCHAR(10)' ()

I left it off for everything except the database migration script. We probably need to do a lot of testing and maybe start catching these tracebacks if we want to enable it in production. That's better done elsewhere.

Comment 10

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

Comment on attachment 8577302 [details] [diff] [review]
fix arg naming, don't truncate!

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

Very sneaky that you only enable traditional mode for the schema changes.

Comment 11

[[github robot]]

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

https://github.com/mozilla/balrog/commit/d15f6cdfa23665c91021ff4b27e60a69e6b4b065
bug 945179: Match on a list of locales, rather than exact only. r=nthomas

Comment 12

[[e-mail] (:bhearsum)]

Comment on attachment 8577302 [details] [diff] [review]
fix arg naming, don't truncate!

(In reply to Nick Thomas [:nthomas] from comment #10)
> Comment on attachment 8577302 [details] [diff] [review]
> fix arg naming, don't truncate!
> 
> Review of attachment 8577302 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Very sneaky that you only enable traditional mode for the schema changes.

Yeah. If we didn't have the release channel in production I'd probably be OK flipping the switch everywhere. Feels too risky to do on a whim now though. I think bug 702271 will be a good place to do that.

Comment 13

[[e-mail] (:bhearsum)]

Planning to push this and bug 1141513 to prod at the same time, provided that's ready prior to RC.

Comment 14

[[e-mail] (:bhearsum)]

This was pushed to prod today, and is working. I did some testing of it when I set-up bug 1134471.