Switch to using a systemd generator for /var by cgwalters · Pull Request #859 · ostreedev/ostree

cgwalters · 2017-05-15T18:02:22Z

If one wants to set up a mount for /var in /etc/fstab, it
won't be mounted since ostree-prepare-root set up a bind mount for
/var to /sysroot/ostree/$stateroot/var, and systemd will take
the already extant mount over what's in /etc/fstab.

There are a few options to fix this, but what I settled on is parsing
/etc/fstab in a generator (exactly like systemd-fstab-generator does),
except here we look for an explicit mount for /var, and if one isn't found,
synthesize the default ostree mount to the stateroot. Another nice property is
that if an admin creates a var.mount unit in /etc for example, that will
also override our mount.

Note that today ostree doesn't hard depend on systemd, so this behavior only
kicks in if we're built with systemd and libmount support (for parsing
/etc/fstab). I didn't really test that case though.

Initially I started writing this as a "pure libc" program, but at one point
decided to use libostree.so to find the booted deployment. That didn't work
out because /boot wasn't necessarily mounted and hence we couldn't find the
bootloader config. A leftover artifact from this is that the generator code
calls into libostree via the "cmd private" infrastructure. But it's an easy way
to share code, and doesn't hurt.

I really have no idea what I was thinking with that list of mount points. It seems arbitrary. Sadly `git log` doesn't help, and there's no comments. Basically, the only mounts we should care about are those that libostree creates. Which are just `/sysroot` and `/var`. Systemd will handle the other things like `/tmp`, it's not our job, and we shouldn't touch them.

By checking the mount status, we avoid remounting things if we don't need to. And printing a single line per mount helps debugging when things go wrong.

If one wants to set up a mount for `/var` in `/etc/fstab`, it won't be mounted since `ostree-prepare-root` set up a bind mount for `/var` to `/sysroot/ostree/$stateroot/var`, and systemd will take the already extant mount over what's in `/etc/fstab`. There are a few options to fix this, but what I settled on is parsing `/etc/fstab` in a generator (exactly like `systemd-fstab-generator` does), except here we look for an explicit mount for `/var`, and if one *isn't* found, synthesize the default ostree mount to the stateroot. Another nice property is that if an admin creates a `var.mount` unit in `/etc` for example, that will also override our mount. Note that today ostree doesn't hard depend on systemd, so this behavior only kicks in if we're built with systemd *and* libmount support (for parsing `/etc/fstab`). I didn't really test that case though. Initially I started writing this as a "pure libc" program, but at one point decided to use `libostree.so` to find the booted deployment. That didn't work out because `/boot` wasn't necessarily mounted and hence we couldn't find the bootloader config. A leftover artifact from this is that the generator code calls into libostree via the "cmd private" infrastructure. But it's an easy way to share code, and doesn't hurt.

jlebon

Looks good overall! Will give it a spin as well.

jlebon · 2017-05-16T13:37:05Z

src/switchroot/ostree-remount.c

          if (errno != EINVAL)
            err (EXIT_FAILURE, "failed to remount %s", target);
        }
+      printf ("Remounted: %s\n", target);


Maybe in the else instead, e.g.

if (mount (...) < 0) { ... } else printf ("Remounted: %s\n", target);

?

We call err and abort the process if it fails.

But it'll still print it if we get EINVAL, right? That can be misleading. (Heck, maybe we should even print a different msg in the EINVAL case).

True, fixed. ⬇️

jlebon · 2017-05-16T14:39:37Z

src/libostree/ostree-impl-system-generator.c

+                               "Documentation=man:ostree(1)\n"
+                               "ConditionKernelCommandLine=!systemd.volatile\n"
+                               /* We need /sysroot mounted writable first */
+                               "After=ostree-remount.service\n"


How about a SourcePath=/etc/fstab here?

Don't we also need a Before=local-fs.target here?

I actually had that originally since I was copying systemd, but then I realized the source isn't /etc/fstab. Right?

Will add the Before=.

Oh right, got confused. Yeah, there's not really an applicable SourcePath here.

jlebon · 2017-05-16T14:57:33Z

configure.ac

+           AC_DEFINE([HAVE_LIBSYSTEMD], 1, [Define if we have libsystemd])
+           systemdsystemgeneratordir=$($PKG_CONFIG --variable=systemdsystemgeneratordir systemd)
+           AC_SUBST(systemdsystemgeneratordir)
+           )


