prepare-root: Set up sysroot readonly in initramfs #2187

cgwalters · 2020-08-31T22:22:06Z

Let's ensure things are right from the start in the initramfs;
this closes off various race conditions. Followup to
3564225

Closes: #2115

openshift-ci-robot · 2020-08-31T22:22:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cgwalters]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cgwalters · 2020-08-31T22:22:42Z

This currently fails in FCOS since igniton-files can't run useradd; still debugging, the mounts all seem right though:

`-/sysroot                            /dev/vda4[/ostree/deploy/fedora-coreos/deploy/24baddf67040f815aa56043b20001aa94c80ca10a7f4b9441e8640562c968318.0]
                                                 xfs     rw,relatime,attr2,inode
  |-/sysroot/sysroot                  /dev/vda4  xfs     ro,relatime,attr2,inode
  |-/sysroot/etc                      /dev/vda4[/ostree/deploy/fedora-coreos/deploy/24baddf67040f815aa56043b20001aa94c80ca10a7f4b9441e8640562c968318.0/etc]
  |                                              xfs     rw,relatime,attr2,inode
  `-/sysroot/usr                      /dev/vda4[/ostree/deploy/fedora-coreos/deploy/24baddf67040f815aa56043b20001aa94c80ca10a7f4b9441e8640562c968318.0/usr]
                                                 xfs     ro,relatime,attr2,inode

smuda · 2021-09-22T02:28:59Z

What is the status of this issue?

I'm running into what looks very much like fedora-coreos issue #746 on stable FCOS releases 34.20210529.3.0 and 34.20210821.3.0 but works on FCOS Testing release 34.20210904.2.0. Is it possible this has been merged to testing but not yet stable?

In the `multipath.partition` test, we mount a multipath device on `/var/lib/containers`, and rely on systemd to create the mountpoint. But because we we weren't deterministically ordered against `ostree-remount.service`, in rare cases we can run approximately at the same time and systemd will try to create the mountpoint during that short window of time when `/var` is read-only. So then the test failed like this: ``` systemd[1]: Found device /dev/disk/by-label/dm-mpath-containers. systemd[1]: var-lib-containers.mount: Failed to check directory /var/lib/containers: No such file or directory systemd[1]: Mounting Mount /var/lib/containers... mount[983]: mount: /var/lib/containers: mount point does not exist. systemd[1]: var-lib-containers.mount: Mount process exited, code=exited, status=32/n/a systemd[1]: var-lib-containers.mount: Failed with result 'exit-code'. systemd[1]: Failed to mount Mount /var/lib/containers. ``` It took me a while to figure out this was the issue, because systemd ignores errors from `mkdir`: https://github.com/systemd/systemd/blob/3a18c0e5f2e4d8d46f3fd11cd0e421f52e727b0d/src/core/mount.c#L1023 An `EROFS` here would've made it much more obvious. Anyway, let's just add an `After=` in the mount unit to fix this. An alternative would've been to create the directory from Ignition (which it would've done automatically for us if we used it to set up the filesystem and mount unit, but we can't do this for non-root multipath, because $reasons). Long-term fix for this is ostreedev/ostree#2187.

jlebon · 2021-09-22T16:18:55Z

What is the status of this issue?

I'm running into what looks very much like fedora-coreos issue #746 on stable FCOS releases 34.20210529.3.0 and 34.20210821.3.0 but works on FCOS Testing release 34.20210904.2.0. Is it possible this has been merged to testing but not yet stable?

Interesting. It's not in any Fedora CoreOS release, so something else must be at play. The latest stable release from this week (34.20210904.3.0) is a promotion of 34.20210904.2.0, so I'd give that a try. If it still doesn't work, please file an issue on the FCOS tracker.

