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.

Sign up for GitHub

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

Refactor StorageConfig so it does not have both imagePath and Uri properties #227

Merged
merged 1 commit into from
Jan 9, 2025
+103 −71
Merged

Refactor StorageConfig so it does not have both imagePath and Uri properties #227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions pkg/config/json_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ var jsonTests = map[string]jsonTest{

return vm
},
expectedJSON: `{"vcpus":3,"memoryBytes":4194304000,"bootloader":{"kind":"linuxBootloader","vmlinuzPath":"/vmlinuz","initrdPath":"/initrd","kernelCmdLine":"console=hvc0"},"devices":[{"kind":"virtioserial","logFile":"/virtioserial"},{"kind":"virtioinput","inputType":"keyboard"},{"kind":"virtiogpu","usesGUI":false,"width":800,"height":600},{"kind":"virtionet","nat":true,"macAddress":"00:11:22:33:44:55"},{"kind":"virtiorng"},{"kind":"virtioblk","devName":"virtio-blk","imagePath":"/virtioblk"},{"kind":"virtiosock","port":1234,"socketURL":"/virtiovsock"},{"kind":"virtiofs","mountTag":"tag","sharedDir":"/virtiofs"},{"kind":"usbmassstorage","devName":"usb-mass-storage","imagePath":"/usbmassstorage","readOnly":true},{"kind":"rosetta","mountTag":"vz-rosetta","installRosetta":false,"ignoreIfMissing":false},{"kind":"nbd", "devName":"nbd", "uri":"uri", "SynchronizationMode":"full","Timeout":1000000}]}`,
expectedJSON: `{"vcpus":3,"memoryBytes":4194304000,"bootloader":{"kind":"linuxBootloader","vmlinuzPath":"/vmlinuz","initrdPath":"/initrd","kernelCmdLine":"console=hvc0"},"devices":[{"kind":"virtioserial","logFile":"/virtioserial"},{"kind":"virtioinput","inputType":"keyboard"},{"kind":"virtiogpu","usesGUI":false,"width":800,"height":600},{"kind":"virtionet","nat":true,"macAddress":"00:11:22:33:44:55"},{"kind":"virtiorng"},{"kind":"virtioblk","devName":"virtio-blk","imagePath":"/virtioblk"},{"kind":"virtiosock","port":1234,"socketURL":"/virtiovsock"},{"kind":"virtiofs","mountTag":"tag","sharedDir":"/virtiofs"},{"kind":"usbmassstorage","devName":"usb-mass-storage","imagePath":"/usbmassstorage","readOnly":true},{"kind":"rosetta","mountTag":"vz-rosetta","installRosetta":false,"ignoreIfMissing":false},{"kind":"nbd", "devName":"nbd", "uri":"uri", "DeviceIdentifier":"", "SynchronizationMode":"full","Timeout":1000000}]}`,
},
}

Expand Down Expand Up @@ -317,7 +317,7 @@ var jsonStabilityTests = map[string]jsonStabilityTest{
return nbd
},
skipFields: []string{"DevName", "ImagePath"},
expectedJSON: `{"kind":"nbd","deviceIdentifier":"DeviceIdentifier","devName":"nbd","uri":"URI","readOnly":true,"SynchronizationMode":"SynchronizationMode","Timeout":2}`,
expectedJSON: `{"kind":"nbd","DeviceIdentifier":"DeviceIdentifier","devName":"nbd","uri":"URI","readOnly":true,"SynchronizationMode":"SynchronizationMode","Timeout":2}`,
},
}

