Support external volumes (block devices) in Ignite VMs #275
Conversation
twelho
added
kind/feature
cmd/ignite/cmd/cmdutil/volumeflag.go
Outdated
| switch len(paths) { | ||
| case 0: | ||
| // No paths given | ||
| continue |
There was a problem hiding this comment.
shouldn't this error? --volume= seem invalid
There was a problem hiding this comment.
I was thinking an input like /path:/path,,/path:/path, but maybe it should error in that case
cmd/ignite/cmd/cmdutil/volumeflag.go
Outdated
| case 2: // We were given | ||
| // Set the VM path and verify it's absolute | ||
| if vmPath = paths[1]; !path.IsAbs(vmPath) { | ||
| return absPathsErr |
There was a problem hiding this comment.
If you like, this can be done here too, but this validation should exist in pkg/apis/ignite/validation
There was a problem hiding this comment.
Will move/add it to validation
cmd/ignite/cmd/cmdutil/volumeflag.go
Outdated
| } | ||
|
|
||
| fallthrough | ||
| case 1: |
There was a problem hiding this comment.
why support --volume /my-block-device? I don't think docker does. I like being explicit in from where to where
There was a problem hiding this comment.
You suggested that we would support single paths, with the same path being applied for both the host and the VM. It's a bit illogical though as on the host that path points to a block device, but in the VM the path is a mounted directory. I'll remove that support.
cmd/ignite/cmd/cmdutil/volumeflag.go
Outdated
| } | ||
|
|
||
| // Verify that the host path points to a device file | ||
| if err := util.DeviceFile(hostPath); err != nil { |
cmd/ignite/cmd/cmdutil/volumeflag.go
Outdated
| } | ||
|
|
||
| // If the VM path is not set, use the host path | ||
| if len(vmPath) == 0 { |
|
|
||
| // Volume defines named storage volume | ||
| type Volume struct { | ||
| Name string `json:"name"` |
There was a problem hiding this comment.
add validation that this must be set, and must match something in volumemounts
There was a problem hiding this comment.
Done in the validation added in ca75db1.
| // VolumeMount defines the mount point for a named volume inside a VM | ||
| type VolumeMount struct { | ||
| Name string `json:"name"` | ||
| MountPath string `json:"mountPath"` |
There was a problem hiding this comment.
Add validation that this field must be set, and must be absolute
There was a problem hiding this comment.
Done in the validation added in ca75db1.
|
|
||
| // VolumeMount defines the mount point for a named volume inside a VM | ||
| type VolumeMount struct { | ||
| Name string `json:"name"` |
There was a problem hiding this comment.
add validation that this must be set, and must match something in volumes
There was a problem hiding this comment.
Done in the validation added in ca75db1.
pkg/container/firecracker.go
Outdated
|
|
||
| cfg.Drives = append(cfg.Drives, models.Drive{ | ||
| DriveID: firecracker.String(strconv.Itoa(i + 2)), | ||
| IsReadOnly: firecracker.Bool(false), |
There was a problem hiding this comment.
Add a TODO to support readonly volumes eventually?
| } | ||
|
|
||
| for _, entry := range entries { | ||
| if entry.isValid() { |
There was a problem hiding this comment.
at least put a warning if it's not valid? log.Errorf?
There was a problem hiding this comment.
It now returns an error in ca75db1, this should never happen thanks to validation.
|
I just rebased and ran autogen/tidy on this PR so that it's well in shape tomorrow when you continue on it. I think it would make sense to squash your commits in a logical order before removing the wip label |
|
The CI test appears to fail on |
Also add them to `v1alpha2` and add conversions for `v1alpha1`.
|
Commit history rebased to exclude CI/build/meta changes. |
This PR adds support for exposing host block devices inside Ignite VMs. It also sports internal automounting support via
/etc/fstab. The volumes are now declared in the API as.spec.storage.volumesand.spec.storage.volumeMountsk8s style, so this changesv1alpha2.Fixes #76.
cc @luxas