Open Bug 815343 Opened 12 years ago

disabling a product in bounceradmin does not stop it from returning 302's

Categories

(Webtools :: Bouncer, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

People

(Reporter: nmaul, Unassigned)

References

()

Details

I believe this is fallout from today's bouncer push, which deployed the logic for the new "healthy" column in mirror_location_mirror_map.

When you disable a product in bounceradmin, it does not stop working as expected. Instead, it continues to work based on the last status entered into mirror_location_mirror_map. I'm worried this might happen when disabling mirrors, too, but I can't as easily verify that at the moment.

As example products, I disabled 'Nagios-Test' and 'Firefox-Releases-Directory', both of which were active (but not actually used, making them good to test with). After disabling, they both still return a 302 when queried:

curl -v 'https://download.mozilla.org/?product=Firefox-Releases-Directory&os=none&lang=en-US'
curl -v 'https://download.mozilla.org/?product=Nagios-Test&os=none&lang=en-US'

I believe this is because they're still present and "active" in the mirror_location_mirror_map table:

mysql> select mirror_id,L.id,L.product_id,O.name,L.path,active,healthy from mirror_location_mirror_map MLMP inner join mirror_locations L on MLMP.location_id = L.id inner join mirror_os O on L.os_id = O.id where active=1 and healthy=0 order by mirror_id;
+-----------+------+------------+------+--------------------+--------+---------+
| mirror_id | id   | product_id | name | path               | active | healthy |
+-----------+------+------------+------+--------------------+--------+---------+
|       259 | 4137 |       1023 | none | /firefox/releases/ |      1 |       0 |
|       259 | 4148 |       1027 | none | /firefox/releases/ |      1 |       0 |
|       486 | 4137 |       1023 | none | /firefox/releases/ |      1 |       0 |
|       486 | 4148 |       1027 | none | /firefox/releases/ |      1 |       0 |
|       492 | 4148 |       1027 | none | /firefox/releases/ |      1 |       0 |
|       492 | 4137 |       1023 | none | /firefox/releases/ |      1 |       0 |
|       493 | 4137 |       1023 | none | /firefox/releases/ |      1 |       0 |
|       493 | 4148 |       1027 | none | /firefox/releases/ |      1 |       0 |
|       494 | 4148 |       1027 | none | /firefox/releases/ |      1 |       0 |
|       494 | 4137 |       1023 | none | /firefox/releases/ |      1 |       0 |
+-----------+------+------------+------+--------------------+--------+---------+
10 rows in set (2.33 sec)

(1023 is Nagios-Test, 1027 is Firefox-Releases-Directory)

If a product is disabled in bounceradmin (that is: the "Active" box is unchecked), sentry will never check it. This means sentry will never change any rows in this table for that product... so the last checked status "wins", forever.

A couple fixes come to mind... both could be implemented singly or together:

1) The php app could check that a requested product is active before serving it. This seems simple. One obvious downside: this is an extra DB hit (or at least a join) on every hit to download.mozilla.org.

2) When a product is deactivated in bounceradmin, all corresponding rows in mirror_location_mirror_map could be removed. This has the advantage of removing "cruft" from this table, and imposes no cost on hits to d.m.o. 1 obvious downside: re-activating a product means it's not "alive" until sentry checks it and says it's okay. This might actually be a good thing.

Another possibility:

3) Sentry could perform a "cleanup" task on each invocation, scanning the mirror_location_mirror_map table and removing cruft like this... any rows corresponding to a disabled product or mirror could be DELETE'd. This has the advantage of being universal- if you change something in the DB directly (not through bounceradmin), sentry would clean up after you. There are probably tens of thousands of cruft lines in there already (2.6M rows currently).


Thoughts on this?
You need to log in before you can comment on or make changes to this bug.