-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Incremental restore: copy files under Vitess dataroot dir #15440
Incremental restore: copy files under Vitess dataroot dir #15440
Conversation
Signed-off-by: Shlomi Noach <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15440 +/- ##
==========================================
+ Coverage 65.64% 65.65% +0.01%
==========================================
Files 1562 1563 +1
Lines 194318 194380 +62
==========================================
+ Hits 127553 127619 +66
+ Misses 66765 66761 -4 ☔ View full report in Codecov by Sentry. |
@@ -1006,7 +1008,7 @@ func (be *BuiltinBackupEngine) restoreFiles(ctx context.Context, params RestoreP | |||
} | |||
|
|||
if bm.Incremental { | |||
createdDir, err = os.MkdirTemp("", "restore-incremental-*") | |||
createdDir, err = os.MkdirTemp(vtenv.VtDataRoot(), "restore-incremental-*") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we copy only the binary logs here or the full backup? If it's the full backup, this can have a significant effect on the storage needed and we might want to make this configurable (I'm assuming that the vtdataroot has another copy of the actual data that was restored prior to this step?). /tmp
may also be direct attached storage or tmpfs
which could cause a significant performance impact when using VTDATAROOT
. It may make sense for this to be a flag with the default value being /tmp
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To answer some of my own questions... 🙂
So we’re restoring an incremental backup here, which was only binary logs to begin with. And incremental is always done by the builtinbackupengine
, regardless of which engine you’re using for full backups.
Converted to |
We chose #15451 as an alternative approach. |
Description
During an incremental restore process, the built-in engine copies backup files onto a temporary directory. Right now this temporary directory is rooted at the default OS temp dir (i.e.
/tmp
on Linux).In
k8s
deployments, this directory is not shared among pods. This means that the files written by the build in engine on one pod, are not visible tomysqlctl
on themysqld
pod, wheremysqlbinlog
binary expects to find and apply them.This PR roots the temporary directory under the Vitess data root.
Related Issue(s)
Fixes #14765
Checklist
Deployment Notes