Fix mount propagation #85
Conversation
To switch root, systemd has to recursively make all mounts private, then after it recursively make all mounts shared. However `/run/mnt/*` and `/writable` are used to be bind mounted in the rest of the file system. For example, there is no reason for mount `/snap/hello/42` to also show up as `/writable/system-data/snap/hello/42` and `/run/mnt/data/system-data/snap/hello/42`.
|
None of these path with be available in |
alfonsosanchezbeato
left a comment
alfonsosanchezbeato
left a comment
There was a problem hiding this comment.
Some minor comments. FTR I have checked and /var/lib/snapd/hostfs/writable is empty in confined snaps and there is something in /var/lib/snapd/hostfs/run/mnt/data/system-data/snap, but just directories and symbolic links, so we should be safe for backwards compatibility there. Also, no interface gives access there at first sight.
| RemainAfterExit=yes | ||
| ExecStart=/usr/lib/core/remount-core-fs | ||
|
|
||
| [Install] |
There was a problem hiding this comment.
Install section is not needed and it looks like in general we don't add it unless it is the case.
There was a problem hiding this comment.
I know. I am not sure how I feel about the way we do it. We put everything in /usr/lib/systemd because /etc/systemd/system is writable. But we can always symlink to /dev/null in /etc to mask services installed in /usr. So the reason we use /usr/lib/systemd does not completely work.
I have added the Install section as indication of where it is expected to be installed. And in that way, we could one day move to another approach than what we are doing.
I can remove it.
There was a problem hiding this comment.
I actually prefer it to stay as I agree it is better to have it as a hint on where the service is expected. But then we should enforce it in next changes as good practice.
There was a problem hiding this comment.
I have created #86 so we only use install sections and generate the symlinks.
| Description=Reset propagation of initial mount points | ||
| DefaultDependencies=no | ||
| Before=local-fs-pre.target | ||
| Before=local-fs.target |
There was a problem hiding this comment.
Is this needed? local-fs-pre.target is expected to happen before afaiu.
There was a problem hiding this comment.
This is in case something happens with local-fs-pre.target, like something making it fail.
Note that I used roughly the same dependencies as systemd-remount-fs.service.
| Before=local-fs-pre.target | ||
| Before=local-fs.target | ||
| Before=shutdown.target | ||
| Wants=local-fs-pre.target |
There was a problem hiding this comment.
local-fs-pre.target is pulled already by local-fs.target
There was a problem hiding this comment.
No. It only has After=. Typically the -pre targets are only added if needed.
alfonsosanchezbeato
left a comment
alfonsosanchezbeato
left a comment
There was a problem hiding this comment.
LGTM, thanks
I have tested and this makes a difference for /writable/system-data/snap/, where mounted snap files are not seen anymore. /run/mnt/data/system-data/snap/ did not actually seem to have the issue though.
| RemainAfterExit=yes | ||
| ExecStart=/usr/lib/core/remount-core-fs | ||
|
|
||
| [Install] |
There was a problem hiding this comment.
I actually prefer it to stay as I agree it is better to have it as a hint on where the service is expected. But then we should enforce it in next changes as good practice.
| Before=local-fs-pre.target | ||
| Before=local-fs.target | ||
| Before=shutdown.target | ||
| Wants=local-fs-pre.target |
2 participants
To switch root, systemd has to recursively make all mounts private, then after it recursively make all mounts shared.
However
/run/mnt/*and/writableare used to be bind mounted in the rest of the file system. For example, there is no reason for mount/snap/hello/42to also show up as/writable/system-data/snap/hello/42and/run/mnt/data/system-data/snap/hello/42.