Page MenuHomePhabricator

createExtensionTables.php makes it super-easy to accidentally create DB on wrong cluster
Open, Needs TriagePublic

Description

Problem

Without knowing (and without intending to), I created several DB tables on the main database cluster (testwikidatawiki) and a whole new database on the s3 cluster for wikidatawiki (which should not exist there at all). The createExtensionTables.php maintenance script did not warn me that was going to happen.

Root Cause

Shortly before enabling GrowthExperiments on Wikidata (T418115), I executed createExtensionTables.php to prepare the extension tables:

[urbanecm@deploy1003 ~]$ mwscript-k8s --comment=T418115 --dblist=wikidata --sal -f WikimediaMaintenance:createExtensionTables.php -- GrowthExperiments
⏳ Starting WikimediaMaintenance:createExtensionTables.php on Kubernetes as job mw-script.eqiad.0or1tu73 ...
🚀 Job is running.
📜 Streaming logs:
WikimediaMaintenance:createExtensionTables.php: Running on wikidata
-----------------------------------------------------------------
testwikidatawiki
-----------------------------------------------------------------
testwikidatawiki Creating GrowthExperiments tables...
testwikidatawiki   sourcing growthexperiments_link_recommendations.sql
testwikidatawiki   sourcing growthexperiments_link_submissions.sql
testwikidatawiki   sourcing growthexperiments_mentee_data.sql
testwikidatawiki   sourcing growthexperiments_mentor_mentee.sql
testwikidatawiki   sourcing growthexperiments_user_impact.sql
testwikidatawiki   done!
-----------------------------------------------------------------
wikidatawiki
-----------------------------------------------------------------
wikidatawiki Creating GrowthExperiments tables...
wikidatawiki   sourcing growthexperiments_link_recommendations.sql
wikidatawiki   sourcing growthexperiments_link_submissions.sql
wikidatawiki   sourcing growthexperiments_mentee_data.sql
wikidatawiki   sourcing growthexperiments_mentor_mentee.sql
wikidatawiki   sourcing growthexperiments_user_impact.sql
wikidatawiki   done!
[urbanecm@deploy1003 ~]$

GrowthExperiments stores its DB tables on the x1 per-wiki cluster. However, those DB tables were not actually created there:

wikiadmin2023@10.64.16.6(testwikidatawiki)> show tables;                                                                                                                                                                                                                     
+---------------------------------------+                                                                                                                                                                                                                                    
| Tables_in_testwikidatawiki            |                                                                                                                                                                                                                                    
+---------------------------------------+                                                                                                                                                                                                                                    
| echo_email_batch                      |                                                                                                                                                                                                                                    
| echo_event                            |                                                                                                                                                                                                                                    
| echo_notification                     |                                                                                                                                                                                                                                    
| echo_target_page                      |                                                                                                                                                                                                                                    
| mediamoderation_scan                  |                                                                                                                                                                                                                                    
| translate_cache                       |                                                                                                                                                                                                                                    
| translate_message_group_subscriptions |                                                                                                                                                                                                                                    
+---------------------------------------+                                                                                                                                                                                                                                    
7 rows in set (0.001 sec)

When I investigated, I found that those tables are on the main DB section instead:

wikiadmin2023@10.64.32.196(testwikidatawiki)> show tables like 'growth%';
+----------------------------------------+
| Tables_in_testwikidatawiki (growth%)   |
+----------------------------------------+
| growthexperiments_link_recommendations |
| growthexperiments_link_submissions     |
| growthexperiments_mentee_data          |
| growthexperiments_mentor_mentee        |
| growthexperiments_user_impact          |
+----------------------------------------+
5 rows in set (0.000 sec)

This happens because the maintenance script creates the tables in the following way:

$geLB = MediaWikiServices::getInstance()->getDBLoadBalancerFactory()
    ->getLoadBalancer( 'virtual-growthexperiments' );
[...]
$dbw = $geLB->getConnection( DB_PRIMARY );
$dbw->sourceFile( ... );

