Service wrapper that doesn't require UUID to be known at config generation time?

[charles-dyfis-net]

The current wrapper requires on-disk configuration to be aware of the filesystem's UUID. If trying to build configuration ahead of a machine's instantiation, this is not ideal.

I've implemented an alternate wrapper as part of a PR to NixOS adding bees support, at https://github.com/NixOS/nixpkgs/blob/7f8d1ddb49963fb04dec2c7669489317948343d7/pkgs/tools/filesystems/bees/bees-service-wrapper (the larger PR is NixOS/nixpkgs#48423). I'm not necessarily proposing that exact code for merge to upstream bees, but having something like it -- permitting increased flexibility -- would be helpful.

[kakra]

I'm having such an idea on my schedule, I think this is a duplicate of #54 then.

[charles-dyfis-net]

Hmm. A good implementation of #54 could moot this, but it depends on the details. :)

One of the early suggestions involved embedding the UUID inside the config files -- that would leave the issue at hand open.

[kakra]

Personally, I'm not very happy with the wrapper and letting it depend only on the UUID. I'd rather have an option to do the mounting outside of the scope of bees, i.e. by attaching the bees service to a systemd mount unit. In the end, you should be able to throw a UUID or mount path at it and it will just work.

[charles-dyfis-net]

@Zygo, would you be willing to review the proposed service wrapper at https://github.com/NixOS/nixpkgs/blob/46e64d619083f573c66d6b4fa426d477e6a5e121/pkgs/tools/filesystems/bees/bees-service-wrapper ?

The initial impetus and need is support for the NixOS configuration model, which requires content only known after a machine has been configured or assembled (like filesystem UUIDs) to be kept out of declarative system configuration. That said, I also tried to rewrite for best practices -- keeping out of reserved all-caps namespace; avoiding unnecessary environment space use; being robust against unusual/unexpected filenames (including those with spaces); etc.

[kakra]

@charles-dyfis-net Is it compatible with existing configuration files? Read: Is there an upgrade path without bad surprises?

[charles-dyfis-net]

Not currently compatible, but that's a reasonable requirement -- I can see about making it so.

[kakra]

@charles-dyfis-net Thanks, that would make reviewing much more motivating :-)

Because I'm open for porting the good ideas of it over here.

[charles-dyfis-net]

https://github.com/NixOS/nixpkgs/pull/48423/files#diff-e8e78f122291b55608e83be5b5cce555 now attempts to be compatible with existing bees config files.

The nix-specific bits are isolated to the header -- in theory, if the service file is updated appropriately (to run a separate invocation for cleanup with ExecStopPost= as given in the Nix config in the linked PR), it should be possible to just port it in directly.

[kakra]

@charles-dyfis-net Give me few days to test this new wrapper here locally with Gentoo. I may create a PR with some simple tweaks so it could end up in a simple patch you need to carry around in your Nix package.

[charles-dyfis-net]

There've been more changes coming out of code review on the Nix side; https://github.com/NixOS/nixpkgs/pull/48423/files#diff-e8e78f122291b55608e83be5b5cce555 is substantially improved.

[kakra]

What does a Nix config file look like?

[charles-dyfis-net]

So, the relevant portion of /etc/nixos/configuration.nix on the host where I test this is...

{ # ...opening and close braces are for the sake of github's syntax highlighting
  # in a real-world configuration, there will also be user account definitions, other services, etc.

  # actual instance configuration
  services.beesd.filesystems = {
    root = { spec="LABEL=nixos"; hashTableSizeMB=8192; extraOptions=[ "--thread-count" "4" ]; };
  };

  # load the bees module definition in-tree, since it isn't merged into nixos upstream yet
  # after upstream merge, only the content above here will be needed.
  imports = [ ./hardware-configuration.nix ./modules/bees.nix ];
  # similarly, override our old nixpkgs to load ./pkgs/bees/default.nix
  nixpkgs.config.packageOverrides = super: {
    bees = pkgs.callPackage ./pkgs/bees { };
  };
}

Everything else -- the /etc content, the systemd services, etc -- is generated by the module code (which, likewise, forces bees to be installed if-and-only-if at least one such instance is defined).

[kakra]

Ah okay, I start to understand. So this is declaration-based config generator what we see here. Why does it need a complete rewrite of the service wrapper then? Why not just wrap another service wrapper around it that generates the config from declaration, translates whatever path you pass in into the UUID, then invokes the original wrapper? Or is the declarative config part only parsed once at deploy time?

[charles-dyfis-net]

Right -- we want to be able to do the generation without needing to inspect runtime state at all; in many cases, before the machine to which the configuration will be deployed has been instantiated or installed at all, and thus before the UUID is even assigned.

In a proper NixOS system, /etc (and everything else outside of /run, /tmp, /home and similar exceptions) consists not of writable files, but as symlinks into a hash-addressed store of content built from declarative definitions; if that store is being served via a network filesystem, it may not be writable at all.

Yes, one can move configuration into /run, and generate those contents at runtime, but that's adding complexity and maintenance load that could be avoided if we could get a single wrapper that works for everyone into upstream bees.

[charles-dyfis-net]

To explicitly make the argument that the changes are for the better, and that the original wrapper is best retired:

• The proposed wrapper doesn't require configuration files at all; all content that could be passed in a config file can instead be passed on the service's command line.
• Using grep to look for configuration files containing a given UUID in a substring (and ignoring all but the first such file matching that string) is innately error-prone, and -- I'd argue -- surprising. Relegating that to fallback compatibility behavior is the first step towards eliminating it.
• Executing arbitrary commands embedded in configuration is one more thing that needs to be locked down if someone is trying to offer only limited administrative control (see the current version of the wrapper using bash -r to honor shell arithmetic embedded in configuration, without allowing other operations to take place)
• Having the same instance of your shell wrapper responsible for both startup and cleanup means that a shell needs to stick around in-memory for the duration of the process, and that cleanup can fail to happen if something kills that process; making systemd's ExecStopPost= responsible for the cleanup avoids the extra process, and reduces the number of failure cases where cleanup can get skipped.
• Making the argument separator between wrapper arguments and daemon arguments explicit (using -- as an end-of-options sigil, per POSIX utility syntax guidelines avoids the need to parse out --help).

...there are a bunch of little implementation-quality nitpicks as well (several being things that http://shellcheck.net/ will catch; others -- like use of variable names outside the space reserved for applications by POSIX -- it won't), but arguing them probably wouldn't be helpful here.

[kakra]

These are a lot of valid points, some of them I also raised here already (tho, with other words).