Repository: osbuild/osbuild
Pull request #1788 (2024-05-15 18:06:01)

runners: revert "runners: clean up temp files before exiting the runner"
This reverts commit bc04bfc366ac785810a38a68a66b51a1b346d739.

The `remove_tmpfiles()` helper is nice but it is also problematic because it creates extra output after the command was run and created output. E.g. a test failure on centos stream9 [0]
```
            r = root.run(["stat", "--format=%a", "/var/tmp"], monitor)
            assert r.returncode == 0
>           assert r.stdout.strip().split("\n")[-1] == "1777"
E           AssertionError: assert '/usr/lib/tmp... such process' == '1777'
E
E             - 1777
E             + /usr/lib/tmpfiles.d/rpcbind.conf:2: Failed to resolve user 'rpc': No such process
```
Here the output from "stat" is not the last output because the rempve_tmpfiles runs `systemd-tmpfiles --clean --remove` which produces some noisy output after stat was run.

This was found by @thozza (thanks!) and discussed in osbuild PR#1785.

There are various ways to fix this, the one is to use the `--graceful` option of systemd-tmpfiles. However that only got added in systemd v256 and centos-stream9 has v252 so that is sadly not an option.

Plus even when avaialble it will produce some informational output like
```
All rules containing unresolvable specifiers will be skipped.
```

Another way would be to sent the output from systemd-tmpfiles cleanup to /dev/null. Not really great as we will not know about real problems or warnings that we should care about.

None of the option above is good. So I started looking at the tmpfiles.d rules and the cleanup and why we are doing it. It was added relatively recently in https://github.com/osbuild/osbuild/pull/1458 and after some medidiation not having it seems to do no harm (details below). The tl;dr is that the buildroot is created inside bubblewrap and the dirs that `--clean` and `--remove` touch are already tmpdirs created just for the buildroot so the cleanup in the runner is redundant (and because the cleanup is now run for each buidlroot.run() command there *might* be unintended conequences but the current rules seem to not have any).

In detail, the tmpfiles_cleanup() does two things:
1. `--clean` It will remove files that are older then the given age in tmpfiles.d. The tmpfiles in centos9 give me the following ages:
```
$ systemd-tmpfiles --cat-config|grep -E '[0-9]+d$'
d /var/lib/systemd/pstore 0755 root root 14d
d /var/lib/systemd/coredump 0755 root root 3d
q /tmp 1777 root root 10d
q /var/tmp 1777 root root 30d
D! /tmp/.X11-unix 1777 root root 10d
D! /tmp/.ICE-unix 1777 root root 10d
D! /tmp/.XIM-unix 1777 root root 10d
D! /tmp/.font-unix 1777 root root 10d
```
Given that we run our commands inside a bubblewrap environment and give it a fresh /run, /tmp, /var [1] there really should be no long lived things and even if there are they are cleaned up from the buildroot itself

2. `--remove` It will remove files marked for removal in tmpdfiles.d. Running it on a centos9 env it yields for me:
```
$ systemd-tmpfiles --cat-config|grep -E '^[rRD]'
R /var/tmp/dnf*/locks/*
r /var/cache/dnf/download_lock.pid
r /var/cache/dnf/metadata_lock.pid
r /var/lib/dnf/rpmdb_lock.pid
r /var/log/log_lock.pid
r! /forcefsck
r! /fastboot
r! /forcequotacheck
D! /var/lib/containers/storage/tmp 0700 root root
D! /run/podman 0700 root root
D! /var/lib/cni/networks
R! /var/tmp/container_images*
D     /run/rpcbind 0700  rpc  rpc  -  -
D /run/sudo/ts 0700 root root
R! /tmp/systemd-private-*
R! /var/tmp/systemd-private-*
r! /var/lib/systemd/coredump/.#*
D! /tmp/.X11-unix 1777 root root 10d
D! /tmp/.ICE-unix 1777 root root 10d
D! /tmp/.XIM-unix 1777 root root 10d
D! /tmp/.font-unix 1777 root root 10d
r! /tmp/.X[0-9]*-lock
```
which is also covered by the bwrap cleanup.

[0] https://artifacts.dev.testing-farm.io/2d07b8f3-5f52-4e61-b1fa-5328a0ff1058/#artifacts-/plans/unit-tests
[1] https://github.com/osbuild/osbuild/blob/main/osbuild/buildroot.py#L218

thozza: I clicked the `rebase` button, since the Gitlab CI was confused. The PR branch was never pushed to Gitlab and thus it didn't run. I didn't find any obvious way to trigger the `Trigger GitLab CI` job  manually.
mvo5: > I clicked the `rebase` button, since the Gitlab CI was confused. The PR branch was never pushed to Gitlab and thus it didn't run. I didn't find any obvious way to trigger the `Trigger GitLab CI` job manually.

