-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
machine/linux: Switch to virtiofs by default #22920
Merged
openshift-merge-bot
merged 3 commits into
containers:main
from
cgwalters:virtiofsd-machine
Jun 24, 2024
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -87,7 +87,7 @@ func GenerateSystemDFilesForVirtiofsMounts(mounts []machine.VirtIoFs) ([]ignitio | |
mountUnit.Add("Mount", "What", "%s") | ||
mountUnit.Add("Mount", "Where", "%s") | ||
mountUnit.Add("Mount", "Type", "virtiofs") | ||
mountUnit.Add("Mount", "Options", "context=\"system_u:object_r:nfs_t:s0\"") | ||
mountUnit.Add("Mount", "Options", fmt.Sprintf("context=\"%s\"", machine.NFSSELinuxContext)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit use %q There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, will do; just waiting to see if anyone else has other comments before doing another update and round of CI. |
||
mountUnit.Add("Install", "WantedBy", "multi-user.target") | ||
mountUnitFile, err := mountUnit.ToString() | ||
if err != nil { | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
//go:build linux || freebsd | ||
|
||
package qemu | ||
|
||
import ( | ||
"fmt" | ||
"os" | ||
"os/exec" | ||
"time" | ||
|
||
"github.com/containers/common/pkg/config" | ||
"github.com/containers/podman/v5/pkg/machine/define" | ||
"github.com/containers/podman/v5/pkg/machine/vmconfigs" | ||
"github.com/containers/storage/pkg/fileutils" | ||
"github.com/sirupsen/logrus" | ||
) | ||
|
||
// VirtiofsdSpawner spawns an instance of virtiofsd | ||
type virtiofsdSpawner struct { | ||
runtimeDir *define.VMFile | ||
binaryPath string | ||
mountCount uint | ||
} | ||
|
||
func newVirtiofsdSpawner(runtimeDir *define.VMFile) (*virtiofsdSpawner, error) { | ||
var binaryPath string | ||
cfg, err := config.Default() | ||
if err != nil { | ||
return nil, err | ||
} | ||
binaryPath, err = cfg.FindHelperBinary("virtiofsd", true) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to find virtiofsd: %w", err) | ||
} | ||
return &virtiofsdSpawner{binaryPath: binaryPath, runtimeDir: runtimeDir}, nil | ||
} | ||
|
||
// createVirtiofsCmd returns a new command instance configured to launch virtiofsd. | ||
func (v *virtiofsdSpawner) createVirtiofsCmd(directory, socketPath string) *exec.Cmd { | ||
args := []string{"--sandbox", "none", "--socket-path", socketPath, "--shared-dir", "."} | ||
// We don't need seccomp filtering; we trust our workloads. This incidentally | ||
// works around issues like https://gitlab.com/virtio-fs/virtiofsd/-/merge_requests/200. | ||
args = append(args, "--seccomp=none") | ||
cmd := exec.Command(v.binaryPath, args...) | ||
// This sets things up so that the `.` we passed in the arguments is the target directory | ||
cmd.Dir = directory | ||
// Quiet the daemon by default | ||
cmd.Env = append(cmd.Environ(), "RUST_LOG=ERROR") | ||
cmd.Stdin = nil | ||
cmd.Stdout = nil | ||
if logrus.IsLevelEnabled(logrus.DebugLevel) { | ||
cmd.Stderr = os.Stderr | ||
} | ||
return cmd | ||
} | ||
|
||
type virtiofsdHelperCmd struct { | ||
command exec.Cmd | ||
socket *define.VMFile | ||
} | ||
|
||
// spawnForMount returns on success a combination of qemu commandline and child process for virtiofsd | ||
func (v *virtiofsdSpawner) spawnForMount(hostmnt *vmconfigs.Mount) ([]string, *virtiofsdHelperCmd, error) { | ||
logrus.Debugf("Initializing virtiofsd mount for %s", hostmnt.Source) | ||
// By far the most common failure to spawn virtiofsd will be a typo'd source directory, | ||
// so let's synchronously check that ourselves here. | ||
if err := fileutils.Exists(hostmnt.Source); err != nil { | ||
return nil, nil, fmt.Errorf("failed to access virtiofs source directory %s", hostmnt.Source) | ||
} | ||
virtiofsChar := fmt.Sprintf("virtiofschar%d", v.mountCount) | ||
virtiofsCharPath, err := v.runtimeDir.AppendToNewVMFile(virtiofsChar, nil) | ||
if err != nil { | ||
return nil, nil, err | ||
} | ||
|
||
qemuCommand := []string{} | ||
|
||
qemuCommand = append(qemuCommand, "-chardev", fmt.Sprintf("socket,id=%s,path=%s", virtiofsChar, virtiofsCharPath.Path)) | ||
qemuCommand = append(qemuCommand, "-device", fmt.Sprintf("vhost-user-fs-pci,queue-size=1024,chardev=%s,tag=%s", virtiofsChar, hostmnt.Tag)) | ||
// TODO: Honor hostmnt.readonly somehow here (add an option to virtiofsd) | ||
virtiofsdCmd := v.createVirtiofsCmd(hostmnt.Source, virtiofsCharPath.Path) | ||
if err := virtiofsdCmd.Start(); err != nil { | ||
return nil, nil, fmt.Errorf("failed to start virtiofsd") | ||
cgwalters marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
// Wait for the socket | ||
for { | ||
if err := fileutils.Exists(virtiofsCharPath.Path); err == nil { | ||
break | ||
} | ||
logrus.Debugf("waiting for virtiofsd socket %q", virtiofsCharPath.Path) | ||
time.Sleep(time.Millisecond * 100) | ||
} | ||
// Increment our count of mounts which are used to create unique names for the devices | ||
v.mountCount += 1 | ||
return qemuCommand, &virtiofsdHelperCmd{ | ||
command: *virtiofsdCmd, | ||
socket: virtiofsCharPath, | ||
}, nil | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I was thinking about this a bit more, and one thing that will probably show up is static package analyzers will warn about the broken symlink (presumably we won't take a hard dep on virtiofsd).
I don't know how annoying it will be though.
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.
Yeah I do wonder if we should add a new podman-machine package that just contains the necessary deps as requires and could add the symlink there. That way users don't get broken every time we add a new dep and they can just have the one mostly empty package just to get further packages.
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.
Yeah, makes sense.