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

Provide NixOS module option to enable the paperless exporter. #242084

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ctheune
Copy link
Contributor

@ctheune ctheune commented Jul 7, 2023

Description of changes

Integrate the paperless document exporter as a backup feature into the module.

Also fixes a configuration (quoting) issue.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jul 7, 2023
@leona-ya leona-ya removed their assignment Jul 7, 2023
@leona-ya leona-ya self-requested a review July 7, 2023 15:43
nixos/modules/services/misc/paperless.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/paperless.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/paperless.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jul 7, 2023
@mweinelt mweinelt requested review from lukegb, gador and erikarvstedt July 7, 2023 20:53
Copy link
Member

@erikarvstedt erikarvstedt left a comment

Choose a reason for hiding this comment

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

Here are fixups for your first commit, which should definitely be merged.
Maybe add a dedicated PR because these changes are orthogonal to backups and entirely uncontroversial.

@erikarvstedt
Copy link
Member

erikarvstedt commented Jul 7, 2023

The term backup is misleading because this only exports the documents but not the database (containing all the metadata).
Is a doc-only export generally useful for users?

Also, the export command used by the backup service creates redundant copies of each doc in multiple formats, which is also not suitable for backups.

@ctheune
Copy link
Contributor Author

ctheune commented Jul 9, 2023

The term backup is misleading because this only exports the documents but not the database (containing all the metadata). Is a doc-only export generally useful for users?

Well it is what is recommended as the official backup strategy according to the manual. It also seems comprehensive with the metadata. I can see all the table content like tags, correspondents, users, even saved filters and UI settings ...

Also, the export command used by the backup service creates redundant copies of each doc in multiple formats, which is also not suitable for backups.

That is configurable using the command parameters that I've made adjustable.

@ctheune
Copy link
Contributor Author

ctheune commented Jul 9, 2023

Here are fixups for your first commit, which should definitely be merged.
Maybe add a dedicated PR because these changes are orthogonal to backups and entirely uncontroversial.

Thanks for those. I'll double check (and I guess I should add a test) that the quoting works both for systemd and the manage command now.

@erikarvstedt
Copy link
Member

It also seems comprehensive with the metadata. I can see all the table content like tags, correspondents, users, even saved filters and UI settings ...

Ah right, the metadata is exported to manifest.json.

That is configurable using the command parameters that I've made adjustable.

These should be fixed so that no redundant content is exported by default.

I'm still not convinced that paperless needs a dedicated backup service. It can be generically backed up like many other database-based NixOS services: Either by (1) snappshotting and transfering the whole of /var/lib or by (2) using services.postgresqlBackup and just rsyncing /var/lib/paperless/.
Note that like (2) the document exporter method used in this PR is not atomic/consistent. Quote from the manual: "Before making backups, make sure that paperless is not running."
Let's delegate the decision to other paperless users. @mweinelt, @Flakebi, @lukegb, @leona-ya, what do you think?

@erikarvstedt
Copy link
Member

Thanks for those. I'll double check (and I guess I should add a test) that the quoting works both for systemd and the manage command now.

There's no quoting involved when setting up the systemd env, so this has always worked correctly.
As for the manage script, escapeShellArg is guaranteed to work. It's a common pattern in NixOS:

"${name}=${escapeShellArg value}"

@mweinelt
Copy link
Member

mweinelt commented Jul 9, 2023

(2) using services.postgresqlBackup and just rsyncing /var/lib/paperless/.

That is what we're doing, but with ZFS snapshots of the data dir.

@leona-ya
Copy link
Member

leona-ya commented Jul 9, 2023

I think there are two different use-cases for backups:

  1. Restore to paperless, after a host failure
  2. Clean backup independent from the software

1 can probably just be handled by backuping postgresql + the paperless state dir.
2 is also an idea that I like very much. It respects the value of the documents (in the metadata, ASNs are relevant, for example).


Probably i would still, even if this feature was available, just make a backup in the style of 1. But I can also understand why people (probably @ctheune) may think different.
If we want to include this I would definitely rename the option from backup to documentExporter. I'm not 100% sure how to decide in the conflict between 'don't overcomplicate module' (is this change something a user should do in their own config) and 'allow users to easily use the features they want for a DMS'.