The virtual-growthexperiments virtual domain is defined in CommonSettings.php:

if ( $wmgUseGrowthExperiments ) {
    wfLoadExtension( 'GrowthExperiments' );
    $wgVirtualDomainsMapping['virtual-growthexperiments'] = [ 'cluster' => 'extension1', 'db' => false ];
}

and as a result, it is configured only when $wmgUseGrowthExperiments is true. However, when $wmgUseGrowthExperiments is true, I already need the DB tables to be created, to avoid "table not existing" error.

Event Timeline

I ended up having to run the maintenance script within mw-debug (where the virtual domain was already defined), and then proceed.

As far as I can see, to resolve the problem, we would need to define the virtual domain unconditionally. If the record is added to wgVirtualDomainsMapping unconditionally (even on wikis where the extension is not enabled), the maintenance script would see it and create the DB tables at the appropriate cluster. As of now, virtual domains are defined within the appropriate if ( $wmgUseXxx ) block, meaning this problem will impact anyone attempting to create DB tables using virtual domains.

I'm also wondering if we should somehow fail if we call DBLoadBalancerFactory::getLoadBalancer( $virtualDomain ) when $virtualDomain does not match any. Currently, this syntax indicates "use the main cluster", but maybe we can signal that in another way (explicitly setting it to null, perhaps), and the no-value-set scenario would indicate an error.

I'm curious what @Ladsgroup thinks about this problem.

Interestingly, the script (somehow) decided to create the wikidatawiki tables on s3 wikidatawiki, not on s8 wikidatawiki (as I would expect when virtualdomain points nowhere):

wikiadmin2023@db1223(wikidatawiki)> show tables;
+----------------------------------------+
| Tables_in_wikidatawiki                 |
+----------------------------------------+
| growthexperiments_link_recommendations |
| growthexperiments_link_submissions     |
| growthexperiments_mentee_data          |
| growthexperiments_mentor_mentee        |
| growthexperiments_user_impact          |
+----------------------------------------+
5 rows in set (0.001 sec)

wikiadmin2023@db1223(wikidatawiki)>

The good question is...why did the script do this?

This is how I located them:

