From 56af1305e79f30a628829f63080d762d6c90792d Mon Sep 17 00:00:00 2001 From: Rameez Sajwani Date: Mon, 11 Jul 2022 10:37:55 -0700 Subject: [PATCH 01/18] Remove built-in decompression flag Signed-off-by: Rameez Sajwani --- .../backup/mysqlctld/backup_mysqlctld_test.go | 1 - .../backup/vtctlbackup/backup_test.go | 1 - .../backup/xtrabackup/xtrabackup_test.go | 1 - .../xtrabackup_stream_test.go | 1 - go/vt/mysqlctl/builtinbackupengine.go | 28 +++++++-------- go/vt/mysqlctl/compression.go | 36 +++---------------- go/vt/mysqlctl/xtrabackupengine.go | 26 ++++++++++---- go/vt/wrangler/testlib/backup_test.go | 4 --- 8 files changed, 38 insertions(+), 60 deletions(-) diff --git a/go/test/endtoend/backup/mysqlctld/backup_mysqlctld_test.go b/go/test/endtoend/backup/mysqlctld/backup_mysqlctld_test.go index 177c1a8b8ff..6751ab1ad42 100644 --- a/go/test/endtoend/backup/mysqlctld/backup_mysqlctld_test.go +++ b/go/test/endtoend/backup/mysqlctld/backup_mysqlctld_test.go @@ -40,7 +40,6 @@ func TestBackupMysqlctldWithlz4Compression(t *testing.T) { func setDefaultCompressionFlag() { *mysqlctl.BuiltinCompressor = "pgzip" - *mysqlctl.BuiltinDecompressor = "auto" *mysqlctl.ExternalCompressorCmd = "" *mysqlctl.ExternalCompressorExt = "" *mysqlctl.ExternalDecompressorCmd = "" diff --git a/go/test/endtoend/backup/vtctlbackup/backup_test.go b/go/test/endtoend/backup/vtctlbackup/backup_test.go index 00e51c435e6..7a9a1f6ac68 100644 --- a/go/test/endtoend/backup/vtctlbackup/backup_test.go +++ b/go/test/endtoend/backup/vtctlbackup/backup_test.go @@ -40,7 +40,6 @@ func TestBackupMainWithZstdCompression(t *testing.T) { func setDefaultCompressionFlag() { *mysqlctl.BuiltinCompressor = "pgzip" - *mysqlctl.BuiltinDecompressor = "auto" *mysqlctl.ExternalCompressorCmd = "" *mysqlctl.ExternalCompressorExt = "" *mysqlctl.ExternalDecompressorCmd = "" diff --git a/go/test/endtoend/backup/xtrabackup/xtrabackup_test.go b/go/test/endtoend/backup/xtrabackup/xtrabackup_test.go index 20ff7841506..8b00c79d460 100644 --- a/go/test/endtoend/backup/xtrabackup/xtrabackup_test.go +++ b/go/test/endtoend/backup/xtrabackup/xtrabackup_test.go @@ -42,7 +42,6 @@ func TestXtrabackWithZstdCompression(t *testing.T) { func setDefaultCompressionFlag() { *mysqlctl.BuiltinCompressor = "pgzip" - *mysqlctl.BuiltinDecompressor = "auto" *mysqlctl.ExternalCompressorCmd = "" *mysqlctl.ExternalCompressorExt = "" *mysqlctl.ExternalDecompressorCmd = "" diff --git a/go/test/endtoend/backup/xtrabackupstream/xtrabackup_stream_test.go b/go/test/endtoend/backup/xtrabackupstream/xtrabackup_stream_test.go index 2e41c6bee5a..01fc5ca6323 100644 --- a/go/test/endtoend/backup/xtrabackupstream/xtrabackup_stream_test.go +++ b/go/test/endtoend/backup/xtrabackupstream/xtrabackup_stream_test.go @@ -40,7 +40,6 @@ func TestXtrabackupStreamWithlz4Compression(t *testing.T) { func setDefaultCompressionFlag() { *mysqlctl.BuiltinCompressor = "pgzip" - *mysqlctl.BuiltinDecompressor = "auto" *mysqlctl.ExternalCompressorCmd = "" *mysqlctl.ExternalCompressorExt = "" *mysqlctl.ExternalDecompressorCmd = "" diff --git a/go/vt/mysqlctl/builtinbackupengine.go b/go/vt/mysqlctl/builtinbackupengine.go index aa1fdec4d8d..f874347a3af 100644 --- a/go/vt/mysqlctl/builtinbackupengine.go +++ b/go/vt/mysqlctl/builtinbackupengine.go @@ -501,7 +501,6 @@ func (be *BuiltinBackupEngine) backupFile(ctx context.Context, params BackupPara // Create the gzip compression pipe, if necessary. var compressor io.WriteCloser if *backupStorageCompress { - if *ExternalCompressorCmd != "" { compressor, err = newExternalCompressor(ctx, *ExternalCompressorCmd, writer, params.Logger) } else { @@ -510,7 +509,6 @@ func (be *BuiltinBackupEngine) backupFile(ctx context.Context, params BackupPara if err != nil { return vterrors.Wrap(err, "can't create compressor") } - writer = compressor } @@ -606,16 +604,7 @@ func (be *BuiltinBackupEngine) restoreFiles(ctx context.Context, params RestoreP // And restore the file. name := fmt.Sprintf("%v", i) params.Logger.Infof("Copying file %v: %v", name, fes[i].Name) - // For backward compatibility. Incase if Manifest is from N-1 binary - // then we assign the default value of compressionEngine. - if bm.CompressionEngine == "" { - bm.CompressionEngine = *BuiltinCompressor - } - var decompEngine = bm.CompressionEngine - if *BuiltinDecompressor != "auto" { - decompEngine = *BuiltinDecompressor - } - err := be.restoreFile(ctx, params, bh, &fes[i], bm.TransformHook, !bm.SkipCompress, decompEngine, name) + err := be.restoreFile(ctx, params, bh, &fes[i], bm.TransformHook, !bm.SkipCompress, bm.CompressionEngine, name) if err != nil { rec.RecordError(vterrors.Wrapf(err, "can't restore file %v to %v", name, fes[i].Name)) } @@ -670,10 +659,21 @@ func (be *BuiltinBackupEngine) restoreFile(ctx context.Context, params RestorePa // Create the uncompresser if needed. if compress { var decompressor io.ReadCloser - if *ExternalDecompressorCmd != "" { - decompressor, err = newExternalDecompressor(ctx, *ExternalDecompressorCmd, reader, params.Logger) + if deCompressionEngine == "" { + // for backward compatibility + deCompressionEngine = "pgzip" + } else { + deCompressionEngine = *ExternalDecompressorCmd + } + decompressor, err = newExternalDecompressor(ctx, deCompressionEngine, reader, params.Logger) } else { + // For backward compatibility. Incase if Manifest is from N-1 binary + // then we assign the default value of compressionEngine. + if deCompressionEngine == "" { + // for backward compatibility + deCompressionEngine = "pgzip" + } decompressor, err = newBuiltinDecompressor(deCompressionEngine, reader, params.Logger) } if err != nil { diff --git a/go/vt/mysqlctl/compression.go b/go/vt/mysqlctl/compression.go index c158f327391..b6094a5cc33 100644 --- a/go/vt/mysqlctl/compression.go +++ b/go/vt/mysqlctl/compression.go @@ -39,15 +39,13 @@ import ( var ( compressionLevel = flag.Int("compression-level", 1, "what level to pass to the compressor") // switch which compressor/decompressor to use - BuiltinCompressor = flag.String("builtin-compressor", "pgzip", "builtin compressor engine to use") - BuiltinDecompressor = flag.String("builtin-decompressor", "auto", "builtin decompressor engine to use") + BuiltinCompressor = flag.String("builtin-compressor", "pgzip", "builtin compressor engine to use") // use and external command to decompress the backups ExternalCompressorCmd = flag.String("external-compressor", "", "command with arguments to use when compressing a backup") ExternalCompressorExt = flag.String("external-compressor-extension", "", "extension to use when using an external compressor") ExternalDecompressorCmd = flag.String("external-decompressor", "", "command with arguments to use when decompressing a backup") - errUnsupportedCompressionEngine = errors.New("unsupported engine") - errUnsupportedCompressionExtension = errors.New("unsupported extension") + errUnsupportedCompressionEngine = errors.New("unsupported engine") // this is used by getEngineFromExtension() to figure out which engine to use in case the user didn't specify engineExtensions = map[string][]string{ @@ -57,15 +55,6 @@ var ( } ) -func getEngineFromExtension(extension string) (string, error) { - for ext, eng := range engineExtensions { - if ext == extension { - return eng[0], nil // we select the first supported engine in auto mode - } - } - return "", fmt.Errorf("%w %q", errUnsupportedCompressionExtension, extension) -} - func getExtensionFromEngine(engine string) (string, error) { for ext, eng := range engineExtensions { for _, e := range eng { @@ -85,7 +74,7 @@ func validateExternalCmd(cmd string) (string, error) { return exec.LookPath(cmd) } -func prepareExternalCompressionCmd(ctx context.Context, cmdStr string) (*exec.Cmd, error) { +func prepareExternalCmd(ctx context.Context, cmdStr string) (*exec.Cmd, error) { cmdArgs, err := shlex.Split(cmdStr) if err != nil { return nil, err @@ -104,7 +93,7 @@ func prepareExternalCompressionCmd(ctx context.Context, cmdStr string) (*exec.Cm func newExternalCompressor(ctx context.Context, cmdStr string, writer io.Writer, logger logutil.Logger) (io.WriteCloser, error) { logger.Infof("Compressing using external command: %q", cmdStr) - cmd, err := prepareExternalCompressionCmd(ctx, cmdStr) + cmd, err := prepareExternalCmd(ctx, cmdStr) if err != nil { return nil, vterrors.Wrap(err, "unable to start external command") } @@ -134,7 +123,7 @@ func newExternalCompressor(ctx context.Context, cmdStr string, writer io.Writer, func newExternalDecompressor(ctx context.Context, cmdStr string, reader io.Reader, logger logutil.Logger) (io.ReadCloser, error) { logger.Infof("Decompressing using external command: %q", cmdStr) - cmd, err := prepareExternalCompressionCmd(ctx, cmdStr) + cmd, err := prepareExternalCmd(ctx, cmdStr) if err != nil { return nil, vterrors.Wrap(err, "unable to start external command") } @@ -159,21 +148,6 @@ func newExternalDecompressor(ctx context.Context, cmdStr string, reader io.Reade return decompressor, nil } -// This is a wrapper to get the right decompressor (see below) based on the extension of the file. -func newBuiltinDecompressorFromExtension(extension, engine string, reader io.Reader, logger logutil.Logger) (decompressor io.ReadCloser, err error) { - // we only infer the engine from the extension is set to "auto", otherwise we use whatever the user selected - if engine == "auto" { - logger.Infof("Builtin decompressor set to auto, checking which engine to decompress based on the extension") - - eng, err := getEngineFromExtension(extension) - if err != nil { - return decompressor, err - } - engine = eng - } - return newBuiltinDecompressor(engine, reader, logger) -} - // This returns a reader that will decompress the underlying provided reader and will use the specified supported engine. func newBuiltinDecompressor(engine string, reader io.Reader, logger logutil.Logger) (decompressor io.ReadCloser, err error) { if engine == "pargzip" { diff --git a/go/vt/mysqlctl/xtrabackupengine.go b/go/vt/mysqlctl/xtrabackupengine.go index e594a582212..13aac09369e 100644 --- a/go/vt/mysqlctl/xtrabackupengine.go +++ b/go/vt/mysqlctl/xtrabackupengine.go @@ -26,7 +26,6 @@ import ( "os" "os/exec" "path" - "path/filepath" "regexp" "strings" "sync" @@ -80,7 +79,8 @@ const ( type xtraBackupManifest struct { // BackupManifest is an anonymous embedding of the base manifest struct. BackupManifest - + // CompressionEngine stores which compression engine was used to originally compress the files. + CompressionEngine string `json:",omitempty"` // Name of the backup file FileName string // Params are the parameters that backup was run with @@ -200,6 +200,8 @@ func (be *XtrabackupEngine) ExecuteBackup(ctx context.Context, params BackupPara Params: *xtrabackupBackupFlags, NumStripes: int32(numStripes), StripeBlockSize: int32(*xtrabackupStripeBlockSize), + // builtin specific field + CompressionEngine: *BuiltinCompressor, } data, err := json.MarshalIndent(bm, "", " ") @@ -274,6 +276,8 @@ func (be *XtrabackupEngine) backupFiles(ctx context.Context, params BackupParams } }() + params.Logger.Infof("backup command: %s", backupProgram) + params.Logger.Infof("arguments: %s", flagsToExec) backupCmd := exec.CommandContext(ctx, backupProgram, flagsToExec...) backupOut, err := backupCmd.StdoutPipe() if err != nil { @@ -533,8 +537,6 @@ func (be *XtrabackupEngine) extractFiles(ctx context.Context, logger logutil.Log } logger.Infof("backup file name: %s", baseFileName) - extension := filepath.Ext(baseFileName) - // Open the source files for reading. srcFiles, err := readStripeFiles(ctx, bh, baseFileName, int(bm.NumStripes), logger) if err != nil { @@ -554,11 +556,21 @@ func (be *XtrabackupEngine) extractFiles(ctx context.Context, logger logutil.Log // Create the decompressor if needed. if compressed { var decompressor io.ReadCloser - + var decompressionEngine = "pgzip" if *ExternalDecompressorCmd != "" { - decompressor, err = newExternalDecompressor(ctx, *ExternalDecompressorCmd, reader, logger) + if bm.CompressionEngine != "" { + // for backward compatibility + decompressionEngine = bm.CompressionEngine + } else { + decompressionEngine = *ExternalDecompressorCmd + } + decompressor, err = newExternalDecompressor(ctx, decompressionEngine, reader, logger) } else { - decompressor, err = newBuiltinDecompressorFromExtension(extension, *BuiltinDecompressor, reader, logger) + if bm.CompressionEngine != "" { + // for backward compatibility + decompressionEngine = bm.CompressionEngine + } + decompressor, err = newBuiltinDecompressor(decompressionEngine, reader, logger) } if err != nil { return vterrors.Wrap(err, "can't create decompressor") diff --git a/go/vt/wrangler/testlib/backup_test.go b/go/vt/wrangler/testlib/backup_test.go index d0f6476b31f..13144bd7eb8 100644 --- a/go/vt/wrangler/testlib/backup_test.go +++ b/go/vt/wrangler/testlib/backup_test.go @@ -75,7 +75,6 @@ func TestBackupRestoreWithPargzip(t *testing.T) { func setDefaultCompressionFlag() { *mysqlctl.BuiltinCompressor = "pgzip" - *mysqlctl.BuiltinDecompressor = "auto" *mysqlctl.ExternalCompressorCmd = "" *mysqlctl.ExternalCompressorExt = "" *mysqlctl.ExternalDecompressorCmd = "" @@ -121,9 +120,6 @@ func testBackupRestore(t *testing.T, cDetails *compressionDetails) error { if cDetails.BuiltinCompressor != "" { *mysqlctl.BuiltinCompressor = cDetails.BuiltinCompressor } - if cDetails.BuiltinDecompressor != "" { - *mysqlctl.BuiltinDecompressor = cDetails.BuiltinDecompressor - } if cDetails.ExternalCompressorCmd != "" { *mysqlctl.ExternalCompressorCmd = cDetails.ExternalCompressorCmd } From 7190a09479970b70d19f2129778b7263bd9d41cd Mon Sep 17 00:00:00 2001 From: Rameez Sajwani Date: Mon, 11 Jul 2022 18:44:13 -0700 Subject: [PATCH 02/18] Fix test failures Signed-off-by: Rameez Sajwani --- go/vt/mysqlctl/xtrabackupengine.go | 10 +++++----- go/vt/wrangler/testlib/backup_test.go | 12 ++++++------ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/go/vt/mysqlctl/xtrabackupengine.go b/go/vt/mysqlctl/xtrabackupengine.go index 13aac09369e..4a6f3313928 100644 --- a/go/vt/mysqlctl/xtrabackupengine.go +++ b/go/vt/mysqlctl/xtrabackupengine.go @@ -556,19 +556,19 @@ func (be *XtrabackupEngine) extractFiles(ctx context.Context, logger logutil.Log // Create the decompressor if needed. if compressed { var decompressor io.ReadCloser - var decompressionEngine = "pgzip" + var decompressionEngine = bm.CompressionEngine if *ExternalDecompressorCmd != "" { - if bm.CompressionEngine != "" { + if decompressionEngine == "" { // for backward compatibility - decompressionEngine = bm.CompressionEngine + decompressionEngine = "pgzip" } else { decompressionEngine = *ExternalDecompressorCmd } decompressor, err = newExternalDecompressor(ctx, decompressionEngine, reader, logger) } else { - if bm.CompressionEngine != "" { + if decompressionEngine == "" { // for backward compatibility - decompressionEngine = bm.CompressionEngine + decompressionEngine = "pgzip" } decompressor, err = newBuiltinDecompressor(decompressionEngine, reader, logger) } diff --git a/go/vt/wrangler/testlib/backup_test.go b/go/vt/wrangler/testlib/backup_test.go index 13144bd7eb8..fefe549c598 100644 --- a/go/vt/wrangler/testlib/backup_test.go +++ b/go/vt/wrangler/testlib/backup_test.go @@ -47,7 +47,6 @@ import ( type compressionDetails struct { BuiltinCompressor string - BuiltinDecompressor string ExternalCompressorCmd string ExternalCompressorExt string ExternalDecompressorCmd string @@ -59,18 +58,19 @@ func TestBackupRestore(t *testing.T) { require.NoError(t, err) } -// TODO: @rameez. I was expecting this test to fail but it turns out -// we infer decompressor through compression engine in builtinEngine. +// Given we infer decompressor through Manifest file. // It is only in xtrabackup where we infer decompressor through extension & BuiltinDecompressor param. func TestBackupRestoreWithPargzip(t *testing.T) { defer setDefaultCompressionFlag() cDetails := &compressionDetails{ - BuiltinCompressor: "pargzip", - BuiltinDecompressor: "lz4", + BuiltinCompressor: "pargzip", + ExternalCompressorCmd: "zstd", + ExternalCompressorExt: ".zst", + ExternalDecompressorCmd: "zstd -d", } err := testBackupRestore(t, cDetails) - require.ErrorContains(t, err, "lz4: bad magic number") + require.NoError(t, err) } func setDefaultCompressionFlag() { From bd6cb5d35bf3bde66d87f22f15b06efa51e1497a Mon Sep 17 00:00:00 2001 From: Rameez Sajwani Date: Mon, 11 Jul 2022 22:12:04 -0700 Subject: [PATCH 03/18] Fix Helpoutput test Signed-off-by: Rameez Sajwani --- go/flags/endtoend/vttablet.txt | 2 -- 1 file changed, 2 deletions(-) diff --git a/go/flags/endtoend/vttablet.txt b/go/flags/endtoend/vttablet.txt index db8b824891f..436bdaaefba 100644 --- a/go/flags/endtoend/vttablet.txt +++ b/go/flags/endtoend/vttablet.txt @@ -59,8 +59,6 @@ Usage of vttablet: PITR restore parameter: username of binlog server. --builtin-compressor string builtin compressor engine to use (default pgzip) - --builtin-decompressor string - builtin decompressor engine to use (default auto) --builtinbackup_mysqld_timeout duration how long to wait for mysqld to shutdown at the start of the backup (default 10m0s) --builtinbackup_progress duration From b27a551f02f6265d95a3f4e31635a864db347c39 Mon Sep 17 00:00:00 2001 From: Rameez Sajwani Date: Tue, 12 Jul 2022 10:51:27 -0700 Subject: [PATCH 04/18] Fixing unit test Signed-off-by: Rameez Sajwani --- go/test/endtoend/backup/vtctlbackup/backup_test.go | 1 + go/test/endtoend/backup/xtrabackup/xtrabackup_test.go | 1 + go/vt/mysqlctl/builtinbackupengine.go | 9 +++++---- go/vt/mysqlctl/xtrabackupengine.go | 9 +++++---- go/vt/wrangler/testlib/backup_test.go | 5 +---- 5 files changed, 13 insertions(+), 12 deletions(-) diff --git a/go/test/endtoend/backup/vtctlbackup/backup_test.go b/go/test/endtoend/backup/vtctlbackup/backup_test.go index 7a9a1f6ac68..e4d5187fe8c 100644 --- a/go/test/endtoend/backup/vtctlbackup/backup_test.go +++ b/go/test/endtoend/backup/vtctlbackup/backup_test.go @@ -30,6 +30,7 @@ func TestBackupMain(t *testing.T) { func TestBackupMainWithZstdCompression(t *testing.T) { defer setDefaultCompressionFlag() cDetails := &CompressionDetails{ + BuiltinCompressor: "lz4", ExternalCompressorCmd: "zstd", ExternalCompressorExt: ".zst", ExternalDecompressorCmd: "zstd -d", diff --git a/go/test/endtoend/backup/xtrabackup/xtrabackup_test.go b/go/test/endtoend/backup/xtrabackup/xtrabackup_test.go index 8b00c79d460..28a5653d9ea 100644 --- a/go/test/endtoend/backup/xtrabackup/xtrabackup_test.go +++ b/go/test/endtoend/backup/xtrabackup/xtrabackup_test.go @@ -31,6 +31,7 @@ func TestXtrabackup(t *testing.T) { func TestXtrabackWithZstdCompression(t *testing.T) { defer setDefaultCompressionFlag() + // BuiltinCompressor will be ignored in this case cDetails := &backup.CompressionDetails{ ExternalCompressorCmd: "zstd", ExternalCompressorExt: ".zst", diff --git a/go/vt/mysqlctl/builtinbackupengine.go b/go/vt/mysqlctl/builtinbackupengine.go index f874347a3af..b46bc925b69 100644 --- a/go/vt/mysqlctl/builtinbackupengine.go +++ b/go/vt/mysqlctl/builtinbackupengine.go @@ -75,7 +75,9 @@ type builtinBackupManifest struct { // BackupManifest is an anonymous embedding of the base manifest struct. BackupManifest - // CompressionEngine stores which compression engine was used to originally compress the files. + // CompressionEngine stores which compression engine was originally provided + // to compress the files. Please note that if user has provided externalCompressorCmd + // then this value will not be used during decompression. CompressionEngine string `json:",omitempty"` // FileEntries contains all the files in the backup @@ -661,15 +663,14 @@ func (be *BuiltinBackupEngine) restoreFile(ctx context.Context, params RestorePa var decompressor io.ReadCloser if *ExternalDecompressorCmd != "" { if deCompressionEngine == "" { - // for backward compatibility + // For backward compatibility. In case if Manifest is from N-1 binary + // then we assign the default value of compressionEngine. deCompressionEngine = "pgzip" } else { deCompressionEngine = *ExternalDecompressorCmd } decompressor, err = newExternalDecompressor(ctx, deCompressionEngine, reader, params.Logger) } else { - // For backward compatibility. Incase if Manifest is from N-1 binary - // then we assign the default value of compressionEngine. if deCompressionEngine == "" { // for backward compatibility deCompressionEngine = "pgzip" diff --git a/go/vt/mysqlctl/xtrabackupengine.go b/go/vt/mysqlctl/xtrabackupengine.go index 4a6f3313928..7acc9552b72 100644 --- a/go/vt/mysqlctl/xtrabackupengine.go +++ b/go/vt/mysqlctl/xtrabackupengine.go @@ -79,7 +79,9 @@ const ( type xtraBackupManifest struct { // BackupManifest is an anonymous embedding of the base manifest struct. BackupManifest - // CompressionEngine stores which compression engine was used to originally compress the files. + // CompressionEngine stores which compression engine was originally provided + // to compress the files. Please note that if user has provided externalCompressorCmd + // then this value will not be used during decompression. CompressionEngine string `json:",omitempty"` // Name of the backup file FileName string @@ -276,8 +278,6 @@ func (be *XtrabackupEngine) backupFiles(ctx context.Context, params BackupParams } }() - params.Logger.Infof("backup command: %s", backupProgram) - params.Logger.Infof("arguments: %s", flagsToExec) backupCmd := exec.CommandContext(ctx, backupProgram, flagsToExec...) backupOut, err := backupCmd.StdoutPipe() if err != nil { @@ -559,7 +559,8 @@ func (be *XtrabackupEngine) extractFiles(ctx context.Context, logger logutil.Log var decompressionEngine = bm.CompressionEngine if *ExternalDecompressorCmd != "" { if decompressionEngine == "" { - // for backward compatibility + // For backward compatibility. Incase if Manifest is from N-1 binary + // then we assign the default value of compressionEngine. decompressionEngine = "pgzip" } else { decompressionEngine = *ExternalDecompressorCmd diff --git a/go/vt/wrangler/testlib/backup_test.go b/go/vt/wrangler/testlib/backup_test.go index fefe549c598..270f8a63e3f 100644 --- a/go/vt/wrangler/testlib/backup_test.go +++ b/go/vt/wrangler/testlib/backup_test.go @@ -63,10 +63,7 @@ func TestBackupRestore(t *testing.T) { func TestBackupRestoreWithPargzip(t *testing.T) { defer setDefaultCompressionFlag() cDetails := &compressionDetails{ - BuiltinCompressor: "pargzip", - ExternalCompressorCmd: "zstd", - ExternalCompressorExt: ".zst", - ExternalDecompressorCmd: "zstd -d", + BuiltinCompressor: "pargzip", } err := testBackupRestore(t, cDetails) From 11911df87f766942b37959b2b9ba6930509a7663 Mon Sep 17 00:00:00 2001 From: Rameez Sajwani Date: Tue, 12 Jul 2022 14:13:04 -0700 Subject: [PATCH 05/18] Adding summary Signed-off-by: Rameez Sajwani --- doc/releasenotes/15_0_0_summary.md | 20 ++++++----- .../backup/vtbackup/backup_only_test.go | 8 ++++- go/vt/mysqlctl/builtinbackupengine.go | 24 ++++++++----- go/vt/mysqlctl/xtrabackupengine.go | 34 +++++++++++-------- 4 files changed, 54 insertions(+), 32 deletions(-) diff --git a/doc/releasenotes/15_0_0_summary.md b/doc/releasenotes/15_0_0_summary.md index 244786b743a..b65294bf202 100644 --- a/doc/releasenotes/15_0_0_summary.md +++ b/doc/releasenotes/15_0_0_summary.md @@ -55,12 +55,11 @@ Please see the VDiff2 [documentation](https://vitess.io/docs/15.0/reference/vrep ### New command line flags and behavior #### Support for additional compressors and decompressors during backup & restore -Backup/Restore now allow you many more options for compression and decompression instead of relying on the default compressor(pgzip). +Backup/Restore now allow you many more options for compression and decompression instead of relying on the default compressor(`pgzip`). There are some built-in compressors which you can use out-of-the-box. Users will need to evaluate which option works best for their use-case. Here are the flags that control this feature - --builtin-compressor -- --builtin-decompressor - --external-compressor - --external-decompressor - --external-compressor-extension @@ -72,15 +71,20 @@ builtin compressor as of today supports the following options - lz4 - zstd -If you want to use any of the builtin compressors, simply set one of the above values for `--builtin-compressor`. You don't need to set -the `--builtin-decompressor` flag in this case as we infer it automatically from the MANIFEST file. The default value for -`--builtin-decompressor` is `auto`. +If you want to use any of the builtin compressors, simply set one of the above values for `--builtin-compressor`. During restore the system +will read the MANIFEST file to get the compressor engine value and use that value for decompression. If you would like to use a custom command or external tool for compression/decompression then you need to provide the full command with arguments to the `--external-compressor` and `--external-decompressor` flags. `--external-compressor-extension` flag also needs to be provided -so that compressed files are created with the correct extension. There is no need to override `--builtin-compressor` and `--builtin-decompressor` -when using an external compressor/decompressor. Please note that if you want the current behavior then you don't need to change anything -in these flags. You can read more about backup & restore [here] (https://vitess.io/docs/15.0/user-guides/operating-vitess/backup-and-restore/). +so that compressed files are created with the correct extension. There is no need to override `--builtin-compressor` when using an external +compressor/decompressor. Please note that if you want the current behavior then you don't need to change anything in these flags. +You can read more about backup & restore [here] (https://vitess.io/docs/15.0/user-guides/operating-vitess/backup-and-restore/). + +Important Note: If you decided to switch from external compressor to built-in compressor at any point in the future. System won't be able to +restore from current backups due to out-dated value of compression engine in MANIFEST file. The Backups taken with the new built-in values should +work during restore. For example if you switch from external compressor `some-custom-tool` to built-in `pgzip`, and you try to restore from +the older backups, then decompression from older backups (which were taken using `some-custom-tool`) will fail, until you have a new backup +with `pgzip`. Please make sure you have thought out all possible scenarios for restore before transitioning from one compressor value to the others. ### Online DDL changes diff --git a/go/test/endtoend/backup/vtbackup/backup_only_test.go b/go/test/endtoend/backup/vtbackup/backup_only_test.go index 008ef269b5a..f4ca094e586 100644 --- a/go/test/endtoend/backup/vtbackup/backup_only_test.go +++ b/go/test/endtoend/backup/vtbackup/backup_only_test.go @@ -24,6 +24,8 @@ import ( "testing" "time" + "vitess.io/vitess/go/vt/mysqlctl" + "github.com/stretchr/testify/require" "vitess.io/vitess/go/test/endtoend/cluster" @@ -135,6 +137,11 @@ func firstBackupTest(t *testing.T, tabletType string) { require.Nil(t, err) cluster.VerifyRowsInTablet(t, replica1, keyspaceName, 2) + // eventhough we change the value of compression it won't effect + // decompression since it gets its value from MANIFEST file, created + // as part of backup. + *mysqlctl.BuiltinCompressor = "lz4" + defer func() { *mysqlctl.BuiltinCompressor = "pgzip" }() // now bring up the other replica, letting it restore from backup. err = localCluster.VtctlclientProcess.InitTablet(replica2, cell, keyspaceName, hostname, shardName) require.Nil(t, err) @@ -160,7 +167,6 @@ func firstBackupTest(t *testing.T, tabletType string) { removeBackups(t) verifyBackupCount(t, shardKsName, 0) - } func vtBackup(t *testing.T, initialBackup bool, restartBeforeBackup bool) { diff --git a/go/vt/mysqlctl/builtinbackupengine.go b/go/vt/mysqlctl/builtinbackupengine.go index b46bc925b69..6fe6ee7f039 100644 --- a/go/vt/mysqlctl/builtinbackupengine.go +++ b/go/vt/mysqlctl/builtinbackupengine.go @@ -342,6 +342,13 @@ func (be *BuiltinBackupEngine) backupFiles(ctx context.Context, params BackupPar } }() + // set the correct value of compression engine used. + // if an external compressor is used, empty engine value + // is saved in the MANIFEST + var compressionEngineValue = *BuiltinCompressor + if *ExternalCompressorCmd != "" { + compressionEngineValue = "no-value" + } // JSON-encode and write the MANIFEST bm := &builtinBackupManifest{ // Common base fields @@ -356,7 +363,7 @@ func (be *BuiltinBackupEngine) backupFiles(ctx context.Context, params BackupPar FileEntries: fes, TransformHook: *backupStorageHook, SkipCompress: !*backupStorageCompress, - CompressionEngine: *BuiltinCompressor, + CompressionEngine: compressionEngineValue, } data, err := json.MarshalIndent(bm, "", " ") if err != nil { @@ -661,19 +668,18 @@ func (be *BuiltinBackupEngine) restoreFile(ctx context.Context, params RestorePa // Create the uncompresser if needed. if compress { var decompressor io.ReadCloser + if deCompressionEngine == "" { + // for backward compatibility + deCompressionEngine = "pgzip" + } if *ExternalDecompressorCmd != "" { - if deCompressionEngine == "" { - // For backward compatibility. In case if Manifest is from N-1 binary - // then we assign the default value of compressionEngine. - deCompressionEngine = "pgzip" - } else { + if deCompressionEngine == "no-value" { deCompressionEngine = *ExternalDecompressorCmd } decompressor, err = newExternalDecompressor(ctx, deCompressionEngine, reader, params.Logger) } else { - if deCompressionEngine == "" { - // for backward compatibility - deCompressionEngine = "pgzip" + if deCompressionEngine == "no-value" { + return fmt.Errorf("%w %q", errUnsupportedCompressionEngine, "no-value") } decompressor, err = newBuiltinDecompressor(deCompressionEngine, reader, params.Logger) } diff --git a/go/vt/mysqlctl/xtrabackupengine.go b/go/vt/mysqlctl/xtrabackupengine.go index 7acc9552b72..bdee9b8cd80 100644 --- a/go/vt/mysqlctl/xtrabackupengine.go +++ b/go/vt/mysqlctl/xtrabackupengine.go @@ -185,6 +185,13 @@ func (be *XtrabackupEngine) ExecuteBackup(ctx context.Context, params BackupPara } defer closeFile(mwc, backupManifestFileName, params.Logger, &finalErr) + // set the correct value of compression engine used. + // if an external compressor is used, empty engine value + // is saved in the MANIFEST + var compressionEngineValue = *BuiltinCompressor + if *ExternalCompressorCmd != "" { + compressionEngineValue = "no-value" + } // JSON-encode and write the MANIFEST bm := &xtraBackupManifest{ // Common base fields @@ -203,7 +210,7 @@ func (be *XtrabackupEngine) ExecuteBackup(ctx context.Context, params BackupPara NumStripes: int32(numStripes), StripeBlockSize: int32(*xtrabackupStripeBlockSize), // builtin specific field - CompressionEngine: *BuiltinCompressor, + CompressionEngine: compressionEngineValue, } data, err := json.MarshalIndent(bm, "", " ") @@ -556,27 +563,26 @@ func (be *XtrabackupEngine) extractFiles(ctx context.Context, logger logutil.Log // Create the decompressor if needed. if compressed { var decompressor io.ReadCloser - var decompressionEngine = bm.CompressionEngine + var deCompressionEngine = bm.CompressionEngine + if deCompressionEngine == "" { + // For backward compatibility. Incase if Manifest is from N-1 binary + // then we assign the default value of compressionEngine. + deCompressionEngine = "pgzip" + } if *ExternalDecompressorCmd != "" { - if decompressionEngine == "" { - // For backward compatibility. Incase if Manifest is from N-1 binary - // then we assign the default value of compressionEngine. - decompressionEngine = "pgzip" - } else { - decompressionEngine = *ExternalDecompressorCmd + if deCompressionEngine == "no-value" { + deCompressionEngine = *ExternalDecompressorCmd } - decompressor, err = newExternalDecompressor(ctx, decompressionEngine, reader, logger) + decompressor, err = newExternalDecompressor(ctx, deCompressionEngine, reader, logger) } else { - if decompressionEngine == "" { - // for backward compatibility - decompressionEngine = "pgzip" + if deCompressionEngine == "no-value" { + return fmt.Errorf("%w %q", errUnsupportedCompressionEngine, "no-value") } - decompressor, err = newBuiltinDecompressor(decompressionEngine, reader, logger) + decompressor, err = newBuiltinDecompressor(deCompressionEngine, reader, logger) } if err != nil { return vterrors.Wrap(err, "can't create decompressor") } - srcDecompressors = append(srcDecompressors, decompressor) reader = decompressor } From 79240f57280ee0f62405d6c3d48afa5b9ba0f4f4 Mon Sep 17 00:00:00 2001 From: Rameez Sajwani Date: Wed, 13 Jul 2022 17:24:00 -0700 Subject: [PATCH 06/18] code cleaning and better summary Signed-off-by: Rameez Sajwani --- doc/releasenotes/15_0_0_summary.md | 13 ++++++----- go/vt/mysqlctl/builtinbackupengine.go | 10 ++++----- go/vt/mysqlctl/compression.go | 32 +++++++++++++++++---------- go/vt/mysqlctl/xtrabackupengine.go | 10 ++++----- 4 files changed, 38 insertions(+), 27 deletions(-) diff --git a/doc/releasenotes/15_0_0_summary.md b/doc/releasenotes/15_0_0_summary.md index b65294bf202..1500c702920 100644 --- a/doc/releasenotes/15_0_0_summary.md +++ b/doc/releasenotes/15_0_0_summary.md @@ -80,11 +80,14 @@ so that compressed files are created with the correct extension. There is no nee compressor/decompressor. Please note that if you want the current behavior then you don't need to change anything in these flags. You can read more about backup & restore [here] (https://vitess.io/docs/15.0/user-guides/operating-vitess/backup-and-restore/). -Important Note: If you decided to switch from external compressor to built-in compressor at any point in the future. System won't be able to -restore from current backups due to out-dated value of compression engine in MANIFEST file. The Backups taken with the new built-in values should -work during restore. For example if you switch from external compressor `some-custom-tool` to built-in `pgzip`, and you try to restore from -the older backups, then decompression from older backups (which were taken using `some-custom-tool`) will fail, until you have a new backup -with `pgzip`. Please make sure you have thought out all possible scenarios for restore before transitioning from one compressor value to the others. +Important Note: If you decided to switch from external compressor to built-in compressor at any point in the future. You might need to do it +in two steps. +- step #1, set `--external-compressor` and `--external-compressor-extension` flag values to empty and change `--builtin-compressor` to desired value. +- Step #2, after few cycles of backups now set `--external-decompressor` flag value to empty. + +The reason we recommend not to change all the values together is because builtin compressor have no way to find out which external decompressor +being used during the previous backups. Please make sure you have thought out all possible scenarios for restore before transitioning from one +compressor value to the others. ### Online DDL changes diff --git a/go/vt/mysqlctl/builtinbackupengine.go b/go/vt/mysqlctl/builtinbackupengine.go index 6fe6ee7f039..775c7d5664c 100644 --- a/go/vt/mysqlctl/builtinbackupengine.go +++ b/go/vt/mysqlctl/builtinbackupengine.go @@ -347,7 +347,7 @@ func (be *BuiltinBackupEngine) backupFiles(ctx context.Context, params BackupPar // is saved in the MANIFEST var compressionEngineValue = *BuiltinCompressor if *ExternalCompressorCmd != "" { - compressionEngineValue = "no-value" + compressionEngineValue = ExternalCompressor } // JSON-encode and write the MANIFEST bm := &builtinBackupManifest{ @@ -670,16 +670,16 @@ func (be *BuiltinBackupEngine) restoreFile(ctx context.Context, params RestorePa var decompressor io.ReadCloser if deCompressionEngine == "" { // for backward compatibility - deCompressionEngine = "pgzip" + deCompressionEngine = PgzipCompressor } if *ExternalDecompressorCmd != "" { - if deCompressionEngine == "no-value" { + if deCompressionEngine == ExternalCompressor { deCompressionEngine = *ExternalDecompressorCmd } decompressor, err = newExternalDecompressor(ctx, deCompressionEngine, reader, params.Logger) } else { - if deCompressionEngine == "no-value" { - return fmt.Errorf("%w %q", errUnsupportedCompressionEngine, "no-value") + if deCompressionEngine == ExternalCompressor { + return fmt.Errorf("%w %q", errUnsupportedCompressionEngine, ExternalCompressor) } decompressor, err = newBuiltinDecompressor(deCompressionEngine, reader, params.Logger) } diff --git a/go/vt/mysqlctl/compression.go b/go/vt/mysqlctl/compression.go index b6094a5cc33..9f4f3842000 100644 --- a/go/vt/mysqlctl/compression.go +++ b/go/vt/mysqlctl/compression.go @@ -36,6 +36,14 @@ import ( "vitess.io/vitess/go/vt/vterrors" ) +const ( + PgzipCompressor = "pgzip" + PargzipCompressor = "pargzip" + ZstdCompressor = "zstd" + Lz4Compressor = "lz4" + ExternalCompressor = "external" +) + var ( compressionLevel = flag.Int("compression-level", 1, "what level to pass to the compressor") // switch which compressor/decompressor to use @@ -49,9 +57,9 @@ var ( // this is used by getEngineFromExtension() to figure out which engine to use in case the user didn't specify engineExtensions = map[string][]string{ - ".gz": {"pgzip", "pargzip"}, - ".lz4": {"lz4"}, - ".zst": {"zstd"}, + ".gz": {PgzipCompressor, PargzipCompressor}, + ".lz4": {Lz4Compressor}, + ".zst": {ZstdCompressor}, } ) @@ -150,21 +158,21 @@ func newExternalDecompressor(ctx context.Context, cmdStr string, reader io.Reade // This returns a reader that will decompress the underlying provided reader and will use the specified supported engine. func newBuiltinDecompressor(engine string, reader io.Reader, logger logutil.Logger) (decompressor io.ReadCloser, err error) { - if engine == "pargzip" { + if engine == PargzipCompressor { logger.Warningf("engine \"pargzip\" doesn't support decompression, using \"pgzip\" instead") - engine = "pgzip" + engine = PgzipCompressor } switch engine { - case "pgzip": + case PgzipCompressor: d, err := pgzip.NewReader(reader) if err != nil { return nil, err } decompressor = d - case "lz4": + case Lz4Compressor: decompressor = ioutil.NopCloser(lz4.NewReader(reader)) - case "zstd": + case ZstdCompressor: d, err := zstd.NewReader(reader) if err != nil { return nil, err @@ -182,26 +190,26 @@ func newBuiltinDecompressor(engine string, reader io.Reader, logger logutil.Logg // This returns a writer that will compress the data using the specified engine before writing to the underlying writer. func newBuiltinCompressor(engine string, writer io.Writer, logger logutil.Logger) (compressor io.WriteCloser, err error) { switch engine { - case "pgzip": + case PgzipCompressor: gzip, err := pgzip.NewWriterLevel(writer, *compressionLevel) if err != nil { return compressor, vterrors.Wrap(err, "cannot create gzip compressor") } gzip.SetConcurrency(*backupCompressBlockSize, *backupCompressBlocks) compressor = gzip - case "pargzip": + case PargzipCompressor: gzip := pargzip.NewWriter(writer) gzip.ChunkSize = *backupCompressBlockSize gzip.Parallel = *backupCompressBlocks gzip.CompressionLevel = *compressionLevel compressor = gzip - case "lz4": + case Lz4Compressor: lz4Writer := lz4.NewWriter(writer).WithConcurrency(*backupCompressBlocks) lz4Writer.Header = lz4.Header{ CompressionLevel: *compressionLevel, } compressor = lz4Writer - case "zstd": + case ZstdCompressor: zst, err := zstd.NewWriter(writer, zstd.WithEncoderLevel(zstd.EncoderLevel(*compressionLevel))) if err != nil { return compressor, vterrors.Wrap(err, "cannot create zstd compressor") diff --git a/go/vt/mysqlctl/xtrabackupengine.go b/go/vt/mysqlctl/xtrabackupengine.go index bdee9b8cd80..82fdc734c06 100644 --- a/go/vt/mysqlctl/xtrabackupengine.go +++ b/go/vt/mysqlctl/xtrabackupengine.go @@ -190,7 +190,7 @@ func (be *XtrabackupEngine) ExecuteBackup(ctx context.Context, params BackupPara // is saved in the MANIFEST var compressionEngineValue = *BuiltinCompressor if *ExternalCompressorCmd != "" { - compressionEngineValue = "no-value" + compressionEngineValue = ExternalCompressor } // JSON-encode and write the MANIFEST bm := &xtraBackupManifest{ @@ -567,16 +567,16 @@ func (be *XtrabackupEngine) extractFiles(ctx context.Context, logger logutil.Log if deCompressionEngine == "" { // For backward compatibility. Incase if Manifest is from N-1 binary // then we assign the default value of compressionEngine. - deCompressionEngine = "pgzip" + deCompressionEngine = PgzipCompressor } if *ExternalDecompressorCmd != "" { - if deCompressionEngine == "no-value" { + if deCompressionEngine == ExternalCompressor { deCompressionEngine = *ExternalDecompressorCmd } decompressor, err = newExternalDecompressor(ctx, deCompressionEngine, reader, logger) } else { - if deCompressionEngine == "no-value" { - return fmt.Errorf("%w %q", errUnsupportedCompressionEngine, "no-value") + if deCompressionEngine == ExternalCompressor { + return fmt.Errorf("%w %q", errUnsupportedCompressionEngine, ExternalCompressor) } decompressor, err = newBuiltinDecompressor(deCompressionEngine, reader, logger) } From 1cddabbfe34411d6f09b23c356282fd5552e1922 Mon Sep 17 00:00:00 2001 From: Rameez Sajwani Date: Thu, 28 Jul 2022 17:31:54 -0700 Subject: [PATCH 07/18] Change builtinCompressor to more generic compression engine name Signed-off-by: Rameez Sajwani --- go/flags/endtoend/vttablet.txt | 4 +- .../backup/mysqlctld/backup_mysqlctld_test.go | 2 +- .../backup/vtbackup/backup_only_test.go | 4 +- .../backup/vtctlbackup/backup_test.go | 2 +- .../backup/vtctlbackup/backup_utils.go | 2 +- .../backup/xtrabackup/xtrabackup_test.go | 4 +- .../xtrabackup_stream_test.go | 2 +- go/vt/mysqlctl/builtinbackupengine.go | 34 +++++++--------- go/vt/mysqlctl/compression.go | 26 +++++++++++-- go/vt/mysqlctl/compression_test.go | 39 +++++++++++++++++++ go/vt/mysqlctl/xtrabackupengine.go | 17 +++----- go/vt/wrangler/testlib/backup_test.go | 4 +- 12 files changed, 95 insertions(+), 45 deletions(-) diff --git a/go/flags/endtoend/vttablet.txt b/go/flags/endtoend/vttablet.txt index 436bdaaefba..4ebb908564d 100644 --- a/go/flags/endtoend/vttablet.txt +++ b/go/flags/endtoend/vttablet.txt @@ -57,8 +57,8 @@ Usage of vttablet: PITR restore parameter: TLS server name (common name) to verify against for the binlog server we are connecting to (If not set: use the hostname or IP supplied in -binlog_host). --binlog_user string PITR restore parameter: username of binlog server. - --builtin-compressor string - builtin compressor engine to use (default pgzip) + --compression-engine-name string + compressor engine used for compression. (default pgzip) --builtinbackup_mysqld_timeout duration how long to wait for mysqld to shutdown at the start of the backup (default 10m0s) --builtinbackup_progress duration diff --git a/go/test/endtoend/backup/mysqlctld/backup_mysqlctld_test.go b/go/test/endtoend/backup/mysqlctld/backup_mysqlctld_test.go index 6751ab1ad42..2af44351fcb 100644 --- a/go/test/endtoend/backup/mysqlctld/backup_mysqlctld_test.go +++ b/go/test/endtoend/backup/mysqlctld/backup_mysqlctld_test.go @@ -39,7 +39,7 @@ func TestBackupMysqlctldWithlz4Compression(t *testing.T) { } func setDefaultCompressionFlag() { - *mysqlctl.BuiltinCompressor = "pgzip" + *mysqlctl.CompressionEngineName = "pgzip" *mysqlctl.ExternalCompressorCmd = "" *mysqlctl.ExternalCompressorExt = "" *mysqlctl.ExternalDecompressorCmd = "" diff --git a/go/test/endtoend/backup/vtbackup/backup_only_test.go b/go/test/endtoend/backup/vtbackup/backup_only_test.go index f4ca094e586..c1c6e04e951 100644 --- a/go/test/endtoend/backup/vtbackup/backup_only_test.go +++ b/go/test/endtoend/backup/vtbackup/backup_only_test.go @@ -140,8 +140,8 @@ func firstBackupTest(t *testing.T, tabletType string) { // eventhough we change the value of compression it won't effect // decompression since it gets its value from MANIFEST file, created // as part of backup. - *mysqlctl.BuiltinCompressor = "lz4" - defer func() { *mysqlctl.BuiltinCompressor = "pgzip" }() + *mysqlctl.CompressionEngineName = "lz4" + defer func() { *mysqlctl.CompressionEngineName = "pgzip" }() // now bring up the other replica, letting it restore from backup. err = localCluster.VtctlclientProcess.InitTablet(replica2, cell, keyspaceName, hostname, shardName) require.Nil(t, err) diff --git a/go/test/endtoend/backup/vtctlbackup/backup_test.go b/go/test/endtoend/backup/vtctlbackup/backup_test.go index e4d5187fe8c..f46d5db3b60 100644 --- a/go/test/endtoend/backup/vtctlbackup/backup_test.go +++ b/go/test/endtoend/backup/vtctlbackup/backup_test.go @@ -40,7 +40,7 @@ func TestBackupMainWithZstdCompression(t *testing.T) { } func setDefaultCompressionFlag() { - *mysqlctl.BuiltinCompressor = "pgzip" + *mysqlctl.CompressionEngineName = "pgzip" *mysqlctl.ExternalCompressorCmd = "" *mysqlctl.ExternalCompressorExt = "" *mysqlctl.ExternalDecompressorCmd = "" diff --git a/go/test/endtoend/backup/vtctlbackup/backup_utils.go b/go/test/endtoend/backup/vtctlbackup/backup_utils.go index 19c73659d5f..573d1e4a26b 100644 --- a/go/test/endtoend/backup/vtctlbackup/backup_utils.go +++ b/go/test/endtoend/backup/vtctlbackup/backup_utils.go @@ -227,7 +227,7 @@ func getCompressorArgs(cDetails *CompressionDetails) []string { } if cDetails.BuiltinCompressor != "" { - args = append(args, fmt.Sprintf("--builtin-compressor=%s", cDetails.BuiltinCompressor)) + args = append(args, fmt.Sprintf("--compression-engine-name=%s", cDetails.BuiltinCompressor)) } if cDetails.BuiltinDecompressor != "" { args = append(args, fmt.Sprintf("--builtin-decompressor=%s", cDetails.BuiltinDecompressor)) diff --git a/go/test/endtoend/backup/xtrabackup/xtrabackup_test.go b/go/test/endtoend/backup/xtrabackup/xtrabackup_test.go index 28a5653d9ea..be1b6700a4b 100644 --- a/go/test/endtoend/backup/xtrabackup/xtrabackup_test.go +++ b/go/test/endtoend/backup/xtrabackup/xtrabackup_test.go @@ -31,7 +31,7 @@ func TestXtrabackup(t *testing.T) { func TestXtrabackWithZstdCompression(t *testing.T) { defer setDefaultCompressionFlag() - // BuiltinCompressor will be ignored in this case + // CompressionEngineName will be ignored in this case cDetails := &backup.CompressionDetails{ ExternalCompressorCmd: "zstd", ExternalCompressorExt: ".zst", @@ -42,7 +42,7 @@ func TestXtrabackWithZstdCompression(t *testing.T) { } func setDefaultCompressionFlag() { - *mysqlctl.BuiltinCompressor = "pgzip" + *mysqlctl.CompressionEngineName = "pgzip" *mysqlctl.ExternalCompressorCmd = "" *mysqlctl.ExternalCompressorExt = "" *mysqlctl.ExternalDecompressorCmd = "" diff --git a/go/test/endtoend/backup/xtrabackupstream/xtrabackup_stream_test.go b/go/test/endtoend/backup/xtrabackupstream/xtrabackup_stream_test.go index 01fc5ca6323..8e5f4805e1f 100644 --- a/go/test/endtoend/backup/xtrabackupstream/xtrabackup_stream_test.go +++ b/go/test/endtoend/backup/xtrabackupstream/xtrabackup_stream_test.go @@ -39,7 +39,7 @@ func TestXtrabackupStreamWithlz4Compression(t *testing.T) { } func setDefaultCompressionFlag() { - *mysqlctl.BuiltinCompressor = "pgzip" + *mysqlctl.CompressionEngineName = "pgzip" *mysqlctl.ExternalCompressorCmd = "" *mysqlctl.ExternalCompressorExt = "" *mysqlctl.ExternalDecompressorCmd = "" diff --git a/go/vt/mysqlctl/builtinbackupengine.go b/go/vt/mysqlctl/builtinbackupengine.go index 775c7d5664c..4f5bcf2837a 100644 --- a/go/vt/mysqlctl/builtinbackupengine.go +++ b/go/vt/mysqlctl/builtinbackupengine.go @@ -342,13 +342,6 @@ func (be *BuiltinBackupEngine) backupFiles(ctx context.Context, params BackupPar } }() - // set the correct value of compression engine used. - // if an external compressor is used, empty engine value - // is saved in the MANIFEST - var compressionEngineValue = *BuiltinCompressor - if *ExternalCompressorCmd != "" { - compressionEngineValue = ExternalCompressor - } // JSON-encode and write the MANIFEST bm := &builtinBackupManifest{ // Common base fields @@ -363,7 +356,7 @@ func (be *BuiltinBackupEngine) backupFiles(ctx context.Context, params BackupPar FileEntries: fes, TransformHook: *backupStorageHook, SkipCompress: !*backupStorageCompress, - CompressionEngine: compressionEngineValue, + CompressionEngine: *CompressionEngineName, } data, err := json.MarshalIndent(bm, "", " ") if err != nil { @@ -513,7 +506,7 @@ func (be *BuiltinBackupEngine) backupFile(ctx context.Context, params BackupPara if *ExternalCompressorCmd != "" { compressor, err = newExternalCompressor(ctx, *ExternalCompressorCmd, writer, params.Logger) } else { - compressor, err = newBuiltinCompressor(*BuiltinCompressor, writer, params.Logger) + compressor, err = newBuiltinCompressor(*CompressionEngineName, writer, params.Logger) } if err != nil { return vterrors.Wrap(err, "can't create compressor") @@ -613,7 +606,7 @@ func (be *BuiltinBackupEngine) restoreFiles(ctx context.Context, params RestoreP // And restore the file. name := fmt.Sprintf("%v", i) params.Logger.Infof("Copying file %v: %v", name, fes[i].Name) - err := be.restoreFile(ctx, params, bh, &fes[i], bm.TransformHook, !bm.SkipCompress, bm.CompressionEngine, name) + err := be.restoreFile(ctx, params, bh, &fes[i], bm, name) if err != nil { rec.RecordError(vterrors.Wrapf(err, "can't restore file %v to %v", name, fes[i].Name)) } @@ -624,7 +617,7 @@ func (be *BuiltinBackupEngine) restoreFiles(ctx context.Context, params RestoreP } // restoreFile restores an individual file. -func (be *BuiltinBackupEngine) restoreFile(ctx context.Context, params RestoreParams, bh backupstorage.BackupHandle, fe *FileEntry, transformHook string, compress bool, deCompressionEngine string, name string) (finalErr error) { +func (be *BuiltinBackupEngine) restoreFile(ctx context.Context, params RestoreParams, bh backupstorage.BackupHandle, fe *FileEntry, bm builtinBackupManifest, name string) (finalErr error) { // Open the source file for reading. source, err := bh.ReadFile(ctx, name) if err != nil { @@ -656,18 +649,19 @@ func (be *BuiltinBackupEngine) restoreFile(ctx context.Context, params RestorePa // Create the external read pipe, if any. var wait hook.WaitFunc - if transformHook != "" { - h := hook.NewHook(transformHook, []string{"-operation", "read"}) + if bm.TransformHook != "" { + h := hook.NewHook(bm.TransformHook, []string{"-operation", "read"}) h.ExtraEnv = params.HookExtraEnv reader, wait, _, err = h.ExecuteAsReadPipe(reader) if err != nil { - return vterrors.Wrapf(err, "'%v' hook returned error", transformHook) + return vterrors.Wrapf(err, "'%v' hook returned error", bm.TransformHook) } } // Create the uncompresser if needed. - if compress { + if !bm.SkipCompress { var decompressor io.ReadCloser + var deCompressionEngine = bm.CompressionEngine if deCompressionEngine == "" { // for backward compatibility deCompressionEngine = PgzipCompressor @@ -675,11 +669,13 @@ func (be *BuiltinBackupEngine) restoreFile(ctx context.Context, params RestorePa if *ExternalDecompressorCmd != "" { if deCompressionEngine == ExternalCompressor { deCompressionEngine = *ExternalDecompressorCmd + decompressor, err = newExternalDecompressor(ctx, deCompressionEngine, reader, params.Logger) + } else { + decompressor, err = newBuiltinDecompressor(deCompressionEngine, reader, params.Logger) } - decompressor, err = newExternalDecompressor(ctx, deCompressionEngine, reader, params.Logger) } else { if deCompressionEngine == ExternalCompressor { - return fmt.Errorf("%w %q", errUnsupportedCompressionEngine, ExternalCompressor) + return fmt.Errorf("%w value: %q", errUnsupportedDeCompressionEngine, ExternalCompressor) } decompressor, err = newBuiltinDecompressor(deCompressionEngine, reader, params.Logger) } @@ -710,10 +706,10 @@ func (be *BuiltinBackupEngine) restoreFile(ctx context.Context, params RestorePa if wait != nil { stderr, err := wait() if stderr != "" { - log.Infof("'%v' hook returned stderr: %v", transformHook, stderr) + log.Infof("'%v' hook returned stderr: %v", bm.TransformHook, stderr) } if err != nil { - return vterrors.Wrapf(err, "'%v' returned error", transformHook) + return vterrors.Wrapf(err, "'%v' returned error", bm.TransformHook) } } diff --git a/go/vt/mysqlctl/compression.go b/go/vt/mysqlctl/compression.go index 9f4f3842000..854e776971b 100644 --- a/go/vt/mysqlctl/compression.go +++ b/go/vt/mysqlctl/compression.go @@ -47,13 +47,14 @@ const ( var ( compressionLevel = flag.Int("compression-level", 1, "what level to pass to the compressor") // switch which compressor/decompressor to use - BuiltinCompressor = flag.String("builtin-compressor", "pgzip", "builtin compressor engine to use") + CompressionEngineName = flag.String("compression-engine-name", "pgzip", "compressor engine used for compression.") // use and external command to decompress the backups ExternalCompressorCmd = flag.String("external-compressor", "", "command with arguments to use when compressing a backup") ExternalCompressorExt = flag.String("external-compressor-extension", "", "extension to use when using an external compressor") ExternalDecompressorCmd = flag.String("external-decompressor", "", "command with arguments to use when decompressing a backup") - errUnsupportedCompressionEngine = errors.New("unsupported engine") + errUnsupportedDeCompressionEngine = errors.New("unsupported engine in manifest. `external' compression engine needs ExternalDecompressorCmd") + errUnsupportedCompressionEngine = errors.New("unsupported engine value for CompressionEngineName. supported values are `external`, `pgzip`, `pargzip`, `zstd`, `lz4`") // this is used by getEngineFromExtension() to figure out which engine to use in case the user didn't specify engineExtensions = map[string][]string{ @@ -82,6 +83,21 @@ func validateExternalCmd(cmd string) (string, error) { return exec.LookPath(cmd) } +// Validate compression engine is one of the supported values. +func validateExternalCompressionEngineName(engine string) error { + switch engine { + case PgzipCompressor: + case PargzipCompressor: + case Lz4Compressor: + case ZstdCompressor: + case ExternalCompressor: + default: + return fmt.Errorf("%w value: %q", errUnsupportedCompressionEngine, engine) + } + + return nil +} + func prepareExternalCmd(ctx context.Context, cmdStr string) (*exec.Cmd, error) { cmdArgs, err := shlex.Split(cmdStr) if err != nil { @@ -100,6 +116,10 @@ func prepareExternalCmd(ctx context.Context, cmdStr string) (*exec.Cmd, error) { // This returns a writer that writes the compressed output of the external command to the provided writer. func newExternalCompressor(ctx context.Context, cmdStr string, writer io.Writer, logger logutil.Logger) (io.WriteCloser, error) { logger.Infof("Compressing using external command: %q", cmdStr) + // validate value of compression engine name + if err := validateExternalCompressionEngineName(*CompressionEngineName); err != nil { + return nil, err + } cmd, err := prepareExternalCmd(ctx, cmdStr) if err != nil { @@ -216,7 +236,7 @@ func newBuiltinCompressor(engine string, writer io.Writer, logger logutil.Logger } compressor = zst default: - err = fmt.Errorf("Unkown compressor engine: %q", engine) + err = fmt.Errorf("%w value: %q", errUnsupportedCompressionEngine, engine) return compressor, err } diff --git a/go/vt/mysqlctl/compression_test.go b/go/vt/mysqlctl/compression_test.go index 2c3d8827228..72cb84b77bf 100644 --- a/go/vt/mysqlctl/compression_test.go +++ b/go/vt/mysqlctl/compression_test.go @@ -27,6 +27,8 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" + "vitess.io/vitess/go/vt/logutil" ) @@ -98,6 +100,17 @@ func TestBuiltinCompressors(t *testing.T) { } } +func TestUnSupportedBuiltinCompressors(t *testing.T) { + logger := logutil.NewMemoryLogger() + + for _, engine := range []string{"external", "foobar"} { + t.Run(engine, func(t *testing.T) { + _, err := newBuiltinCompressor(engine, nil, logger) + require.ErrorContains(t, err, "unsupported engine value for CompressionEngineName. supported values are `external`, `pgzip`, `pargzip`, `zstd`, `lz4` value:") + }) + } +} + func TestExternalCompressors(t *testing.T) { data := []byte("foo bar foobar") logger := logutil.NewMemoryLogger() @@ -194,3 +207,29 @@ func TestValidateExternalCmd(t *testing.T) { }) } } + +func TestValidateCompressionEngineName(t *testing.T) { + tests := []struct { + engineName string + errStr string + }{ + // we expect ls to be on PATH as it is a basic command part of busybox and most containers + {"external", ""}, + {"foobar", "unsupported engine value for CompressionEngineName. supported values are `external`, `pgzip`, `pargzip`, `zstd`, `lz4` value: \"foobar\""}, + } + + for i, tt := range tests { + t.Run(fmt.Sprintf("Test #%d", i+1), func(t *testing.T) { + err := validateExternalCompressionEngineName(tt.engineName) + if tt.errStr == "" { + if err != nil { + t.Errorf("Expected result \"%v\", got \"%v\"", "", err) + } + } else { + if !strings.Contains(fmt.Sprintf("%v", err), tt.errStr) { + t.Errorf("Expected result \"%v\", got \"%v\"", tt.errStr, err) + } + } + }) + } +} diff --git a/go/vt/mysqlctl/xtrabackupengine.go b/go/vt/mysqlctl/xtrabackupengine.go index 82fdc734c06..bebb44761f1 100644 --- a/go/vt/mysqlctl/xtrabackupengine.go +++ b/go/vt/mysqlctl/xtrabackupengine.go @@ -111,7 +111,7 @@ func (be *XtrabackupEngine) backupFileName() string { if *ExternalDecompressorCmd != "" { fileName += *ExternalCompressorExt } else { - if ext, err := getExtensionFromEngine(*BuiltinCompressor); err != nil { + if ext, err := getExtensionFromEngine(*CompressionEngineName); err != nil { // there is a check for this, but just in case that fails, we set a extension to the file fileName += ".unknown" } else { @@ -185,13 +185,6 @@ func (be *XtrabackupEngine) ExecuteBackup(ctx context.Context, params BackupPara } defer closeFile(mwc, backupManifestFileName, params.Logger, &finalErr) - // set the correct value of compression engine used. - // if an external compressor is used, empty engine value - // is saved in the MANIFEST - var compressionEngineValue = *BuiltinCompressor - if *ExternalCompressorCmd != "" { - compressionEngineValue = ExternalCompressor - } // JSON-encode and write the MANIFEST bm := &xtraBackupManifest{ // Common base fields @@ -210,7 +203,7 @@ func (be *XtrabackupEngine) ExecuteBackup(ctx context.Context, params BackupPara NumStripes: int32(numStripes), StripeBlockSize: int32(*xtrabackupStripeBlockSize), // builtin specific field - CompressionEngine: compressionEngineValue, + CompressionEngine: *CompressionEngineName, } data, err := json.MarshalIndent(bm, "", " ") @@ -310,7 +303,7 @@ func (be *XtrabackupEngine) backupFiles(ctx context.Context, params BackupParams if *ExternalCompressorCmd != "" { compressor, err = newExternalCompressor(ctx, *ExternalCompressorCmd, writer, params.Logger) } else { - compressor, err = newBuiltinCompressor(*BuiltinCompressor, writer, params.Logger) + compressor, err = newBuiltinCompressor(*CompressionEngineName, writer, params.Logger) } if err != nil { return replicationPosition, vterrors.Wrap(err, "can't create compressor") @@ -572,8 +565,10 @@ func (be *XtrabackupEngine) extractFiles(ctx context.Context, logger logutil.Log if *ExternalDecompressorCmd != "" { if deCompressionEngine == ExternalCompressor { deCompressionEngine = *ExternalDecompressorCmd + decompressor, err = newExternalDecompressor(ctx, deCompressionEngine, reader, logger) + } else { + decompressor, err = newBuiltinDecompressor(deCompressionEngine, reader, logger) } - decompressor, err = newExternalDecompressor(ctx, deCompressionEngine, reader, logger) } else { if deCompressionEngine == ExternalCompressor { return fmt.Errorf("%w %q", errUnsupportedCompressionEngine, ExternalCompressor) diff --git a/go/vt/wrangler/testlib/backup_test.go b/go/vt/wrangler/testlib/backup_test.go index 270f8a63e3f..d75b5e2b963 100644 --- a/go/vt/wrangler/testlib/backup_test.go +++ b/go/vt/wrangler/testlib/backup_test.go @@ -71,7 +71,7 @@ func TestBackupRestoreWithPargzip(t *testing.T) { } func setDefaultCompressionFlag() { - *mysqlctl.BuiltinCompressor = "pgzip" + *mysqlctl.CompressionEngineName = "pgzip" *mysqlctl.ExternalCompressorCmd = "" *mysqlctl.ExternalCompressorExt = "" *mysqlctl.ExternalDecompressorCmd = "" @@ -115,7 +115,7 @@ func testBackupRestore(t *testing.T, cDetails *compressionDetails) error { *backupstorage.BackupStorageImplementation = "file" if cDetails != nil { if cDetails.BuiltinCompressor != "" { - *mysqlctl.BuiltinCompressor = cDetails.BuiltinCompressor + *mysqlctl.CompressionEngineName = cDetails.BuiltinCompressor } if cDetails.ExternalCompressorCmd != "" { *mysqlctl.ExternalCompressorCmd = cDetails.ExternalCompressorCmd From 335ca1509ed2e925713f79682e1fb80a19127c56 Mon Sep 17 00:00:00 2001 From: Rameez Sajwani Date: Fri, 29 Jul 2022 14:25:29 -0700 Subject: [PATCH 08/18] Fixing / Adding new test case Signed-off-by: Rameez Sajwani --- go/flags/endtoend/vttablet.txt | 3 +- .../backup/mysqlctld/backup_mysqlctld_test.go | 2 +- .../backup/vtctlbackup/backup_test.go | 2 +- .../backup/vtctlbackup/backup_utils.go | 123 +++++++++++++++--- .../backup/xtrabackup/xtrabackup_test.go | 2 +- .../xtrabackup_stream_test.go | 2 +- 6 files changed, 108 insertions(+), 26 deletions(-) diff --git a/go/flags/endtoend/vttablet.txt b/go/flags/endtoend/vttablet.txt index ca914ea9100..ba51d3a46b2 100644 --- a/go/flags/endtoend/vttablet.txt +++ b/go/flags/endtoend/vttablet.txt @@ -27,12 +27,11 @@ Usage of vttablet: --binlog_ssl_key string PITR restore parameter: Filename containing mTLS client private key for use in binlog server authentication. --binlog_ssl_server_name string PITR restore parameter: TLS server name (common name) to verify against for the binlog server we are connecting to (If not set: use the hostname or IP supplied in -binlog_host). --binlog_user string PITR restore parameter: username of binlog server. - --compression-engine-name string compressor engine used for compression. (default "pgzip") - --builtin-decompressor string builtin decompressor engine to use (default "auto") --builtinbackup_mysqld_timeout duration how long to wait for mysqld to shutdown at the start of the backup (default 10m0s) --builtinbackup_progress duration how often to send progress updates when backing up large files (default 5s) --catch-sigpipe catch and ignore SIGPIPE on stdout and stderr if specified --ceph_backup_storage_config string Path to JSON config file for ceph backup storage. (default "ceph_backup_config.json") + --compression-engine-name string compressor engine used for compression. (default "pgzip") --compression-level int what level to pass to the compressor (default 1) --consul_auth_static_file string JSON File to read the topos/tokens from. --cpu_profile string deprecated: use '-pprof=cpu' instead diff --git a/go/test/endtoend/backup/mysqlctld/backup_mysqlctld_test.go b/go/test/endtoend/backup/mysqlctld/backup_mysqlctld_test.go index 63f92395ddd..fad48725c43 100644 --- a/go/test/endtoend/backup/mysqlctld/backup_mysqlctld_test.go +++ b/go/test/endtoend/backup/mysqlctld/backup_mysqlctld_test.go @@ -32,7 +32,7 @@ func TestBackupMysqlctld(t *testing.T) { func TestBackupMysqlctldWithlz4Compression(t *testing.T) { defer setDefaultCompressionFlag() cDetails := &backup.CompressionDetails{ - BuiltinCompressor: "lz4", + CompressorEngineName: "lz4", } backup.TestBackup(t, backup.Mysqlctld, "xbstream", 0, cDetails, []string{"TestReplicaBackup", "TestPrimaryBackup"}) diff --git a/go/test/endtoend/backup/vtctlbackup/backup_test.go b/go/test/endtoend/backup/vtctlbackup/backup_test.go index e9e65358ff1..2cac60a82bd 100644 --- a/go/test/endtoend/backup/vtctlbackup/backup_test.go +++ b/go/test/endtoend/backup/vtctlbackup/backup_test.go @@ -30,7 +30,7 @@ func TestBackupMain(t *testing.T) { func TestBackupMainWithZstdCompression(t *testing.T) { defer setDefaultCompressionFlag() cDetails := &CompressionDetails{ - BuiltinCompressor: "lz4", + CompressorEngineName: "zstd", ExternalCompressorCmd: "zstd", ExternalCompressorExt: ".zst", ExternalDecompressorCmd: "zstd -d", diff --git a/go/test/endtoend/backup/vtctlbackup/backup_utils.go b/go/test/endtoend/backup/vtctlbackup/backup_utils.go index 2277eb17efa..f4a2bb47d07 100644 --- a/go/test/endtoend/backup/vtctlbackup/backup_utils.go +++ b/go/test/endtoend/backup/vtctlbackup/backup_utils.go @@ -28,11 +28,10 @@ import ( "testing" "time" - "vitess.io/vitess/go/vt/proto/vtrpc" - "vitess.io/vitess/go/vt/vterrors" - "vitess.io/vitess/go/vt/mysqlctl" "vitess.io/vitess/go/vt/proto/topodata" + "vitess.io/vitess/go/vt/proto/vtrpc" + "vitess.io/vitess/go/vt/vterrors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -83,8 +82,7 @@ var ( ) type CompressionDetails struct { - BuiltinCompressor string - BuiltinDecompressor string + CompressorEngineName string ExternalCompressorCmd string ExternalCompressorExt string ExternalDecompressorCmd string @@ -226,11 +224,8 @@ func getCompressorArgs(cDetails *CompressionDetails) []string { return args } - if cDetails.BuiltinCompressor != "" { - args = append(args, fmt.Sprintf("--compression-engine-name=%s", cDetails.BuiltinCompressor)) - } - if cDetails.BuiltinDecompressor != "" { - args = append(args, fmt.Sprintf("--builtin-decompressor=%s", cDetails.BuiltinDecompressor)) + if cDetails.CompressorEngineName != "" { + args = append(args, fmt.Sprintf("--compression-engine-name=%s", cDetails.CompressorEngineName)) } if cDetails.ExternalCompressorCmd != "" { args = append(args, fmt.Sprintf("--external-compressor=%s", cDetails.ExternalCompressorCmd)) @@ -246,6 +241,22 @@ func getCompressorArgs(cDetails *CompressionDetails) []string { } +func updateCompressorArgs(commonArgs []string, cDetails *CompressionDetails) []string { + if cDetails == nil { + return commonArgs + } + + for i, s := range commonArgs { + if strings.Contains(s, "--compression-engine-name") == true || strings.Contains(s, "--external-compressor") == true || + strings.Contains(s, "--external-compressor-extension") == true || strings.Contains(s, "--external-decompressor") == true { + commonArgs = append(commonArgs[:i], commonArgs[i+1:]...) + } + } + + commonArgs = append(commonArgs, getCompressorArgs(cDetails)...) + return commonArgs +} + // TearDownCluster shuts down all cluster processes func TearDownCluster() { localCluster.Teardown() @@ -299,6 +310,10 @@ func TestBackup(t *testing.T, setupType int, streamMode string, stripes int, cDe name: "TestPrimaryReplicaSameBackup", method: primaryReplicaSameBackup, }, // + { + name: "primaryReplicaSameBackupModifiedCompressionEngine", + method: primaryReplicaSameBackupModifiedCompressionEngine, + }, // { name: "TestRestoreOldPrimaryByRestart", method: restoreOldPrimaryByRestart, @@ -322,7 +337,6 @@ func TestBackup(t *testing.T, setupType int, streamMode string, stripes int, cDe defer TearDownCluster() // Run all the backup tests - for _, test := range testMethods { if len(runSpecific) > 0 && !isRegistered(test.name, runSpecific) { continue @@ -381,7 +395,7 @@ func primaryBackup(t *testing.T) { _, err = primary.VttabletProcess.QueryTablet("insert into vt_insert_test (msg) values ('test2')", keyspaceName, true) require.Nil(t, err) - restoreWaitForBackup(t, "replica") + restoreWaitForBackup(t, "replica", nil) err = replica2.VttabletProcess.WaitForTabletStatusesForTimeout([]string{"SERVING"}, timeout) require.Nil(t, err) @@ -446,7 +460,6 @@ func primaryReplicaSameBackup(t *testing.T) { require.Nil(t, err) // now bring up the other replica, letting it restore from backup. - restoreWaitForBackup(t, "replica") err = replica2.VttabletProcess.WaitForTabletStatusesForTimeout([]string{"SERVING"}, timeout) require.Nil(t, err) @@ -477,14 +490,82 @@ func primaryReplicaSameBackup(t *testing.T) { err = localCluster.VtctlclientProcess.ExecuteCommand("Backup", replica1.Alias) require.Nil(t, err) - // Insert more data on replica2 (current primary). - _, err = replica2.VttabletProcess.QueryTablet("insert into vt_insert_test (msg) values ('test4')", keyspaceName, true) + // Force replica1 to restore from backup. + verifyRestoreTablet(t, replica1, "SERVING") + + cluster.VerifyRowsInTablet(t, replica1, keyspaceName, 4) + err = replica2.VttabletProcess.TearDown() + require.Nil(t, err) + restartPrimaryAndReplica(t) +} + +// Test a primary and replica from the same backup. +// +// Check that a replica and primary both restored from the same backup +// can replicate successfully. +func primaryReplicaSameBackupModifiedCompressionEngine(t *testing.T) { + // insert data on primary, wait for replica to get it + verifyInitialReplication(t) + + // backup the replica + err := localCluster.VtctlclientProcess.ExecuteCommand("Backup", replica1.Alias) + require.Nil(t, err) + + // insert more data on the primary + _, err = primary.VttabletProcess.QueryTablet("insert into vt_insert_test (msg) values ('test2')", keyspaceName, true) + require.Nil(t, err) + + // now bring up the other replica, with change in compression engine + // this is to verify that restore will read engine name from manifest instead of reading the new values + cDetails := &CompressionDetails{ + CompressorEngineName: "pgzip", + ExternalCompressorCmd: "gzip -c", + ExternalCompressorExt: "", + ExternalDecompressorCmd: "", + } + restoreWaitForBackup(t, "replica", cDetails) + err = replica2.VttabletProcess.WaitForTabletStatusesForTimeout([]string{"SERVING"}, timeout) + require.Nil(t, err) + + // check the new replica has the data + cluster.VerifyRowsInTablet(t, replica2, keyspaceName, 2) + + // Promote replica2 to primary + err = localCluster.VtctlclientProcess.ExecuteCommand("PlannedReparentShard", "--", + "--keyspace_shard", shardKsName, + "--new_primary", replica2.Alias) + require.Nil(t, err) + + // insert more data on replica2 (current primary) + _, err = replica2.VttabletProcess.QueryTablet("insert into vt_insert_test (msg) values ('test3')", keyspaceName, true) require.Nil(t, err) // Force replica1 to restore from backup. verifyRestoreTablet(t, replica1, "SERVING") - cluster.VerifyRowsInTablet(t, replica1, keyspaceName, 4) + // wait for replica1 to catch up. + cluster.VerifyRowsInTablet(t, replica1, keyspaceName, 3) + + // Promote replica1 to primary + err = localCluster.VtctlclientProcess.ExecuteCommand("PlannedReparentShard", "--", + "--keyspace_shard", shardKsName, + "--new_primary", replica1.Alias) + require.Nil(t, err) + + // Insert more data on replica1 (current primary). + _, err = replica1.VttabletProcess.QueryTablet("insert into vt_insert_test (msg) values ('test4')", keyspaceName, true) + require.Nil(t, err) + + // wait for replica2 to catch up. + cluster.VerifyRowsInTablet(t, replica2, keyspaceName, 4) + + // Now take replica2 backup with gzip (new compressor) + err = localCluster.VtctlclientProcess.ExecuteCommand("Backup", replica2.Alias) + require.Nil(t, err) + + // Force replica2 to restore from backup. + verifyRestoreTablet(t, replica2, "SERVING") + cluster.VerifyRowsInTablet(t, replica2, keyspaceName, 4) err = replica2.VttabletProcess.TearDown() require.Nil(t, err) restartPrimaryAndReplica(t) @@ -672,7 +753,7 @@ func terminatedRestore(t *testing.T) { // // func vtctlBackup(t *testing.T, tabletType string) { - restoreWaitForBackup(t, tabletType) + restoreWaitForBackup(t, tabletType, nil) verifyInitialReplication(t) err := localCluster.VtctlclientProcess.ExecuteCommand("Backup", replica1.Alias) @@ -715,11 +796,14 @@ func verifyInitialReplication(t *testing.T) { // Override the backup engine implementation to a non-existent one for restore. // This setting should only matter for taking new backups. We should be able // to restore a previous backup successfully regardless of this setting. -func restoreWaitForBackup(t *testing.T, tabletType string) { +func restoreWaitForBackup(t *testing.T, tabletType string, cDetails *CompressionDetails) { replica2.Type = tabletType replica2.ValidateTabletRestart(t) replicaTabletArgs := commonTabletArg - replicaTabletArgs = append(replicaTabletArgs, "--backup_engine_implementation", "fake_implementation") + if cDetails != nil { + replicaTabletArgs = updateCompressorArgs(replicaTabletArgs, cDetails) + } + //replicaTabletArgs = append(replicaTabletArgs, "--backup_engine_implementation", "fake_implementation") replicaTabletArgs = append(replicaTabletArgs, "--wait_for_backup_interval", "1s") replicaTabletArgs = append(replicaTabletArgs, "--init_tablet_type", tabletType) replica2.VttabletProcess.ExtraArgs = replicaTabletArgs @@ -740,7 +824,6 @@ func verifyAfterRemovingBackupNoBackupShouldBePresent(t *testing.T, backups []st } func verifyRestoreTablet(t *testing.T, tablet *cluster.Vttablet, status string) { - tablet.ValidateTabletRestart(t) tablet.VttabletProcess.ServingStatus = "" err := tablet.VttabletProcess.Setup() diff --git a/go/test/endtoend/backup/xtrabackup/xtrabackup_test.go b/go/test/endtoend/backup/xtrabackup/xtrabackup_test.go index be1b6700a4b..40100b100bd 100644 --- a/go/test/endtoend/backup/xtrabackup/xtrabackup_test.go +++ b/go/test/endtoend/backup/xtrabackup/xtrabackup_test.go @@ -31,8 +31,8 @@ func TestXtrabackup(t *testing.T) { func TestXtrabackWithZstdCompression(t *testing.T) { defer setDefaultCompressionFlag() - // CompressionEngineName will be ignored in this case cDetails := &backup.CompressionDetails{ + CompressorEngineName: "zstd", ExternalCompressorCmd: "zstd", ExternalCompressorExt: ".zst", ExternalDecompressorCmd: "zstd -d", diff --git a/go/test/endtoend/backup/xtrabackupstream/xtrabackup_stream_test.go b/go/test/endtoend/backup/xtrabackupstream/xtrabackup_stream_test.go index 8e5f4805e1f..10fe7945511 100644 --- a/go/test/endtoend/backup/xtrabackupstream/xtrabackup_stream_test.go +++ b/go/test/endtoend/backup/xtrabackupstream/xtrabackup_stream_test.go @@ -32,7 +32,7 @@ func TestXtrabackupStream(t *testing.T) { func TestXtrabackupStreamWithlz4Compression(t *testing.T) { defer setDefaultCompressionFlag() cDetails := &backup.CompressionDetails{ - BuiltinCompressor: "lz4", + CompressorEngineName: "lz4", } backup.TestBackup(t, backup.XtraBackup, "xbstream", 8, cDetails, []string{"TestReplicaBackup"}) From e58148d9cb7fe287b30805d66760f5b9e48d92f0 Mon Sep 17 00:00:00 2001 From: Rameez Sajwani Date: Fri, 29 Jul 2022 15:01:34 -0700 Subject: [PATCH 09/18] Fix summary & static code analysis Signed-off-by: Rameez Sajwani --- .../endtoend/backup/vtctlbackup/backup_utils.go | 14 +++++++++----- .../xtrabackupstream/xtrabackup_stream_test.go | 2 +- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/go/test/endtoend/backup/vtctlbackup/backup_utils.go b/go/test/endtoend/backup/vtctlbackup/backup_utils.go index f4a2bb47d07..aa8102239e2 100644 --- a/go/test/endtoend/backup/vtctlbackup/backup_utils.go +++ b/go/test/endtoend/backup/vtctlbackup/backup_utils.go @@ -247,8 +247,8 @@ func updateCompressorArgs(commonArgs []string, cDetails *CompressionDetails) []s } for i, s := range commonArgs { - if strings.Contains(s, "--compression-engine-name") == true || strings.Contains(s, "--external-compressor") == true || - strings.Contains(s, "--external-compressor-extension") == true || strings.Contains(s, "--external-decompressor") == true { + if strings.Contains(s, "--compression-engine-name") || strings.Contains(s, "--external-compressor") || + strings.Contains(s, "--external-compressor-extension") || strings.Contains(s, "--external-decompressor") { commonArgs = append(commonArgs[:i], commonArgs[i+1:]...) } } @@ -460,6 +460,7 @@ func primaryReplicaSameBackup(t *testing.T) { require.Nil(t, err) // now bring up the other replica, letting it restore from backup. + restoreWaitForBackup(t, "replica", nil) err = replica2.VttabletProcess.WaitForTabletStatusesForTimeout([]string{"SERVING"}, timeout) require.Nil(t, err) @@ -490,6 +491,10 @@ func primaryReplicaSameBackup(t *testing.T) { err = localCluster.VtctlclientProcess.ExecuteCommand("Backup", replica1.Alias) require.Nil(t, err) + // Insert more data on replica2 (current primary). + _, err = replica2.VttabletProcess.QueryTablet("insert into vt_insert_test (msg) values ('test4')", keyspaceName, true) + require.Nil(t, err) + // Force replica1 to restore from backup. verifyRestoreTablet(t, replica1, "SERVING") @@ -502,7 +507,7 @@ func primaryReplicaSameBackup(t *testing.T) { // Test a primary and replica from the same backup. // // Check that a replica and primary both restored from the same backup -// can replicate successfully. +// We change compression alogrithm in between but it should not break any restore functionality func primaryReplicaSameBackupModifiedCompressionEngine(t *testing.T) { // insert data on primary, wait for replica to get it verifyInitialReplication(t) @@ -520,7 +525,7 @@ func primaryReplicaSameBackupModifiedCompressionEngine(t *testing.T) { cDetails := &CompressionDetails{ CompressorEngineName: "pgzip", ExternalCompressorCmd: "gzip -c", - ExternalCompressorExt: "", + ExternalCompressorExt: ".gz", ExternalDecompressorCmd: "", } restoreWaitForBackup(t, "replica", cDetails) @@ -803,7 +808,6 @@ func restoreWaitForBackup(t *testing.T, tabletType string, cDetails *Compression if cDetails != nil { replicaTabletArgs = updateCompressorArgs(replicaTabletArgs, cDetails) } - //replicaTabletArgs = append(replicaTabletArgs, "--backup_engine_implementation", "fake_implementation") replicaTabletArgs = append(replicaTabletArgs, "--wait_for_backup_interval", "1s") replicaTabletArgs = append(replicaTabletArgs, "--init_tablet_type", tabletType) replica2.VttabletProcess.ExtraArgs = replicaTabletArgs diff --git a/go/test/endtoend/backup/xtrabackupstream/xtrabackup_stream_test.go b/go/test/endtoend/backup/xtrabackupstream/xtrabackup_stream_test.go index 10fe7945511..e5ca596138f 100644 --- a/go/test/endtoend/backup/xtrabackupstream/xtrabackup_stream_test.go +++ b/go/test/endtoend/backup/xtrabackupstream/xtrabackup_stream_test.go @@ -35,7 +35,7 @@ func TestXtrabackupStreamWithlz4Compression(t *testing.T) { CompressorEngineName: "lz4", } - backup.TestBackup(t, backup.XtraBackup, "xbstream", 8, cDetails, []string{"TestReplicaBackup"}) + backup.TestBackup(t, backup.XtraBackup, "xbstream", 8, cDetails, []string{"primaryReplicaSameBackupModifiedCompressionEngine"}) } func setDefaultCompressionFlag() { From 06d402eb3fde1e9d4fcbe055f51b21f35dff7d45 Mon Sep 17 00:00:00 2001 From: Rameez Sajwani Date: Sun, 31 Jul 2022 19:02:52 -0700 Subject: [PATCH 10/18] Adding fake backup impl in test Signed-off-by: Rameez Sajwani --- go/test/endtoend/backup/vtctlbackup/backup_utils.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/go/test/endtoend/backup/vtctlbackup/backup_utils.go b/go/test/endtoend/backup/vtctlbackup/backup_utils.go index aa8102239e2..1923d3b6941 100644 --- a/go/test/endtoend/backup/vtctlbackup/backup_utils.go +++ b/go/test/endtoend/backup/vtctlbackup/backup_utils.go @@ -395,7 +395,7 @@ func primaryBackup(t *testing.T) { _, err = primary.VttabletProcess.QueryTablet("insert into vt_insert_test (msg) values ('test2')", keyspaceName, true) require.Nil(t, err) - restoreWaitForBackup(t, "replica", nil) + restoreWaitForBackup(t, "replica", nil, true) err = replica2.VttabletProcess.WaitForTabletStatusesForTimeout([]string{"SERVING"}, timeout) require.Nil(t, err) @@ -460,7 +460,7 @@ func primaryReplicaSameBackup(t *testing.T) { require.Nil(t, err) // now bring up the other replica, letting it restore from backup. - restoreWaitForBackup(t, "replica", nil) + restoreWaitForBackup(t, "replica", nil, true) err = replica2.VttabletProcess.WaitForTabletStatusesForTimeout([]string{"SERVING"}, timeout) require.Nil(t, err) @@ -528,7 +528,7 @@ func primaryReplicaSameBackupModifiedCompressionEngine(t *testing.T) { ExternalCompressorExt: ".gz", ExternalDecompressorCmd: "", } - restoreWaitForBackup(t, "replica", cDetails) + restoreWaitForBackup(t, "replica", cDetails, false) err = replica2.VttabletProcess.WaitForTabletStatusesForTimeout([]string{"SERVING"}, timeout) require.Nil(t, err) @@ -758,7 +758,7 @@ func terminatedRestore(t *testing.T) { // // func vtctlBackup(t *testing.T, tabletType string) { - restoreWaitForBackup(t, tabletType, nil) + restoreWaitForBackup(t, tabletType, nil, true) verifyInitialReplication(t) err := localCluster.VtctlclientProcess.ExecuteCommand("Backup", replica1.Alias) @@ -801,13 +801,16 @@ func verifyInitialReplication(t *testing.T) { // Override the backup engine implementation to a non-existent one for restore. // This setting should only matter for taking new backups. We should be able // to restore a previous backup successfully regardless of this setting. -func restoreWaitForBackup(t *testing.T, tabletType string, cDetails *CompressionDetails) { +func restoreWaitForBackup(t *testing.T, tabletType string, cDetails *CompressionDetails, fakeImpl bool) { replica2.Type = tabletType replica2.ValidateTabletRestart(t) replicaTabletArgs := commonTabletArg if cDetails != nil { replicaTabletArgs = updateCompressorArgs(replicaTabletArgs, cDetails) } + if fakeImpl { + replicaTabletArgs = append(replicaTabletArgs, "--backup_engine_implementation", "fake_implementation") + } replicaTabletArgs = append(replicaTabletArgs, "--wait_for_backup_interval", "1s") replicaTabletArgs = append(replicaTabletArgs, "--init_tablet_type", tabletType) replica2.VttabletProcess.ExtraArgs = replicaTabletArgs From 6f8f741e36cf2a2caa3c6abad9e5b7c0072e39a2 Mon Sep 17 00:00:00 2001 From: Rameez Sajwani Date: Sun, 31 Jul 2022 22:04:58 -0700 Subject: [PATCH 11/18] Adding time sleep in between test Signed-off-by: Rameez Sajwani --- go/test/endtoend/backup/vtctlbackup/backup_utils.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/go/test/endtoend/backup/vtctlbackup/backup_utils.go b/go/test/endtoend/backup/vtctlbackup/backup_utils.go index 1923d3b6941..1486d07fbe7 100644 --- a/go/test/endtoend/backup/vtctlbackup/backup_utils.go +++ b/go/test/endtoend/backup/vtctlbackup/backup_utils.go @@ -512,6 +512,11 @@ func primaryReplicaSameBackupModifiedCompressionEngine(t *testing.T) { // insert data on primary, wait for replica to get it verifyInitialReplication(t) + // TODO: The following Sleep in introduced as it seems like the previous step doesn't fully complete, causing + // this test to be flaky. Sleep seems to solve the problem. Need to fix this in a better way and Wait for + // previous test to complete (suspicion: MySQL does not fully start) + time.Sleep(5 * time.Second) + // backup the replica err := localCluster.VtctlclientProcess.ExecuteCommand("Backup", replica1.Alias) require.Nil(t, err) From 63fff2d9d0e0bfe6c8ab29e92add1347527a8328 Mon Sep 17 00:00:00 2001 From: Rameez Sajwani Date: Mon, 1 Aug 2022 10:35:10 -0700 Subject: [PATCH 12/18] Fixing summary and adding comments Signed-off-by: Rameez Sajwani --- doc/releasenotes/15_0_0_summary.md | 26 ++++++++++++------- .../backup/vtctlbackup/backup_utils.go | 3 +++ go/vt/mysqlctl/builtinbackupengine.go | 3 ++- go/vt/mysqlctl/xtrabackupengine.go | 3 ++- 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/doc/releasenotes/15_0_0_summary.md b/doc/releasenotes/15_0_0_summary.md index 15e743e9964..57eaec4d202 100644 --- a/doc/releasenotes/15_0_0_summary.md +++ b/doc/releasenotes/15_0_0_summary.md @@ -98,33 +98,39 @@ Backup/Restore now allow you many more options for compression and decompression There are some built-in compressors which you can use out-of-the-box. Users will need to evaluate which option works best for their use-case. Here are the flags that control this feature -- --builtin-compressor +- --compression-engine-name - --external-compressor - --external-decompressor - --external-compressor-extension - --compression-level -builtin compressor as of today supports the following options +`--compression-engine-name` specifies the engine used for compression. It can have one of the following values + - pgzip - pargzip - lz4 - zstd +- external -If you want to use any of the builtin compressors, simply set one of the above values for `--builtin-compressor`. During restore the system -will read the MANIFEST file to get the compressor engine value and use that value for decompression. +where 'external' is set only when we are using compressor other than the ones mentioned above. If you want to use any of the built-in compressors, +simply set one of the above values for `--compression-engine-name`. Value specified in `--compression-engine-name` is saved in MANIFEST, +which is later read by restore routine to decide what engine to use for decompression. Default value for engine is 'pgzip'. If you would like to use a custom command or external tool for compression/decompression then you need to provide the full command with arguments to the `--external-compressor` and `--external-decompressor` flags. `--external-compressor-extension` flag also needs to be provided -so that compressed files are created with the correct extension. There is no need to override `--builtin-compressor` when using an external -compressor/decompressor. Please note that if you want the current behavior then you don't need to change anything in these flags. +so that compressed files are created with the correct extension. If the external command is not using any of the built-in compression engine +(i-e pgzip, pargzip, lz4 or zstd) then you need to set `--compression-engine-name` to value 'external'. + +Please note that if you want the current production behavior then you don't need to change any of these flags. You can read more about backup & restore [here] (https://vitess.io/docs/15.0/user-guides/operating-vitess/backup-and-restore/). -Important Note: If you decided to switch from external compressor to built-in compressor at any point in the future. You might need to do it -in two steps. -- step #1, set `--external-compressor` and `--external-compressor-extension` flag values to empty and change `--builtin-compressor` to desired value. +Important Note: If you decided to switch from external compressor to built-in supported compressors (i-e pgzi,, pargzip, lz4 or zstd) at any point +in the future. You might need to do it in two steps. + +- step #1, set `--external-compressor` and `--external-compressor-extension` flag values to empty and change `--compression-engine-name` to desired value. - Step #2, after few cycles of backups now set `--external-decompressor` flag value to empty. -The reason we recommend not to change all the values together is because builtin compressor have no way to find out which external decompressor +The reason we recommend not to change all the values together is because restore routine have no way to find out which external decompressor being used during the previous backups. Please make sure you have thought out all possible scenarios for restore before transitioning from one compressor value to the others. diff --git a/go/test/endtoend/backup/vtctlbackup/backup_utils.go b/go/test/endtoend/backup/vtctlbackup/backup_utils.go index 1486d07fbe7..707b59abe15 100644 --- a/go/test/endtoend/backup/vtctlbackup/backup_utils.go +++ b/go/test/endtoend/backup/vtctlbackup/backup_utils.go @@ -241,11 +241,13 @@ func getCompressorArgs(cDetails *CompressionDetails) []string { } +// update arguments with new values of compressionDetail. func updateCompressorArgs(commonArgs []string, cDetails *CompressionDetails) []string { if cDetails == nil { return commonArgs } + // remove if any compression flag already exists for i, s := range commonArgs { if strings.Contains(s, "--compression-engine-name") || strings.Contains(s, "--external-compressor") || strings.Contains(s, "--external-compressor-extension") || strings.Contains(s, "--external-decompressor") { @@ -253,6 +255,7 @@ func updateCompressorArgs(commonArgs []string, cDetails *CompressionDetails) []s } } + // update it with new values commonArgs = append(commonArgs, getCompressorArgs(cDetails)...) return commonArgs } diff --git a/go/vt/mysqlctl/builtinbackupengine.go b/go/vt/mysqlctl/builtinbackupengine.go index 4f5bcf2837a..c7b5f0d2556 100644 --- a/go/vt/mysqlctl/builtinbackupengine.go +++ b/go/vt/mysqlctl/builtinbackupengine.go @@ -77,7 +77,8 @@ type builtinBackupManifest struct { // CompressionEngine stores which compression engine was originally provided // to compress the files. Please note that if user has provided externalCompressorCmd - // then this value will not be used during decompression. + // then it will contain value 'external'. This field is used during restore routine to + // get a hint about what kind of compression was used. CompressionEngine string `json:",omitempty"` // FileEntries contains all the files in the backup diff --git a/go/vt/mysqlctl/xtrabackupengine.go b/go/vt/mysqlctl/xtrabackupengine.go index bebb44761f1..20a45f4dd2e 100644 --- a/go/vt/mysqlctl/xtrabackupengine.go +++ b/go/vt/mysqlctl/xtrabackupengine.go @@ -81,7 +81,8 @@ type xtraBackupManifest struct { BackupManifest // CompressionEngine stores which compression engine was originally provided // to compress the files. Please note that if user has provided externalCompressorCmd - // then this value will not be used during decompression. + // then it will contain value 'external'. This field is used during restore routine to + // get a hint about what kind of compression was used. CompressionEngine string `json:",omitempty"` // Name of the backup file FileName string From 4255327732ab790949edc3ac43ae50843c2a72bd Mon Sep 17 00:00:00 2001 From: Rameez Sajwani Date: Mon, 8 Aug 2022 12:48:55 -0700 Subject: [PATCH 13/18] Feedback on summary Signed-off-by: Rameez Sajwani --- doc/releasenotes/15_0_0_summary.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/releasenotes/15_0_0_summary.md b/doc/releasenotes/15_0_0_summary.md index 57eaec4d202..6d63c574271 100644 --- a/doc/releasenotes/15_0_0_summary.md +++ b/doc/releasenotes/15_0_0_summary.md @@ -112,7 +112,7 @@ use-case. Here are the flags that control this feature - zstd - external -where 'external' is set only when we are using compressor other than the ones mentioned above. If you want to use any of the built-in compressors, +where 'external' is set only when we are using custom command or external tool other than the ones mentioned above. If you want to use any of the built-in compressors, simply set one of the above values for `--compression-engine-name`. Value specified in `--compression-engine-name` is saved in MANIFEST, which is later read by restore routine to decide what engine to use for decompression. Default value for engine is 'pgzip'. @@ -124,11 +124,11 @@ so that compressed files are created with the correct extension. If the external Please note that if you want the current production behavior then you don't need to change any of these flags. You can read more about backup & restore [here] (https://vitess.io/docs/15.0/user-guides/operating-vitess/backup-and-restore/). -Important Note: If you decided to switch from external compressor to built-in supported compressors (i-e pgzi,, pargzip, lz4 or zstd) at any point +If you decided to switch from external compressor to built-in supported compressors (i-e pgzip, pargzip, lz4 or zstd) at any point in the future. You might need to do it in two steps. - step #1, set `--external-compressor` and `--external-compressor-extension` flag values to empty and change `--compression-engine-name` to desired value. -- Step #2, after few cycles of backups now set `--external-decompressor` flag value to empty. +- Step #2, after at least one cycle of backup with new configuration, you can set `--external-decompressor` flag value to empty. The reason we recommend not to change all the values together is because restore routine have no way to find out which external decompressor being used during the previous backups. Please make sure you have thought out all possible scenarios for restore before transitioning from one From 0ea91f0654d68c14ac1fbda5fb2e1fe1b48f2e05 Mon Sep 17 00:00:00 2001 From: Rameez Sajwani Date: Tue, 9 Aug 2022 16:37:06 -0700 Subject: [PATCH 14/18] Code review feedback Signed-off-by: Rameez Sajwani --- doc/releasenotes/15_0_0_summary.md | 9 +++++---- go/vt/mysqlctl/compression.go | 4 ++-- go/vt/wrangler/testlib/backup_test.go | 8 ++++---- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/doc/releasenotes/15_0_0_summary.md b/doc/releasenotes/15_0_0_summary.md index 6d63c574271..e3dd49da2ce 100644 --- a/doc/releasenotes/15_0_0_summary.md +++ b/doc/releasenotes/15_0_0_summary.md @@ -112,13 +112,14 @@ use-case. Here are the flags that control this feature - zstd - external -where 'external' is set only when we are using custom command or external tool other than the ones mentioned above. If you want to use any of the built-in compressors, -simply set one of the above values for `--compression-engine-name`. Value specified in `--compression-engine-name` is saved in MANIFEST, -which is later read by restore routine to decide what engine to use for decompression. Default value for engine is 'pgzip'. +where 'external' is set only when using a custom command or tool other than the ones that are already provided. +If you want to use any of the built-in compressors, simply set one of the above values for `--compression-engine-name`. The value +specified in `--compression-engine-name` is saved in the backup MANIFEST, which is later read by the restore process to decide which +engine to use for decompression. Default value for engine is 'pgzip'. If you would like to use a custom command or external tool for compression/decompression then you need to provide the full command with arguments to the `--external-compressor` and `--external-decompressor` flags. `--external-compressor-extension` flag also needs to be provided -so that compressed files are created with the correct extension. If the external command is not using any of the built-in compression engine +so that compressed files are created with the correct extension. If the external command is not using any of the built-in compression engines (i-e pgzip, pargzip, lz4 or zstd) then you need to set `--compression-engine-name` to value 'external'. Please note that if you want the current production behavior then you don't need to change any of these flags. diff --git a/go/vt/mysqlctl/compression.go b/go/vt/mysqlctl/compression.go index 854e776971b..0c1c0d34b29 100644 --- a/go/vt/mysqlctl/compression.go +++ b/go/vt/mysqlctl/compression.go @@ -53,8 +53,8 @@ var ( ExternalCompressorExt = flag.String("external-compressor-extension", "", "extension to use when using an external compressor") ExternalDecompressorCmd = flag.String("external-decompressor", "", "command with arguments to use when decompressing a backup") - errUnsupportedDeCompressionEngine = errors.New("unsupported engine in manifest. `external' compression engine needs ExternalDecompressorCmd") - errUnsupportedCompressionEngine = errors.New("unsupported engine value for CompressionEngineName. supported values are `external`, `pgzip`, `pargzip`, `zstd`, `lz4`") + errUnsupportedDeCompressionEngine = errors.New("unsupported engine in MANIFEST. You need to provide --external-decompressor if using 'external' compression engine") + errUnsupportedCompressionEngine = errors.New("unsupported engine value for --compression-engine-name. supported values are 'external', 'pgzip', 'pargzip', 'zstd', 'lz4'") // this is used by getEngineFromExtension() to figure out which engine to use in case the user didn't specify engineExtensions = map[string][]string{ diff --git a/go/vt/wrangler/testlib/backup_test.go b/go/vt/wrangler/testlib/backup_test.go index 49f72580320..f866e7ff5bd 100644 --- a/go/vt/wrangler/testlib/backup_test.go +++ b/go/vt/wrangler/testlib/backup_test.go @@ -46,7 +46,7 @@ import ( ) type compressionDetails struct { - BuiltinCompressor string + CompressionEngineName string ExternalCompressorCmd string ExternalCompressorExt string ExternalDecompressorCmd string @@ -63,7 +63,7 @@ func TestBackupRestore(t *testing.T) { func TestBackupRestoreWithPargzip(t *testing.T) { defer setDefaultCompressionFlag() cDetails := &compressionDetails{ - BuiltinCompressor: "pargzip", + CompressionEngineName: "pargzip", } err := testBackupRestore(t, cDetails) @@ -114,8 +114,8 @@ func testBackupRestore(t *testing.T, cDetails *compressionDetails) error { filebackupstorage.FileBackupStorageRoot = fbsRoot backupstorage.BackupStorageImplementation = "file" if cDetails != nil { - if cDetails.BuiltinCompressor != "" { - *mysqlctl.CompressionEngineName = cDetails.BuiltinCompressor + if cDetails.CompressionEngineName != "" { + *mysqlctl.CompressionEngineName = cDetails.CompressionEngineName } if cDetails.ExternalCompressorCmd != "" { *mysqlctl.ExternalCompressorCmd = cDetails.ExternalCompressorCmd From f423de3d8790d041b8d17e943fa071e440720e8b Mon Sep 17 00:00:00 2001 From: Rameez Sajwani Date: Tue, 9 Aug 2022 22:08:07 -0700 Subject: [PATCH 15/18] Fixing comment Signed-off-by: Rameez Sajwani --- go/vt/wrangler/testlib/backup_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/go/vt/wrangler/testlib/backup_test.go b/go/vt/wrangler/testlib/backup_test.go index f866e7ff5bd..07129c5d157 100644 --- a/go/vt/wrangler/testlib/backup_test.go +++ b/go/vt/wrangler/testlib/backup_test.go @@ -58,8 +58,6 @@ func TestBackupRestore(t *testing.T) { require.NoError(t, err) } -// Given we infer decompressor through Manifest file. -// It is only in xtrabackup where we infer decompressor through extension & BuiltinDecompressor param. func TestBackupRestoreWithPargzip(t *testing.T) { defer setDefaultCompressionFlag() cDetails := &compressionDetails{ From b9c65117dedce9a574e7d825cd7702c4ed920bdc Mon Sep 17 00:00:00 2001 From: Rameez Sajwani Date: Wed, 10 Aug 2022 08:28:05 -0700 Subject: [PATCH 16/18] Fixing default value in summary Signed-off-by: Rameez Sajwani --- doc/releasenotes/15_0_0_summary.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/releasenotes/15_0_0_summary.md b/doc/releasenotes/15_0_0_summary.md index e3dd49da2ce..2df3bcd29d0 100644 --- a/doc/releasenotes/15_0_0_summary.md +++ b/doc/releasenotes/15_0_0_summary.md @@ -106,7 +106,7 @@ use-case. Here are the flags that control this feature `--compression-engine-name` specifies the engine used for compression. It can have one of the following values -- pgzip +- pgzip (Default) - pargzip - lz4 - zstd From 3c072efc6b53fbb7e8c2d7610bf9ab835a7ed96d Mon Sep 17 00:00:00 2001 From: Rameez Sajwani Date: Wed, 10 Aug 2022 10:45:47 -0700 Subject: [PATCH 17/18] Fixing test cases Signed-off-by: Rameez Sajwani --- go/vt/mysqlctl/compression_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/vt/mysqlctl/compression_test.go b/go/vt/mysqlctl/compression_test.go index 72cb84b77bf..4215761dbe7 100644 --- a/go/vt/mysqlctl/compression_test.go +++ b/go/vt/mysqlctl/compression_test.go @@ -106,7 +106,7 @@ func TestUnSupportedBuiltinCompressors(t *testing.T) { for _, engine := range []string{"external", "foobar"} { t.Run(engine, func(t *testing.T) { _, err := newBuiltinCompressor(engine, nil, logger) - require.ErrorContains(t, err, "unsupported engine value for CompressionEngineName. supported values are `external`, `pgzip`, `pargzip`, `zstd`, `lz4` value:") + require.ErrorContains(t, err, "unsupported engine value for --compression-engine-name. supported values are 'external', 'pgzip', 'pargzip', 'zstd', 'lz4' value:") }) } } @@ -215,7 +215,7 @@ func TestValidateCompressionEngineName(t *testing.T) { }{ // we expect ls to be on PATH as it is a basic command part of busybox and most containers {"external", ""}, - {"foobar", "unsupported engine value for CompressionEngineName. supported values are `external`, `pgzip`, `pargzip`, `zstd`, `lz4` value: \"foobar\""}, + {"foobar", "unsupported engine value for --compression-engine-name. supported values are 'external', 'pgzip', 'pargzip', 'zstd', 'lz4' value: \"foobar\""}, } for i, tt := range tests { From a56f9d2dacc9b7db244321660915de3d13a50df4 Mon Sep 17 00:00:00 2001 From: Rameez Sajwani Date: Wed, 10 Aug 2022 15:32:18 -0700 Subject: [PATCH 18/18] More summary fixes Signed-off-by: Rameez Sajwani --- .../upgrade_downgrade_test_backups_manual.yml | 14 +++++++------- doc/releasenotes/15_0_0_summary.md | 10 +++++----- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/.github/workflows/upgrade_downgrade_test_backups_manual.yml b/.github/workflows/upgrade_downgrade_test_backups_manual.yml index cb8b9a22760..1ef0bdb5233 100644 --- a/.github/workflows/upgrade_downgrade_test_backups_manual.yml +++ b/.github/workflows/upgrade_downgrade_test_backups_manual.yml @@ -120,13 +120,13 @@ jobs: #sudo apt-get update #sudo DEBIAN_FRONTEND="noninteractive" apt-get install -y mysql-server mysql-client #### - wget -c https://cdn.mysql.com/archives/mysql-8.0/mysql-common_8.0.29-1ubuntu20.04_amd64.deb \ - https://cdn.mysql.com/archives/mysql-8.0/mysql-community-client-core_8.0.29-1ubuntu20.04_amd64.deb \ - https://cdn.mysql.com/archives/mysql-8.0/mysql-community-client-plugins_8.0.29-1ubuntu20.04_amd64.deb \ - https://cdn.mysql.com/archives/mysql-8.0/mysql-client_8.0.29-1ubuntu20.04_amd64.deb \ - https://cdn.mysql.com/archives/mysql-8.0/mysql-community-server-core_8.0.29-1ubuntu20.04_amd64.deb \ - https://cdn.mysql.com/archives/mysql-8.0/mysql-community-server_8.0.29-1ubuntu20.04_amd64.deb \ - https://cdn.mysql.com/archives/mysql-8.0/mysql-community-client_8.0.29-1ubuntu20.04_amd64.deb + wget -c https://cdn.mysql.com/archives/mysql-8.0/mysql-common_8.0.28-1ubuntu20.04_amd64.deb \ + https://cdn.mysql.com/archives/mysql-8.0/mysql-community-client-core_8.0.28-1ubuntu20.04_amd64.deb \ + https://cdn.mysql.com/archives/mysql-8.0/mysql-community-client-plugins_8.0.28-1ubuntu20.04_amd64.deb \ + https://cdn.mysql.com/archives/mysql-8.0/mysql-client_8.0.28-1ubuntu20.04_amd64.deb \ + https://cdn.mysql.com/archives/mysql-8.0/mysql-community-server-core_8.0.28-1ubuntu20.04_amd64.deb \ + https://cdn.mysql.com/archives/mysql-8.0/mysql-community-server_8.0.28-1ubuntu20.04_amd64.deb \ + https://cdn.mysql.com/archives/mysql-8.0/mysql-community-client_8.0.28-1ubuntu20.04_amd64.deb sudo DEBIAN_FRONTEND="noninteractive" apt-get install -y ./mysql-*.deb # Install everything else we need, and configure diff --git a/doc/releasenotes/15_0_0_summary.md b/doc/releasenotes/15_0_0_summary.md index 01c78ea2c20..b52735109bb 100644 --- a/doc/releasenotes/15_0_0_summary.md +++ b/doc/releasenotes/15_0_0_summary.md @@ -126,15 +126,15 @@ so that compressed files are created with the correct extension. If the external Please note that if you want the current production behavior then you don't need to change any of these flags. You can read more about backup & restore [here] (https://vitess.io/docs/15.0/user-guides/operating-vitess/backup-and-restore/). -If you decided to switch from external compressor to built-in supported compressors (i-e pgzip, pargzip, lz4 or zstd) at any point -in the future. You might need to do it in two steps. +If you decided to switch from an external compressor to one of the built-in supported compressors (i-e pgzip, pargzip, lz4 or zstd) at any point +in the future, you will need to do it in two steps. - step #1, set `--external-compressor` and `--external-compressor-extension` flag values to empty and change `--compression-engine-name` to desired value. - Step #2, after at least one cycle of backup with new configuration, you can set `--external-decompressor` flag value to empty. -The reason we recommend not to change all the values together is because restore routine have no way to find out which external decompressor -being used during the previous backups. Please make sure you have thought out all possible scenarios for restore before transitioning from one -compressor value to the others. +The reason you cannot change all the values together is because the restore process will then have no way to find out which external decompressor +should be used to process the previous backup. Please make sure you have thought out all possible scenarios for restore before transitioning from one +compression engine to another. ### Online DDL changes