Expand Down
104 changes: 72 additions & 32 deletions pkg/config/virtio.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ type VirtioVsock struct {

// VirtioBlk configures a disk device.
type VirtioBlk struct {
StorageConfig
DiskStorageConfig
DeviceIdentifier string `json:"deviceIdentifier,omitempty"`
}

Expand All @@ -82,7 +82,7 @@ type RosettaShare struct {

// NVMExpressController configures a NVMe controller in the guest
type NVMExpressController struct {
StorageConfig
DiskStorageConfig
}

// VirtioRng configures a random number generator (RNG) device.
Expand Down Expand Up @@ -117,7 +117,8 @@ const (
)

type NetworkBlockDevice struct {
VirtioBlk
NetworkBlockStorageConfig
DeviceIdentifier string
Timeout time.Duration
SynchronizationMode NBDSynchronizationMode
}
Expand Down Expand Up @@ -479,8 +480,10 @@ func (dev *VirtioRng) FromOptions(options []option) error {

func nvmExpressControllerNewEmpty() *NVMExpressController {
return &NVMExpressController{
StorageConfig: StorageConfig{
DevName: "nvme",
DiskStorageConfig: DiskStorageConfig{
StorageConfig: StorageConfig{
DevName: "nvme",
},
},
}
}
Expand All @@ -496,8 +499,10 @@ func NVMExpressControllerNew(imagePath string) (*NVMExpressController, error) {

func virtioBlkNewEmpty() *VirtioBlk {
return &VirtioBlk{
StorageConfig: StorageConfig{
DevName: "virtio-blk",
DiskStorageConfig: DiskStorageConfig{
StorageConfig: StorageConfig{
DevName: "virtio-blk",
},
},
DeviceIdentifier: "",
}
Expand Down Expand Up @@ -527,11 +532,11 @@ func (dev *VirtioBlk) FromOptions(options []option) error {
}
}

return dev.StorageConfig.FromOptions(unhandledOpts)
return dev.DiskStorageConfig.FromOptions(unhandledOpts)
}

func (dev *VirtioBlk) ToCmdLine() ([]string, error) {
cmdLine, err := dev.StorageConfig.ToCmdLine()
cmdLine, err := dev.DiskStorageConfig.ToCmdLine()
if err != nil {
return []string{}, err
}
Expand Down Expand Up @@ -681,12 +686,12 @@ func (dev *RosettaShare) FromOptions(options []option) error {

func networkBlockDeviceNewEmpty() *NetworkBlockDevice {
return &NetworkBlockDevice{
VirtioBlk: VirtioBlk{
NetworkBlockStorageConfig: NetworkBlockStorageConfig{
StorageConfig: StorageConfig{
DevName: "nbd",
},
DeviceIdentifier: "",
},
DeviceIdentifier: "",
Timeout: time.Duration(15000 * time.Millisecond), // set a default timeout to 15s
SynchronizationMode: SynchronizationFullMode, // default mode to full
}
Expand All @@ -707,14 +712,17 @@ func NetworkBlockDeviceNew(uri string, timeout uint32, synchronization NBDSynchr
}

func (nbd *NetworkBlockDevice) ToCmdLine() ([]string, error) {
cmdLine, err := nbd.VirtioBlk.ToCmdLine()
cmdLine, err := nbd.NetworkBlockStorageConfig.ToCmdLine()
if err != nil {
return []string{}, err
}
if len(cmdLine) != 2 {
return []string{}, fmt.Errorf("unexpected storage config commandline")
}

if nbd.DeviceIdentifier != "" {
cmdLine[1] = fmt.Sprintf("%s,deviceId=%s", cmdLine[1], nbd.DeviceIdentifier)
}
if nbd.Timeout.Milliseconds() > 0 {
cmdLine[1] = fmt.Sprintf("%s,timeout=%d", cmdLine[1], nbd.Timeout.Milliseconds())
}
Expand All @@ -729,6 +737,8 @@ func (nbd *NetworkBlockDevice) FromOptions(options []option) error {
unhandledOpts := []option{}
for _, option := range options {
switch option.key {
case "deviceId":
nbd.DeviceIdentifier = option.value
case "timeout":
timeoutMS, err := strconv.ParseInt(option.value, 10, 32)
if err != nil {
Expand All @@ -749,17 +759,19 @@ func (nbd *NetworkBlockDevice) FromOptions(options []option) error {
}
}

return nbd.VirtioBlk.FromOptions(unhandledOpts)
return nbd.NetworkBlockStorageConfig.FromOptions(unhandledOpts)
}

type USBMassStorage struct {
StorageConfig
DiskStorageConfig
}

func usbMassStorageNewEmpty() *USBMassStorage {
return &USBMassStorage{
StorageConfig{
DevName: "usb-mass-storage",
DiskStorageConfig: DiskStorageConfig{
StorageConfig: StorageConfig{
DevName: "usb-mass-storage",
},
},
}
}
Expand All @@ -779,38 +791,66 @@ func (dev *USBMassStorage) SetReadOnly(readOnly bool) {

// StorageConfig configures a disk device.
type StorageConfig struct {
DevName string `json:"devName"`
DevName string `json:"devName"`
ReadOnly bool `json:"readOnly,omitempty"`
}

type DiskStorageConfig struct {
StorageConfig
ImagePath string `json:"imagePath,omitempty"`
URI string `json:"uri,omitempty"`
ReadOnly bool `json:"readOnly,omitempty"`
}

func (config *StorageConfig) ToCmdLine() ([]string, error) {
if config.ImagePath != "" && config.URI != "" {
return nil, fmt.Errorf("%s devices cannot have both path to a disk image and a uri to a remote block device", config.DevName)
type NetworkBlockStorageConfig struct {
StorageConfig
URI string `json:"uri,omitempty"`
}

func (config *DiskStorageConfig) ToCmdLine() ([]string, error) {
if config.ImagePath == "" {
return nil, fmt.Errorf("%s devices need the path to a disk image", config.DevName)
}
if config.ImagePath == "" && config.URI == "" {
return nil, fmt.Errorf("%s devices need a path to a disk image or a uri to a remote block device", config.DevName)

value := fmt.Sprintf("%s,path=%s", config.DevName, config.ImagePath)

if config.ReadOnly {
value += ",readonly"
}
var value string
if config.ImagePath != "" {
value = fmt.Sprintf("%s,path=%s", config.DevName, config.ImagePath)
return []string{"--device", value}, nil
}

func (config *DiskStorageConfig) FromOptions(options []option) error {
for _, option := range options {
switch option.key {
case "path":
config.ImagePath = option.value
case "readonly":
if option.value != "" {
return fmt.Errorf("unexpected value for virtio-blk 'readonly' option: %s", option.value)
}
config.ReadOnly = true
default:
return fmt.Errorf("unknown option for %s devices: %s", config.DevName, option.key)
}
}
if config.URI != "" {
value = fmt.Sprintf("%s,uri=%s", config.DevName, config.URI)
return nil
}

func (config *NetworkBlockStorageConfig) ToCmdLine() ([]string, error) {
if config.URI == "" {
return nil, fmt.Errorf("%s devices need the uri to a remote block device", config.DevName)
}

value := fmt.Sprintf("%s,uri=%s", config.DevName, config.URI)

if config.ReadOnly {
value += ",readonly"
}
return []string{"--device", value}, nil
}

func (config *StorageConfig) FromOptions(options []option) error {
func (config *NetworkBlockStorageConfig) FromOptions(options []option) error {
for _, option := range options {
switch option.key {
case "path":
config.ImagePath = option.value
case "uri":
config.URI = option.value
case "readonly":
Expand Down
56 changes: 24 additions & 32 deletions pkg/config/virtio_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@ var virtioDevTests = map[string]virtioDevTest{
"NewVirtioBlk": {
newDev: func() (VirtioDevice, error) { return VirtioBlkNew("/foo/bar") },
expectedDev: &VirtioBlk{
StorageConfig: StorageConfig{
DevName: "virtio-blk",
DiskStorageConfig: DiskStorageConfig{
StorageConfig: StorageConfig{
DevName: "virtio-blk",
},
ImagePath: "/foo/bar",
},
DeviceIdentifier: "",
Expand All @@ -38,8 +40,10 @@ var virtioDevTests = map[string]virtioDevTest{
return dev, nil
},
expectedDev: &VirtioBlk{
StorageConfig: StorageConfig{
DevName: "virtio-blk",
DiskStorageConfig: DiskStorageConfig{
StorageConfig: StorageConfig{
DevName: "virtio-blk",
},
ImagePath: "/foo/bar",
},
DeviceIdentifier: "test",
Expand All @@ -50,8 +54,10 @@ var virtioDevTests = map[string]virtioDevTest{
"NewNVMe": {
newDev: func() (VirtioDevice, error) { return NVMExpressControllerNew("/foo/bar") },
expectedDev: &NVMExpressController{
StorageConfig: StorageConfig{
DevName: "nvme",
DiskStorageConfig: DiskStorageConfig{
StorageConfig: StorageConfig{
DevName: "nvme",
},
ImagePath: "/foo/bar",
},
},
Expand Down Expand Up @@ -156,8 +162,10 @@ var virtioDevTests = map[string]virtioDevTest{
"NewUSBMassStorage": {
newDev: func() (VirtioDevice, error) { return USBMassStorageNew("/foo/bar") },
expectedDev: &USBMassStorage{
StorageConfig: StorageConfig{
DevName: "usb-mass-storage",
DiskStorageConfig: DiskStorageConfig{
StorageConfig: StorageConfig{
DevName: "usb-mass-storage",
},
ImagePath: "/foo/bar",
},
},
Expand All @@ -173,10 +181,12 @@ var virtioDevTests = map[string]virtioDevTest{
return dev, err
},
expectedDev: &USBMassStorage{
StorageConfig: StorageConfig{
DevName: "usb-mass-storage",
DiskStorageConfig: DiskStorageConfig{
StorageConfig: StorageConfig{
DevName: "usb-mass-storage",
ReadOnly: true,
},
ImagePath: "/foo/bar",
ReadOnly: true,
},
},
expectedCmdLine: []string{"--device", "usb-mass-storage,path=/foo/bar,readonly"},
Expand Down Expand Up @@ -223,36 +233,18 @@ var virtioDevTests = map[string]virtioDevTest{
return NetworkBlockDeviceNew("nbd://1.1.1.1:10000", 1000, SynchronizationNoneMode)
},
expectedDev: &NetworkBlockDevice{
VirtioBlk: VirtioBlk{
NetworkBlockStorageConfig: NetworkBlockStorageConfig{
StorageConfig: StorageConfig{
DevName: "nbd",
URI: "nbd://1.1.1.1:10000",
},
DeviceIdentifier: "",
URI: "nbd://1.1.1.1:10000",
},
DeviceIdentifier: "",
Timeout: time.Duration(1000 * time.Millisecond),
SynchronizationMode: SynchronizationNoneMode,
},
expectedCmdLine: []string{"--device", "nbd,uri=nbd://1.1.1.1:10000,timeout=1000,sync=none"},
},
"StorageConfigErrorImageUri": {
newDev: func() (VirtioDevice, error) {
return &StorageConfig{
DevName: "dev",
ImagePath: "path",
URI: "uri",
}, nil
},
errorMsg: "dev devices cannot have both path to a disk image and a uri to a remote block device",
},
"StorageConfigErrorNoImageOrUri": {
newDev: func() (VirtioDevice, error) {
return &StorageConfig{
DevName: "dev",
}, nil
},
errorMsg: "dev devices need a path to a disk image or a uri to a remote block device",
},
}

func testVirtioDev(t *testing.T, test *virtioDevTest) {
Expand Down
10 changes: 5 additions & 5 deletions pkg/vf/virtio.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ type vzNetworkBlockDevice struct {
}

func (dev *NVMExpressController) toVz() (vz.StorageDeviceConfiguration, error) {
var storageConfig StorageConfig = StorageConfig(dev.StorageConfig)
var storageConfig DiskStorageConfig = DiskStorageConfig(dev.DiskStorageConfig)
attachment, err := storageConfig.toVz()
if err != nil {
return nil, err
Expand All @@ -60,7 +60,7 @@ func (dev *NVMExpressController) AddToVirtualMachineConfig(vmConfig *VirtualMach
}

func (dev *VirtioBlk) toVz() (vz.StorageDeviceConfiguration, error) {
var storageConfig StorageConfig = StorageConfig(dev.StorageConfig)
var storageConfig DiskStorageConfig = DiskStorageConfig(dev.DiskStorageConfig)
attachment, err := storageConfig.toVz()
if err != nil {
return nil, err
Expand Down Expand Up @@ -431,7 +431,7 @@ func AddToVirtualMachineConfig(vmConfig *VirtualMachineConfiguration, dev config
}
}

func (config *StorageConfig) toVz() (vz.StorageDeviceAttachment, error) {
func (config *DiskStorageConfig) toVz() (vz.StorageDeviceAttachment, error) {
if config.ImagePath == "" {
return nil, fmt.Errorf("missing mandatory 'path' option for %s device", config.DevName)
}
Expand All @@ -441,7 +441,7 @@ func (config *StorageConfig) toVz() (vz.StorageDeviceAttachment, error) {
}

func (dev *USBMassStorage) toVz() (vz.StorageDeviceConfiguration, error) {
var storageConfig StorageConfig = StorageConfig(dev.StorageConfig)
var storageConfig DiskStorageConfig = DiskStorageConfig(dev.DiskStorageConfig)
attachment, err := storageConfig.toVz()
if err != nil {
return nil, err
Expand All @@ -460,6 +460,6 @@ func (dev *USBMassStorage) AddToVirtualMachineConfig(vmConfig *VirtualMachineCon
return nil
}

type StorageConfig config.StorageConfig
type DiskStorageConfig config.DiskStorageConfig

type USBMassStorage config.USBMassStorage