Page MenuHomePhabricator

Write an `--output <file>` mode for `storage dump` which can gzip
Closed, ResolvedPublic

Description

We're doing some real stupid garbage in the Phacility deployment scripts right now to get a safe database dump:

// NOTE: We're using `set -o pipefail` so this command fails if any
// part (in particular, the `bin/storage dump`) fails. This is specific
// to bash and the default shell is dash under Ubuntu, so we need to
// invoke bash explicitly.

$command = csprintf(
  'set -o pipefail && PHACILITY_INSTANCE=%s %s dump | gzip > %s',
  $instance,
  $this->getPath('lib/phabricator/bin/storage'),
  $database_path);

$t_start = microtime(true);
try {
  execx('/bin/bash -c %s', $command);
} catch (CommandException $ex) {
  try {
    // Try to remove the possibly-empty but certainly-invalid file, if
    // it exists.
    Filesystem::remove($database_path);
  } catch (Exception $ex) {
    // If anything went wrong, ignore the cleanup failure; we just want
    // to raise the root cause.
  }
  throw $ex;
}
$t_end = microtime(true);

This would be enormously cleaner (and more portable) as:

bin/storage dump --gzip --outfile <filename>

One tricky bit is that we should ideally handle this in a stream-oriented way -- not by buffering the entire dump into memory, gzipping it, and then writing it to disk. This may be a little tricky to do safely since I don't think we do anything similar elsewhere yet.

Revisions and Commits

Restricted Differential Revision
Restricted Differential Revision

Event Timeline

epriestley raised the priority of this task from to Wishlist.
epriestley updated the task description. (Show Details)
epriestley added a subscriber: epriestley.

I'm running into a need for this to make automated export reliable, too, and don't really want to put a second copy of set -o pipefail in the codebase.

epriestley raised the priority of this task from Wishlist to Normal.
epriestley added a revision: Restricted Differential Revision.Aug 17 2016, 1:35 PM
epriestley added a revision: Restricted Differential Revision.Aug 17 2016, 3:12 PM
epriestley added a commit: Restricted Diffusion Commit.Aug 17 2016, 4:03 PM
epriestley added a commit: Restricted Diffusion Commit.

This seems to be working, I think the last step is replacing the original call in the backup workflow.

rCORE9d89c996844c4599afc1cec9454a71dd80a69c6c threw out the -o pipefail nonsense in CoreHostBackupWorkflow.