From 2a17756f45369f96d5bc8cf87238da2129e93197 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Wed, 15 May 2024 18:25:32 +0200 Subject: [PATCH] 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 --- osbuild/util/runners.py | 7 +------ runners/org.osbuild.centos9 | 1 - runners/org.osbuild.fedora30 | 1 - runners/org.osbuild.fedora38 | 1 - runners/org.osbuild.rhel81 | 1 - runners/org.osbuild.rhel82 | 1 - 6 files changed, 1 insertion(+), 11 deletions(-) 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)