Skip to content
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

applehv: allow virtiofs to mount to / #20612

Merged
merged 1 commit into from
Nov 7, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 43 additions & 4 deletions pkg/machine/applehv/machine.go
Original file line number Diff line number Diff line change
Expand Up @@ -1084,23 +1084,31 @@ func (m *MacMachine) isIncompatible() bool {
}

func generateSystemDFilesForVirtiofsMounts(mounts []machine.VirtIoFs) []machine.Unit {
var unitFiles []machine.Unit
// mounting in fcos with virtiofs is a bit of a dance. we need a unit file for the mount, a unit file
// for automatic mounting on boot, and a "preparatory" service file that disables FCOS security, performs
// the mkdir of the mount point, and then re-enables security. This must be done for each mount.

var unitFiles []machine.Unit
for _, mnt := range mounts {
// Here we are looping the mounts and for each mount, we are adding two unit files
// for virtiofs. One unit file is the mount itself and the second is to automount it
// on boot.
autoMountUnit := `[Automount]
Where=%s
[Install]
WantedBy=multi-user.target

[Unit]
Description=Mount virtiofs volume %s
`
mountUnit := `[Mount]
What=%s
Where=%s
Type=virtiofs
[Install]
WantedBy=multi-user.target`

[Install]
WantedBy=multi-user.target
`
virtiofsAutomount := machine.Unit{
Enabled: machine.BoolToPtr(true),
Name: fmt.Sprintf("%s.automount", mnt.Tag),
Expand All @@ -1111,7 +1119,38 @@ WantedBy=multi-user.target`
Name: fmt.Sprintf("%s.mount", mnt.Tag),
Contents: machine.StrToPtr(fmt.Sprintf(mountUnit, mnt.Tag, mnt.Target)),
}
unitFiles = append(unitFiles, virtiofsAutomount, virtiofsMount)

// This "unit" simulates something like systemctl enable virtiofs-mount-prepare@
enablePrep := machine.Unit{
Enabled: machine.BoolToPtr(true),
Name: fmt.Sprintf("virtiofs-mount-prepare@%s.service", mnt.Tag),
}

unitFiles = append(unitFiles, virtiofsAutomount, virtiofsMount, enablePrep)
}

// mount prep is a way to workaround the FCOS limitation of creating directories
// at the rootfs / and then mounting to them.
mountPrep := `
[Unit]
Description=Allow virtios to mount to /
DefaultDependencies=no
ConditionPathExists=!%f

[Service]
Type=oneshot
ExecStartPre=chattr -i /
ExecStart=mkdir -p '%f'
ExecStopPost=chattr +i /
Comment on lines +1142 to +1144
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Luap99 is right it's racy


[Install]
WantedBy=remote-fs.target
`
virtioFSChattr := machine.Unit{
Contents: machine.StrToPtr(mountPrep),
Name: "[email protected]",
}
unitFiles = append(unitFiles, virtioFSChattr)

return unitFiles
}