Backup improvements. #2263

alainjobart · 2016-11-16T23:45:27Z

Supports new features:

backup files can now go through transforms. Can be used to encrypt / decrypt backups on the fly. The compression is now also optional.
if backup fails to write to backupstorage, we still restore the state of the MySQL daemon, instead of leaving it in a broken state.
added context to backupstorage API.

alainjobart · 2016-11-17T17:33:08Z

This is ready for review. I will squash everything before merge, but maybe for review individual commits are better.

@michael-berlin you're the primary review, @enisoc this is a FYI, feel free to review.

enisoc · 2016-11-17T19:15:45Z

go/vt/mysqlctl/backup.go

+		filterOrTee = tee
+	}
+
+	// Create the gzip compression filter


Now that it's possible to plug in their own filter, users may want the option to disable built-in gzip compression. For example, maybe they want to use the filter hook to use a different compression algorithm, or their filter produces encrypted output that will only get bigger if sent through gzip.

mberlin-bot · 2016-11-17T22:31:07Z

Overall the change looks good.

I left several comments around naming the feature and how to enable it via a flag.

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, 3 of 3 files at r4, 3 of 3 files at r5, 8 of 8 files at r6.
Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

bootstrap.sh, line 138 at r6 (raw file):

vthook/backup_filter

Can you please include the word "test" in the filename?

Otherwise it's misleading and people would think that this file is the default backup filter.

go/vt/hook/hook.go, line 145 at r3 (raw file):

exits

typo: exit

go/vt/hook/hook.go, line 164 at r3 (raw file):

a wait() method

Optional: You could also define this as function type.

go/vt/hook/hook.go, line 165 at r3 (raw file):

Wait()

I prefer "cmd.Wait()" to avoid confusing it with the previous "wait()" mentioning.

go/vt/hook/hook.go, line 175 at r3 (raw file):

wc

This probably stands for "WriteCloser".

What about naming this and the writer "in" and "out" to better describe the data flow?

go/vt/hook/hook.go, line 161 at r4 (raw file):

Filter

I think "filter" is the wrong word and we should name it differently (throughout the codebase).

At no point data is getting filtered i.e. no backup data is getting omitted.

Instead, it gets transformed or processed e.g. compressed or encrypted.

go/vt/mysqlctl/backup.go, line 200 at r1 (raw file):

// Take the backup, and either AbortBackup or EndBackup.

Is this new error handling covered by any test? It would be nice to have?

go/vt/mysqlctl/backup.go, line 222 at r1 (raw file):

useable

nit: s/useable/usable/ to be consistent

go/vt/mysqlctl/backup.go, line 327 at r1 (raw file):

findFilesTobackup

nit: Can you please capitalize the "b" in "backup"?

go/vt/mysqlctl/backup.go, line 423 at r4 (raw file):

hwc

Same comment as in the other file.

I think his variable name is not intuitive.

Hook Write Closer?

This is more the transformed output.

go/vt/mysqlctl/backup.go, line 423 at r4 (raw file):

hclose

"close" in the variable name is confusing. I would expect "wait" here instead e.g. "hwait"?

go/vt/mysqlctl/backup.go, line 424 at r4 (raw file):

  h.ExtraEnv = hookExtraEnv
  hwc, hclose, status, err := h.ExecuteAsWriteFilter(tee)
  var filterOrTee io.Writer

Do you need to preserve the original value of tee?

Why not just override it?

filterOrTee is very long. Is it really relevant if the output is transformed/processed or not?

go/vt/mysqlctl/backup.go, line 62 at r5 (raw file):

  // and when decoding a backup, it is read from the manifest,
  // and sent to the filter hook again.
  backupStorageFilter = flag.String("backup_storage_filter", "", "if set, the backup_filter hook is used for processing backup files, with this parameter as 'filter'.")

I'm not in favor of this flag.

Instead, I think we should provide a flag where the user can specify the backup transformation/processing hook which should be used.

If somebody wants to run Vitess with two different hooks (like you do in this test), they would do so at the hook level. We don't need the "--filter" flag in that case.

Note that in the future we could provide multiple default implementations for transformation e.g. one for gzip. Users could then select which one they want to use by specifying the flag.

When we have such a hook flag, you could also the bogus value "-backup_storage_filter=enabled" in the test below.

go/vt/mysqlctl/backup.go, line 438 at r5 (raw file):

backup_filter

I would prefer if we don't hardcode this and instead provide a flag for that.

go/vt/mysqlctl/backupstorage/interface.go, line 73 at r6 (raw file):

remembered

"stored" ?

test/vthook-backup_filter, line 13 at r4 (raw file):

# Any error is displayed to stderr, and triggers an 'exit 1'.

while [[ $# -gt 1 ]]

Please move "do" on the same line:

while [[ $# -gt 1 ]]; do

test/vthook-backup_filter, line 15 at r4 (raw file):

while [[ $# -gt 1 ]]
do
key="$1"

Please indent the while block by two spaces.

Comments from Reviewable

alainjobart-bot · 2016-11-18T18:41:04Z

Review status: 0 of 13 files reviewed at latest revision, 18 unresolved discussions.

bootstrap.sh, line 138 at r6 (raw file):