Thanks! It seems to have not helped :( I will close/reopen and see if that fixes the CI problem. 
achilleas-k: Schutzbot's alive!!  :tada: 


Patch:
diff --git a/osbuild/util/runners.py b/osbuild/util/runners.py
index ec2a3fec..99f5de91 100644
--- a/osbuild/util/runners.py
+++ b/osbuild/util/runners.py
@@ -62,10 +62,6 @@ def tmpfiles():
     subprocess.run(["systemd-tmpfiles", "--create"], check=False)
 
 
-def remove_tmpfiles():
-    subprocess.run(["systemd-tmpfiles", "--clean", "--remove"], check=False)
-
-
 def nsswitch():
     # the default behavior is fine, but using nss-resolve does not
     # necessarily work in a non-booted container, so make sure that
@@ -94,8 +90,7 @@ def sequoia():
     # images).
     os.makedirs("/etc/crypto-policies", exist_ok=True)
     shutil.copytree(
-        "/usr/share/crypto-policies/back-ends/DEFAULT",
-        "/etc/crypto-policies/back-ends"
+        "/usr/share/crypto-policies/back-ends/DEFAULT", "/etc/crypto-policies/back-ends"
     )
 
 
diff --git a/runners/org.osbuild.centos9 b/runners/org.osbuild.centos9
index b9a6c397..3e0f60f0 100755
--- a/runners/org.osbuild.centos9
+++ b/runners/org.osbuild.centos9
@@ -14,6 +14,5 @@ if __name__ == "__main__":
             runners.tmpfiles()
             runners.nsswitch()
             r = subprocess.run(sys.argv[1:], check=False)
-            runners.remove_tmpfiles()
 
         sys.exit(r.returncode)
diff --git a/runners/org.osbuild.fedora30 b/runners/org.osbuild.fedora30
index b9a6c397..3e0f60f0 100755
--- a/runners/org.osbuild.fedora30
+++ b/runners/org.osbuild.fedora30
@@ -14,6 +14,5 @@ if __name__ == "__main__":
             runners.tmpfiles()
             runners.nsswitch()
             r = subprocess.run(sys.argv[1:], check=False)
-            runners.remove_tmpfiles()
 
         sys.exit(r.returncode)
diff --git a/runners/org.osbuild.fedora38 b/runners/org.osbuild.fedora38
index 45a7c245..2d0b8198 100755
--- a/runners/org.osbuild.fedora38
+++ b/runners/org.osbuild.fedora38
@@ -15,6 +15,5 @@ if __name__ == "__main__":
             runners.nsswitch()
             runners.sequoia()
             r = subprocess.run(sys.argv[1:], check=False)
-            runners.remove_tmpfiles()
 
         sys.exit(r.returncode)
diff --git a/runners/org.osbuild.rhel81 b/runners/org.osbuild.rhel81
index 9a911dea..db7fe78c 100755
--- a/runners/org.osbuild.rhel81
+++ b/runners/org.osbuild.rhel81
@@ -41,6 +41,5 @@ if __name__ == "__main__":
             os_release()
             runners.python_alternatives()
             r = subprocess.run(sys.argv[1:], check=False)
-            runners.remove_tmpfiles()
 
         sys.exit(r.returncode)
diff --git a/runners/org.osbuild.rhel82 b/runners/org.osbuild.rhel82
index 1c14e9da..101c5771 100755
--- a/runners/org.osbuild.rhel82
+++ b/runners/org.osbuild.rhel82
@@ -16,6 +16,5 @@ if __name__ == "__main__":
             runners.python_alternatives()
             env = runners.quirks()
             r = subprocess.run(sys.argv[1:], env=env, check=False)
-            runners.remove_tmpfiles()
 
         sys.exit(r.returncode)
diff --git a/stages/org.osbuild.systemd.unit.create b/stages/org.osbuild.systemd.unit.create
index f0b61016..2c367f3f 100755
--- a/stages/org.osbuild.systemd.unit.create
+++ b/stages/org.osbuild.systemd.unit.create
@@ -5,13 +5,24 @@ import sys
 import osbuild.api
 
 
-def main(tree, options):
-    filename = options["filename"]
+def validate(filename, cfg):
     # ensure the service name does not exceed maximum filename length
     if len(filename) > 255:
         raise ValueError(f"Error: the {filename} service exceeds the maximum filename length.")
 
+    # Filename extension must match the config:
+    #   .service requires a Service section
+    #   .mount requires a Mount section
+    if filename.endswith(".service") and "Service" not in cfg:
+        raise ValueError(f"Error: {filename} unit requires Service section")
+    if filename.endswith(".mount") and "Mount" not in cfg:
+        raise ValueError(f"Error: {filename} unit requires Mount section")
+
+
+def main(tree, options):
+    filename = options["filename"]
     cfg = options["config"]
+    validate(filename, cfg)
 
     # We trick configparser into letting us write multiple instances of the same option by writing them as keys with no
     # value, so we enable allow_no_value
diff --git a/stages/org.osbuild.systemd.unit.create.meta.json b/stages/org.osbuild.systemd.unit.create.meta.json
index 05431429..2446a8c0 100644
--- a/stages/org.osbuild.systemd.unit.create.meta.json
+++ b/stages/org.osbuild.systemd.unit.create.meta.json
@@ -1,13 +1,13 @@
 {
-  "summary": "Create Systemd services unit file",
+  "summary": "Create a Systemd unit file",
   "description": [
-    "This stage allows to create Systemd unit files in",
-    "`/usr/lib/systemd/system/`. The `filename` property specifies the",
-    "'.service' file to be added. These names are validated using the",
-    "same rules as specified by systemd.unit(5) and they must contain the",
-    "'.service' suffix (other types of unit files are not supported).",
-    "'unit-type' determines if the unit file is created at global or",
-    "system level,'unit-path' will determine the systemd unit load path.",
+    "This stage allows to create Systemd unit files. The `filename` property",
+    "specifies the, '.service' or '.mount' file to be added. These names are",
+    "validated using the, same rules as specified by systemd.unit(5) and they",
+    "must contain the, '.service' or '.mount' suffix (other types of unit files",
+    "are not supported). 'unit-type' determines if the unit file is created at",
+    "'global' (user) or 'system' paths,'unit-path' will determine the systemd",
+    "unit load path.",
     "",
     "The Unit configuration can currently specify the following subset",
     "of options:",
@@ -28,6 +28,11 @@
     "    - 'ExecStart' - [string]",
     "    - 'Environment' - [object]",
     "    - 'EnvironmentFile' - [string]",
+    "  - 'Mount' section",
+    "    - 'What' - string",
+    "    - 'Where' - string",
+    "    - 'Type' - string",
+    "    - 'Options' - string",
     "  - 'Install' section",
     "    - 'WantedBy' - [string]",
     "    - 'RequiredBy' - [string]"
@@ -41,7 +46,7 @@
     "properties": {
       "filename": {
         "type": "string",
-        "pattern": "^[\\w:.\\\\-]+[@]{0,1}[\\w:.\\\\-]*\\.service$"
+        "pattern": "^[\\w:.\\\\-]+[@]{0,1}[\\w:.\\\\-]*\\.(service|mount)$"
       },
       "unit-type": {
         "type": "string",
@@ -64,10 +69,29 @@
       "config": {
         "additionalProperties": false,
         "type": "object",
-        "required": [
-          "Unit",
-          "Service",
-          "Install"
+        "oneOf": [
+          {
+            "required": [
+              "Unit",
+              "Service",
+              "Install"
+            ],
+            "not": {
+              "required": [
+                "Mount"
+              ]
+            }
+          },
+          {
+            "required": [
+              "Mount"
+            ],
+            "not": {
+              "required": [
+                "Service"
+              ]
+            }
+          }
         ],
         "description": "Configuration for a '.service' unit.",
         "properties": {
@@ -186,6 +210,32 @@
               }
             }
           },
+          "Mount": {
+            "additionalProperties": false,
+            "type": "object",
+            "description": "'Mount' configuration section of a unit file.",
+            "required": [
+              "What"
+            ],
+            "properties": {
+              "What": {
+                "description": "Absolute path to device node",
+                "type": "string"
+              },
+              "Where": {
+                "description": "Absolute path for a file or directory for the mountpoint",
+                "type": "string"
+              },
+              "Type": {
+                "descriptions": "File system type",
+                "type": "string"
+              },
+              "Options": {
+                "descriptions": "Mount options to use when mounting",
+                "type": "string"
+              }
+            }
+          },
           "Install": {
             "additionalProperties": false,
             "type": "object",


Test patch:
diff --git a/stages/test/test_systemd_unit_create.py b/stages/test/test_systemd_unit_create.py
index 35e27a79..a748e2f0 100644
--- a/stages/test/test_systemd_unit_create.py
+++ b/stages/test/test_systemd_unit_create.py
@@ -21,16 +21,42 @@ STAGE_NAME = "org.osbuild.systemd.unit.create"
                 "Service": {},
                 "Install": {},
             },
-        }, ""),
+        },
+        "",
+    ),
+    (
+        {
+            "filename": "foo.mount",
+            "config": {
+                "Unit": {},
+                "Mount": {"What": ""},
+                "Install": {},
+            },
+        },
+        "",
+    ),
+    (
+        {
+            "filename": "foo.mount",
+            "config": {
+                "Mount": {"What": ""},
+            },
+        },
+        "",
+    ),
     # bad
     ({"config": {"Unit": {}, "Service": {}, "Install": {}}}, "'filename' is a required property"),
     ({"filename": "foo.service"}, "'config' is a required property"),
     ({"filename": "foo.service", "config": {"Service": {}, "Install": {}}},
-     "'Unit' is a required property"),
+     "{'Service': {}, 'Install': {}} is not valid under any of the given schemas"),
     ({"filename": "foo.service", "config": {"Unit": {}, "Install": {}}},
-     "'Service' is a required property"),
+     "{'Unit': {}, 'Install': {}} is not valid under any of the given schemas"),
     ({"filename": "foo.service", "config": {"Unit": {}, "Service": {}}},
-     "'Install' is a required property"),
+     "{'Unit': {}, 'Service': {}} is not valid under any of the given schemas"),
+    ({"filename": "foo.service", "config": {"Service": {}, "Mount": {"What": ""}}},
+     "{'Service': {}, 'Mount': {'What': ''}} is not valid under any of the given schemas"),
+    ({"filename": "foo.service", "config": {"Unit": {}, "Service": {}, "Mount": {"What": ""}, "Install": {}}},
+     "{'Unit': {}, 'Service': {}, 'Mount': {'What': ''}, 'Install': {}} is not valid under any of the given schemas"),
 ])
 @pytest.mark.parametrize("stage_schema", ["1"], indirect=True)
 def test_schema_validation(stage_schema, test_data, expected_err):
