-
-
Notifications
You must be signed in to change notification settings - Fork 19.3k
Contracts for file backups #522054
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
base: master
Are you sure you want to change the base?
Contracts for file backups #522054
Changes from all commits
796ad66
1ab9dfe
951f1b2
f8ea94f
eb52551
52fdc3d
aed52a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -220,10 +220,9 @@ let | |||||
| )} | ||||||
| ''; | ||||||
|
|
||||||
| # Returns a singleton list, due to usage of lib.optional | ||||||
| mkBorgWrapper = | ||||||
| name: cfg: | ||||||
| lib.optional (cfg.wrapper != "" && cfg.wrapper != null) (mkWrapperDrv { | ||||||
| mkWrapperDrv { | ||||||
| original = lib.getExe config.services.borgbackup.package; | ||||||
| name = cfg.wrapper; | ||||||
| set = { | ||||||
|
|
@@ -232,7 +231,58 @@ let | |||||
| // (mkPassEnv cfg) | ||||||
| // cfg.environment; | ||||||
| extraArgs = cfg.extraArgs or null; | ||||||
| }); | ||||||
| }; | ||||||
|
|
||||||
| mkBackupContractScript = | ||||||
| name: backup: | ||||||
| let | ||||||
| script = mkBorgWrapper name backup; | ||||||
| in | ||||||
| # I chose to base the contract script on the existing script | ||||||
| # to avoid duplicating the environment variables logic. | ||||||
| # Similarly, I based the backup command on the systemd service | ||||||
| # because it already handled giving the source directories. | ||||||
| # I prioritized a small diff size. | ||||||
| pkgs.writeShellApplication { | ||||||
| name = "borgbackup-${name}"; | ||||||
| text = '' | ||||||
| usage() { | ||||||
| echo "$0 snapshots" | ||||||
| echo "$0 backup" | ||||||
| echo "$0 restore <snapshot>" | ||||||
| echo "$0 exec <arg> [<arg>...]" | ||||||
| } | ||||||
|
|
||||||
| if [ -z "''${1:-}" ]; then | ||||||
| usage | ||||||
| exit 0 | ||||||
| fi | ||||||
|
|
||||||
| if [ "$1" = "restore" ]; then | ||||||
| shift | ||||||
| snapshot="$1" | ||||||
|
|
||||||
| echo "Will restore snapshot '$snapshot'" | ||||||
|
|
||||||
| (cd / && exec ${script}/bin/borg-job-${name} extract "::$snapshot") | ||||||
| elif [ "$1" = "backup" ]; then | ||||||
| shift | ||||||
|
|
||||||
| systemctl start --wait borgbackup-job-${name} | ||||||
| elif [ "$1" = "snapshots" ]; then | ||||||
| shift | ||||||
|
|
||||||
| ${script}/bin/borg-job-${name} list --short | ||||||
| elif [ "$1" = "exec" ]; then | ||||||
| shift | ||||||
|
|
||||||
| exec ${script}/bin/borg-job-${name} "$@" | ||||||
| else | ||||||
| usage | ||||||
| exit 1 | ||||||
| fi | ||||||
| ''; | ||||||
| }; | ||||||
|
|
||||||
| # Paths listed in ReadWritePaths must exist before service is started | ||||||
| mkTmpfiles = | ||||||
|
|
@@ -423,7 +473,11 @@ in | |||||
|
|
||||||
| paths = lib.mkOption { | ||||||
| type = with lib.types; nullOr (coercedTo str lib.singleton (listOf str)); | ||||||
| default = null; | ||||||
| default = | ||||||
| if config.fileBackupContract == null then | ||||||
| null | ||||||
| else | ||||||
| config.fileBackupContract.input.sourceDirectories; | ||||||
| description = '' | ||||||
| Path(s) to back up. | ||||||
| Mutually exclusive with {option}`dumpCommand`. | ||||||
|
|
@@ -518,7 +572,8 @@ in | |||||
| User or group need read permission | ||||||
| for the specified {option}`paths`. | ||||||
| ''; | ||||||
| default = "root"; | ||||||
| default = | ||||||
| if config.fileBackupContract == null then "root" else config.fileBackupContract.input.user; | ||||||
| }; | ||||||
|
|
||||||
| group = lib.mkOption { | ||||||
|
|
@@ -603,7 +658,9 @@ in | |||||
| Can not be set when {option}`createCommand` is set to | ||||||
| `import-tar`. | ||||||
| ''; | ||||||
| default = [ ]; | ||||||
| default = lib.optionals (config.fileBackupContract == null) ( | ||||||
| map (n: "sh:**/${n}") config.fileBackupContract.input.excludePatterns | ||||||
| ); | ||||||
| example = [ | ||||||
| "/home/*/.cache" | ||||||
| "/nix" | ||||||
|
|
@@ -830,6 +887,79 @@ in | |||||
| default = [ ]; | ||||||
| example = [ "--cleanup-commits" ]; | ||||||
| }; | ||||||
|
|
||||||
| fileBackupContract = lib.mkOption { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
there's more places like this, but i'd consider this for perhaps brevity + discoverability (going by the contract name) |
||||||
| description = "Provider of the fileBackup contract."; | ||||||
| default = null; | ||||||
| type = lib.types.nullOr ( | ||||||
| lib.types.submodule { | ||||||
| options = { | ||||||
| input = { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
may be a more robust name when switching context between provider vs consumer, on top of facilitating calling the consumer (in pre-resolution context) a requester |
||||||
| user = lib.mkOption { | ||||||
| description = '' | ||||||
| As which user the backup should run. | ||||||
| ''; | ||||||
| type = lib.types.str; | ||||||
| example = "vaultwarden"; | ||||||
| }; | ||||||
|
|
||||||
| sourceDirectories = lib.mkOption { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The naming is unfortunate, the fileBackup contract only allows backing up sourceDirectories. Something like paths sounds broader. Another option that does seem lacking is retention. Disk space is not unlimited so pruning of backups is pretty much essential. Having that in its own contract feels like it would decouple things more than necessary.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That’s a good point. The name file comes from the need to distinguish between backing up actual files on disk and not streams. Streams would be coming from the stdout of programs, for example for backing up a database. This is purposely not proposed here to keep the scope somewhat small but I do have an implementation. I don’t know if there’s a better name than path but path does encapsulate both files and directories. At least to my knowledge. Then we could rename the option sourcePaths.
That’s indeed a valid concern but IMO it doesn’t belong in this contract which is only used to let a service tell what to back up. The contract sits between the consumer and the provider. Retention is more of a concern between the backup service and the user, so between the provider and the user. Now having the user be able to tell its preference for backup retention once and that be applied to all backup services is an interesting consideration.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do services just get to request backups? Because just because the service maintainer thinks something needs backups doesn't mean that I as a user of the service agree. I'd expect an opt-in mechanism and if I'm opting in explicitly, having the retention settings right there as well makes it a lot easier to maintain an overview. Doing a systemwide default for backup retention is good but control over individual backups is also important I don't use the same retention for my various backups.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's fair. Currently, it's all opt-in anyway so it matches your expectations. I was a bit extrapolating there. Also, since the contract option is just one more option in the restic service, you can still access all existing restic options and set retention per service this way: {
services.restic.backups."home-assistant" = {
// Here goes "normal" Restic options
initialize = true;
repository = "/opt/restic-repos/${name}";
passwordFile = "${pkgs.writeText "password" "restic-${name}-password"}";
pruneOpts = [
"--keep-daily 7"
"--keep-weekly 5"
"--keep-monthly 12"
"--keep-yearly 75"
];
// Contract option
fileBackupContract.input = config.services.home-assistant.fileBackupContract.input;
};
} |
||||||
| description = '' | ||||||
| Directories to backup. | ||||||
| ''; | ||||||
| type = lib.types.nonEmptyListOf lib.types.str; | ||||||
| example = [ | ||||||
| "/var/lib/vaultwarden" | ||||||
| ]; | ||||||
| }; | ||||||
|
|
||||||
| excludePatterns = lib.mkOption { | ||||||
| description = '' | ||||||
| File patterns to exclude. | ||||||
| ''; | ||||||
| type = lib.types.listOf lib.types.str; | ||||||
| default = [ ]; | ||||||
| }; | ||||||
| }; | ||||||
| output = { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
so, input/output terminology takes the perspective from the provider, yet gets more confusing from the perspective of the consumer, where that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking about this more, I think now |
||||||
| script = lib.mkOption { | ||||||
| description = '' | ||||||
| Script that provide the following commands | ||||||
| and runs with the necessary environment variables: | ||||||
|
|
||||||
| - Listing snapshots: | ||||||
|
|
||||||
| ```bash | ||||||
| <script> snaphots | ||||||
| ``` | ||||||
|
|
||||||
| - Restore a snapshot: | ||||||
|
|
||||||
| ```bash | ||||||
| <script> restore <snapshot> | ||||||
| ``` | ||||||
|
|
||||||
| - Take a backup: | ||||||
|
|
||||||
| ```bash | ||||||
| <script> backup | ||||||
| ``` | ||||||
|
|
||||||
| - Pass a custom command to the underlying provider: | ||||||
|
|
||||||
| ```bash | ||||||
| <script> exec <arg> [<arg> ...] | ||||||
| ``` | ||||||
| ''; | ||||||
| type = lib.types.package; | ||||||
| default = mkBackupContractScript name config; | ||||||
| defaultText = "/path/to/script"; | ||||||
| }; | ||||||
| }; | ||||||
| }; | ||||||
| } | ||||||
| ); | ||||||
| }; | ||||||
| }; | ||||||
| } | ||||||
| ) | ||||||
|
|
@@ -961,7 +1091,9 @@ in | |||||
| environment.systemPackages = [ | ||||||
| config.services.borgbackup.package | ||||||
| ] | ||||||
| ++ (lib.flatten (lib.mapAttrsToList mkBorgWrapper jobs)); | ||||||
| ++ (lib.mapAttrsToList mkBorgWrapper ( | ||||||
| lib.filterAttrs (_: cfg: cfg.wrapper != "" && cfg.wrapper != null) jobs | ||||||
| )); | ||||||
| } | ||||||
| ); | ||||||
| } | ||||||
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.
Nit
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.
in the falsy case the suggestion returns empty list rather than null, which as per the declared type i'm not sure is intended here
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.
Indeed, I'm not aware of a function to do that.