Run "DatabaseSetup" checks against all configured hosts

Summary:
Ref T10759. Currently, these checks run only against configured masters. Instead, check every host.

These checks also sort of cheat through restart during a recovery, when some hosts will be unreachable: they test for "disaster" by seeing if no masters are reachable, and just skip all the checks in that case.

This is bad for at least two reasons:

  - After recent changes, it is possible that //some// masters are dead but it's still OK to start. For example, "slowvote" may have no master, but everything else is reachable. We can safely run without slowvote.
  - It's possible to start during a disaster and miss important setup checks completely, since we skip them, get a clean bill of health, and never re-test them.

Instead:

  - Test each host individually.
  - Fundamental problems (lack of InnoDB, bad schema) are fatal on any host.
  - If we can't connect, raise it as a //warning// to make sure we check it later. If you start during a disaster, we still want to make sure that schemata are up to date if you later recover a host.

In particular, I'm going to add these checks soon:

  - Fatal if a "master" is replicating.
  - Fatal if a "replica" is not replicating.
  - Fatal if a database partition config differs from web partition config.
  - When we let a database off with a warning because it's down, and later upgrade it to a fatal because we discover it is broken after it comes up again, fatal everything. Currently, we keep running if we "discover" the presence of new fatals after surviving setup checks for the first time.

Test Plan:
  - Configured with multiple masters, intentionally broke one (simulating a disaster where one master is lost), saw Phabricator still startup.
  - Tested individual setup checks by intentionally breaking them.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10759

Differential Revision: https://secure.phabricator.com/D16902

src/aphront/configuration/AphrontApplicationConfiguration.php

@@ -89,7 +89,8 @@ abstract class AphrontApplicationConfiguration extends Phobject {
 
     if ($database_exception) {
       $issue = PhabricatorSetupIssue::newDatabaseConnectionIssue(
-        $database_exception);
+        $database_exception,
+        true);
       $response = PhabricatorSetupCheck::newIssueResponse($issue);
       return self::writeResponse($sink, $response);
     }

src/applications/config/check/PhabricatorDatabaseSetupCheck.php

@@ -43,26 +43,23 @@ final class PhabricatorDatabaseSetupCheck extends PhabricatorSetupCheck {
             $port));
     }
 
-    $masters = PhabricatorDatabaseRef::getMasterDatabaseRefs();
-    if (!$masters) {
-      // If we're implicitly in read-only mode during disaster recovery,
-      // don't bother with these setup checks.
-      return;
-    }
+    $refs = PhabricatorDatabaseRef::getActiveDatabaseRefs();
+    $refs = mpull($refs, null, 'getRefKey');
 
-    foreach ($masters as $master) {
-      if ($this->checkMasterDatabase($master)) {
-        break;
-      }
-    }
-  }
+    // Test if we can connect to each database first. If we can not connect
+    // to a particular database, we only raise a warning: this allows new web
+    // nodes to start during a disaster, when some databases may be correctly
+    // configured but not reachable.
 
