From 5a0b7398cabd8b3bfe0b8b068bc93e87d75b71f4 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 14 Apr 2016 11:34:16 -0700
Subject: [PATCH] Give bin/storage some replica-aware options

Summary:
Fixes T10758.

  - Adds a "--host" flag. If you specify this, we read your cluster config. This lets you dump from a replica.
  - Adds a "--for-replica" flag to `storage dump`. This makes `mysqldump` include a `CHANGE MASTER ...` statement in the output, which is useful when setting up a replica for the first time.

Test Plan:
  - Dumped master and replica cluster databases.
  - Dumped non-cluster databases.
  - Ran various other commands (help, status, etc).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10758

Differential Revision: https://secure.phabricator.com/D15714
---
 scripts/sql/manage_storage.php                | 41 +++++++++++--
 .../user/cluster/cluster_databases.diviner    | 11 +++-
 ...abricatorStorageManagementDumpWorkflow.php | 57 ++++++++++++++-----
 3 files changed, 87 insertions(+), 22 deletions(-)

diff --git a/scripts/sql/manage_storage.php b/scripts/sql/manage_storage.php
index 82ba9c9333..7ad94277f7 100755
--- a/scripts/sql/manage_storage.php
+++ b/scripts/sql/manage_storage.php
@@ -30,6 +30,12 @@ try {
         'help'    => pht(
           'Do not prompt before performing dangerous operations.'),
       ),
+      array(
+        'name' => 'host',
+        'param' => 'hostname',
+        'help' => pht(
+          'Connect to __host__ instead of the default host.'),
+      ),
       array(
         'name'    => 'user',
         'short'   => 'u',
@@ -75,10 +81,37 @@ try {
 // First, test that the Phabricator configuration is set up correctly. After
 // we know this works we'll test any administrative credentials specifically.
 
-$ref = PhabricatorDatabaseRef::getMasterDatabaseRef();
-if (!$ref) {
-  throw new Exception(
-    pht('No database master is configured.'));
+$host = $args->getArg('host');
+if (strlen($host)) {
+  $ref = null;
+
+  $refs = PhabricatorDatabaseRef::getLiveRefs();
+
+  // Include the master in case the user is just specifying a redundant
+  // "--host" flag for no reason and does not actually have a database
+  // cluster configured.
+  $refs[] = PhabricatorDatabaseRef::getMasterDatabaseRef();
+
+  foreach ($refs as $possible_ref) {
+    if ($possible_ref->getHost() == $host) {
+      $ref = $possible_ref;
+      break;
+    }
+  }
+
+  if (!$ref) {
+    throw new PhutilArgumentUsageException(
+      pht(
+        'There is no configured database on host "%s". This command can '.
+        'only interact with configured databases.',
+        $host));
+  }
+} else {
+  $ref = PhabricatorDatabaseRef::getMasterDatabaseRef();
+  if (!$ref) {
+    throw new Exception(
+      pht('No database master is configured.'));
+  }
 }
 
 $default_user = $ref->getUser();
diff --git a/src/docs/user/cluster/cluster_databases.diviner b/src/docs/user/cluster/cluster_databases.diviner
index 7e9f2c8a03..cfe684c601 100644
--- a/src/docs/user/cluster/cluster_databases.diviner
+++ b/src/docs/user/cluster/cluster_databases.diviner
@@ -296,10 +296,15 @@ safely pull dumps from a replica instead of the master. This operation can
 be slow, so offloading it to a replica can make the performance of the master
 more consistent.
 
-To dump from a replica, wait for this TODO to be resolved and then do whatever
-it says to do:
+To dump from a replica, you can use `bin/storage dump --host <host>` to
+control which host the command connects to. (You may still want to execute
+this command //from// that host, to avoid sending the whole dump over the
+network).
 
-TODO: Make `bin/storage dump` replica-aware. See T10758.
+With the `--for-replica` flag, the `bin/storage dump` command creates dumps
+with `--dump-slave`, which includes a `CHANGE MASTER` statement in the output.
+This may be helpful when initially setting up new replicas, as it can make it
+easier to change the binlog coordinates to the correct position for the dump.
 
 With recent versions of MySQL, it is also possible to configure a //delayed//
 replica which intentionally lags behind the master (say, by 12 hours). In the
diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDumpWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDumpWorkflow.php
index 38ce117a08..243aaecbba 100644
--- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDumpWorkflow.php
+++ b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementDumpWorkflow.php
@@ -7,11 +7,20 @@ final class PhabricatorStorageManagementDumpWorkflow
     $this
       ->setName('dump')
       ->setExamples('**dump** [__options__]')
-      ->setSynopsis(pht('Dump all data in storage to stdout.'));
+      ->setSynopsis(pht('Dump all data in storage to stdout.'))
+      ->setArguments(
+        array(
+          array(
+            'name' => 'for-replica',
+            'help' => pht(
+              'Add __--dump-slave__ to the __mysqldump__ command, '.
+              'generating a CHANGE MASTER statement in the output.'),
+          ),
+        ));
   }
 
   public function didExecute(PhutilArgumentParser $args) {
-    $api     = $this->getAPI();
+    $api = $this->getAPI();
     $patches = $this->getPatches();
 
     $console = PhutilConsole::getConsole();
@@ -33,26 +42,44 @@ final class PhabricatorStorageManagementDumpWorkflow
 
     list($host, $port) = $this->getBareHostAndPort($api->getHost());
 
-    $flag_password = '';
     $password = $api->getPassword();
     if ($password) {
       if (strlen($password->openEnvelope())) {
-        $flag_password = csprintf('-p%P', $password);
+        $has_password = true;
       }
     }
 
-    $flag_port = $port
-      ? csprintf('--port %d', $port)
-      : '';
+    $argv = array();
+    $argv[] = '--hex-blob';
+    $argv[] = '--single-transaction';
+    $argv[] = '--default-character-set=utf8';
 
-    return phutil_passthru(
-      'mysqldump --hex-blob --single-transaction --default-character-set=utf8 '.
-      '-u %s %C -h %s %C --databases %Ls',
-      $api->getUser(),
-      $flag_password,
-      $host,
-      $flag_port,
-      $databases);
+    if ($args->getArg('for-replica')) {
+      $argv[] = '--dump-slave';
+    }
+
+    $argv[] = '-u';
+    $argv[] = $api->getUser();
+    $argv[] = '-h';
+    $argv[] = $host;
+
+    if ($port) {
+      $argv[] = '--port';
+      $argv[] = $port;
+    }
+
+    $argv[] = '--databases';
+    foreach ($databases as $database) {
+      $argv[] = $database;
+    }
+
+    if ($has_password) {
+      $err = phutil_passthru('mysqldump -p%P %Ls', $password, $argv);
+    } else {
+      $err = phutil_passthru('mysqldump %Ls', $argv);
+    }
+
+    return $err;
   }
 
 }