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

apple virtiofs: fix racy mount setup #23118

Merged
merged 2 commits into from
Jun 27, 2024
Merged
Show file tree
Hide file tree
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
2 changes: 1 addition & 1 deletion .cirrus.yml
Original file line number Diff line number Diff line change
Expand Up @@ -796,7 +796,7 @@ podman_machine_mac_task:
clone_script: # artifacts from osx_alt_build_task
- mkdir -p $CIRRUS_WORKING_DIR
- cd $CIRRUS_WORKING_DIR
- $ARTCURL/OSX%20Cross/repo/repo.tbz
- $ARTCURL/Build%20for%20MacOS%20amd64%2Barm64/repo/repo.tbz
- tar xjf repo.tbz
# This host is/was shared with potentially many other CI tasks.
# The previous task may have been canceled or aborted.
Expand Down
79 changes: 38 additions & 41 deletions pkg/machine/apple/apple.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,18 +72,7 @@ func GenerateSystemDFilesForVirtiofsMounts(mounts []machine.VirtIoFs) ([]ignitio

unitFiles := make([]ignition.Unit, 0, len(mounts))
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 := parser.NewUnitFile()
autoMountUnit.Add("Automount", "Where", "%s")
autoMountUnit.Add("Install", "WantedBy", "multi-user.target")
autoMountUnit.Add("Unit", "Description", "Mount virtiofs volume %s")
autoMountUnitFile, err := autoMountUnit.ToString()
if err != nil {
return nil, err
}

// Create mount unit for each mount
mountUnit := parser.NewUnitFile()
mountUnit.Add("Mount", "What", "%s")
mountUnit.Add("Mount", "Where", "%s")
Expand All @@ -95,49 +84,57 @@ func GenerateSystemDFilesForVirtiofsMounts(mounts []machine.VirtIoFs) ([]ignitio
return nil, err
}

virtiofsAutomount := ignition.Unit{
Enabled: ignition.BoolToPtr(true),
Name: fmt.Sprintf("%s.automount", parser.PathEscape(mnt.Target)),
Contents: ignition.StrToPtr(fmt.Sprintf(autoMountUnitFile, mnt.Tag, mnt.Target)),
}
virtiofsMount := ignition.Unit{
Enabled: ignition.BoolToPtr(true),
Name: fmt.Sprintf("%s.mount", parser.PathEscape(mnt.Target)),
Contents: ignition.StrToPtr(fmt.Sprintf(mountUnitFile, mnt.Tag, mnt.Target)),
}

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

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

// mount prep is a way to workaround the FCOS limitation of creating directories
// This is a way to workaround the FCOS limitation of creating directories
// at the rootfs / and then mounting to them.
mountPrep := parser.NewUnitFile()
mountPrep.Add("Unit", "Description", "Allow virtios to mount to /")
mountPrep.Add("Unit", "DefaultDependencies", "no")
mountPrep.Add("Unit", "ConditionPathExists", "!%f")

mountPrep.Add("Service", "Type", "oneshot")
mountPrep.Add("Service", "ExecStartPre", "chattr -i /")
mountPrep.Add("Service", "ExecStart", "mkdir -p '%f'")
mountPrep.Add("Service", "ExecStopPost", "chattr +i /")

mountPrep.Add("Install", "WantedBy", "remote-fs.target")
mountPrepFile, err := mountPrep.ToString()
immutableRootOff := parser.NewUnitFile()
immutableRootOff.Add("Unit", "Description", "Allow systemd to create mount points on /")
immutableRootOff.Add("Unit", "DefaultDependencies", "no")

immutableRootOff.Add("Service", "Type", "oneshot")
immutableRootOff.Add("Service", "ExecStart", "chattr -i /")

immutableRootOff.Add("Install", "WantedBy", "remote-fs-pre.target")
immutableRootOffFile, err := immutableRootOff.ToString()
if err != nil {
return nil, err
}

immutableRootOffUnit := ignition.Unit{
Contents: ignition.StrToPtr(immutableRootOffFile),
Name: "immutable-root-off.service",
Enabled: ignition.BoolToPtr(true),
}
unitFiles = append(unitFiles, immutableRootOffUnit)

immutableRootOn := parser.NewUnitFile()
immutableRootOn.Add("Unit", "Description", "Set / back to immutable after mounts are done")
immutableRootOn.Add("Unit", "DefaultDependencies", "no")
immutableRootOn.Add("Unit", "After", "remote-fs.target")

immutableRootOn.Add("Service", "Type", "oneshot")
immutableRootOn.Add("Service", "ExecStart", "chattr +i /")

immutableRootOn.Add("Install", "WantedBy", "remote-fs.target")
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member Author

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.

immutableRootOnFile, err := immutableRootOn.ToString()
if err != nil {
return nil, err
}

virtioFSChattr := ignition.Unit{
Contents: ignition.StrToPtr(mountPrepFile),
Name: "[email protected]",
immutableRootOnUnit := ignition.Unit{
Contents: ignition.StrToPtr(immutableRootOnFile),
Name: "immutable-root-on.service",
Enabled: ignition.BoolToPtr(true),
}
unitFiles = append(unitFiles, virtioFSChattr)
unitFiles = append(unitFiles, immutableRootOnUnit)

return unitFiles, nil
}
Expand Down