feat: add support for NBD #212
Conversation
|
Skipping CI for Draft Pull Request. |
|
Added initial tests and documentation. I plan to also add an e2e test. |
After looking again, I'll skip the e2e test bc I need Mac 14 to work and the current workflow skip it, so maybe we can work on it in a separate PR if we want to do it. For the rest it should be ready for a review. We just need to merge #214 and rebase it. |
pkg/vf/virtio.go
Outdated
| @@ -289,6 +290,73 @@ func (dev *VirtioVsock) AddToVirtualMachineConfig(vmConfig *VirtualMachineConfig | |||
| return nil | |||
| } | |||
|
|
|||
| func (dev *NetworkBlockDevice) toVz() (vz.StorageDeviceConfiguration, error) { | |||
| if dev.Uri == "" || dev.DeviceIdentifier == "" { | |||
| return nil, fmt.Errorf("missing required options for NBD device: 'uri' and 'deviceId' must be specified") | |||
There was a problem hiding this comment.
here you check for empty options, but do you ever check to see if they are valid? If you do and you confident in that, maybe move this to that section?
pkg/vf/virtio.go
Outdated
| } | ||
|
|
||
| func (dev *NetworkBlockDevice) SyncronizationModeVZ() vz.DiskSynchronizationMode { | ||
| switch dev.SynchronizationMode { |
There was a problem hiding this comment.
if with an early return and then return the default ?
pkg/vf/virtio.go
Outdated
| attachment := dev.Attachment() | ||
| if nbdAttachment, isNbdAttachment := attachment.(*vz.NetworkBlockDeviceStorageDeviceAttachment); isNbdAttachment { | ||
| nbdId, err := dev.(*vz.VirtioBlockDeviceConfiguration).BlockDeviceIdentifier() | ||
| if err != nil || nbdId == "" { |
There was a problem hiding this comment.
it looks like we have comingled errors here ... or is it not possible to have an error when calling BlockDeviceIdentifier? Maybe that func should return a defined error like ErrNoBlockDevID so you can later compare it?
There was a problem hiding this comment.
I cannot change BlockDeviceIdentifier directly as it is part of an external dep but I think the nbdID check can also be omitted here. I already verify if the identifier is set by the user earlier. Here I just retrieve it
lstocchi
force-pushed
the
i129
branch
2 times, most recently
from
53125ed to
26dd623
Compare
|
@baude updated 👍 |
|
Forgot to mark it ready 😄 |
This patch adds support for NBD. The NetworkBlockDevice protocol allows remote block devices to be accessed over the network. By leveraging it we can connect our VM to a remote block device and use it as a local disk. User can specify the timeout and the synchronization mode (none or full). More info ati https://developer.apple.com/documentation/virtualization/vzdisksynchronizationmode?language=objc It also listens to changes to the network block device attachment, so if there is some connectivity changes (connect, disconnect), the user is informed. To achieve it, it leverages the delegate provided by the Virtualization Framework. More info at https://developer.apple.com/documentation/virtualization/vznetworkblockdevicestoragedeviceattachmentdelegate?language=objc This feature is only available for macOS 14.0+ Signed-off-by: Luca Stocchi <lstocchi@redhat.com>
add documentation to explain how to use the new NetworkBlockDevice device. Args list and examples are provided in the usage.md file Signed-off-by: Luca Stocchi <lstocchi@redhat.com>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfergeau The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
crc-org#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 crc-org#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 <lstocchi@redhat.com>
This patch adds support for NBD. The NetworkBlockDevice protocol allows remote block devices to be accessed over the network. By leveraging it we can connect our VM to a remote block device and use it as a local disk.
User can specify the timeout and the synchronization mode (none or full). More info at https://developer.apple.com/documentation/virtualization/vzdisksynchronizationmode?language=objc
It also listens to changes to the network block device attachment, so if there is some connectivity changes (connect, disconnect), the user is informed. To achieve it, it leverages the delegate provided by the Virtualization Framework. More info at https://developer.apple.com/documentation/virtualization/vznetworkblockdevicestoragedeviceattachmentdelegate?language=objc
This feature is only available for macOS 14.0+ and requires an update to the VZ library. So this implementation could be modified based on the work done there.
It needs Code-Hex/vz#156 and Code-Hex/vz#166
It resolves #129