minor feedback grab-bag

[cgwalters]

I looked through the code some while trying out stratis+Ignition for Fedora CoreOS. This is some of my notes for minor feedback while I was doing that. I hope this is useful - I'm mostly (ab)using GH issues as a communication channel/discussion forum. Feel free to close this if you want to take some bits of this as separate issues!

Overall I think https://github.com/coreos/bootupd/ may have some code/ideas you would find useful - and please let me know if there's anything (crate, code, ideas) you think is working well in Stratis that bootupd might be missing.

• Could use my openat-ext crate to write files more atomically https://github.com/coreos/bootupd/blob/6e1c089ccfea1aa6b1651c5eced7bba4c4048811/src/backend/statefile.rs#L95 (uses O_TMPFILE, also makes it convenient to serialize directly to file instead of memory)
• systemd sandboxing is a good idea; potentially start with e.g. ProtectHome=yes
• Could a private mount namespace make sense for some cases? bootupd uses MountFlags=slave
• Can we use ConditionPathExists=/path/to/stratis/state to avoid running if not used?
• Consider e.g. anyhow+Context to add information to errors. I hit a plain mkdir: Permission denied with 2.1.0 - IMO better would be e.g. error: daemon startup: failed to create state directory: mkdir(/stratis): Permission denied.

[jbaublitz]

Hi @cgwalters, thanks for the feedback!

Could use my openat-ext crate to write files more atomically https://github.com/coreos/bootupd/blob/6e1c089ccfea1aa6b1651c5eced7bba4c4048811/src/backend/statefile.rs#L95 (uses O_TMPFILE, also makes it convenient to serialize directly to file instead of memory)

Can you give a little bit more detail in which context and part of stratisd you would see this used?

systemd sandboxing is a good idea; potentially start with e.g. ProtectHome=yes

After reading up on this, I don't think that this would cause any issues given that we never try to access /root, /home, or /run/user. Given that any users mounting Stratis filesystems at any of the above directories would not require support from stratisd to do so, I think this is safe, but we should probably discuss a little bit more just to be sure I'm not missing anything.

Could a private mount namespace make sense for some cases? bootupd uses MountFlags=slave

Can you give an example of where you see this used? We already do this in more recent versions of stratisd. I'm happy to provide more detail on where we're using this.

Can we use ConditionPathExists=/path/to/stratis/state to avoid running if not used?

What Stratis state are you referring to here? I'm not exactly sure in what case you would want us to avoid running stratisd.

Consider e.g. anyhow+Context to add information to errors. I hit a plain mkdir: Permission denied with 2.1.0 - IMO better would be e.g. error: daemon startup: failed to create state directory: mkdir(/stratis): Permission denied.

We're definitely continuing work on this area, and we've put up a number of PRs to accomplish this as we find error messages that don't provide adequate information. However, you may want to use a later version of stratisd that provides symlinks at /dev/stratis because that change was in part made to conform to the filesystem higherarchy that most Linux users expect for devices, and it should resolve your problem.

[cgwalters]

Can you give a little bit more detail in which context and part of stratisd you would see this used?

https://github.com/stratis-storage/stratisd/pull/2381 is one I saw.

[cgwalters]

After reading up on this, I don't think that this would cause any issues given that we never try to access /root, /home, or /run/user. Given that any users mounting Stratis filesystems at any of the above directories would not require support from stratisd to do so, I think this is safe, but we should probably discuss a little bit more just to be sure I'm not missing anything.

Yeah, it'd obviously be very surprising if somehow stratisd itself somehow ended up accessing /home but IMO we can't be too careful with code that runs as root.

What Stratis state are you referring to here? I'm not exactly sure in what case you would want us to avoid running stratisd.

I don't know Stratis well enough to say, but broadly speaking the idea here is that if you don't have any Stratis volumes, the daemon wouldn't start (if we can detect that by the presence of a simple file) or it would auto-exit (perhaps on idle). (auto-exit-on-idle is actually really hard to do with DBus, rpm-ostree has some quite complex code for this - see https://github.com/coreos/rpm-ostree/pull/660 ).

Basically it's good for startup time for things to not start at all unless needed, and it's good for OS components to return RAM to the user if they're not doing anything. (This matters a lot more for rpm-ostree because we end up keeping e.g. the Fedora RPM repodata in memory and that's huuuuge)

(Again none of this is a big deal, just some minor feedback)

[jbaublitz]

After reading up on this, I don't think that this would cause any issues given that we never try to access /root, /home, or /run/user. Given that any users mounting Stratis filesystems at any of the above directories would not require support from stratisd to do so, I think this is safe, but we should probably discuss a little bit more just to be sure I'm not missing anything.

Yeah, it'd obviously be very surprising if somehow stratisd itself somehow ended up accessing /home but IMO we can't be too careful with code that runs as root.

Okay, great! I'll discuss with the team.

What Stratis state are you referring to here? I'm not exactly sure in what case you would want us to avoid running stratisd.

I don't know Stratis well enough to say, but broadly speaking the idea here is that if you don't have any Stratis volumes, the daemon wouldn't start (if we can detect that by the presence of a simple file) or it would auto-exit (perhaps on idle). (auto-exit-on-idle is actually really hard to do with DBus, rpm-ostree has some quite complex code for this - see coreos/rpm-ostree#660 ).

Basically it's good for startup time for things to not start at all unless needed, and it's good for OS components to return RAM to the user if they're not doing anything. (This matters a lot more for rpm-ostree because we end up keeping e.g. the Fedora RPM repodata in memory and that's huuuuge)

(Again none of this is a big deal, just some minor feedback)

This one is a little trickier because we listen for udev events. Just because stratisd detects that there are no devices currently does not mean that we won't receive udev events that a Stratis device has appeared. We always expect stratisd to be running to catch those udev events and I'm not sure there's a way around this.