In the `multipath.partition` test, we mount a multipath device on `/var/lib/containers`, and rely on systemd to create the mountpoint. But because we we weren't deterministically ordered against `ostree-remount.service`, in rare cases we can run approximately at the same time and systemd will try to create the mountpoint during that short window of time when `/var` is read-only. So then the test failed like this: ``` systemd[1]: Found device /dev/disk/by-label/dm-mpath-containers. systemd[1]: var-lib-containers.mount: Failed to check directory /var/lib/containers: No such file or directory systemd[1]: Mounting Mount /var/lib/containers... mount[983]: mount: /var/lib/containers: mount point does not exist. systemd[1]: var-lib-containers.mount: Mount process exited, code=exited, status=32/n/a systemd[1]: var-lib-containers.mount: Failed with result 'exit-code'. systemd[1]: Failed to mount Mount /var/lib/containers. ``` It took me a while to figure out this was the issue, because systemd ignores errors from `mkdir`: https://github.com/systemd/systemd/blob/3a18c0e5f2e4d8d46f3fd11cd0e421f52e727b0d/src/core/mount.c#L1023 An `EROFS` here would've made it much more obvious. Anyway, let's just add an `After=` in the mount unit to fix this. An alternative would've been to create the directory from Ignition (which it would've done automatically for us if we used it to set up the filesystem and mount unit, but we can't do this for non-root multipath, because $reasons). Long-term fix for this is ostreedev/ostree#2187.

lucab · 2021-10-12T11:48:40Z

This currently fails in FCOS since igniton-files can't run useradd; still debugging

For reference, I think this was failing because there was the following leftover code:

  /* Default to true, but in the systemd case, default to false because it's handled by
   * ostree-system-generator. */
  bool mount_var = true;
#ifdef HAVE_SYSTEMD_AND_LIBMOUNT
  mount_var = false;
#endif

I've force-pushed the branch behind this PR to rebase on current main and to actually perform the RW bind-mount of var.

That plus coreos/fedora-coreos-config#1270 brought me to a booting image.
Initial boot seems fine, but still need to do further manual poking.

cgwalters

Thanks so much for working on this!

src/switchroot/ostree-prepare-root.c

lucab · 2021-10-25T08:23:58Z

This should be ready for review now.

cgwalters

Looking quite good, I just have a few minor nits. Thanks again for working on this!

src/switchroot/ostree-prepare-root.c

cgwalters · 2021-10-25T13:22:39Z

src/switchroot/ostree-prepare-root.c

