containers / podman

Podman: A tool for managing OCI containers and pods.
https://podman.io
Apache License 2.0
22.36k stars 2.31k forks source link

apple virtiofs: fix racy mount setup #23118

Closed Luap99 closed 3 days ago

Luap99 commented 3 days ago

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] https://github.com/containers/podman/pull/20612#discussion_r1384846241

Fixes https://github.com/containers/podman/issues/22569

Does this PR introduce a user-facing change?

Fixed a race condition which could prevent the mount of some podman machine volumes inside the VM.
openshift-ci[bot] commented 3 days ago

[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: - ~~[OWNERS](https://github.com/containers/podman/blob/main/OWNERS)~~ [Luap99] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
packit-as-a-service[bot] commented 3 days ago

Ephemeral COPR build failed. @containers/packit-build please check.

Luap99 commented 3 days ago

@ashley-cui @baude PTAL

Luap99 commented 3 days ago

FYI This was my test commit https://github.com/containers/podman/pull/23118/commits/1b4ffadfc2aba9bfa4a6733e49fdd1305dc7fad8 and the logs for it https://api.cirrus-ci.com/v1/artifact/task/4925116956540928/html/machine-mac-podman-darwin-rootless-host-sqlite.log.html#t--run-basic-podman-commands-Volume-ops--1

ashley-cui commented 3 days ago

Not an expert here but LGTM

rhatdan commented 3 days ago

/lgtm

cfergeau commented 2 days ago

I believe this comment https://github.com/containers/podman/blob/c86386ed06c209c79c6826f9cf72b89d5d69dd10/pkg/machine/apple/apple.go#L69-L71 is out of date after this PR.