Skip to content

Commit 37b8ec5

Browse files
author
epriestley
committed
Provide an "--output" mode for "bin/storage dump" with better error handling
Summary: Ref T6996. If you do this kind of thing in the shell, you don't get a good error by default if the `dump` command fails: ``` $ bin/storage dump | gzip > output.sql.gz ``` This can be worked around with some elaborate bash tricks, but they're really clunky and uninintuitive. We also need to do this in several places (while writing backups; while performing exports), and I don't want to copy clunky bash tricks all over the codebase. Instead, provide `--output` and `--compress` flags which just do this processing inside `bin/storage dump`. It will fail appropriately if any of the underlying operations fail. This also makes the write a little safer (refuses to overwrite) and the code more reusable. Test Plan: - Did three dumps, with no flags, `--output`, and `--output --compress`. - Verified all three took similar amounts of time and were identical except for "Date Exported" timestamps in comments (except that the compressed one was compressed). - Used `gunzip` to examine the compressed one, verified it was really compressed. - Faked a write error, saw properly command behavior (clean up file + exit with error). Reviewers: chad Reviewed By: chad Maniphest Tasks: T6996 Differential Revision: https://secure.phabricator.com/D16407
1 parent f659b87 commit 37b8ec5

File tree

1 file changed

+125
-3
lines changed

1 file changed

+125
-3
lines changed

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

Lines changed: 125 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,26 @@ protected function didConstruct() {
1616
'Add __--master-data__ to the __mysqldump__ command, '.
1717
'generating a CHANGE MASTER statement in the output.'),
1818
),
19+
array(
20+
'name' => 'output',
21+
'param' => 'file',
22+
'help' => pht(
23+
'Write output directly to disk. This handles errors better '.
24+
'than using pipes. Use with __--compress__ to gzip the '.
25+
'output.'),
26+
),
27+
array(
28+
'name' => 'compress',
29+
'help' => pht(
30+
'With __--output__, write a compressed file to disk instead '.
31+
'of a plaintext file.'),
32+
),
33+
array(
34+
'name' => 'overwrite',
35+
'help' => pht(
36+
'With __--output__, overwrite the output file if it already '.
37+
'exists.'),
38+
),
1939
));
2040
}
2141

@@ -55,6 +75,38 @@ public function didExecute(PhutilArgumentParser $args) {
5575
}
5676
}
5777

78+
$output_file = $args->getArg('output');
79+
$is_compress = $args->getArg('compress');
80+
$is_overwrite = $args->getArg('overwrite');
81+
82+
if ($is_compress) {
83+
if ($output_file === null) {
84+
throw new PhutilArgumentUsageException(
85+
pht(
86+
'The "--compress" flag can only be used alongside "--output".'));
87+
}
88+
}
89+
90+
if ($is_overwrite) {
91+
if ($output_file === null) {
92+
throw new PhutilArgumentUsageException(
93+
pht(
94+
'The "--overwrite" flag can only be used alongside "--output".'));
95+
}
96+
}
97+
98+
if ($output_file !== null) {
99+
if (Filesystem::pathExists($output_file)) {
100+
if (!$is_overwrite) {
101+
throw new PhutilArgumentUsageException(
102+
pht(
103+
'Output file "%s" already exists. Use "--overwrite" '.
104+
'to overwrite.',
105+
$output_file));
106+
}
107+
}
108+
}
109+
58110
$argv = array();
59111
$argv[] = '--hex-blob';
60112
$argv[] = '--single-transaction';
@@ -79,13 +131,83 @@ public function didExecute(PhutilArgumentParser $args) {
79131
$argv[] = $database;
80132
}
81133

134+
82135
if ($has_password) {
83-
$err = phutil_passthru('mysqldump -p%P %Ls', $password, $argv);
136+
$command = csprintf('mysqldump -p%P %Ls', $password, $argv);
137+
} else {
138+
$command = csprintf('mysqldump %Ls', $argv);
139+
}
140+
141+
// If we aren't writing to a file, just passthru the command.
142+
if ($output_file === null) {
143+
return phutil_passthru('%C', $command);
144+
}
145+
146+
// If we are writing to a file, stream the command output to disk. This
147+
// mode makes sure the whole command fails if there's an error (commonly,
148+
// a full disk). See T6996 for discussion.
149+
150+
if ($is_compress) {
151+
$file = gzopen($output_file, 'wb');
84152
} else {
85-
$err = phutil_passthru('mysqldump %Ls', $argv);
153+
$file = fopen($output_file, 'wb');
154+
}
155+
156+
if (!$file) {
157+
throw new Exception(
158+
pht(
159+
'Failed to open file "%s" for writing.',
160+
$file));
161+
}
162+
163+
$future = new ExecFuture('%C', $command);
164+
165+
$lines = new LinesOfALargeExecFuture($future);
166+
167+
try {
168+
foreach ($lines as $line) {
169+
$line = $line."\n";
170+
if ($is_compress) {
171+
$ok = gzwrite($file, $line);
172+
} else {
173+
$ok = fwrite($file, $line);
174+
}
175+
176+
if ($ok !== strlen($line)) {
177+
throw new Exception(
178+
pht(
179+
'Failed to write %d byte(s) to file "%s".',
180+
new PhutilNumber(strlen($line)),
181+
$output_file));
182+
}
183+
}
184+
185+
if ($is_compress) {
186+
$ok = gzclose($file);
187+
} else {
188+
$ok = fclose($file);
189+
}
190+
191+
if ($ok !== true) {
192+
throw new Exception(
193+
pht(
194+
'Failed to close file "%s".',
195+
$output_file));
196+
}
197+
} catch (Exception $ex) {
198+
// If we might have written a partial file to disk, try to remove it so
199+
// we don't leave any confusing artifacts laying around.
200+
201+
try {
202+
Filesystem::remove($output_file);
203+
} catch (Exception $ex) {
204+
// Ignore any errors we hit.
205+
}
206+
207+
throw $ex;
86208
}
87209

88-
return $err;
210+
return 0;
89211
}
90212

91213
}

0 commit comments

Comments
 (0)