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

VM: Avoid Invalid compat argument from virtiofsd #14744

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
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
49 changes: 29 additions & 20 deletions lxd/device/disk.go
Original file line number Diff line number Diff line change
Expand Up @@ -880,7 +880,8 @@ func (d *disk) startContainer() (*deviceConfig.RunConfig, error) {
}

// Mount the source in the instance devices directory.
revertFunc, sourceDevPath, isFile, err := d.createDevice(srcPath)
devPath := d.getDevicePath(d.name, d.config)
revertFunc, isFile, err := d.createDevice(srcPath, devPath)
if err != nil {
return nil, err
}
Expand All @@ -896,7 +897,7 @@ func (d *disk) startContainer() (*deviceConfig.RunConfig, error) {
// Instruct LXD to perform the mount.
runConf.Mounts = append(runConf.Mounts, deviceConfig.MountEntryItem{
DevName: d.name,
DevPath: sourceDevPath,
DevPath: devPath,
TargetPath: relativeDestPath,
FSType: "none",
Opts: options,
Expand Down Expand Up @@ -949,6 +950,13 @@ func (d *disk) detectVMPoolMountOpts() []string {
return opts
}

// virtiofsdSourceEncode encodes a path string to be used as part of a disk source on viriofsd.
// virtiofsd fails if the source property contains an equal sign.
// The encoding scheme replaces "-" with "--" and then "=" with "-".
func (d *disk) virtiofsdSourceEncode(text string) string {
return strings.Replace(strings.Replace(text, "-", "--", -1), "=", "-", -1)
}

// startVM starts the disk device for a virtual machine instance.
func (d *disk) startVM() (*deviceConfig.RunConfig, error) {
runConf := deviceConfig.RunConfig{}
Expand Down Expand Up @@ -1149,16 +1157,20 @@ func (d *disk) startVM() (*deviceConfig.RunConfig, error) {
return nil, fmt.Errorf(`Missing mount "path" setting`)
}

// Escaping to avoid invalid source errors.
devPath := d.virtiofsdSourceEncode(d.getDevicePath(d.name, d.config))

// Mount the source in the instance devices directory.
// This will ensure that if the exported directory configured as readonly that this
// takes effect event if using virtio-fs (which doesn't support read only mode) by
// having the underlying mount setup as readonly.
var revertFunc func()
revertFunc, mount.DevPath, _, err = d.createDevice(mount.DevPath)
revertFunc, _, err = d.createDevice(mount.DevPath, devPath)
if err != nil {
return nil, err
}

mount.DevPath = devPath
revert.Add(revertFunc)

mount.TargetPath = d.config["path"]
Expand Down Expand Up @@ -1630,13 +1642,10 @@ func (d *disk) mountPoolVolume() (func(), string, *storagePools.MountInfo, error
// createDevice creates a disk device mount on host.
// The srcPath argument is the source of the disk device on the host.
// Returns the created device path, and whether the path is a file or not.
func (d *disk) createDevice(srcPath string) (func(), string, bool, error) {
func (d *disk) createDevice(srcPath string, devPath string) (func(), bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

as an aside, the createDevice function returns a revert hook func() but to make this clearer please can you change the type of the return value to revert.Hook.

revert := revert.New()
defer revert.Fail()

// Paths.
devPath := d.getDevicePath(d.name, d.config)

isReadOnly := shared.IsTrue(d.config["readonly"])
isRecursive := shared.IsTrue(d.config["recursive"])

Expand All @@ -1656,7 +1665,7 @@ func (d *disk) createDevice(srcPath string) (func(), string, bool, error) {
// Get the mount options.
mntSrcPath, fsOptions, fsErr := diskCephfsOptions(clusterName, userName, mdsName, mdsPath)
if fsErr != nil {
return nil, "", false, fsErr
return nil, false, fsErr
}

// Join the options with any provided by the user.
Expand All @@ -1676,41 +1685,41 @@ func (d *disk) createDevice(srcPath string) (func(), string, bool, error) {
// Map the RBD.
rbdPath, err := diskCephRbdMap(clusterName, userName, poolName, volumeName)
if err != nil {
return nil, "", false, diskSourceNotFoundError{msg: "Failed mapping Ceph RBD volume", err: err}
return nil, false, diskSourceNotFoundError{msg: "Failed mapping Ceph RBD volume", err: err}
}

fsName, err = BlockFsDetect(rbdPath)
if err != nil {
return nil, "", false, fmt.Errorf("Failed detecting source path %q block device filesystem: %w", rbdPath, err)
return nil, false, fmt.Errorf("Failed detecting source path %q block device filesystem: %w", rbdPath, err)
}

// Record the device path.
err = d.volatileSet(map[string]string{"ceph_rbd": rbdPath})
if err != nil {
return nil, "", false, err
return nil, false, err
}

srcPath = rbdPath
isFile = false
} else {
fileInfo, err := os.Stat(srcPath)
if err != nil {
return nil, "", false, fmt.Errorf("Failed accessing source path %q: %w", srcPath, err)
return nil, false, fmt.Errorf("Failed accessing source path %q: %w", srcPath, err)
}

fileMode := fileInfo.Mode()
if shared.IsBlockdev(fileMode) {
fsName, err = BlockFsDetect(srcPath)
if err != nil {
return nil, "", false, fmt.Errorf("Failed detecting source path %q block device filesystem: %w", srcPath, err)
return nil, false, fmt.Errorf("Failed detecting source path %q block device filesystem: %w", srcPath, err)
}
} else if !fileMode.IsDir() {
isFile = true
}

f, err := d.localSourceOpen(srcPath)
if err != nil {
return nil, "", false, err
return nil, false, err
}

defer func() { _ = f.Close() }()
Expand All @@ -1723,30 +1732,30 @@ func (d *disk) createDevice(srcPath string) (func(), string, bool, error) {
if !shared.PathExists(d.inst.DevicesPath()) {
err := os.Mkdir(d.inst.DevicesPath(), 0711)
if err != nil {
return nil, "", false, err
return nil, false, err
}
}

// Clean any existing entry.
if shared.PathExists(devPath) {
err := os.Remove(devPath)
if err != nil {
return nil, "", false, err
return nil, false, err
}
}

// Create the mount point.
if isFile {
f, err := os.Create(devPath)
if err != nil {
return nil, "", false, err
return nil, false, err
}

_ = f.Close()
} else {
err := os.Mkdir(devPath, 0700)
if err != nil {
return nil, "", false, err
return nil, false, err
}
}

Expand All @@ -1757,14 +1766,14 @@ func (d *disk) createDevice(srcPath string) (func(), string, bool, error) {
// Mount the fs.
err := DiskMount(srcPath, devPath, isRecursive, d.config["propagation"], mntOptions, fsName)
if err != nil {
return nil, "", false, err
return nil, false, err
}

revert.Add(func() { _ = DiskMountClear(devPath) })

cleanup := revert.Clone().Fail // Clone before calling revert.Success() so we can return the Fail func.
revert.Success()
return cleanup, devPath, isFile, err
return cleanup, isFile, err
}

// localSourceOpen opens a local disk source path and returns a file handle to it.
Expand Down
6 changes: 3 additions & 3 deletions lxd/instance_put.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func instancePut(d *Daemon, r *http.Request) response.Response {

projectName := request.ProjectParam(r)

// Get the container
// Get the instance's name
name, err := url.PathUnescape(mux.Vars(r)["name"])
if err != nil {
return response.SmartError(err)
Expand All @@ -83,7 +83,7 @@ func instancePut(d *Daemon, r *http.Request) response.Response {
return response.BadRequest(fmt.Errorf("Invalid instance name"))
}

// Handle requests targeted to a container on a different node
// Handle requests targeted to an instance on a different node
resp, err := forwardedResponseIfInstanceIsRemote(s, r, projectName, name, instanceType)
if err != nil {
return response.SmartError(err)
Expand Down Expand Up @@ -164,7 +164,7 @@ func instancePut(d *Daemon, r *http.Request) response.Response {
return response.SmartError(err)
}

// Update container configuration
// Update instance configuration
do = func(op *operations.Operation) error {
defer unlock()

Expand Down
Loading