chanbackup: archive old channel backup files #9232
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Abdulkbk
force-pushed
the
archive-channel-backups
branch
from
bd7fb87 to
71ee590
Compare
Abdulkbk
force-pushed
the
archive-channel-backups
branch
from
71ee590 to
375e0fb
Compare
|
@Abdulkbk, remember to re-request review from reviewers when ready |
|
LGTM 👌 |
Abdulkbk
changed the title
|
Could you do a rebase and push so the CI can run again, wanna check what's going in the logs but they are expired. |
Abdulkbk
force-pushed
the
archive-channel-backups
branch
from
375e0fb to
ade9d50
Compare
|
I've rebased and pushed |
yyforyongyu
added
enhancement
chanbackup/backupfile.go
Outdated
| @@ -117,6 +130,19 @@ func (b *MultiFile) UpdateAndSwap(newBackup PackedMulti) error { | |||
| return fmt.Errorf("unable to close file: %w", err) | |||
| } | |||
|
|
|||
| // We check if the main backup file exists, if it does we archive it | |||
| // before replacing it with the new backup. | |||
| if _, err := os.Stat(b.fileName); err == nil { | |||
There was a problem hiding this comment.
Q: should we return the error here instead?
There was a problem hiding this comment.
If we return an error here, LND will fail to start during the first run because channel.backup does not exist then. Should we add a log here instead?
I will also update the comment to capture this.
There was a problem hiding this comment.
I would consider this a two-step process - during the initialization phase, we create all the necessary dirs, similar to how to handle other file creation or db init, which is done in the config. Then the second phase is when we actually make updates to it - this way we can always catch errors earlier if the creation of the folder fails. That being said, it def needs more refactoring so maybe a future PR.
As for now, I think a debug log can be nice - so at least we won't let the error passes silently.
There was a problem hiding this comment.
I added a debug log 👍
config.go
Outdated
| @@ -360,6 +366,8 @@ type Config struct { | |||
| MaxPendingChannels int `long:"maxpendingchannels" description:"The maximum number of incoming pending channels permitted per peer."` | |||
| BackupFilePath string `long:"backupfilepath" description:"The target location of the channel backup file"` | |||
|
|
|||
| DisableBackupArchive bool `long:"disable-backup-archive" description:"If set to true, channel backups will be deleted or replaced rather than being archived to a separate location."` | |||
There was a problem hiding this comment.
think it'd be better if we could name it ArchiveBackup - then we don't need to use a double negative to decide whether we wanna create this archive or not.
There was a problem hiding this comment.
The problem with this approach is when you try to disable this feature (since its enabled by default) using a CLI arg. Running the command lnd --archivebackup=false results in an error although setting archivebackup=false in the lnd.conf file works fine.
I think the alternative is to disable this feature by default and then the user can then run lnd --archivebackup to enable it without getting any errors. In this case there won't be any need for lnd --archivebackup=false
There was a problem hiding this comment.
hmm ok so this is on by default, then nvm.
There was a problem hiding this comment.
Alternative naming would be no-backup-archive, which is a bit shorter and fits in better with other no- flags we have.
There was a problem hiding this comment.
now using the no-backup-archive, and it looks much neater.
chanbackup/backupfile_test.go
Outdated
| @@ -94,7 +94,7 @@ func TestUpdateAndSwap(t *testing.T) { | |||
| }, | |||
| } | |||
| for i, testCase := range testCases { | |||
| backupFile := NewMultiFile(testCase.fileName) | |||
| backupFile := NewMultiFile(testCase.fileName, false) | |||
There was a problem hiding this comment.
think this test should also be updated to check the new behavior, maybe a new test TestUpdateAndSwapWithArchive.
There was a problem hiding this comment.
The test is in the next commit ade9d50, I will squash it into the first commit.
There was a problem hiding this comment.
yeah that test focuses on createArchiveFile, which is great, but we should also test NewMultiFile(filename, true) to check that when the archive flag is on, we correctly create the archive and swap the file.
There was a problem hiding this comment.
I updated the TestUpdateAndSwap to evaluate the new behavior. I think adding a new test would lead to much duplication.
Abdulkbk
force-pushed
the
archive-channel-backups
branch
3 times, most recently
from
bdcf713 to
8cf0088
Compare
|
Hi @yyforyongyu , thank you for the review. I've addressed your feedback and left some comments/questions. |
Abdulkbk
force-pushed
the
archive-channel-backups
branch
from
8cf0088 to
1051ab4
Compare
|
I think this PR is ready for another look :) |
chanbackup/backupfile.go
Outdated
| } else { | ||
| log.Infof("Archiving old backup file at %v", b.fileName) | ||
|
|
||
| if err := createArchiveFile( |
There was a problem hiding this comment.
nit: this is pre-existing so no need to change, but for future PRs we'd prefer
err := createArchiveFile(...)
if err != nil {
...as it saves more indentation space and IMO a bit nicer to read.
There was a problem hiding this comment.
I have updated this to use the suggested
err := createArchiveFile(...)
if err != nil {
...It's more readable like you mentioned 👍
| }, | ||
|
|
||
| // Test with deleteOldBackup set to false - should create | ||
| // archive. |
chanbackup/backupfile.go
Outdated
| // Archive the existing main backup file before creating a new one. | ||
| // Note that if no backup file exists yet (e.g. on the first startup | ||
| // of the node), os.Stat will return an error, which we just log | ||
| // and not error out. | ||
| _, err = os.Stat(b.fileName) | ||
| if err != nil { | ||
| log.Debugf("Unable to get backup file info: %v", err) | ||
| } else { | ||
| log.Infof("Archiving old backup file at %v", b.fileName) | ||
|
|
||
| if err := createArchiveFile( | ||
| b.archiveDir, b.fileName, | ||
| ); err != nil { | ||
| return fmt.Errorf("unable to archive old backup file:"+ | ||
| " %w", err) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I think all of this logic should go into createArchiveFile().
Then you don't need the } else { case here either, because you can just do:
_, err = os.Stat(b.fileName)
if err != nil {
log.Debugf("Unable to get backup file info: %v", err)
return nil
}
There was a problem hiding this comment.
Now that I use the lnrpc.FileFileExists, the } else { is not there anymore.
backupExists := lnrpc.FileExists(b.fileName)
if backupExists {
log.Infof("Archiving old channel backup to %v",
b.archiveDir)
err := createArchiveFile(
b.archiveDir, b.fileName,
)
if err != nil {
return fmt.Errorf("unable to archive old "+
"channel backup file: %w", err)
}
}
chanbackup/backupfile.go
Outdated
| // and not error out. | ||
| _, err = os.Stat(b.fileName) | ||
| if err != nil { | ||
| log.Debugf("Unable to get backup file info: %v", err) |
There was a problem hiding this comment.
If you saw this error message as a user, would you be able to:
- Know what's going on?
- Tell if you need to react to it or not?
I think we should either:
- Test if the error is a "file not found error" (see ) and if it is not log anything.
Line 8 in 70c874b
- Directly use
lnrpc.FileExistsbefore even trying to do anything. - Update the error message to be more informative, perhaps something like "Cannot archive channel backup file, unable to get file info: %v".
There was a problem hiding this comment.
I go with 2, called the lnrpc.FileExists directly and if it returns true we proceed with creating the archive.
chanbackup/backupfile.go
Outdated
| } | ||
| defer func() { | ||
| if cerr := oldBackupFile.Close(); cerr != nil && err == nil { | ||
| err = fmt.Errorf("unable to close old backup file: %w", |
There was a problem hiding this comment.
I think this assignment doesn't do anything, since we don't have a named return variable. So perhaps just log the error? Same further below.
There was a problem hiding this comment.
Now logging the error 👍
chanbackup/backupfile_test.go
Outdated
| { | ||
| name: "invalid archive directory permissions", | ||
| setup: func() { | ||
| // Create dir with no write permissions |
There was a problem hiding this comment.
nit: missing punctuation in comment, here and below.
chanbackup/backupfile.go
Outdated
| ); err != nil { | ||
| return fmt.Errorf("unable to archive old backup file:"+ | ||
| " %w", err) | ||
| if !b.deleteOldBackup { |
There was a problem hiding this comment.
One more flag to pass into createArchiveFile, then this can just be an early return.
No need to create that many levels of nesting.
There was a problem hiding this comment.
One more flag to pass into createArchiveFile,
I didn't quite understand this part, but now that we're using lnrpc.FileExists and the previous nesting is gone, does this solve the issue?
if !b.noBackupArchive {
// Archive the main backup file if it exists before replacing
// it with a new one.
backupExists := lnrpc.FileExists(b.fileName)
if backupExists {
log.Infof("Archiving old channel backup to %v",
b.archiveDir)
err := createArchiveFile(
b.archiveDir, b.fileName,
)
if err != nil {
return fmt.Errorf("unable to archive old "+
"channel backup file: %w", err)
}
}
}| ) | ||
| // The archived content should match the previous | ||
| // backup (newPackedMulti) that was just swapped out. | ||
| assertBackupMatches(t, archiveFile, newPackedMulti) |
There was a problem hiding this comment.
Just add a continue here? Whenever I see an } else { that indicates to me that something can probably be optimized to make reading the code more easy.
There was a problem hiding this comment.
Done.
Whenever I see an } else { that indicates to me that something can probably be optimized to make reading the code more easy.
great to have this information 👍
config.go
Outdated
| @@ -360,6 +366,8 @@ type Config struct { | |||
| MaxPendingChannels int `long:"maxpendingchannels" description:"The maximum number of incoming pending channels permitted per peer."` | |||
| BackupFilePath string `long:"backupfilepath" description:"The target location of the channel backup file"` | |||
|
|
|||
| DisableBackupArchive bool `long:"disable-backup-archive" description:"If set to true, channel backups will be deleted or replaced rather than being archived to a separate location."` | |||
There was a problem hiding this comment.
Alternative naming would be no-backup-archive, which is a bit shorter and fits in better with other no- flags we have.
config.go
Outdated
| // channel backups. When false (default), old backups are archived to a | ||
| // designated location. When true, old backups are simply deleted or | ||
| // replaced. | ||
| defaultDisableBackupArchive = false |
There was a problem hiding this comment.
A boolean value always has false as the default. I don't think we need this constant or any assignment in DefaultConfig().
In this commit, we first check if a previous backup file exists, if it does we copy it to archive folder before replacing it with a new backup file. We also added a test for archiving chan backups.
Abdulkbk
force-pushed
the
archive-channel-backups
branch
from
1051ab4 to
76a3b56
Compare
In this commit, we add the --no-backup-archive with a default as false. When set to true then previous channel backup file will not be archived but replaced. We also modify TestUpdateAndSwap test to make sure the new behaviour works as expected.
Abdulkbk
force-pushed
the
archive-channel-backups
branch
from
76a3b56 to
55f328c
Compare
Thank you for taking a look @guggero. I have addressed your feedback and added some comments as well. |
Closes #8906
Change Description
From the feature description:
This PR allows for the archiving of channel backups, enabling users to choose whether to permanently delete previous backup files or archive them by moving them to a designated archives folder.
Problem:
LND currently overwrites old channel backup files when creating new ones. This means:
Solution:
We modify the current flow of creating
channel.backup. We start by creating a folder in the same directory where we store thechannel.backupfile (chan-backup-archives). We ensure that thechannel.backupfile is copied to the archive directory and timestamped before finally replacing it with the new file.We set the LND's default behavior to archive old channel backups, but we provide a configuration option that can be passed as an argument or specified in
lnd.confto disable this behavior. This option allows LND to deletechannel.backupfiles instead of archiving them. This can be achieved by settingno-backup-archive=trueinlnd.confor passed as argument:Steps to Test
no-backup-archive=trueis not set inlnd.conf):channel.backupfile is stored. You will find a new folder calledchan-backup-archives, where the backup files are archived.To test disabling this feature
chan-backup-archivesdirectory will not be created (if it does not already exist). If it already exists a new archive will not be created.Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.