@@ -46,6 +72,54 @@ def test_schema_validation(stage_schema, test_data, expected_err):
         testutil.assert_jsonschema_error_contains(res, expected_err, expected_num_errs=1)
 
 
+@pytest.mark.parametrize("test_data,expected_err", [
+    # good
+    (
+        {
+            "filename": "foo.service",
+            "config": {
+                "Unit": {},
+                "Service": {},
+                "Install": {},
+            },
+        },
+        "",
+    ),
+    (
+        {
+            "filename": "foo.mount",
+            "config": {
+                "Unit": {},
+                "Mount": {"What": ""},
+                "Install": {},
+            },
+        },
+        "",
+    ),
+    (
+        {
+            "filename": "foo.mount",
+            "config": {
+                "Mount": {"What": ""},
+            },
+        },
+        "",
+    ),
+    # bad
+    ({"filename": "something.service", "config": {"Unit": {}, "Mount": {}, "Install": {}}},
+     "Error: something.service unit requires Service section"),
+    ({"filename": "data-gifs-cats.mount", "config": {"Unit": {}, "Service": {}, "Install": {}}},
+     "Error: data-gifs-cats.mount unit requires Mount section"),
+])
+def test_name_config_match(tmp_path, stage_module, test_data, expected_err):
+    expected_unit_path = tmp_path / "usr/lib/systemd/system"
+    expected_unit_path.mkdir(parents=True, exist_ok=True)
+    try:
+        stage_module.main(tmp_path, test_data)
+    except ValueError as ve:
+        assert expected_err == str(ve)
+
+
 @pytest.mark.parametrize("unit_type,unit_path,expected_prefix", [
     ("system", "usr", "usr/lib/systemd/system"),
     ("system", "etc", "etc/systemd/system"),
@@ -174,3 +248,80 @@ def test_systemd_unit_create(tmp_path, stage_module, unit_type, unit_path, expec
     RequiredBy=multi-user.target
 
     """)
+
+
+@pytest.mark.parametrize("unit_type,unit_path,expected_prefix", [
+    ("system", "usr", "usr/lib/systemd/system"),
+    ("system", "etc", "etc/systemd/system"),
+    ("global", "usr", "usr/lib/systemd/user"),
+    ("global", "etc", "etc/systemd/user"),
+])
+def test_systemd_unit_create_mount(tmp_path, stage_module, unit_type, unit_path, expected_prefix):
+    options = {
+        "filename": "data-test.mount",
+        "unit-type": unit_type,
+        "unit-path": unit_path,
+        "config": {
+            "Unit": {
+                "DefaultDependencies": True,
+                "ConditionPathExists": [
+                    "/data"
+                ],
+                "ConditionPathIsDirectory": [
+                    "/data"
+                ],
+                "Before": [
+                    "local-fs.target",
+                ],
+            },
+            "Mount": {
+                "What": "/dev/sda2",
+                "Where": "/data/test",
+                "Type": "xfs",
+                "Options": "rw,noatime"
+            }
+        }
+    }
+
+    # 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 / "data-test.mount"
+    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]
+    DefaultDependencies=True
+    ConditionPathExists=/data
+    ConditionPathIsDirectory=/data
+    Before=local-fs.target
+
+    [Mount]
+    What=/dev/sda2
+    Where=/data/test
+    Type=xfs
+    Options=rw,noatime
+
+    """)