gador/nixpkgs
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

lib/services: fix reload/readiness bugs and add compliance coverage

Browse Source 
Fixes several correctness bugs primarily around modular services' recent
reload/notification options (#535695) and adds compliance coverage to guard them.

Fixes:

- `lib/services/service.nix`:
  - the reload-conflict assertion had inverted polarity, so it fired on the
    default configuration
  - the `mkIf` guard on `process.reloadCommand` had a misplaced paren, applying
    `!= null` to the `mkIf` result rather than to the condition
  - `process.reloadSignal` derives `process.reloadCommand`, so the assertion
    could not check `reloadCommand != null` -- that fired on every signal-only
    service. The command is now derived at `mkDefault` priority and the
    assertion is gated on `options.process.reloadCommand.highestPrio`, firing
    only when the user also set `reloadCommand` explicitly.
  - change `notificationProtocol` to a sub-module type
- `nixos/modules/system/service/systemd/service.nix`:
  - `systemd.mainExecReload`'s default ran `escapeSystemdExecArgs` (a list
    escaper) on the `nullOr str` `process.reloadCommand`; this threw
    `expected a list but found a string` and would have mangled `$MAINPID`. It
    now uses `process.reloadCommand` verbatim.
  - the `Type` default read a non-existent
    `config.serviceManager.notificationProtocol` instead of
    `config.notificationProtocol`.

Tests:

Extend the modular-service compliance suite to guard the above:

- Portable (manager-agnostic) eval assertions: `reloadSignal` derives
  `reloadCommand`, the conflict assertion does not fire on signal-only services
  but does when both are set explicitly, and `notificationProtocol.systemd`/`.s6`
  default to `false`.
- systemd-specific eval assertions: `serviceConfig.Type` (simple/notify) and
  `serviceConfig.ExecReload` are asserted on the resolved host units. This
  directly guards the `mainExecReload` fix, which threw before it.
- Runtime reload compliance test: a nested reloadable sub-service is started and
  reloaded, asserting the service observed the reload (recorded a SIGHUP marker).
  `callReload` receives the service's name path (the list of names from the
  top-level service down to the target sub-service); each integration joins it
  per its own unit-naming convention (NixOS dash-joins to the systemd unit name,
  e.g. `reload-inner.service`). Keeping it a path list rather than a read-only
  submodule option keeps the suite manager-agnostic.
- `doc/build-helpers/testers.chapter.md`: document `callReload`.

Follow-up to #535695.

Signed-off-by: cinereal <cinereal@riseup.net>
Assisted-by: Claude:claude-opus-4-8
This commit is contained in:
cinereal
2026-07-18 10:25:34 +02:00
parent f9feb60dba
commit ae9994806c
5 changed files with 260 additions and 50 deletions
Download Patch File Download Diff File Expand all files Collapse all files
doc/build-helpers/testers.chapter.md
+10
View File
@@ -758,6 +758,7 @@ An attribute set of derivations which perform the tests during their build.
- Output attribute `config` is the resulting evaluated services attrset (e.g., the value of the `system.services` option in NixOS).
This attribute must be available even if `checkDrv` would fail.
- Output attribute `checkDrv` is a representative derivation whose existence and buildability prove the eval is sound (e.g., `system.build.toplevel` in NixOS, but could perhaps be more specific in the case of another process manager integration).
- The generic tester only reads `config` and `checkDrv`. An integration may return additional attributes for its own integration-specific eval checks. Such extra attributes are optional.
`mkTest` (function)
@@ -772,6 +773,14 @@ An attribute set of derivations which perform the tests during their build.
: Path to a directory writable by service processes and readable by `testExe`.
The integration must ensure this directory is available when the services and `testExe` run.
`callReload` (function)
: `path -> string`.
Given a service's name `path` (the list of service names from the top-level service down to the target sub-service, e.g. `[ "reload" "inner" ]`), returns a shell command that reloads that service.
The command is embedded in `testExe` and executed with sufficient privilege to reload the service (e.g. as root in the test VM).
There is no manager-agnostic reload command, so every integration must provide this; the integration joins the `path` per its own unit-naming convention (the suite does not assume one).
On NixOS the `path` dash-joins into the systemd unit name with a `.service` suffix, so the command is `systemctl reload ${lib.concatStringsSep "-" path}.service` (a top-level service is a single-element path `[ "svc" ]` -> `svc.service`; a nested sub-service `[ "parent" "child" ]` -> `parent-child.service`).
:::{.example #ex-modularServiceCompliance-nixos}
# NixOS invocation of the compliance suite
@@ -802,6 +811,7 @@ recurseIntoAttrs (
config = machine.config.system.services;
checkDrv = machine.config.system.build.toplevel;
};
callReload = path: "systemctl reload ${lib.concatStringsSep "-" path}.service";
mkTest =
{
name,
lib/services/service.nix
+19 -14
View File
@@ -7,10 +7,11 @@
{
lib,
config,
options,
...
}:
let
inherit (lib) mkOption types;
inherit (lib) mkEnableOption mkOption types;
pathOrStr = types.coercedTo types.path (x: "${x}") types.str;
in
{
@@ -40,7 +41,7 @@ in
visible = "shallow";
};
process = {
argv = lib.mkOption {
argv = mkOption {
type = types.listOf pathOrStr;
example = lib.literalExpression ''[ (lib.getExe config.package) "--nobackground" ]'';
description = ''
@@ -72,15 +73,12 @@ in
};
notificationProtocol = mkOption {
type = types.listOf (
types.enum [
"systemd"
"s6"
]
);
default = [ ];
apply = v: lib.unique v;
type = types.submodule {
options = {
systemd = mkEnableOption "Whether the service supports systemd-notify.";
s6 = mkEnableOption "Whether the service supports s6-notify.";
};
};
description = ''
Notification protocol that this service supports with the underlying service manager.
'';
@@ -90,13 +88,20 @@ in
config = {
assertions = [
{
assertion = config.process.reloadSignal != null && config.process.reloadCommand != null;
# `reloadSignal` derives `reloadCommand` at `mkDefault` priority below, so a
# conflict only exists when the user *also* set `reloadCommand` explicitly.
# An explicit (non-`mkDefault`) definition has `defaultOverridePriority`.
assertion =
!(
config.process.reloadSignal != null
&& options.process.reloadCommand.highestPrio <= lib.modules.defaultOverridePriority
);
message = "reloadSignal conflicts with reloadCommand. Please either use reloadSignal or reloadCommand.";
}
];
process.reloadCommand = (lib.mkIf config.process.reloadSignal != null) (
lib.mkForce "${pkgs.coreutils}/bin/kill -${config.process.reloadSignal} $MAINPID"
process.reloadCommand = lib.mkIf (config.process.reloadSignal != null) (
lib.mkDefault "${pkgs.coreutils}/bin/kill -${config.process.reloadSignal} $MAINPID"
);
};
}
nixos/modules/system/service/systemd/service.nix
+10 -16
View File
@@ -126,33 +126,29 @@ in
`%i` (instance), `%t` (runtime directory), and environment variables using
`''${VAR}` syntax in your command line.
By default, it is either set to null or this is set to the escaped version of {option}`process.reloadCommand`
when specified to prevent systemd substitution. Set this option explicitly to enable
systemd's substitution features.
By default, it is set to {option}`process.reloadCommand` when specified, or an
empty string otherwise. Because {option}`process.reloadCommand` is already a
command line (not an argument list), it is used verbatim so that references
like `$MAINPID` are preserved.
To extend {option}`process.reloadCommand` with systemd specifiers, you can append
to the escaped arguments:
to the command line:
```nix
systemd.mainExecReload =
config.systemd.lib.escapeSystemdExecArgs config.process.reload + " --systemd-unit %n";
config.process.reloadCommand + " --systemd-unit %n";
```
This pattern allows you to pass the unit name (or other systemd specifiers)
as additional arguments while keeping the base command from {option}`process.argv`
properly escaped.
as additional arguments.
See {manpage}`systemd.service(5)` (section "COMMAND LINES") for details on
variable substitution and {manpage}`systemd.unit(5)` (section "SPECIFIERS")
for available specifiers like `%n`, `%i`, `%t`.
'';
type = types.nullOr types.str;
default =
if config.process.reloadCommand then
config.systemd.lib.escapeSystemdExecArgs config.process.reloadCommand
else
"";
defaultText = lib.literalExpression "config.systemd.lib.escapeSystemdExecArgs config.process.reloadCommand";
default = if config.process.reloadCommand != null then config.process.reloadCommand else "";
defaultText = lib.literalExpression "config.process.reloadCommand";
};
systemd.services = mkOption {
@@ -213,9 +209,7 @@ in
wantedBy = lib.mkDefault [ "multi-user.target" ];
serviceConfig = {
ExecReload = config.systemd.mainExecReload;
Type = lib.mkDefault (
if (config.serviceManager.notificationProtocol == "systemd") then "notify" else "simple"
);
Type = lib.mkDefault (if config.notificationProtocol.systemd then "notify" else "simple");
Restart = lib.mkDefault "always";
RestartSec = lib.mkDefault "5";
ExecStart = [
nixos/tests/system-services-compliance.nix
+94 -13
View File
@@ -7,6 +7,23 @@
let
sharedDir = "/tmp/modular-service-compliance";
inherit (pkgs) lib coreutils;
evalSystemServices =
services:
evalSystem (
{ ... }:
{
system.services = services;
system.stateVersion = "25.05";
fileSystems."/" = {
device = "/test/dummy";
fsType = "auto";
};
boot.loader.grub.enable = false;
}
);
in
let
suite = pkgs.testers.modularServiceCompliance {
@@ -15,23 +32,13 @@ let
evalConfig =
{ services }:
let
machine = evalSystem (
{ ... }:
{
system.services = services;
system.stateVersion = "25.05";
fileSystems."/" = {
device = "/test/dummy";
fsType = "auto";
};
boot.loader.grub.enable = false;
}
);
machine = evalSystemServices services;
in
{
config = machine.config.system.services;
checkDrv = machine.config.system.build.toplevel;
};
callReload = path: "systemctl reload ${lib.concatStringsSep "-" path}.service";
mkTest =
{
name,
@@ -49,6 +56,80 @@ let
meta.maintainers = with pkgs.lib.maintainers; [ roberth ];
};
};
# systemd-specific eval assertions. serviceConfig.Type/ExecReload only exist on
# the resolved host units, so a fresh eval is used per case.
systemdEvalTests =
let
defaultUnits =
(evalSystemServices {
service.process.argv = [ "${coreutils}/bin/true" ];
}).config.systemd.services;
notifyUnits =
(evalSystemServices {
service = {
process.argv = [ "${coreutils}/bin/true" ];
notificationProtocol.systemd = true;
};
}).config.systemd.services;
reloadUnits =
(evalSystemServices {
service = {
process.argv = [ "${coreutils}/bin/true" ];
process.reloadSignal = "HUP";
};
}).config.systemd.services;
in
{
testDefaultType = {
expr = defaultUnits.service.serviceConfig.Type;
expected = "simple";
};
testNotifyType = {
expr = notifyUnits.service.serviceConfig.Type;
expected = "notify";
};
testReloadExecReload = {
expr = reloadUnits.service.serviceConfig.ExecReload;
expected = "${coreutils}/bin/kill -HUP $MAINPID";
};
};
systemdEval = pkgs.stdenvNoCC.mkDerivation (finalAttrs: {
__structuredAttrs = true;
name = "system-services-compliance-systemd-eval-report";
passthru = {
tests = systemdEvalTests;
failures = lib.runTests finalAttrs.finalPackage.tests;
};
testResults = lib.mapAttrs (_: test: test.expr == test.expected) finalAttrs.finalPackage.tests;
buildCommand = ''
touch $out
for testName in "''${!testResults[@]}"; do
if [[ -n "''${testResults[$testName]}" ]]; then
echo "PASS $testName"
else
echo "FAIL $testName"
fi
done
''
+ lib.optionalString (lib.any (v: !v) (lib.attrValues finalAttrs.testResults)) ''
{
echo ""
echo "systemd-specific eval-level compliance failures:"
for testName in "''${!testResults[@]}"; do
if [[ -z "''${testResults[$testName]}" ]]; then
echo "- $testName"
fi
done
} >&2
exit 1
'';
});
in
# Please the callTest pattern.
@@ -64,4 +145,4 @@ pkgs.lib.mapAttrs (
test = v;
driver = v;
}
) suite
) (suite // { systemd-eval = systemdEval; })
pkgs/build-support/testers/modular-service-compliance.nix
+127 -7
View File
@@ -15,6 +15,7 @@
evalConfig,
mkTest,
sharedDir,
callReload,
namePrefix ? "modular-service-compliance",
}:
@@ -45,6 +46,11 @@ let
];
warnings = [ "compliance child warning" ];
};
# A sub-service exercising reload derivation from a reload signal.
services.reloadee = {
process.argv = [ "${coreutils}/bin/true" ];
process.reloadSignal = "HUP";
};
};
};
};
@@ -65,7 +71,7 @@ let
};
testAssertions = {
expr = builtins.elem {
expr = lib.elem {
assertion = true;
message = "compliance test assertion";
} c.assertions;
@@ -73,12 +79,12 @@ let
};
testWarnings = {
expr = builtins.elem "compliance test warning" c.warnings;
expr = lib.elem "compliance test warning" c.warnings;
expected = true;
};
testSubServiceAssertions = {
expr = builtins.elem {
expr = lib.elem {
assertion = true;
message = "compliance child assertion";
} c.services.child.assertions;
@@ -86,14 +92,58 @@ let
};
testSubServiceWarnings = {
expr = builtins.elem "compliance child warning" c.services.child.warnings;
expr = lib.elem "compliance child warning" c.services.child.warnings;
expected = true;
};
# The reload-conflict assertion must not fire (its `assertion` must hold) for a
# service that sets only reloadSignal (guards the inverted-assertion fix, and the
# priority-aware conflict detection: reloadSignal derives reloadCommand internally,
# which must not be mistaken for a user-set conflict).
testNoReloadConflict = {
expr = lib.any (
a:
a.message
== "reloadSignal conflicts with reloadCommand. Please either use reloadSignal or reloadCommand."
&& !a.assertion
) c.services.reloadee.assertions;
expected = false;
};
# Setting process.reloadSignal derives process.reloadCommand
# (guards the misplaced-paren mkIf fix).
testReloadSignalDerivesCommand = {
expr = c.services.reloadee.process.reloadCommand;
expected = "${coreutils}/bin/kill -HUP $MAINPID";
};
# notificationProtocol submodule bools default to false.
testNotificationProtocolSystemdDefault = {
expr = c.notificationProtocol.systemd;
expected = false;
};
testNotificationProtocolS6Default = {
expr = c.notificationProtocol.s6;
expected = false;
};
# Setting both reloadSignal and reloadCommand explicitly is a genuine conflict,
# so the assertion must fire. Separate eval: checkDrv would fail on this.
testReloadConflictFires = {
expr = lib.any (
a:
a.message
== "reloadSignal conflicts with reloadCommand. Please either use reloadSignal or reloadCommand."
&& !a.assertion
) conflictEval.config.conflict.assertions;
expected = true;
};
# Separate eval for a failing assertion — checkDrv would fail here,
# so we only access config.
testFailingAssertionValue = {
expr = builtins.elem {
expr = lib.elem {
assertion = false;
message = "compliance failing assertion";
} failingEval.config.failing.assertions;
@@ -101,6 +151,16 @@ let
};
};
conflictEval = evalConfig {
services = {
conflict = {
process.argv = [ "${coreutils}/bin/true" ];
process.reloadSignal = "HUP";
process.reloadCommand = "${coreutils}/bin/kill -HUP $MAINPID";
};
};
};
failingEval = evalConfig {
services = {
failing = {
@@ -129,6 +189,24 @@ let
exec "${coreutils}/bin/sleep" infinity
'';
/**
A reloadable service script. Like `svc`, it namespaces a comms subdirectory
by its first argument and records the remaining arguments. It traps SIGHUP
and appends a marker line to `$dir/reloads` on each reload, then stays alive
as the trapping shell itself (no `exec`, so the trap and MAINPID are kept).
*/
reloadableSvc = writeShellScript "${namePrefix}-reloadable-svc" ''
id="$1"; shift
dir="${sharedDir}/$id"
mkdir -p "$dir"
: > "$dir/reloads"
reload() { printf 'reloaded\n' >> "$dir/reloads"; }
trap reload HUP
echo "$$" > "$dir/pid"
printf '%s\n' "$@" > "$dir/args"
while true; do "${coreutils}/bin/sleep" 1; done
'';
mkArgv =
id: extraArgs:
[
@@ -171,6 +249,27 @@ let
inherit text;
});
# The reload runtime test's service tree. The reloadable unit is the *sub*-service,
# so its name path `[ "reload" "inner" ]` handed to `callReload` exercises the
# integration's nested unit naming (NixOS dash-joins to `reload-inner.service`).
reloadServices = {
reload = {
# Parent must run something; a bare keep-alive is enough.
process.argv = [
reloadableSvc
"reload-parent"
];
services.inner.process = {
argv = [
reloadableSvc
"reload-inner"
];
# The script traps SIGHUP; reloadSignal derives the manager's reload command.
reloadSignal = "HUP";
};
};
};
in
{
# Eval-level tests: config structure, evaluated in the integration's
@@ -186,9 +285,9 @@ in
representative = evalResult.checkDrv;
passthru = {
tests = evalTestDefs;
failures = lib.runTests finalAttrs.passthru.tests;
failures = lib.runTests finalAttrs.finalPackage.tests;
};
testResults = lib.mapAttrs (_: test: test.expr == test.expected) finalAttrs.passthru.tests;
testResults = lib.mapAttrs (_: test: test.expr == test.expected) finalAttrs.finalPackage.tests;
buildCommand = ''
touch $out
for testName in "''${!testResults[@]}"; do
@@ -247,5 +346,26 @@ in
'';
};
# Runtime reload: start a reloadable service, ask the integration to reload it,
# and assert the service observed the reload (recorded a SIGHUP marker).
reload = mkTest {
name = "${namePrefix}-reload";
services = reloadServices;
testExe = mkTestScript "reload" ''
${waitAndCheck "reload-inner" [ ]}
${callReload [
"reload"
"inner"
]}
timeout=30; elapsed=0
while ! grep -qx "reloaded" "${sharedDir}/reload-inner/reloads" && [ "$elapsed" -lt "$timeout" ]; do
sleep 1; elapsed=$((elapsed + 1))
done
grep -qx "reloaded" "${sharedDir}/reload-inner/reloads" \
|| { echo "reload: no reload recorded after ''${timeout}s"; cat "${sharedDir}/reload-inner/reloads"; exit 1; }
echo "reload: reload observed"
'';
};
# See also the manual compliance items in doc/build-helpers/testers.chapter.md.
}