@erikarvstedt
Copy link
Member

The export service just boils down to this simple snippet, which we could simply add to the NixOS manual:

{ config, ... }:
let
  paperless = config.services.paperless;
in
  systemd.services.paperless-export = {
    startAt = "daily";
    serviceConfig = {
      User = paperless.user;
      ExecStart = ''
        ${paperless.dataDir}/paperless-manage document_exporter <export_dir> --no-progress-bar --no-color --compare-checksums --delete
      '';
    };
  };

@erikarvstedt
Copy link
Member

@Atemu, maybe let's first decide if or in what form we want to include this.

@ctheune
Copy link
Contributor Author

ctheune commented Jul 12, 2023

Here are fixups for your first commit, which should definitely be merged.
Maybe add a dedicated PR because these changes are orthogonal to backups and entirely uncontroversial.

Thanks. I've created #243084 for the orthogonal changes.

@ctheune
Copy link
Contributor Author

ctheune commented Jul 12, 2023

Alright. I'm happy to follow pretty much most of the suggestions, but as the PR itself is still under question, I'll postpone that.

Having to take paperless offline was something I overlooked and I guess the exporter could arrange for that.

Whether to include it, I see the following parts that need discussion:

  1. Not using the name "backup" – sure, I can perfectly live with calling it documentExporter as @leona-ya suggested.
  2. I'm running it with the built-in sqlite thing and I don't have snapshots as simple solution available in my environment, so the document exporter seems the most attractive route to me but I'd prefer it to be part of the tested feature set. Apparently it did not run out of the box due to the quoting issue and I'd love if we can avoid regressions here that everyone has to fix themselves.
  3. Complication of the module ... well ... not sure what the metric here is in value per complication ;) ... as a user I would have loved this being already in there as keeping my data safe is kind of the job of a DMS and we see similar complications in other database modules.

@ctheune ctheune changed the title Paperless backup master Provide NixOS module option to enable the paperless exporter. Sep 10, 2023
@ctheune ctheune force-pushed the paperless-backup-master branch from 3be7d20 to 224f1f0 Compare September 10, 2023 14:10
@github-actions github-actions bot added 8.has: documentation This PR adds or changes documentation 8.has: changelog labels Sep 10, 2023
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 10, 2023
Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

A couple of things that jumped to my eye; mostly nits.

nixos/modules/services/misc/paperless.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/paperless.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/paperless.nix Outdated Show resolved Hide resolved
nixos/doc/manual/release-notes/rl-2311.section.md Outdated Show resolved Hide resolved
nixos/modules/services/misc/paperless.nix Outdated Show resolved Hide resolved
@Atemu
Copy link
Member

Atemu commented Sep 10, 2023

At NixCon, @ctheune, @leona-ya and I decided that we would like to have this functionality in Nixpkgs, so I think this is ready to go after perhaps a few minor fixups.

@ctheune could you add me (and perhaps yourself aswell) to the meta.maintainers of the module?

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
@ctheune ctheune force-pushed the paperless-backup-master branch from 224f1f0 to bb33fee Compare January 2, 2025 12:38
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 2, 2025
@ctheune ctheune force-pushed the paperless-backup-master branch 4 times, most recently from 8823606 to d99b206 Compare January 2, 2025 13:11
@ctheune
Copy link
Contributor Author

ctheune commented Jan 2, 2025

Alright. I've finally mustered up energy to try and wrap this up once more. I went through the whole history and discussion and consistently switched the terminology from backup to export and integrated all suggestions from @Atemu. Some suggestions were outdated by now. And I decided to stick with the pre/post script options.

I'm currently fighting that the paperless-ngx package doesn't want to build on master (failing on unit tests)...

@ctheune ctheune requested review from Atemu and leona-ya January 2, 2025 13:23
@ctheune ctheune marked this pull request as ready for review January 2, 2025 13:24
Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

A few improvements and nits but this looks very good to me now.