+  /* Link to the deployment's /var */
+  if (mount_var)
+    {
+      if (snprintf (srcpath, sizeof(srcpath), "%s/../../var", deploy_path) < 0)


I notice that the path is different here; the previous code used a relative path:
mount ("../../var", "var", NULL, MS_BIND, NULL)
does that have any effect on the visible result in e.g. findmnt?

Just curious if you changed this intentionally.

It was semi-intentional, as I was trying to make it work initially by using absolute paths for my own sanity/readability.
It doesn't have any visible side-effects, as the kernel performs path normalization on mount anyway.
In order to reduce the scope of changes in this PR, I've kept using relative paths everywhere.

Let's ensure things are right from the start in the initramfs; this closes off various race conditions. Followup to ostreedev@3564225 Closes: ostreedev#2115

lucab · 2021-11-04T13:47:52Z

I've drained all the drive-by changes into separate PRs (all already merged: #2471, #2472, #2473, #2475), and adopted a few other suggestions from previous review.
Fully rebased on main now and ready to go.

cgwalters · 2021-11-04T13:59:42Z

I've drained all the drive-by changes into separate PRs (all already merged: #2471, #2472, #2473, #2475), and adopted a few other suggestions from previous review.

Thanks so much for doing that! In the end, I am fairly hopeful this change will sail through regression free, but if we discover a problem at least now it's easy to revert just this last bit while keeping all the cleanups, etc.

So, I just now discovered because I'm listed as the original author, I can't approve this PR. Which is clearly a gap in the system. Anyways, from my PoV feel free to approve the PR!

jlebon

Nice and clean. Thanks a lot for working on this!

openshift-ci-robot added the do-not-merge/work-in-progress label Aug 31, 2020

openshift-ci-robot requested review from d4s and mwleeds August 31, 2020 22:22

openshift-ci-robot added the approved label Aug 31, 2020

cgwalters mentioned this pull request Jan 15, 2021

fails to boot in CoreOS from git master #2268

Closed

ashcrow mentioned this pull request Feb 1, 2021

commit: Optionally translate ima xattr #2272

Closed

jlebon mentioned this pull request Feb 18, 2021

Booting to baremetal drops to emergency mode due to ostree-remount.service failure. coreos/fedora-coreos-tracker#746

Closed

cgwalters force-pushed the sysroot-ro-initramfs branch from c266f77 to d7b6666 Compare March 26, 2021 19:13

cgwalters mentioned this pull request Jun 15, 2021

ostree-remount.service fails on Rpi4 coreos/fedora-coreos-tracker#867

Closed

jlebon mentioned this pull request Sep 22, 2021

tests: squash ro /var race in multipath.partition coreos/coreos-assembler#2457

Merged

jlebon mentioned this pull request Sep 28, 2021

bootup: ostree-remount may race with systemd-backlight and fail coreos/fedora-coreos-tracker#975

Closed

lucab mentioned this pull request Oct 4, 2021

Enable sysroot.readonly at initrd time #2115

Closed

openshift-ci bot added the needs-rebase label Oct 10, 2021

lucab force-pushed the sysroot-ro-initramfs branch from d7b6666 to 571d33a Compare October 12, 2021 11:44

openshift-ci bot removed the needs-rebase label Oct 12, 2021

lucab mentioned this pull request Oct 12, 2021

[WIP] dracut/ignition-ostree: avoid double /sysroot/var mounts coreos/fedora-coreos-config#1270

Closed

lucab force-pushed the sysroot-ro-initramfs branch 2 times, most recently from c57b48f to 9c0fbd2 Compare October 18, 2021 13:12

cgwalters commented Oct 18, 2021

View reviewed changes

lucab force-pushed the sysroot-ro-initramfs branch 3 times, most recently from d32dc2d to d813818 Compare October 25, 2021 07:14

lucab changed the title ~~WIP: prepare-root: Set up sysroot readonly in initramfs~~ prepare-root: Set up sysroot readonly in initramfs Oct 25, 2021

openshift-ci bot removed the do-not-merge/work-in-progress label Oct 25, 2021

cgwalters commented Oct 25, 2021

View reviewed changes

lucab mentioned this pull request Oct 26, 2021

prepare-root: check return codes for errors when assembling paths #2472

Merged

openshift-ci bot added the needs-rebase label Oct 27, 2021

prepare-root: Set up sysroot readonly in initramfs

c553b5c

Let's ensure things are right from the start in the initramfs; this closes off various race conditions. Followup to ostreedev@3564225 Closes: ostreedev#2115

lucab force-pushed the sysroot-ro-initramfs branch from d813818 to c553b5c Compare November 4, 2021 11:58

openshift-ci bot removed the needs-rebase label Nov 4, 2021

cgwalters enabled auto-merge November 4, 2021 13:59

jlebon approved these changes Nov 4, 2021

View reviewed changes

cgwalters merged commit e580eca into ostreedev:main Nov 4, 2021

lucab mentioned this pull request Nov 15, 2021

[RFC] systemd-generators: perform ostree-remount early on coreos/fedora-coreos-config#1257

Closed

lucab mentioned this pull request Nov 23, 2021

Release 2021.6 #2489

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prepare-root: Set up sysroot readonly in initramfs #2187

prepare-root: Set up sysroot readonly in initramfs #2187

cgwalters commented Aug 31, 2020

openshift-ci-robot commented Aug 31, 2020

cgwalters commented Aug 31, 2020

smuda commented Sep 22, 2021 •

edited

Loading

jlebon commented Sep 22, 2021

lucab commented Oct 12, 2021 •

edited

Loading

cgwalters left a comment

lucab commented Oct 25, 2021

cgwalters left a comment

cgwalters Oct 25, 2021

lucab Nov 4, 2021

lucab commented Nov 4, 2021

cgwalters commented Nov 4, 2021

jlebon left a comment

prepare-root: Set up sysroot readonly in initramfs #2187

prepare-root: Set up sysroot readonly in initramfs #2187

Conversation

cgwalters commented Aug 31, 2020

openshift-ci-robot commented Aug 31, 2020

cgwalters commented Aug 31, 2020

smuda commented Sep 22, 2021 • edited Loading

jlebon commented Sep 22, 2021

lucab commented Oct 12, 2021 • edited Loading

cgwalters left a comment

Choose a reason for hiding this comment

lucab commented Oct 25, 2021

cgwalters left a comment

Choose a reason for hiding this comment

cgwalters Oct 25, 2021

Choose a reason for hiding this comment

lucab Nov 4, 2021

Choose a reason for hiding this comment

lucab commented Nov 4, 2021

cgwalters commented Nov 4, 2021

jlebon left a comment

Choose a reason for hiding this comment

smuda commented Sep 22, 2021 •

edited

Loading

lucab commented Oct 12, 2021 •

edited

Loading