Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tmpfiles.d support creating btrfs subvolumes on ostree systems #18502

Closed
AdrianVovk opened this issue Feb 8, 2021 · 8 comments · Fixed by #18522
Closed

tmpfiles.d support creating btrfs subvolumes on ostree systems #18502

AdrianVovk opened this issue Feb 8, 2021 · 8 comments · Fixed by #18522
Labels
RFE 🎁 Request for Enhancement, i.e. a feature request tmpfiles

Comments

@AdrianVovk
Copy link
Contributor

Is your feature request related to a problem? Please describe.
I'd like tmpfiles to create a few btrfs subvolumes for me when booting from an empty /var. Problem is, on OSTree systems, / is not considered to be a btrfs subvolume, and so v/q/Q ends up creating basic directories instead of subvolumes. I put the real system root into a subvolume, and at runtime that ends up being put into /sysroot (which is considered to be a subvolume)

Describe the solution you'd like
I think a few options would work well for my use case:
A) Somehow detect this case (/sysroot is a subvolume, /run/ostree-booted, things like that)
B) Create a way to override this check in a system (maybe a drop-in that sets an environment variable on systemd-tmpfiles-setup.service or a config file?)
C) Only enforce this check when a container runtime is detected (since it was implemented for container managers that don't know about subvolumes, according to #1915)

Describe alternatives you've considered
A script that creates subvolumes before tmpfiles runs? That seems messy/ugly. Or, OSTree could create subvolumes instead of directories when checking out of the repo and into what eventually gets mounted to /, but that seems overkill (OSTree itself gains nothing from this)

The systemd version you checked that didn't have the feature you are asking for
247

@poettering
Copy link
Member

I don't think we should add explicit ostree support.

Consider prepping a patch that adds an env var $SYSTEMD_TMPFILES_FORCE_SUBVOL or so, which when set to 1 will make v/q create subvols, regardless of any other check. And if set to 0, not create it. Would be happy to review/merge such a patch

@poettering poettering added tmpfiles RFE 🎁 Request for Enhancement, i.e. a feature request labels Feb 8, 2021
@AdrianVovk
Copy link
Contributor Author

@poettering Can do. Any thoughts on detecting a container runtime instead? Either works for me

@poettering
Copy link
Member

poettering commented Feb 8, 2021

I don't like container checks. They are a last resort thing, or should be used when we actually really want something that is container specific. But I fail to see why the logic to use subvols when we are running in a subvol was any more right or more wrong when running on a container than on a host.

i think ostree (or your specific case) is really the outlier here, in that the OS is not subvolume managed, but you want subvolumes anyway. It has little to do with container or not.

i.e. a container could also run ostree, or not run it, and things wouldn't be any clearer there.

@AdrianVovk
Copy link
Contributor Author

AdrianVovk commented Feb 9, 2021

@poettering I need to separately check if the filesystem is btrfs, since before the btrfs_is_subvol also handled the btrfs_is_filesystem check. Is splitting btrfs_is_filesystem into btrfs_is_filesystem(char* path) and btrfs_is_filesystem_fd(int fd) out of scope for this PR? It would be in a separate commit than the tmpfiles changes but in the same PR

@poettering
Copy link
Member

Not sure I follow? the new env var should have no effect if the backing fs isn't btrfs anyway. i.e. you shouldn't be able to turn on the subvol logic on ext4 just by setting the env var, it would fail then anyway. The new env var should simply tweak the if (btrfs_is_subvol(empty_to_root(arg_root)) <= 0)… block in create_directory_or_subvolume() to shortcut things if the env var is set.

use getenv_bool() for checking the env var, btw

@AdrianVovk
Copy link
Contributor Author

@poettering The filesystem check happens in btrfs_is_subvol, so if the env var is set and btrfs_is_subvol is short-circuited away then the filesystem check won't happen.

And yep that's what I'm using for the env var :)

Should I update the man page to reflect that this env var exists? Sorry for all the questions I've never contributed to systemd before I want to make sure I get this right

@poettering
Copy link
Member

again, the check doesn't have to happen, if we don't want to create a subvol... hence not following?

env vars are our way to expose stuff that is not primary user-facing feaures, but more hacker stuff that only those in the know should use. We document those in docs/ENVIRONMENT.md

@AdrianVovk
Copy link
Contributor Author

@poettering The way I understand it: if SYSTEMD_TMPFILES_FORCE_SUBVOL=1, tmpfiles will ignore the result of btrfs_is_subvol and fall through to btrfs_make_subvol_fd. In the event that the variable is set and the filesystem is ext4, then nothing ever checks the fs type and tmpfiles will attempt to make a subvolume on ext4.

I think I'll implement the check so you can look at the code in the PR and review from there. I'm probably not explaining it clearly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFE 🎁 Request for Enhancement, i.e. a feature request tmpfiles
Development

Successfully merging a pull request may close this issue.

2 participants