-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat(storage): restore tool #16352
feat(storage): restore tool #16352
Conversation
8606b7a
to
55eb211
Compare
Adds a restore tool which does offline restore of data and metadata.
55eb211
to
fd869ef
Compare
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.
Small nits and a couple of questions, but otherwise looks awesome to me!
cmd/influxd/restore/command.go
Outdated
|
||
func restoreEngine() error { | ||
dataDir := filepath.Join(flags.enginePath, "/data") | ||
if err := os.Mkdir(dataDir, 0777); err != nil { |
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.
Should this be os.MkdirAll(dataDir, 0777)
in case we need to define parents?
cmd/influxd/restore/command.go
Outdated
return err | ||
} | ||
|
||
return os.Mkdir(flags.enginePath, 0777) |
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.
I think this should be os.MkdirAll(flags.enginePath, 0777)
to ensure the parents are made too. Since we check to see if /var/lib/path/to/data/engine
exists in L145, we should make sure that this is fully created. Alternatively if we're just changing the perms we should use Chmod
?
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.
I would rather not worry about chmod'ing an existing dir here. I agree that MkdirAll
makes sense.
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.
Looks good, found a few small issues. I would like to test this out before it gets merged. I'll comment when I've done that.
cmd/influxd/restore/command.go
Outdated
return err | ||
} | ||
|
||
return os.Mkdir(flags.enginePath, 0777) |
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.
I would rather not worry about chmod'ing an existing dir here. I agree that MkdirAll
makes sense.
cmd/influxd/restore/command.go
Outdated
|
||
func restoreBolt() error { | ||
backupBolt := filepath.Join(flags.backupPath, bolt.DefaultFilename) | ||
f, err := os.OpenFile(backupBolt, os.O_RDONLY, 0666) |
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.
Use os.Open
instead.
} | ||
defer f.Close() | ||
|
||
w, err := os.OpenFile(flags.boltPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0666) |
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 not use os.Open
here, as the file is opened for writing)
count := 0 | ||
err := filepath.Walk(flags.backupPath, func(path string, info os.FileInfo, err error) error { | ||
if strings.Contains(path, ".tsm") { | ||
f, err := os.OpenFile(path, os.O_RDONLY, 0600) |
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.
Use os.Open
Tested by creating a backup, restoring and verifying:
|
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.
One more fix to the MkdirAll
and it's good to go from me!
cmd/influxd/restore/command.go
Outdated
return err | ||
} | ||
|
||
return os.Mkdir(flags.enginePath, 0777) |
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.
This one still should be changed to MkdirAll
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.
LGTM!
Replaced by #16504 |
Adds an offline restore tool to replace existing data and metadata with a backup.
Closes #15605
Closes #15606