-
Notifications
You must be signed in to change notification settings - Fork 354
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
A few more patches for rpmostreepayload + /var (and other) mounts #1124
Conversation
In rhinstaller#903 we changed all of rpmostreepayload's internal mounts to be recursive to fix `/boot/efi`. And indeed all should be recursive, except one - the `/sysroot` mount. By asking for that to be recursive we've created a loop. I noticed a while ago that the output of `findmnt` during an install was insane (the deployment directory was visible twice, etc.), but never debugged it. It turns out this was the cause. Fixing this makes the mounts look a lot more sane, and will help for future work I have for `/var` mounts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me. Thanks for this PR.
Just one question and one small code style tweak from my point of view.
self._safeExecWithRedirect("mount", | ||
["--bind", src, src]) | ||
self._safeExecWithRedirect("mount", | ||
["--bind", "-o", "remount,ro", src, src]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you have two steps for the ro
mount?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's how it works, see man mount
.
mount(8) since v2.27 allows to change the mount options by passing the relevant options along with --bind. For example: mount --bind,ro foo foo This feature is not supported by the Linux kernel; it is implemented in userspace by an additional mount(2) remounting system call. This solution is not atomic. The alternative (classic) way to create a read-only bind mount is to use the remount operation, for example: mount --bind olddir newdir mount -o remount,bind,ro olddir newdir
(We could use the mount --bind,ro
feature but I'd need to undo it when backporting to RHEL)
@@ -239,46 +239,73 @@ def install(self): | |||
|
|||
mainctx.pop_thread_default() | |||
|
|||
# Internal API for setting up bind mounts between the physical root and | |||
# sysroot, also ensures we track them in self._internal_mounts so we can | |||
# cleanly unmount them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add this documentation comment below the function name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I tweaked it into a pydoc comment as well. ⬇️
Jenkins, it's ok to test. |
8558975
to
2e87cd6
Compare
Please fix this issue:
I think we can merge this when this error above will be fixed. |
We want to support admin-defined mounts like `/var`, or default mounts like `/home`, which using ostree is really `/var/home`. Some work landed upstream for this already, and worked when backported to RHEL, but since 664ef7b things need to be handled differently. With this patch, we explicitly iterate over all of the Blivet storage mounts, and bind mount them from the physical root into the sysroot. This allows us to remove our special casing for e.g. `/boot`. We introduce an internal helper API for this which I think makes the code a lot more readable and clear; it handles adding the physical/sysroot path to arguments rather than doing the string concatenation in each caller, and has clear keyword args for the different bind types.
Currently ostree hardcodes creating e.g. `/var/lib`, see: https://github.com/ostreedev/ostree/blob/82024f161b280b4ef70880ddd5bc83ad812e6f0a/src/libostree/ostree-sysroot.c#L1388 I'd like to remove that, as it doesn't work in the case where in Anaconda the admin sets up a `/var` mountpoint. Unfortunately, we can't just tell `systemd-tmpfiles` to use `--prefix=/var/lib`, as that will create a lot of directories for which we don't have NSS configuration, and there's no `--only=/var/lib` or similar. Hence we just `mkdir -p` it explicitly.
In order to support `/home`, we need to make the directories in `/var`. So first handle `/var`, then tmpfiles for subdirectories which can be used as mounts like `/var/home`, then any further mounts, then the rest of tmpfiles.
2e87cd6
to
fd0ce54
Compare
Updated ⬆️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me!
Thanks for this update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
This will close https://github.com/rhinstaller/anaconda/issues/1007
I'm tempted to squash the patches...but the series I think is quite useful to fully
see the chain of bugfixes and rationale.