applehv: allow virtiofs to mount to / #20612
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude 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 |
openshift-ci
bot
added
the
approved
|
@rhatdan ptal |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
FCOS has a security limitation where new directories cannot be added to the root / directory of its filesystem. This PR uses the work-around discussed in coreos/rpm-ostree#337 (comment) to temporarily disable the limitation, perform the mkdir, and then re-enable the limitation. This PR allows mounts on the applehv to actually work. [NO NEW TESTS NEEDED] Signed-off-by: Brent Baude <[email protected]>
baude
force-pushed
the
applehvvirtiosfsmounts
branch
from
c53693a
to
d44f71c
Compare
|
LGTM |
|
Cockpit tests failed for commit c53693a55e4f1f46f498185df2b9b705713121a4. @martinpitt, @jelly, @mvollmer please check. |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
LGTM |
|
Cockpit tests failed for commit d44f71c. @martinpitt, @jelly, @mvollmer please check. |
|
/lgtm |
openshift-merge-bot
bot
merged commit f47a85f
into
containers:main
| ExecStartPre=chattr -i / | ||
| ExecStart=mkdir -p '%f' | ||
| ExecStopPost=chattr +i / |
There was a problem hiding this comment.
Isn't this racy? While doing this with one unit is fine you do it in x units at the same time because systemd will start the units at the same time and systemd start in parallel by default unless explicit dependencies are configured.
Therefore this order is not safe at all. We could end up with something like this:
| unit 1 | unit 2 |
|---|---|
| chattr -i / | chattr -i / |
| mkdir | |
| chattr +i / | |
| mkdir <-- this will fail | |
| chattr +i / |
I think it would be much safer to do it in one unit with one chattr -i / then mkdir all you volumes and then one chattr +i / at the end, this would remove all races.
There was a problem hiding this comment.
my understanding is that multiple units do not start fire at the same time in this case. Do you know for certain otherwise? And if so, I'd love to hear a better approach. In this case, I am stuck between what needs to be done and FCOS. If we cannot solve it and FCOS refuses to change, then we will need to drop FCOS for all podman machine operating systems.
@dustymabe do you have anything to add here about whether a race may occur?
There was a problem hiding this comment.
I pretty sure systemd start the units in parallel if it can (i.e no After= or similar), of course these commands here all are super quick so I think it would be very hard to actually encounter this in the wild.
As far as fixing I think you can just write the volume paths directly into the unit mkdir $vol1 $vol2 $vol3 instead of using the template.
There was a problem hiding this comment.
I agree this may expose a race condition as systemd could run all the units in parallel. If this did expose a race condition and we knew the paths up front then just doing it in a single unit would certainly prevent the race.
There was a problem hiding this comment.
@Luap99 is right it's racy
github-actions
bot
added
the
locked - please file new issue/PR
FCOS has a security limitation where new directories cannot be added to the root / directory of its filesystem. This PR uses the work-around discussed in coreos/rpm-ostree#337 (comment) to temporarily disable the limitation, perform the mkdir, and then re-enable the limitation.
This PR allows mounts on the applehv to actually work.
Does this PR introduce a user-facing change?