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

Create qbittorrent-nox service #279716

Conversation

camilosampedro
Copy link

@camilosampedro camilosampedro commented Jan 9, 2024

Description of changes

The qbittorrent-nox package already exists, which is a headless version of qbittorrent (Only accessible via web). It being a headless service makes it a good candidate for being configured via services.* options, so I opened this PR for adding some basic configurations for it to run as a Systemd service.

This is my first contribution to the project, so I will greatly appreciate any comments and suggestions!

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 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.

Add a 👍 reaction to pull requests you find important.

Currently there is a qbittorrent-nox package, but no configurations to make it a systemd service.
I placed several basic options for this service, which should be good enough to start using the service.
These are required for setting a gid and uid for the systemd unit
@camilosampedro
Copy link
Author

camilosampedro commented Jan 14, 2024

I was able to test it on a clean install inside of a QEMU VM

The thing I couldn't do is to run the review command. The whole VM seems to crash when I run:

nix-shell -p nixpkgs-review --run "nixpkgs-review pr 279716"

This crashes the VM even when running it without sudo permissions. I wonder if this is a bug and if there's anything to do about it.

@camilosampedro camilosampedro changed the title Create qbittorrent nox service Create qbittorrent-nox service Jan 14, 2024
@camilosampedro camilosampedro marked this pull request as ready for review January 14, 2024 22:25
@camilosampedro camilosampedro marked this pull request as draft January 14, 2024 22:28
@camilosampedro camilosampedro marked this pull request as ready for review January 14, 2024 23:00
Copy link
Contributor

@ambroisie ambroisie left a comment

Choose a reason for hiding this comment

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

What about a downloadDir? Is there any specific setup necessary there?

@@ -356,6 +356,7 @@ in
rstudio-server = 324;
localtimed = 325;
automatic-timezoned = 326;
qbittorrent-nox = 327;
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be done anymore, you should use (in order of preference):

  • systemd's DynamicUser (potentially with User if you need a named user)
  • users.users.<name>.

Given the usual approach for HTPC/seedbox services, using users.users is probably the way to go.

@@ -666,6 +667,7 @@ in
rstudio-server = 324;
localtimed = 325;
automatic-timezoned = 326;
qbittorrent-nox = 327;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same note as the uid.

in {
options = {
services = {
qbittorrent-nox = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would instead name it qbittorent, add a package option, and use qbittorent-nox as the default package.

options = {
services = {
qbittorrent-nox = {
enable = mkEnableOption (lib.mdDoc "qbittorrent-nox daemon");
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to my proposition to rename the module:

Suggested change
enable = mkEnableOption (lib.mdDoc "qbittorrent-nox daemon");
enable = mkEnableOption (lib.mdDoc "qBittorent daemon");

Comment on lines +17 to +19
description = lib.mdDoc ''
qbittorrent-nox web UI port.
'';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description = lib.mdDoc ''
qbittorrent-nox web UI port.
'';
description = lib.mdDoc "qBittorent web UI port"';

systemd.services.qbittorrent-nox = {
after = [ "network.target" "local-fs.target" "network-online.target" "nss-lookup.target" ];
wantedBy = [ "multi-user.target" ];
path = [ cfg.package ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed, better served by systemd's service hardening options.

PrivateTmp = "false";
TimeoutStopSec = 1800;
};
# preStart = preStart;
Copy link
Contributor

Choose a reason for hiding this comment

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

Dead code.

})
];

environment.systemPackages = [ cfg.package ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

users.users = mkIf (cfg.user == "qbittorrent") {
qbittorrent = {
group = cfg.group;
uid = config.ids.uids.qbittorrent-nox;
Copy link
Contributor

Choose a reason for hiding this comment

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

As explained, please refrain from adding more uid/gid values to NixOS.

Suggested change
uid = config.ids.uids.qbittorrent-nox;


users.groups = mkIf (cfg.group == "qbittorrent") {
qbittorrent = {
gid = config.ids.gids.qbittorrent-nox;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same remark as for uid.

Suggested change
gid = config.ids.gids.qbittorrent-nox;

@ambroisie
Copy link
Contributor

I'm also unsure whether we still need to use lib.mdDoc (maybe for backporting? I will have to check).

I would also argue that with lib; over the whole file is over-reaching. I'd be fine with using it in the options = ... block though.

@@ -0,0 +1,141 @@
{ config, lib, pkgs, ... }:

with lib;
Copy link
Member

Choose a reason for hiding this comment

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

you should avoid this kind of use of with to address #208242 for this module.

use inherits where you find yourself excessively reusing lib.foo

};
};

dataDir = mkOption {
Copy link
Member

Choose a reason for hiding this comment

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

is a dataDir true here for qbit, profiles seems to be the way it manages itself, a profilesdir commandline option is available.

ExecStart = ''
${cfg.package}/bin/qbittorrent-nox \
--profile=${cfg.dataDir} \
--webui-port=${toString cfg.web.port} \
Copy link
Member

Choose a reason for hiding this comment

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

neither port options should be done with the command line options, qbittorrent has a configuration file we can define these in, we know where its state will be after all

@fsnkty
Copy link
Member

fsnkty commented Feb 6, 2024

seems i should have looked harder before i started on this, see master...nu-nu-ko:nixpkgs:init-nixos-qbittorrent
differences between our work now are

  1. qBittorrent.conf options thanks to gendeepINI ( thanks eclairevoyant )
  2. I'm missing openFirewall ( i want to include in config values here )
  3. I'm missing a type for the configs options.
  4. I'm missing any tests and docs additions.
  5. I apply much more service hardening. ( the rw paths will obviously need to become as set in the service options / the defaults. ) for a exposure level of 1.1

I'll draft my own PR if I can get openFirewall and rw paths setting done in an acceptable way myself.
open to however you may want to work with this or not, would love to have the config options available with nix instead of expecting the user to write a file out themselves etc.

as reference for how my module can be used right now i have a working use case in my systems repo here

would also appreciate your insight as another current qbit user / someone who wants a service module for it.
as well as testing on aarch-64

@camilosampedro
Copy link
Author

@nu-nu-ko Thank you for working on those changes, and sorry I've been absent these past days.

I'd be ok with you opening your PR, using that as the actual PR that introduces this service, and I can help testing it on my machine.

Let me know and I'll close this PR.

@fsnkty
Copy link
Member

fsnkty commented Feb 11, 2024

I've created #287923 with the current issues listed, I have only somewhat addressed the read write directorys and open ports issues as defaults should be filled in properly.

please ensure that PR has the same goal and functionality as you do here before closing anything, thank you.

@camilosampedro
Copy link
Author

@nu-nu-ko Your PR seems to be following my goal. I'll be doing some tests and commenting on that PR. I'll close this PR for now

@fsnkty fsnkty mentioned this pull request Feb 17, 2024
24 tasks
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