Skip to content
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

wallet, rpc: Implement backup file management functionality #1735

Merged
merged 4 commits into from
Sep 13, 2020

Conversation

jamescowens
Copy link
Member

@jamescowens jamescowens commented Jun 15, 2020

This PR includes a new ManageBackups function. This function can be invoked at any time by calling the corresponding managebackups rpc wrapper function. It will also be called when the backupwallet rpc function is executed (either manually or by automated schedule).

ManageBackups automates the deletion of old backup files, and is based on the core of the log archivers I wrote for the scraper log and the new main log. I will discuss it from the point of view of the rpc wrapper function managebackups.

managebackups takes two optional arguments (that must be specified as a pair if specified at all).

retention_by_number
retention_by_days

Both of these numbers must be non-negative integers.

For the function to do anything, -maintainbackupretention must be set as an argument to the gridcoin executable, or specified in the config file with maintainbackupretention=1. If maintainbackupretention is not set, the function will exit immediately. This conservative approach is to ENSURE that there is positive intent to delete old backup files.

If no arguments are provided, then the two settings will be picked up from the config file, if provided

walletbackupretainnumfiles=
walletbackupretainnumdays=

The function does validation of the settings, and if both are specified < 7, then both are clamped to 7 to prevent an unintended disaster.

The default settings, if neither setting is specified, are 365 for each, which guarantees that a minimum of 365 files, or 365 days of each type will be retained, whichever is greater.

The function works by taking each file type, which for this purpose is the filename prefix in front of the datetime string, and sorting (for each file type), by the datetime in descending order. Then the function deletes all files that either are beyond the desired file count OR earlier than the retention days, whichever results in the MOST files being retained.

Please see the testing log for examples of the output of the function. Here is the trivial JSON output where no deleted files were found...
{ "manage_backup_file_retention_success": true, "number_of_files_removed": 0, "files_removed": [ ] }

I have not put zip compression in here yet. That is reserved for a future PR after more discussion.

@jamescowens jamescowens added this to the Fern milestone Jun 15, 2020
@jamescowens jamescowens force-pushed the managebackups branch 2 times, most recently from 210537c to 97cf51b Compare June 15, 2020 03:01
@jamescowens jamescowens requested a review from cyrossignol June 15, 2020 03:06
@jamescowens jamescowens self-assigned this Jun 15, 2020
@jamescowens
Copy link
Member Author

@jamescowens jamescowens marked this pull request as ready for review June 15, 2020 03:18
@jamescowens jamescowens force-pushed the managebackups branch 2 times, most recently from 4f5cb41 to e619c60 Compare June 15, 2020 05:36
@div72
Copy link
Member

div72 commented Jun 15, 2020

Wouldn't it be better if manage_backup_retention check was in backupwallet? I would expect the wallet to manage backups if I called managebackups with the proper arguments regardless of options in the conf.

@jamescowens
Copy link
Member Author

jamescowens commented Jun 15, 2020

No. I can understand your point of view, but this is on purpose. I expressly want the inconvenience of someone having to go to the trouble of putting that setting in the config file to expressly indicate the intent to delete backup files. I even considered requiring all three settings in the config file for the function to operate, and provide no ability to pass arguments about retention from the command line. The main purpose of the managebackups command is to provide a convenient way to synchronize the deleting of old files out of the backup directory as part of a scripting routine that would do something else with the files that are left, such as copy them elsewhere for better safekeeping.

The managebackups function makes it easy to test the underlying function as part of making this PR. I seriously considered removing it altogether and only calling the underlying function ManageBackups from the backupwallet function.

managebackups It is not really meant for "casual use". Considering that the function managebackups can be called with no arguments, which then defaults to 365 365, or whatever the other two settings are in the config file, this could result in unintended consequences if someone were just "trying it out" without really looking carefully at how it works. Maybe we should have it require the two arguments, and not pay attention to -managebackupretention, but then that is in tension with reading them from the config file, and would be confusing.

@cyrossignol and I were on the fence about providing this functionality at all, because you are playing with fire, so we are going to be extra conservative.

@jamescowens jamescowens changed the title Implement backup file management functionality wallet: Implement backup file management functionality Jun 16, 2020
Copy link
Member

@cyrossignol cyrossignol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might change the names to: prunebackups, PruneBackups(), -backupretention. Or maybe "purge". When I read "manage", I can't help thinking that I'm about to start configuring something.

Going to test...

@jamescowens
Copy link
Member Author

That is a good point, and I am not religious about the name...

@jamescowens
Copy link
Member Author

I like purgebackups better, although it gets a little muddy if we add compression capability, which I intend to do.

@cyrossignol
Copy link
Member

Oh yeah, forgot about that... perhaps something along the lines of {run,apply,force}backupmaintenance or just maintainbackups.

@jamescowens jamescowens modified the milestones: Fern, Gladys Jun 25, 2020
@jamescowens
Copy link
Member Author

Moving to Gladys. We just don't have time to certify this as part of Fern, because Fern has gotten so big.

jamescowens and others added 3 commits September 5, 2020 09:03
Also includes managebackups rpc function, and call to
ManageBackups as part of the backupwallet function.
Co-authored-by: RoboticMind <30132912+RoboticMind@users.noreply.github.com>
@jamescowens
Copy link
Member Author

I rebased and changed the name of the rpc and underlying function to maintainbackups in accordance to @cyrossignol's suggestion, which I like. I think this is actually ready for testing on testnet. Let's get it in and get some experience with it. I do not want to do too much more with this one. If we decide to do compression, it should be done in a separate PR.

@jamescowens jamescowens merged commit e3d3847 into gridcoin-community:development Sep 13, 2020
@jamescowens jamescowens changed the title wallet: Implement backup file management functionality wallet, rpc: Implement backup file management functionality Sep 19, 2020
@jamescowens jamescowens deleted the managebackups branch November 7, 2020 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants