Repository: osbuild/osbuild
Pull request #1741 (2024-04-16 19:02:52)

systemd.unit.create: support the After option in the Unit section (HMS-3814)
**stage/systemd.unit.create: add After option**

Support the After option in the Unit section of the unit file.

---

**test/systemd_unit_create: Wants, Requires, After**

Add test values for Wants, Requires, and After.
Adding multiple values to test that arrays work and made sure they're
all different.
The units need to be valid, real unit names otherwise the
'systemd-analyze verify' check will fail.

---

**stage/systemd.unit.create: move systemd-analyze verify to tests**

Verifying the systemd unit also checks if any referred systemd units
(Wants, Requires, After) exist and if all commands in Exec exist and are
executable.  Without '--root', the systemd-analyze verify command is
testing this against files in the build root, which isn't valid.

Units and binaries might not exist in the build root when referenced in
the image root tree, making the unit fail when when it's valid.
Conversely, the verification can succeed by finding executables in the
build root that don't exist in the image root tree when it should be
failing.

When verifying user units, systemd expects runtime directories.

All of this makes it quite difficult to verify systemd units properly
when building an image.  The call is useful for making sure the unit is
structured properly, but the user unit verification setup is difficult
to accomplish in a general way while building.

Remove the systemd-analyze verify step from the stage.  Move it to the
unit test so that we have some assurance that our unit file structure is
correct and things work as expected.  Create referenced unit files and
commands to make the unit valid.

---


ezr-ondrej: Quite cool addition :orange_heart: 
achilleas-k: Needs one more fix.  The `systemd-analyze verify` in the stage should be run with the `--root` option because it checks if the units it references are valid (`Wants` etc) and that executables in `Exec` exist and are executable.  That also needs some fiddling in the unit test to work.
achilleas-k: The change in cd791401217adbfadb60bd5f17f19375c2d0cf41 makes the `systemd-analyze verify` step in the stage verify against the root tree, so units and commands must exist in the target tree for it to be pass (which is the Right Thing).  However, this doesn't seem to work with user unit files.

The test manifest in `test/data/stages/systemd.unit.create/b.json` for example fails with:
```
Failed to lookup RuntimeDirectory path: No such device or address
Failed to initialize manager: No such device or address
```
achilleas-k: I ended up removing the `systemd-analyze verify` step from the stage and added it to the unit test but only for system services.  The reasons are described in the last commit (ping @mvo5).

I kept a version of some of the things I tried for getting it right here: https://github.com/osbuild/osbuild/compare/main...achilleas-k:osbuild:systemd-after-verify-stuff

The summary is that it gets very tricky to verify user units in a non-booted system.  We *could* skip verification only for user units, but it feels weird to have it only for certain situations.  User unit verification did work for me when I would bind mount the build root's `/run` into the tree, but I don't know if there are any cases that would make this fail unexpectedly.

If anyone feels strongly about adding the verification back, I can add it with the `/run` bind mount (and figure out how to get around it in the unit test).

Patch:
diff --git a/stages/org.osbuild.systemd.unit.create b/stages/org.osbuild.systemd.unit.create
index f4c78181..f0b61016 100755
--- a/stages/org.osbuild.systemd.unit.create
+++ b/stages/org.osbuild.systemd.unit.create
@@ -1,6 +1,5 @@
 #!/usr/bin/python3
 import configparser
-import subprocess
 import sys
 
 import osbuild.api
@@ -48,8 +47,6 @@ def main(tree, options):
     with open(f"{systemd_dir}/{filename}", "w", encoding="utf8") as f:
         config.write(f, space_around_delimiters=False)
 
-    subprocess.run(["systemd-analyze", "verify", f"{systemd_dir}/{filename}"], check=True)
-
 
 if __name__ == '__main__':
     args = osbuild.api.arguments()
diff --git a/stages/org.osbuild.systemd.unit.create.meta.json b/stages/org.osbuild.systemd.unit.create.meta.json
index d64d01ec..8919faab 100644
--- a/stages/org.osbuild.systemd.unit.create.meta.json
+++ b/stages/org.osbuild.systemd.unit.create.meta.json
@@ -18,6 +18,7 @@
     "    - 'DefaultDependencies' - bool",
     "    - 'Requires' - [strings]",
     "    - 'Wants' - [strings]",
+    "    - 'After' - [strings]",
     "  - 'Service' section",
     "    - 'Type' - string",
     "    - 'RemainAfterExit' - bool",
@@ -83,6 +84,12 @@
                   "type": "string"
                 }
               },
+              "After": {
+                "type": "array",
+                "items": {
+                  "type": "string"
+                }
+              },
               "Requires": {
                 "type": "array",
                 "items": {


Test patch:
diff --git a/stages/test/test_systemd_unit_create.py b/stages/test/test_systemd_unit_create.py
index b686261d..a6c22c95 100644
--- a/stages/test/test_systemd_unit_create.py
+++ b/stages/test/test_systemd_unit_create.py
@@ -1,7 +1,7 @@
 #!/usr/bin/python3
 
 import os
-import os.path
+import subprocess
 import textwrap
 
 import pytest
@@ -66,14 +66,27 @@ def test_systemd_unit_create(tmp_path, stage_module, unit_type, unit_path, expec
                 ],
                 "ConditionPathIsDirectory": [
                     "|!/etc/mydir"
-                ]
+                ],
+                # need to use real units otherwise the validation will fail
+                "Wants": [
+                    "basic.target",
+                    "sysinit.target",
+                ],
+                "Requires": [
+                    "paths.target",
+                    "sockets.target",
+                ],
+                "After": [
+                    "sysinit.target",
+                    "default.target",
+                ],
             },
             "Service": {
                 "Type": "oneshot",
                 "RemainAfterExit": True,
                 "ExecStart": [
-                    "mkdir -p /etc/mydir",
-                    "touch /etc/myfile"
+                    "/usr/bin/mkdir -p /etc/mydir",
+                    "/usr/bin/touch /etc/myfile"
                 ],
                 "Environment": [
                     {
@@ -100,24 +113,53 @@ def test_systemd_unit_create(tmp_path, stage_module, unit_type, unit_path, expec
             }
         }
     }
+
+    # create units in the tree for systemd-analyze verify
+    systemd_dir = "usr/lib/systemd/system/"
+    # if a unit file is empty, it gets detected as "masked" and fails verification
+    testutil.make_fake_tree(tmp_path, {
+        systemd_dir + "basic.target": "[Unit]\n",
+        systemd_dir + "sysinit.target": "[Unit]\n",
+        systemd_dir + "paths.target": "[Unit]\n",
+        systemd_dir + "sockets.target": "[Unit]\n",
+        "usr/bin/mkdir": "fake-mkdir",
+        "usr/bin/touch": "fake-touch",
+    })
+    for bin_name in ["mkdir", "touch"]:
+        os.chmod(tmp_path / "usr/bin" / bin_name, mode=0o755)
+
     expected_unit_path = tmp_path / expected_prefix / "create-directory.service"
-    # should the stage create the dir?
-    expected_unit_path.parent.mkdir(parents=True)
+    expected_unit_path.parent.mkdir(parents=True, exist_ok=True)
 
     stage_module.main(tmp_path, options)
     assert os.path.exists(expected_unit_path)
+    if unit_type == "system":
+        # When verifying user units, systemd expects runtime directories and more generally a booted system. Setting
+        # something up like that to verify it is more trouble than it's worth, especially since the system units are
+        # identical.
+        subprocess.run(["systemd-analyze", "verify",
+                        "--man=no",  # disable manpage checks
+                        f"--root={tmp_path}",  # verify commands and units in the root tree
+                        expected_unit_path],
+                       check=True)
     assert expected_unit_path.read_text(encoding="utf-8") == textwrap.dedent("""\
     [Unit]
     Description=Create directory
     DefaultDependencies=False
     ConditionPathExists=|!/etc/myfile
     ConditionPathIsDirectory=|!/etc/mydir
+    Wants=basic.target
+    Wants=sysinit.target
+    Requires=paths.target
+    Requires=sockets.target
+    After=sysinit.target
+    After=default.target
 
     [Service]
     Type=oneshot
     RemainAfterExit=True
-    ExecStart=mkdir -p /etc/mydir
-    ExecStart=touch /etc/myfile
+    ExecStart=/usr/bin/mkdir -p /etc/mydir
+    ExecStart=/usr/bin/touch /etc/myfile
     Environment="DEBUG=1"
     Environment="TRACE=1"
     EnvironmentFile=/etc/example.env