www-data@mw-experimental:/$ grep wikidata /srv/mediawiki/dblists/growthexperiments.dblist # ensure GrowthExperiments is not enabled on wikidata according to mw-experimental
www-data@mw-experimental:/$ php /srv/mediawiki/multiversion/MWScript.php shell.php --wiki=wikidatawiki
Psy Shell v0.12.20 (PHP 8.3.31 — cli) by Justin Hileman
> $lbf = \MediaWiki\MediaWikiServices::getInstance()->getDBLoadBalancerFactory()
= Wikimedia\Rdbms\LBFactoryMulti {#1645}

> $lbf->getLoadBalancer( 'virtual-growthexperiments' )->getConnection(DB_PRIMARY)->getServer()
= "10.64.48.160"

> $lbf->getLoadBalancer()->getConnection(DB_PRIMARY)->getServer()
= "10.64.134.10"
[urbanecm@deploy1003 ~]$ host 10.64.48.160 # s3 master
160.48.64.10.in-addr.arpa domain name pointer db1223.eqiad.wmnet.
[urbanecm@deploy1003 ~]$ host 10.64.134.10 # s8 master
10.134.64.10.in-addr.arpa domain name pointer db1193.eqiad.wmnet.
[urbanecm@deploy1003 ~]$

The accidental creation of wikidatawiki at s3 makes me think... Should the script even attempt to run CREATE DATABASE IF NOT EXISTS? I can't think of a scenario where an x1 database wouldn't already exist... addWiki.php should handle provisioning it.

So, there seems to be at least two action items from this:

  • Declare virtual domains unconditionally: move $wgVirtualDomainsMapping declarations in CommonSettings.php outside of their respective $wmgUseXxx blocks
  • Remove CREATE DATABASE commands from createExtensionTables: the tables creation script should create only tables; there should be no situation in which the x1 database (or any other subdatabase) doesn't exist already
  • Optionally, ensure undeclared vritual domain results in error in DB load balancer factory: if we introduce a special value (e.g. null) to represent "use the main DB", we can detect this sort of misconfiguration automatically, and abort rather than miscreating

I'm also curious why $lbf->getLoadBalancer( $virtualDomain ) with an unrecognized $virtualDomain returns a pointer to s3. I would expect the main section (or an error), but definitely not s3...

I'm also curious why $lbf->getLoadBalancer( $virtualDomain ) with an unrecognized $virtualDomain returns a pointer to s3. I would expect the main section (or an error), but definitely not s3...

Answering that mystery: LBFactory::getLoadBalancer() only runs virtual domain handling if the string is in virtualDomains. When the extension is not yet installed, its virtual domain is both unmapped and undeclared, so it is NOT a part of virtualDomains. Subsequently, virtual domain handling doesn't run, and virtual-growthexperiments is passed to LBFactory::getMainLB() verbatim.

This means it eventually got all the way down to DatabaseDomain::newFromId(), where it was treated as the database + prefix tuple, representing DB called virtual with a prefix of growthexperiments. Since the virtual DB does not have its own section defined in the WMF config (as it is not expected to exist), it got directed to s3 and not s8.

Fortunately (?), the faulty DB name of virtual only lived in getMainLB(). In getConnection(), the right DB name was passed again, resulting in a wikidatawiki on s3.

  • Declare virtual domains unconditionally: move $wgVirtualDomainsMapping declarations in CommonSettings.php outside of their respective $wmgUseXxx blocks

Actually, this wouldn't be enough, as it wouldn't register the virtual domain, meaning it would still skip the virtual domain handling logic, resulting in pretty much the same problem. Virtual domain registration comes from DatabaseVirtualDomains in extension.json, and it doesn't have a corresponding config variable, meaning we cannot pre-register the virtual domain in config.

We could change the if from in_array( $domain, $this->virtualDomains ) to in_array( $domain, $this->virtualDomains ) || array_key_exists( $domain, $this->virtualDomainsMapping ), but that would render the virtual domain registration essentially meaningless (unregistered & mapped virtual domains would behave the same as registered & mapped). Maybe this is what we want after all? But it was likely introduced there for a reason.

This is probably closer to our Rdbms lib than to WikimediaMaintenance.

Another possible action item here: include the DB server where the creation happened in the output. While this wouldn't prevent the issue from happening, it would allow me to double check it got created at the expected place.

The accidental creation of wikidatawiki at s3 makes me think... Should the script even attempt to run CREATE DATABASE IF NOT EXISTS? I can't think of a scenario where an x1 database wouldn't already exist... addWiki.php should handle provisioning it.

So, there seems to be at least two action items from this:

  • Declare virtual domains unconditionally: move $wgVirtualDomainsMapping declarations in CommonSettings.php outside of their respective $wmgUseXxx blocks

Yes. Totally. For cases that it doesn't change per-wiki (except being installed or not), I honestly think we should collect all of them somewhere central in CS.php instead of being scattered.

  • Remove CREATE DATABASE commands from createExtensionTables: the tables creation script should create only tables; there should be no situation in which the x1 database (or any other subdatabase) doesn't exist already

Yes. I think addWiki.php creates the database on x1 during creation. Worth double checking but if that's the case, there shouldn't be any code that creates the db. Creating the db is not the job of the schema change. We never do that.

  • Optionally, ensure undeclared vritual domain results in error in DB load balancer factory: if we introduce a special value (e.g. null) to represent "use the main DB", we can detect this sort of misconfiguration automatically, and abort rather than miscreating

I don't think it's worth the effort but an okay hardening. My personal take is that for example for the sake of tests and ease of configuration in third parties, the default behavior should be that if the virtual domain is not configured, fall back to the main db. Of course if the virtual domain is not registered, it's a different story but I can easily imagine cases in gray areas (the extension is not loaded yet, etc.)

I'm also curious why $lbf->getLoadBalancer( $virtualDomain ) with an unrecognized $virtualDomain returns a pointer to s3. I would expect the main section (or an error), but definitely not s3...

Maybe because of testwikidatawiki? that's on s3

Thanks for taking a look, @Ladsgroup! However, note T429302#12022937 as well (which also explains the last point, or at least my understanding of it). As of now, purely declaring virtual domains unconditionally would not help (we only look at the mappings if the virtual domain is registered in at an extension.json via DatabaseVirtualDomains, which is not the case if the extension is not loaded).

What would you think about essentially abandoning the concept of registering virtual domains and relying solely on config?

The problem is on how to distinguish between a foreign domain (let's say you want to query commonswiki db from a client wiki to see if the file exist or not which should error out if not configured) and actual virtual domain (which if not configured, should fall back to the main db).

That being said, when I was building virtual domains, we didn't have a guideline of how to name them and then after going back and forth, we settled on it starting with 'virtual-'. And checking all of registered virtual domains, they all start with virtual-:

> var_dump( \MediaWiki\MediaWikiServices::getInstance()->getDBLoadBalancerFactoryConfig()->getConfig()['virtualDomains'] );
array(27) {
  [0]=>
  string(20) "virtual-urlshortener"
  [1]=>
  string(22) "virtual-globalblocking"
  [2]=>
  string(19) "virtual-centralauth"
  [3]=>
  string(23) "virtual-apifeatureusage"
  [4]=>
  string(21) "virtual-centralnotice"
  [5]=>
  string(19) "virtual-globalusage"
  [6]=>
  string(21) "virtual-bouncehandler"
  [7]=>
  string(17) "virtual-translate"
  [8]=>
  string(19) "virtual-LoginNotify"
  [9]=>
  string(10) "virtual-cx"
  [10]=>
  string(22) "virtual-wikibase-terms"
  [11]=>
  string(23) "virtual-globaljsonlinks"
  [12]=>
  string(13) "virtual-oauth"
  [13]=>
  string(16) "virtual-oathauth"
  [14]=>
  string(23) "virtual-mediamoderation"
  [15]=>
  string(24) "virtual-checkuser-global"
  [16]=>
  string(17) "virtual-checkuser"
  [17]=>
  string(20) "virtual-readinglists"
  [18]=>
  string(25) "virtual-growthexperiments"
  [19]=>
  string(17) "virtual-awstorage"
  [20]=>
  string(27) "virtual-wikifunctions-usage"
  [21]=>
  string(22) "virtual-campaignevents"
  [22]=>
  string(17) "virtual-dashboard"
  [23]=>
  string(20) "virtual-botpasswords"
  [24]=>
  string(17) "virtual-interwiki"
  [25]=>
  string(31) "virtual-interwiki-interlanguage"
  [26]=>
  string(13) "virtual-links"
}

We can rely on that instead of forcing people to register a virtual domain.

Sounds good, thanks for all the feedback. I logged subtasks for the action items we agreed on.

Optionally, ensure undeclared vritual domain results in error in DB load balancer factory: if we introduce a special value (e.g. null) to represent "use the main DB", we can detect this sort of misconfiguration automatically, and abort rather than miscreating

I don't think it's worth the effort but an okay hardening. My personal take is that for example for the sake of tests and ease of configuration in third parties, the default behavior should be that if the virtual domain is not configured, fall back to the main db. Of course if the virtual domain is not registered, it's a different story but I can easily imagine cases in gray areas (the extension is not loaded yet, etc.)

On second thought, this is not only not worth the effort, but also dangerous. If a wiki does not actually have more than one DB, it would natutrally not configure anything in its config, and we would want to fail back to the main DB. This was intended as the last possible defense anyway – since it is not actually possible, we would need to just rely on the config being available (T434330) and actually used even w/o the extension installed (T434329).