Skip to content

Commit

Permalink
Refactor StorageConfig
Browse files Browse the repository at this point in the history 
#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 #212 (comment) 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 <[email protected]>
  • Loading branch information
@lstocchi
lstocchi committed Nov 25, 2024
1 parent 4af22e9 commit 8df513f
Show file tree
Hide file tree
Showing 4 changed files with 103 additions and 71 deletions.
4 changes: 2 additions & 2 deletions pkg/config/json_test.go
View file
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
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
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
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

0 comments on commit 8df513f

Please sign in to comment.