From 8df513ff434237a3c2fd7a31271cf2850698d54e Mon Sep 17 00:00:00 2001 From: Luca Stocchi Date: Mon, 25 Nov 2024 15:26:35 +0100 Subject: [PATCH] Refactor StorageConfig https://github.com/crc-org/vfkit/pull/212 introduced a new property to the StorageConfig struct (URI) and, by doing so, now we have to check if we are dealing with a disk storage or a remote disk device by checking imagePath and Uri fields. An idea that came up in https://github.com/crc-org/vfkit/pull/212#discussion_r1854008958 was to refactor the StorageConfig struct to be extended by a DiskStorageConfig and a NetworkBlockStorageConfig. This PR refactors the code based on that suggestion. Signed-off-by: Luca Stocchi --- pkg/config/json_test.go | 4 +- pkg/config/virtio.go | 104 ++++++++++++++++++++++++++------------ pkg/config/virtio_test.go | 56 +++++++++----------- pkg/vf/virtio.go | 10 ++-- 4 files changed, 103 insertions(+), 71 deletions(-) diff --git a/pkg/config/json_test.go b/pkg/config/json_test.go index 3df9fcc..c074353 100644 --- a/pkg/config/json_test.go +++ b/pkg/config/json_test.go @@ -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}]}`, }, } @@ -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}`, }, } diff --git a/pkg/config/virtio.go b/pkg/config/virtio.go index 1278cad..4213de6 100644 --- a/pkg/config/virtio.go +++ b/pkg/config/virtio.go @@ -59,7 +59,7 @@ type VirtioVsock struct { // VirtioBlk configures a disk device. type VirtioBlk struct { - StorageConfig + DiskStorageConfig DeviceIdentifier string `json:"deviceIdentifier,omitempty"` } @@ -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. @@ -117,7 +117,8 @@ const ( ) type NetworkBlockDevice struct { - VirtioBlk + NetworkBlockStorageConfig + DeviceIdentifier string Timeout time.Duration SynchronizationMode NBDSynchronizationMode } @@ -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", + }, }, } } @@ -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: "", } @@ -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 } @@ -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 } @@ -707,7 +712,7 @@ 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 } @@ -715,6 +720,9 @@ func (nbd *NetworkBlockDevice) ToCmdLine() ([]string, error) { 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()) } @@ -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 { @@ -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", + }, }, } } @@ -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": diff --git a/pkg/config/virtio_test.go b/pkg/config/virtio_test.go index 5829c6e..afba0af 100644 --- a/pkg/config/virtio_test.go +++ b/pkg/config/virtio_test.go @@ -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: "", @@ -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", @@ -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", }, }, @@ -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", }, }, @@ -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"}, @@ -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) { diff --git a/pkg/vf/virtio.go b/pkg/vf/virtio.go index 500f8ee..5c50db8 100644 --- a/pkg/vf/virtio.go +++ b/pkg/vf/virtio.go @@ -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 @@ -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 @@ -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) } @@ -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 @@ -460,6 +460,6 @@ func (dev *USBMassStorage) AddToVirtualMachineConfig(vmConfig *VirtualMachineCon return nil } -type StorageConfig config.StorageConfig +type DiskStorageConfig config.DiskStorageConfig type USBMassStorage config.USBMassStorage