-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
apple virtiofs: fix racy mount setup #23118
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99 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:
Approvers can indicate their approval by writing |
Ephemeral COPR build failed. @containers/packit-build please check. |
The task got renamed but didn't fix the URL for the machine test task only the artifacts task url was fixed. Fixes 439fe90 ("Minor: Rename the OSX Cross task") Signed-off-by: Paul Holzinger <[email protected]>
One problem on FCOS is that the root directory is immutable, as such in order to mount arbitrary paths from the host we must make it mutable again and create these dir on boot in order to be able to mount there. The current logic was racy as it used one unit for each path and they all did chattr -i /; mkdir -p $path; chattr -i / and systemd can run these units in parallel. That means it was possible for another unit to make / immutable before the unit could do the mkdir. I pointed this out on the original PR[1] but we never followed up on it... Now this here changes several things. First have one unit that does the chattr -i / (immutable-root-off.service), it is hooked into remote-fs-pre.target which means it is executed before the network mounts (virtiofs) are done. Then we have another unit that does chattr +i / (immutable-root-on.service) which turn the immutable root back on after remote-fs.target which means all mount are done at this point. Additionally the automount unit is removed because it does not add any value for us and it was borken anyway as it used the virtiofs tag as path so systemd just ignored it. [1] containers#20612 (comment) Fixes containers#22569 Signed-off-by: Paul Holzinger <[email protected]>
@ashley-cui @baude PTAL |
FYI This was my test commit 1b4ffad and the logs for it |
Not an expert here but LGTM |
immutableRootOn.Add("Service", "Type", "oneshot") | ||
immutableRootOn.Add("Service", "ExecStart", "chattr +i /") | ||
|
||
immutableRootOn.Add("Install", "WantedBy", "remote-fs.target") |
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.
systemd makes my head hurt, so this is probably a stupid question, but: line 121 is After remote-fs.target
, and this one is WantedBy remote-fs.target
, isn't that a circular dependency loop?
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.
No, https://www.freedesktop.org/software/systemd/man/latest/systemd.unit.html#Wants=
Note that requirement dependencies do not influence the order in which services are started or stopped. This has to be configured independently with the After= or Before= options. If unit foo.service pulls in unit bar.service as configured with Wants= and no ordering is configured with After= or Before=, then both units will be started simultaneously and without any delay between them if foo.service is activated.
The WantedBy is used by systemctl enable to active the unit when that given target is reached. We can still say we want to run this unit of this one. But I can double check the logs
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.
core@localhost:~$ systemctl list-dependencies --reverse immutable-root-off.service
immutable-root-off.service
● └─remote-fs-pre.target
○ ├─dracut-initqueue.service
○ ├─iscsi-shutdown.service
○ ├─iscsid.service
○ ├─iscsiuio.service
● └─nfs-client.target
● ├─multi-user.target
○ │ └─graphical.target
● └─remote-fs.target
○ ├─dracut-pre-pivot.service
● └─multi-user.target
○ └─graphical.target
core@localhost:~$ systemctl list-dependencies --reverse immutable-root-on.service
immutable-root-on.service
● └─remote-fs.target
○ ├─dracut-pre-pivot.service
● └─multi-user.target
○ └─graphical.target
I think this shows it nicely enough to understand.
/lgtm |
I believe this comment podman/pkg/machine/apple/apple.go Lines 69 to 71 in c86386e
|
One problem on FCOS is that the root directory is immutable, as such in
order to mount arbitrary paths from the host we must make it mutable
again and create these dir on boot in order to be able to mount there.
The current logic was racy as it used one unit for each path and they
all did chattr -i /; mkdir -p $path; chattr -i / and systemd can run
these units in parallel. That means it was possible for another unit to
make / immutable before the unit could do the mkdir. I pointed this out
on the original PR[1] but we never followed up on it...
Now this here changes several things. First have one unit that does the
chattr -i / (immutable-root-off.service), it is hooked into
remote-fs-pre.target which means it is executed before the network
mounts (virtiofs) are done.
Then we have another unit that does chattr +i /
(immutable-root-on.service) which turn the immutable root back on after
remote-fs.target which means all mount are done at this point.
Additionally the automount unit is removed because it does not add any
value for us and it was borken anyway as it used the virtiofs tag as
path so systemd just ignored it.
[1] #20612 (comment)
Fixes #22569
Does this PR introduce a user-facing change?