install: Fix broken warn_on_host_root check #910
Conversation
There was a problem hiding this comment.
Thanks for spotting this regression...clearly we should have some CI coverage that verifies we print the warning.
This code is quite complex and entangled now. I am wondering if we couldn't fix this by just moving the warning up above where we re-compute and fsopts.root_path.
Actually also staring at this code, I did #912
Arguably this was not a true regression but more like an inconsistency in a new feature - just a new installation target option (ostree) we added didn't have the warning we usually have for normal systems, so can't blame the CI for not covering something that never existed
Yeah I wanted to do that, but that would mean the check would run before prepare_install which would change the behavior and I wasn't confident this is desired. I'll gladly change the implementation if you think this is not a problem |
#912 would address that right? |
Ah yes, not directly, I'll wait for #912 to merge then I'll adjust this PR to fully address this |
omertuc
force-pushed
the
warn_on_host
branch
from
daddb84 to
ee1f0d1
Compare
The `warn_on_host_root` check was broken when we added support for installing on already-ostree systems (but only for already-ostree systems). See containers#907 The solution is to use the original user provided root_path for the fd passed to warn_on_host_root, rather than the modified one, as that will always match /proc/0/root's fsid (in ostree systems systemd is running with the deployment root as its root, and this is what we have mounted as /:/target) Signed-off-by: Omer Tuchfeld <[email protected]>
omertuc
force-pushed
the
warn_on_host
branch
from
ee1f0d1 to
b57fcad
Compare
The
warn_on_host_rootcheck was broken when we added support for installing on already-ostree systems.See #907
The solution is to use the original user provided root_path for the fd passed to warn_on_host_root, rather than the modified one, as that will always match /proc/0/root's fsid (in ostree systems systemd is running with the deployment root as its root, and this is what we have mounted as /:/target)