-  private function checkMasterDatabase(PhabricatorDatabaseRef $master) {
-    $conn_raw = $master->newManagementConnection();
+    $connect_map = array();
+    $any_connection = false;
+    foreach ($refs as $ref_key => $ref) {
+      $conn_raw = $ref->newManagementConnection();
 
       try {
         queryfx($conn_raw, 'SELECT 1');
         $database_exception = null;
+        $any_connection = true;
       } catch (AphrontInvalidCredentialsQueryException $ex) {
         $database_exception = $ex;
       } catch (AphrontConnectionQueryException $ex) {
@@ -70,11 +67,35 @@ final class PhabricatorDatabaseSetupCheck extends PhabricatorSetupCheck {
       }
 
       if ($database_exception) {
-      $issue = PhabricatorSetupIssue::newDatabaseConnectionIssue(
-        $database_exception);
-      $this->addIssue($issue);
-      return true;
+        $connect_map[$ref_key] = $database_exception;
+        unset($refs[$ref_key]);
       }
+    }
+
+    if ($connect_map) {
+      // This is only a fatal error if we could not connect to anything. If
+      // possible, we still want to start if some database hosts can not be
+      // reached.
+      $is_fatal = !$any_connection;
+
+      foreach ($connect_map as $ref_key => $database_exception) {
+        $issue = PhabricatorSetupIssue::newDatabaseConnectionIssue(
+          $database_exception,
+          $is_fatal);
+        $this->addIssue($issue);
+      }
+    }
+
+    foreach ($refs as $ref_key => $ref) {
+      if ($this->executeRefChecks($ref)) {
+        return;
+      }
+    }
+  }
+
+  private function executeRefChecks(PhabricatorDatabaseRef $ref) {
+    $conn_raw = $ref->newManagementConnection();
+    $ref_key = $ref->getRefKey();
 
     $engines = queryfx_all($conn_raw, 'SHOW ENGINES');
     $engines = ipull($engines, 'Support', 'Engine');
@@ -82,17 +103,19 @@ final class PhabricatorDatabaseSetupCheck extends PhabricatorSetupCheck {
     $innodb = idx($engines, 'InnoDB');
     if ($innodb != 'YES' && $innodb != 'DEFAULT') {
       $message = pht(
-        "The 'InnoDB' engine is not available in MySQL. Enable InnoDB in ".
-        "your MySQL configuration.".
+        'The "InnoDB" engine is not available in MySQL (on host "%s"). '.
+        'Enable InnoDB in your MySQL configuration.'.
         "\n\n".
-        "(If you aleady created tables, MySQL incorrectly used some other ".
-        "engine to create them. You need to convert them or drop and ".
-        "reinitialize them.)");
+        '(If you aleady created tables, MySQL incorrectly used some other '.
+        'engine to create them. You need to convert them or drop and '.
+        'reinitialize them.)',
+        $ref_key);
 
       $this->newIssue('mysql.innodb')
         ->setName(pht('MySQL InnoDB Engine Not Available'))
         ->setMessage($message)
         ->setIsFatal(true);
+
       return true;
     }
 
@@ -103,18 +126,20 @@ final class PhabricatorDatabaseSetupCheck extends PhabricatorSetupCheck {
 
     if (empty($databases[$namespace.'_meta_data'])) {
       $message = pht(
-        "Run the storage upgrade script to setup Phabricator's database ".
-        "schema.");
+        'Run the storage upgrade script to setup databases (host "%s" has '.
+        'not been initialized).',
+        $ref_key);
 
       $this->newIssue('storage.upgrade')
         ->setName(pht('Setup MySQL Schema'))
         ->setMessage($message)
         ->setIsFatal(true)
         ->addCommand(hsprintf('<tt>phabricator/ $</tt> ./bin/storage upgrade'));
+
       return true;
     }
 
-    $conn_meta = $master->newApplicationConnection(
+    $conn_meta = $ref->newApplicationConnection(
       $namespace.'_meta_data');
 
     $applied = queryfx_all($conn_meta, 'SELECT patch FROM patch_status');
@@ -124,15 +149,19 @@ final class PhabricatorDatabaseSetupCheck extends PhabricatorSetupCheck {
     $diff = array_diff_key($all, $applied);
 
     if ($diff) {
+      $message = pht(
+        'Run the storage upgrade script to upgrade databases (host "%s" is '.
+        'out of date). Missing patches: %s.',
+        $ref_key,
+        implode(', ', array_keys($diff)));
+
       $this->newIssue('storage.patch')
         ->setName(pht('Upgrade MySQL Schema'))
-        ->setMessage(
-          pht(
-            "Run the storage upgrade script to upgrade Phabricator's ".
-            "database schema. Missing patches:<br />%s<br />",
-            phutil_implode_html(phutil_tag('br'), array_keys($diff))))
+        ->setIsFatal(true)
+        ->setMessage($message)
         ->addCommand(
           hsprintf('<tt>phabricator/ $</tt> ./bin/storage upgrade'));
+
       return true;
     }
   }

src/applications/config/issue/PhabricatorSetupIssue.php

@@ -21,7 +21,8 @@ final class PhabricatorSetupIssue extends Phobject {
   private $links;
 
   public static function newDatabaseConnectionIssue(
-    AphrontQueryException $ex) {
+    AphrontQueryException $ex,
+    $is_fatal) {
 
     $message = pht(
       "Unable to connect to MySQL!\n\n".
@@ -29,15 +30,21 @@ final class PhabricatorSetupIssue extends Phobject {
       "Make sure Phabricator and MySQL are correctly configured.",
       $ex->getMessage());
 
-    return id(new self())
+    $issue = id(new self())
       ->setIssueKey('mysql.connect')
       ->setName(pht('Can Not Connect to MySQL'))
       ->setMessage($message)
-      ->setIsFatal(true)
+      ->setIsFatal($is_fatal)
       ->addRelatedPhabricatorConfig('mysql.host')
       ->addRelatedPhabricatorConfig('mysql.port')
       ->addRelatedPhabricatorConfig('mysql.user')
       ->addRelatedPhabricatorConfig('mysql.pass');
+
+    if (PhabricatorEnv::getEnvConfig('cluster.databases')) {
+      $issue->addRelatedPhabricatorConfig('cluster.databases');
+    }
+
+    return $issue;
   }
 
   public function addCommand($command) {

src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDestroyWorkflow.php

@@ -23,8 +23,6 @@ final class PhabricatorStorageManagementDestroyWorkflow
   public function didExecute(PhutilArgumentParser $args) {
     $console = PhutilConsole::getConsole();
 
-    $api = $this->getSingleAPI();
-
     if (!$this->isDryRun() && !$this->isForce()) {
       $console->writeOut(
         phutil_console_wrap(
@@ -44,6 +42,8 @@ final class PhabricatorStorageManagementDestroyWorkflow
       }
     }
 
+    $apis = $this->getMasterAPIs();
+    foreach ($apis as $api) {
       $patches = $this->getPatches();
 
       if ($args->getArg('unittest-fixtures')) {
@@ -81,7 +81,12 @@ final class PhabricatorStorageManagementDestroyWorkflow
       }
 
       if (!$this->isDryRun()) {
-      $console->writeOut("%s\n", pht('Storage was destroyed.'));
+        $console->writeOut(
+          "%s\n",
+          pht(
+            'Storage on "%s" was destroyed.',
+            $api->getRef()->getRefKey()));
+      }
     }
 
     return 0;