Update auto-tmpfiles code to skip already specified directories

[cgwalters]

systemd started shipping tmpfiles.d snippets for a variety of directories; this causes conflict warnings at runtime.

rpm-ostree would need to scan the tmpfiles.d snippets at install time and skip synthesizing entries if they are already specified.

[jlebon]

This is still relevant; I think we've just gotten used to seeing the duplicate warnings heh.

[HuijingHei]

I think you mean the duplicated info in systemd-tmpfiles-setup.

$ journalctl -u systemd-tmpfiles-setup
...
Oct 24 06:58:39 localhost systemd-tmpfiles[1481]: /usr/lib/tmpfiles.d/tmp.conf:12: Duplicate line for path "/var/tmp", ignoring.
Oct 24 06:58:39 localhost systemd-tmpfiles[1481]: /usr/lib/tmpfiles.d/var.conf:14: Duplicate line for path "/var/log", ignoring.
Oct 24 06:58:39 localhost systemd-tmpfiles[1481]: /usr/lib/tmpfiles.d/var.conf:19: Duplicate line for path "/var/cache", ignoring.
Oct 24 06:58:39 localhost systemd-tmpfiles[1481]: /usr/lib/tmpfiles.d/var.conf:21: Duplicate line for path "/var/lib", ignoring.
Oct 24 06:58:39 localhost systemd-tmpfiles[1481]: /usr/lib/tmpfiles.d/var.conf:23: Duplicate line for path "/var/spool", ignoring.
Oct 24 06:58:39 localhost systemd-tmpfiles[1481]: "/home" already exists and is not a directory.
Oct 24 06:58:39 localhost systemd-tmpfiles[1481]: "/srv" already exists and is not a directory.
Oct 24 06:58:39 localhost systemd-tmpfiles[1481]: "/root" already exists and is not a directory.

[chrisawi]

This seems to be preventing cleanup of /var/tmp by systemd-tmpfiles-clean.service on F38/F39 Silverblue.

pkg-filesystem.conf contains

d /var/tmp 1777 root root - -

whereas systemd's tmp.conf is

q /var/tmp 1777 root root 30d

[cgwalters]

Thanks, that is a pretty important bug.

[jlebon]

rpm-ostree would need to scan the tmpfiles.d snippets at install time and skip synthesizing entries if they are already specified.

@HuijingHei brought up internally that this is not as simple because the auto-conversion we do at package import time may be duplicating a tmpfiles.d dropin that lives in a separate package. This is in fact the case for all the warnings we currently see in FCOS at least:

[root@cosa-devsh tmpfiles.d]# journalctl --grep 'Duplicate line'
Nov 16 19:58:12 localhost systemd-tmpfiles[1619]: /usr/lib/tmpfiles.d/tmp.conf:12: Duplicate line for path "/var/tmp", ignoring.
Nov 16 19:58:12 localhost systemd-tmpfiles[1619]: /usr/lib/tmpfiles.d/var.conf:14: Duplicate line for path "/var/log", ignoring.
Nov 16 19:58:12 localhost systemd-tmpfiles[1619]: /usr/lib/tmpfiles.d/var.conf:19: Duplicate line for path "/var/cache", ignoring.
Nov 16 19:58:12 localhost systemd-tmpfiles[1619]: /usr/lib/tmpfiles.d/var.conf:21: Duplicate line for path "/var/lib", ignoring.
Nov 16 19:58:12 localhost systemd-tmpfiles[1619]: /usr/lib/tmpfiles.d/var.conf:23: Duplicate line for path "/var/spool", ignoring.

Both tmp.conf and var.conf come from systemd, but the auto-generation we do comes from filesystem.

Hmm, one way to fix #26 (comment) is to prefix our auto-generated dropins with e.g. zzzz- to ensure they're ordered last. That wouldn't fix the warnings, but at least the right entry would be given priority.

Fixing the warnings I think would require doing this in a postprocessing pass after all the packages are installed where we scan and collect all the non-generated entries, then hardlink-break & edit the auto-generated dropins to filter out duplicate entries.

[jlebon]

One interesting note while looking at this: systemd-tmpfiles warns only when there are incompatible duplicate lines. Otherwise, it doesn't mind them. (https://github.com/systemd/systemd/blob/905dd9d6e644fbcf12185cd45bbd05e08ac0e926/src/tmpfiles/tmpfiles.c#L3903-L3907 -> https://github.com/systemd/systemd/blob/905dd9d6e644fbcf12185cd45bbd05e08ac0e926/src/tmpfiles/tmpfiles.c#L3413-L3414).

We do have instances of generated tmpfiles.d dropins that duplicate dropins within the same package, but they're just silently ignored. E.g.:

[root@cosa-devsh tmpfiles.d]# cat mdadm.conf
d /run/mdadm 0710 root root -
[root@cosa-devsh tmpfiles.d]# cat pkg-mdadm.conf
d /run/mdadm 0710 root root - -

[jlebon]

Fixing the warnings I think would require doing this in a postprocessing pass after all the packages are installed where we scan and collect all the non-generated entries, then hardlink-break & edit the auto-generated dropins to filter out duplicate entries.

Or something like:

1. at assembly time, skip checkout of auto-generated dropins and instead collect the entries in them
2. once assembled, collect all the entries from the non-auto-generated dropins in the rootfs
3. write a single e.g. rpm-ostree-autoconverted.conf with all the non-duplicate entries

Also awkward, but more elegant than post-editing checked out files I think.

[HuijingHei]

Fixing the warnings I think would require doing this in a postprocessing pass after all the packages are installed where we scan and collect all the non-generated entries, then hardlink-break & edit the auto-generated dropins to filter out duplicate entries.

Or something like:

1. at assembly time, skip checkout of auto-generated dropins and instead collect the entries in them
2. once assembled, collect all the entries from the non-auto-generated dropins in the rootfs
3. write a single e.g. rpm-ostree-autoconverted.conf with all the non-duplicate entries

Also awkward, but more elegant than post-editing checked out files I think.

Thanks @jlebon for the pointer, will look at it.

[jlebon]

Something a bit more concrete/detailed:

1. in the importer code, change the filename to our auto-generated tmpfiles.d entries to e.g. rpm-ostree-autovar-$pkg.conf
2. in checkout_filter() in rpmostree-core.cxx, do something like:
   • if path is under /usr/lib/tmpfiles.d/:
     • read in all the entries in the config (we don't need a full parser here, just enough to pick up the path field)
     • if path has prefix 'rpm-ostree-autovar-`, add those entries to hash table A and skip checkout
     • otherwise, add the entries to hash table B and allow checkout
3. in rpmostree_context_assemble(), after all the packages have been checked out, call a function that creates a new rpm-ostree-2-pkg-autovar.conf containing all the entries in hash table A that are not in B

[HuijingHei]

Thanks @jlebon for the guidance.

For the auto-generated tmpfiles.d entries, we are using pkg-$pkg.conf, maybe we can still use it?

The tricky thing here is checkout_filter() will be triggered if there is files_skip || files_remove_regex, else checkout_filter() will be skipped.
For example, when checkout package filesystem, the files_skip is NULL, we can not trigger checkout_filter(), so can not skip /usr/lib/tmpfiles.d/pkg-filesystem.conf.

[jlebon]

Had a real-time chat with @HuijingHei about this. One thing we realized is that in the client-side case, the base checkout doesn't go through filtering. And adding a checkout filter to the base just for this feels like overhead. Now, we don't need to have this working client-side too, but if we can have it work both server and client-side for the same amount of work, that seems worth it.

So the suggestion I made is to do it all in step 3 essentially. I.e. from rpmostree_context_assemble(), we call a function like rpmostreecxx::deduplicate_tmpfiles_entries(tmprootfs_dfd), which scans the dropins, sorts them into two HashMaps and then unlink all the pkg-* files and create a single pkg-rpm-ostree-autovar.conf (re-using the same prefix ensures that this same logic works client-side).

For the auto-generated tmpfiles.d entries, we are using pkg-$pkg.conf, maybe we can still use it?

Yeah, I'm OK with that. The reason I suggested changing the prefix is so that it's more obviously rpm-ostree-owned. In practice, at least in FCOS right now, no package ships a dropin in the form pkg-....

[HuijingHei]

So I guess I'm leaning more now towards just pulling the trigger on not calling convert_var_to_tmpfiles_d() or generating rpm-ostree-1-autovar.conf at all when in unified core mode.

Thanks @jlebon , tried this but get error like:

   installroot: + ostree --repo=/var/tmp/test.JHW2Mm/repo ls fedora/x86_64/coreos/testing-devel /var/lib/rpm
   installroot: + echo found /var/lib/rpm in commit
   
   basic: Committing...done
   basic: error: Writing commit: While writing rootfs to mtree: Failed to look up SELinux label for '/var/lib/nfs/rpc_pipefs'

Add convert_var_to_tmpfiles_d() and append the files in delete some files list to

rpm-ostree/rust/src/composepost.rs

Lines 729 to 736 in ae2002c

	static KNOWN_STATE_FILES: &[&str] = &[
	// https://bugzilla.redhat.com/show_bug.cgi?id=789407
	"var/lib/systemd/random-seed",
	"var/lib/systemd/catalog/database",
	"var/lib/plymouth/boot-duration",
	// These two are part of systemd's var.tmp
	"var/log/wtmp",
	"var/log/btmp",

        // dup in pkg-rpm-ostree-autovar.conf
        "var/lib/alternatives",
        // dup in rpm-ostree-0-integration.conf
        "var/lib/rpm",
        "var/lib/vagrant",

These files would be deleted before converting path, so the entries will not be written in rpm-ostree-1-autovar.conf, it might be risky to remove var/lib directly.

But can still get duplicated /var/lib:

$ cat rpm-ostree-1-autovar.conf
d /var/lib 0755 root root - -

[HuijingHei]

Think more about this, one concern is how to skip trigger deduplicate_tmpfiles_entries() if there is nothing changes under /usr/lib/tmpfiles.d/, and it is difficult to remove the related tmpfiles entries if we run rpm-ostree override remove the package.

[jlebon]

Ahhh yes, good point. convert_var_to_tmpfiles_d() doesn't only create tmpfiles.d entries but also empties out /var as it goes. So I think what I'm saying is that in unified core mode, we should be able to just directly delete everything in /var. E.g. something like

diff --git a/src/libpriv/rpmostree-postprocess.cxx b/src/libpriv/rpmostree-postprocess.cxx
index 0b0b33d4..ed450ddb 100644
--- a/src/libpriv/rpmostree-postprocess.cxx
+++ b/src/libpriv/rpmostree-postprocess.cxx
@@ -425,7 +425,20 @@ postprocess_final (int rootfs_dfd, rpmostreecxx::Treefile &treefile, gboolean un
 
   ROSCXX_TRY (rootfs_prepare_links (rootfs_dfd), error);
 
-  ROSCXX_TRY (convert_var_to_tmpfiles_d (rootfs_dfd, *cancellable), error);
+  if (!unified_core_mode)
+    ROSCXX_TRY (convert_var_to_tmpfiles_d (rootfs_dfd, *cancellable), error);
+  else
+    {
+      /* In unified core mode, /var entries are converted to tmpfiles.d at
+       * import time and scriptlets are prevented from writing to /var. What
+       * remains is just the compat symlinks that we created ourselves, which we
+       * should stop writing since it duplicates other tmpfiles.d entries. */
+      if (!glnx_shutil_rm_rf_at (rootfs_dfd, "var", cancellable, error))
+        return FALSE;
+      /* but we still want the mount point as part of the OSTree commit */
+      if (!mkdirat (rootfs_dfd, "var", 0755))
+        return glnx_throw_errno_prefix (error, "mkdirat(var)");
+    }
 
   if (!rpmostree_rootfs_postprocess_common (rootfs_dfd, cancellable, error))
     return FALSE;

But let me think through it a bit more tomorrow and verify that this is safe to do.

Think more about this, one concern is how to skip trigger deduplicate_tmpfiles_entries() if there is nothing changes under /usr/lib/tmpfiles.d/, and it is difficult to remove the related tmpfiles entries if we run rpm-ostree override remove the package.

Yes, good insight. I think that means the "prefix our output with pkg- so it gets read client-side" trick isn't quite right. Probably the way we should look at this is that rpm-ostree-autovar.conf is a derived result and we need to make sure we can re-derive it from scratch client-side if e.g. the user removes or replaces a package.

So maybe we can tweak it like this:

• the importer code puts the generated tmpfiles.d dropins in e.g. /usr/lib/rpm-ostree/tmpfiles.d instead of the canonical place
• deduplicate_tmpfiles_entries() builds one hashmap from /usr/lib/rpm-ostree/tmpfiles.d and another from /usr/lib/tmpfiles.d and generates rpm-ostree-autovar.conf from the difference
• delete_package_from_root() is updated to remove from /usr/lib/rpm-ostree/tmpfiles.d

[HuijingHei]

Thanks @jlebon for the pointer, will try this.

[jlebon]

But let me think through it a bit more tomorrow and verify that this is safe to do.

So yeah, I do think this is safe to do. For reference, I verified /var is what we'd expect at that point in the compose:

# ls -lR var
var:
total 0
drwxr-xr-x. 1 root root 44 Nov 30 11:22 lib

var/lib:
total 12
lrwxrwxrwx. 1 root root 26 Nov 30 11:12 alternatives -> ../../usr/lib/alternatives
lrwxrwxrwx. 1 root root 19 Nov 30 11:22 rpm -> ../../usr/share/rpm
lrwxrwxrwx. 1 root root 21 Nov 30 11:12 vagrant -> ../../usr/lib/vagrant

So nothing surprising there; it basically matches the tmpfiles.d entries we derived into rpm-ostree-1-autovar.conf.

Note though that we're still creating those symlinks at assembly time so that they're part of the scriptlet environments. We're just not converting them to yet another tmpfiles.d dropin.

Also, the importer currently has code to automatically generate an entry for the /var/lib/vagrant symlink when importing vagrant. (It's the same logic that generates an entry for /var/lib/alternatives when composing server-side.) I've verified layering vagrant client-side still works and the symlink shows up.

I think one thing we could do in the future is run systemd-tmpfiles to populate /var during assembly so the tmpfiles.d dropins are canonical even for scriptlet environments. That should allow us to drop rootfs_prepare_links() entirely.