Can you provide a --with-systemdsystemgeneratordir option here? So that I can do unprivileged installs without completely turning off systemd. I can add it as a follow-up PR too.

…erbose

jlebon · 2017-05-16T16:13:01Z

Verified working with /var as an fstab mount and not!
@rh-atomic-bot r+ 102ef0d

rh-atomic-bot · 2017-05-16T16:13:07Z

⌛ Testing commit 102ef0d with merge 3070588...

By checking the mount status, we avoid remounting things if we don't need to. And printing a single line per mount helps debugging when things go wrong. Closes: #859 Approved by: jlebon

If one wants to set up a mount for `/var` in `/etc/fstab`, it won't be mounted since `ostree-prepare-root` set up a bind mount for `/var` to `/sysroot/ostree/$stateroot/var`, and systemd will take the already extant mount over what's in `/etc/fstab`. There are a few options to fix this, but what I settled on is parsing `/etc/fstab` in a generator (exactly like `systemd-fstab-generator` does), except here we look for an explicit mount for `/var`, and if one *isn't* found, synthesize the default ostree mount to the stateroot. Another nice property is that if an admin creates a `var.mount` unit in `/etc` for example, that will also override our mount. Note that today ostree doesn't hard depend on systemd, so this behavior only kicks in if we're built with systemd *and* libmount support (for parsing `/etc/fstab`). I didn't really test that case though. Initially I started writing this as a "pure libc" program, but at one point decided to use `libostree.so` to find the booted deployment. That didn't work out because `/boot` wasn't necessarily mounted and hence we couldn't find the bootloader config. A leftover artifact from this is that the generator code calls into libostree via the "cmd private" infrastructure. But it's an easy way to share code, and doesn't hurt. Closes: #859 Approved by: jlebon

rh-atomic-bot · 2017-05-16T16:27:40Z

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing 3070588 to master...

cgwalters added 3 commits May 15, 2017 14:02

switchroot/remount: Check mount status before remounting, be verbose

89fadf2

By checking the mount status, we avoid remounting things if we don't need to. And printing a single line per mount helps debugging when things go wrong.

cgwalters mentioned this pull request May 15, 2017

support /var as a mount #855

Closed

cgwalters added 2 commits May 15, 2017 17:38

fixup! Switch to using a systemd generator for /var

978aad7

fixup! Switch to using a systemd generator for /var

17abdcb

jlebon reviewed May 16, 2017

View reviewed changes

cgwalters added 3 commits May 16, 2017 11:17

fixup! Switch to using a systemd generator for /var

5b211ce

fixup! Switch to using a systemd generator for /var

7766665

fixup! switchroot/remount: Check mount status before remounting, be v…

102ef0d

…erbose

cgwalters mentioned this pull request May 16, 2017

Release 2017.6 #865

Closed

cgwalters added the homu/approved label May 16, 2017

rh-atomic-bot closed this in f21f500 May 16, 2017

This was referenced May 18, 2017

Replace rpmbuild dependency with new rpmwriter module projectatomic/atomic#1005

Closed

Replace tarfile with tar on ostree import projectatomic/atomic#1007

Closed

This was referenced Jun 2, 2017

Use systemd mount unit file for extra volume mount in compat mode projectatomic/container-storage-setup#245

Closed

create unit file for disabling tmpfs on tmp coreos/rpm-ostree#820

Open

cgwalters mentioned this pull request Jun 16, 2017

Please clarify: does libostree require systemd? #934

Closed

cgwalters mentioned this pull request Jun 25, 2017

Render mount source as /var #960

Open

jlebon mentioned this pull request Jun 7, 2018

Ignition: useradd failure during initial boot openshift/os#86

Closed

LorbusChris pushed a commit to LorbusChris/ostree-spec that referenced this pull request Oct 23, 2018

spec: update for ostreedev/ostree#859

dd409a4

cgwalters mentioned this pull request Apr 30, 2020

[WIP]Mount raid0 device to /var openshift/release#8692

Closed

Conversation

cgwalters commented May 15, 2017

Uh oh!

jlebon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlebon commented May 16, 2017

Uh oh!

rh-atomic-bot commented May 16, 2017

Uh oh!

rh-atomic-bot commented May 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants