go/vt/mysqlctl: add configurable read buffer to builtin backups #12073

maxenglander · 2023-01-11T04:11:07Z

Description

The builtin backup engine reads files from disk, optionally compresses them, and sends them to backupstorage.

Currently, the backup engine uses the *os.File returned by os.Open to read files from disk.

vitess/go/vt/mysqlctl/builtinbackupengine.go

Line 166 in 7fc1b48

if fd, err = os.Open(name); err != nil {

It then passes this to io.Copy.

vitess/go/vt/mysqlctl/builtinbackupengine.go

Line 718 in 7fc1b48

_, err = io.Copy(writer, br)

io.Copy reads in blocks of 32*1024 bytes

This is not optimal in all environments. In some environments, it's better to read bigger blocks less frequently.

Changes

This PR:

Adds support for read-buffering.
Makes both read and write buffering configurable via flags.
Decouples the write buffer sizes for writing-files-to-disk-during-restore vs. writing-to-backupstorage-during-backups.

Evidence

Coming up with a benchmark to prove that this is useful is difficult, but anecdotally I've seen a modest performance improvement from increasing the size of the read buffer. I'm not sure if the performance improvement is coming from using the hardware more effectively, or if there are more subtle interactions happening between all the moving pieces of mysqlctl.Backup.

My test environment looks like this:

Shard with a single ~37 GiB table (mostly UUIDs).
Run vtbackup on...
An AWS r5a.xlarge (4 vcpu, 32 GiB mem).
With an EBS GP3 provisioned with 6K IOPS and 250 MiB/s throughput.
In a container, with 24 GiB mem limit, no CPU limit.
Backupstorage is a customized version of s3backupstorage that does some extra stuff.
Using compression-engine=external, external-compressor="zstd -c -T4 -1".

Running vtbackup, the baseline backup time is ~215s.
When I add a read buffer of 2MiB, that goes down to ~175s (~18% improvement).

If we feel it's needed, I can try to put together more complete & reproducible evidence.

Related Issue(s)

Fixes #12069

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Documentation: content/en/docs: v17 --builtinbackup-file-read-buffer-size and --builtinbackup-file-write-buffer-size website#1365

Deployment Notes

vitess-bot · 2023-01-11T04:11:09Z

Signed-off-by: Max Englander <[email protected]>

maxenglander · 2023-01-11T19:01:20Z

go/vt/mysqlctl/builtinbackupengine.go

@@ -63,6 +62,16 @@ var (
 	BuiltinBackupMysqldTimeout = 10 * time.Minute

 	builtinBackupProgress = 5 * time.Second
+
+	// Controls the size of blocks read from disk during backups.
+	builtinBackupFileReadBufferSize = 0


Set to zero so that this is opt-in. When set to zero the Golang hard-coded buffer size of 32*1024 is used.

maxenglander · 2023-01-11T19:03:44Z

go/vt/mysqlctl/builtinbackupengine.go

+
+	// Controls the byte block size of writes to backupstorage during backups.
+	// The backupstorage may be a physical file, network, or something else.
+	builtinBackupStorageWriteBufferSize = 2 * 1024 * 1024 /* 2 MiB */


The current behavior is that we use the same 2 MiB buffer when writing files to disk during restores or writing to backupstorage during backups.

Backupstorage may or may not go directly to disk, so I think it makes sense to decouple these buffers to they can be tuned independently.

I think it would be even better if builtinbackup didn't do any buffering when reading to or writing from backupstorage, and instead would be better to let each backupstorage engine decide how to handle read/write buffering, maybe with tunable flags.

rsajwani · 2023-01-26T17:09:20Z

Changes look good to me .. Few observations

we need to update release notes with this change
we need to update website with this change
I am ok with this change. We are adding metrics for backup and restore time. Is it possible if we deploy that change first collect data for a week or so and then add this change. This will help us gauge if the values are helping us or not.

rsajwani · 2023-01-26T17:14:38Z

go/vt/mysqlctl/builtinbackupengine.go

@@ -128,6 +137,8 @@ func init() {
 func registerBuiltinBackupEngineFlags(fs *pflag.FlagSet) {
 	fs.DurationVar(&BuiltinBackupMysqldTimeout, "builtinbackup_mysqld_timeout", BuiltinBackupMysqldTimeout, "how long to wait for mysqld to shutdown at the start of the backup.")
 	fs.DurationVar(&builtinBackupProgress, "builtinbackup_progress", builtinBackupProgress, "how often to send progress updates when backing up large files.")
+	fs.IntVar(&builtinBackupFileReadBufferSize, "builtinbackup-file-read-buffer-size", builtinBackupFileReadBufferSize, "read files from disk in blocks of this many bytes. Golang defaults are used when set to 0.")


minor: should we make it UintVar instead?

I think you need to generate help text again ... Rest looks good

Signed-off-by: Max Englander <[email protected]>

…ltinbackup-file-write-buffer-size Signed-off-by: Max Englander <[email protected]>

Signed-off-by: Max Englander <[email protected]>

maxenglander · 2023-01-27T03:01:53Z

Is it possible if we deploy that change first collect data for a week or so and then add this change. This will help us gauge if the values are helping us or not.

That's definitely possible. This PR does not change current behavior, so in my opinion it would be fine to merge this first, but wait until after metrics PR is in prod before turning on these flags in prod. I leave it to you to decide :)

Signed-off-by: Max Englander <[email protected]>

rsajwani · 2023-02-06T22:19:29Z

Looks good to me .. there is failure in vreplication_across_db_versions, which is because of #12253.

doc/releasenotes/17_0_0_summary.md

deepthi · 2023-02-08T19:41:12Z

go/vt/mysqlctl/xtrabackupengine.go

@@ -66,6 +66,7 @@ var (

 const (
 	streamModeTar        = "tar"
+	writerBufferSize     = 2 * 1024 * 1024 /*2 MiB*/


why was this moved from builtin to this file? It's not actually used by xtrabackupengine AFAICT.

It is used!

vitess/go/vt/mysqlctl/xtrabackupengine.go

Line 327 in da69672

buffer := bufio.NewWriterSize(file, writerBufferSize)

ah it was already being used. And it is no longer used in builtin. 👍

Co-authored-by: Deepthi Sigireddi <[email protected]> Signed-off-by: Max Englander <[email protected]>

Signed-off-by: Max Englander <[email protected]>

mattlord · 2023-02-08T17:50:45Z

go/flags/endtoend/vttestserver.txt

+      --builtinbackup-file-read-buffer-size uint                         read files from disk in blocks of this many bytes. Golang defaults are used when set to 0.
+      --builtinbackup-file-write-buffer-size uint                        write files to disk in blocks of this many bytes. (default 2097152)


These have nothing to do with block device block sizes, right? If so, IMO it would be a little more clear to say chunks instead of blocks in this context. Or it may be even more clear/precise to say that we use a buffer of this size each time that we read/write backup data from disk?

maxenglander force-pushed the maxeng-compress-read-buf branch from 1b9d15d to 7f53b0a Compare January 11, 2023 06:06

go/vt/mysqlctl: add configurable read buffer to builtin backups

1abc241

Signed-off-by: Max Englander <[email protected]>

maxenglander force-pushed the maxeng-compress-read-buf branch from 7f53b0a to 1abc241 Compare January 11, 2023 06:32

maxenglander commented Jan 11, 2023

View reviewed changes

maxenglander added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Backup and Restore labels Jan 11, 2023

maxenglander marked this pull request as ready for review January 11, 2023 19:04

maxenglander requested review from deepthi, mattlord and rsajwani as code owners January 11, 2023 19:04

rsajwani reviewed Jan 26, 2023

View reviewed changes

maxenglander added 2 commits January 26, 2023 16:45

pr review: use uint

3404450

Signed-off-by: Max Englander <[email protected]>

doc/releasenotes: v16 --builtinbackup-file-read-buffer-size and --bui…

2c649a8

…ltinbackup-file-write-buffer-size Signed-off-by: Max Englander <[email protected]>

maxenglander requested review from frouioui and GuptaManan100 as code owners January 26, 2023 21:55

maxenglander mentioned this pull request Jan 26, 2023

content/en/docs: v17 --builtinbackup-file-read-buffer-size and --builtinbackup-file-write-buffer-size vitessio/website#1365

Merged

update flags txt

6d43daa

Signed-off-by: Max Englander <[email protected]>

maxenglander requested a review from rsajwani January 27, 2023 03:49

maxenglander added 2 commits February 6, 2023 10:47

make part of v17

4a7b8a6

Signed-off-by: Max Englander <[email protected]>

merge <- main

2790b51

Signed-off-by: Max Englander <[email protected]>

rsajwani approved these changes Feb 6, 2023

View reviewed changes

deepthi reviewed Feb 8, 2023

View reviewed changes

maxenglander and others added 2 commits February 8, 2023 14:43

Update doc/releasenotes/17_0_0_summary.md

80bd81b

Co-authored-by: Deepthi Sigireddi <[email protected]> Signed-off-by: Max Englander <[email protected]>

improve wording -block

0db048d

Signed-off-by: Max Englander <[email protected]>

maxenglander requested a review from rohit-nayak-ps as a code owner February 8, 2023 20:37

maxenglander requested review from harshit-gangal, shlomi-noach, systay, arthurschreiber, ajm188, notfelineit, vmg, derekperkins and dkhenry as code owners February 8, 2023 20:37

maxenglander force-pushed the maxeng-compress-read-buf branch from c64e4cf to 0db048d Compare February 8, 2023 20:39

mattlord approved these changes Feb 9, 2023

View reviewed changes

mattlord merged commit b22b419 into vitessio:main Feb 9, 2023

mattlord deleted the maxeng-compress-read-buf branch February 9, 2023 18:35

maxenglander mentioned this pull request Mar 6, 2023

go/{vt,flags}: register builtin backup flags with vtbackup #12558

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

go/vt/mysqlctl: add configurable read buffer to builtin backups #12073

go/vt/mysqlctl: add configurable read buffer to builtin backups #12073

maxenglander commented Jan 11, 2023 •

edited

Loading

vitess-bot bot commented Jan 11, 2023 •

edited by deepthi

Loading

maxenglander Jan 11, 2023

maxenglander Jan 11, 2023

rsajwani commented Jan 26, 2023

rsajwani Jan 26, 2023

maxenglander Jan 26, 2023

rsajwani Jan 26, 2023

maxenglander Jan 27, 2023

maxenglander commented Jan 27, 2023 •

edited

Loading

rsajwani commented Feb 6, 2023

deepthi Feb 8, 2023

maxenglander Feb 8, 2023

deepthi Feb 8, 2023

mattlord Feb 8, 2023

		--builtinbackup-file-read-buffer-size uint read files from disk in blocks of this many bytes. Golang defaults are used when set to 0.
		--builtinbackup-file-write-buffer-size uint write files to disk in blocks of this many bytes. (default 2097152)

go/vt/mysqlctl: add configurable read buffer to builtin backups #12073

go/vt/mysqlctl: add configurable read buffer to builtin backups #12073

Conversation

maxenglander commented Jan 11, 2023 • edited Loading

Description

Changes

Evidence

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Jan 11, 2023 • edited by deepthi Loading

Review Checklist

General

If a new flag is being introduced:

If a workflow is added or modified:

Bug fixes

Non-trivial changes

New/Existing features

Backward compatibility

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsajwani commented Jan 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxenglander commented Jan 27, 2023 • edited Loading

rsajwani commented Feb 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxenglander commented Jan 11, 2023 •

edited

Loading

vitess-bot bot commented Jan 11, 2023 •

edited by deepthi

Loading

maxenglander commented Jan 27, 2023 •

edited

Loading