nixos/modules/services/misc/paperless.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/paperless.nix Show resolved Hide resolved
nixos/modules/services/misc/paperless.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/paperless.nix Outdated Show resolved Hide resolved
Comment on lines 283 to 286
"no-progress-bar" = true;
"--no-color" = true;
"--compare-checksums" = true;
"--delete" = true;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this works with dashes in the names.

This would also get overridden whole when anything is customised which is a bit annoying. The complexity of solving that (i.e. submodule) might not be worth it though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the dashes were a mistake. For some reason I couldn't run the tests yesterday, but they work now. I think was invoking the tests incorrectly and ended up with a wrong package in my environment...

I was expecting the override to be tricky here, too. I'd be happy to switch to a list ... however, the override question still stands. We could split the defaults and keep --no-color and --no-progress-bar statically and only put --compare-checksums and --delete-checksums in the configurable settings. =

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

To allow users to override exporter options, set the options value in config so that they get the standard priority instead of the OptionDefault priority.

Copy link
Contributor Author

@ctheune ctheune Jan 3, 2025

Choose a reason for hiding this comment

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

Ah, you mean when using the attrset-based version? And then use the defaultText to show the defaults? What's the idiom to keep those in sync?

Copy link
Member

@erikarvstedt erikarvstedt Jan 3, 2025

Choose a reason for hiding this comment

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

In this case, you could use this:

let
  defaultExporterOptions = {
    no-progress-bar = true;
    no-color = true;
    compare-checksums = true;
    delete = true;
  };
in {
  options = {
    services.paperless = {
      exporter.options = lib.mkOption {
        defaultText = lib.generators.toPretty {} defaultExporterOptions;
        # ...
      };
    };
  };

  config = {
    services.paperless.exporter.options = defaultExporterOptions;
  };
}

Copy link
Member

Choose a reason for hiding this comment

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

@ctheune, yes (with mkOptionDefault instead of mkDefault).
Because most importantly, the apply approach breaks this use case:

# Override all default options
exporter.options = lib.mkForce {
   ...
}

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's what my concern was. I had a reply typed out where I also acknowledged that this is more of a theoretical concern (and well beyond bikeshedding I might add), but it didn't send properly due to a spotty internet connection.

As mentioned in the initial comment, I'd be fine if this overrode the default value entirely too; this is polish/bikeshedding.

Especially given that there will hopefully be a proper solution for cases like this soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll wrap this up then and hope the shed is sufficiently colorful now. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ctheune, yes (with mkOptionDefault instead of mkDefault). Because most importantly, the apply approach breaks this use case:

Hmm. My test was satisfied with mkDefault, however, to apply mkOptionDefault I had to adapt it a bit further and use v.default instead of v.

I've committed the current state with mkOptionDefault.

Copy link
Member

Choose a reason for hiding this comment

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

(You forgot to push.)

nixos/modules/services/misc/paperless.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/paperless.nix Show resolved Hide resolved
nixos/modules/services/misc/paperless.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/paperless.nix Show resolved Hide resolved
nixos/tests/paperless.nix Outdated Show resolved Hide resolved
@ctheune ctheune force-pushed the paperless-backup-master branch 3 times, most recently from 211f996 to e7ee089 Compare January 4, 2025 09:44
Paperless includes a document exporter that can be used for
backups.

This extends the module to provide a way to enable and configure
a timer, the backup parameters and allow providing a post-processing
script (e.g. to ship the backup somewhere else, clean up, ...).

It works out of the box when just enabling it but can be customized.

Includes suitable tests.
@ctheune ctheune force-pushed the paperless-backup-master branch from e7ee089 to ed36619 Compare January 4, 2025 15:48
nixos/tests/paperless.nix Show resolved Hide resolved
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 4, 2025
Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Migrated my config over to this and it was a breeze.

Atemu/nixos-config@c34c9e9

'';
};

options = lib.mkOption {
Copy link
Member

Choose a reason for hiding this comment

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

One last nit: This should probably be called settings in accordance with RFC42.

Comment on lines +262 to +263
default = cfg.dataDir + "/exports";
defaultText = "\${dataDir}/exports";
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Any specific reason this is exports (plural) by default?

I always used export and was confused by this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants