10:15 a.m.
https://bugzilla.redhat.com/show_bug.cgi?id=2070530
This patch touches some very dark corners of virt-v2v, namely target
bus assignment and Linux configuration file device name rewriting.
I have only lightly tested it by converting mxie's own NVMe guest
(which worked).
Rich.
10:15 a.m.
We model NVMe devices in the source hypervisor.
We currently assume that no one is using the namespaces feature of
NVMe, ie. that each source device will appear in a Linux guest as
/dev/nvme0n1, /dev/nvme1n1, etc. We could fix this if it is a
problem, but it requires adjusting the current assumption for
removable devices that slots are simple integers.
The devices are mapped to virtio-blk, so in the target the device name
has to change from /dev/nvme0 to /dev/vda (etc.)
Reported-by: Ming Xie
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2070530
---
convert/convert_linux.ml | 28 ++++++-----
convert/target_bus_assignment.ml | 4 +-
input/parse_domain_from_vmx.ml | 26 +++++++++-
lib/types.ml | 3 +-
lib/types.mli | 2 +-
tests/test-v2v-i-vmx-6.expected | 22 +++++++++
tests/test-v2v-i-vmx-6.vmx | 84 ++++++++++++++++++++++++++++++++
tests/test-v2v-i-vmx.sh | 4 +-
8 files changed, 154 insertions(+), 19 deletions(-)
diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
index 58a2a2a84d..d5beb309c8 100644
--- a/convert/convert_linux.ml
+++ b/convert/convert_linux.ml
@@ -1022,17 +1022,23 @@ let convert (g : G.guestfs) source inspect keep_serial_console _
=
let map =
List.mapi (
- fun i disk ->
- let block_prefix_before_conversion =
- match disk.s_controller with
- | Some Source_IDE -> ide_block_prefix
- | Some (Source_virtio_SCSI | Source_SCSI | Source_SATA) -> "sd"
- | Some Source_virtio_blk -> "vd"
- | None ->
- (* This is basically a guess. It assumes the source used IDE. *)
- ide_block_prefix in
- let source_dev = block_prefix_before_conversion ^ drive_name i in
- let target_dev = block_prefix_after_conversion ^ drive_name i in
+ fun i { s_controller } ->
+ let device_name_before_conversion i =
+ match s_controller with
+ | None
+ (* If we don't have a controller, guess. Assumes the
+ * source used IDE.
+ *)
+ | Some Source_IDE ->
+ ide_block_prefix ^ drive_name i
+ | Some (Source_virtio_SCSI | Source_SCSI | Source_SATA) ->
+ "sd" ^ drive_name i
+ | Some Source_virtio_blk -> "vd" ^ drive_name i
+ | Some Source_NVME ->
+ (* For NVMe assume no one is using namespaces. *)
+ "nvme" ^ drive_name i ^ "n1" in
+ let source_dev = device_name_before_conversion i in
+ let target_dev = device_name_before_conversion i in
source_dev, target_dev
) source.s_disks in
diff --git a/convert/target_bus_assignment.ml b/convert/target_bus_assignment.ml
index 4b56a6e171..f8675cf29b 100644
--- a/convert/target_bus_assignment.ml
+++ b/convert/target_bus_assignment.ml
@@ -73,8 +73,8 @@ let rec target_bus_assignment source_disks source_removables guestcaps
=
| None -> ide_bus (* Wild guess, but should be safe. *)
| Some Source_virtio_blk -> virtio_blk_bus
| Some Source_IDE -> ide_bus
- | Some (Source_virtio_SCSI | Source_SCSI | Source_SATA) ->
- scsi_bus in
+ | Some (Source_virtio_SCSI | Source_SCSI | Source_SATA |
+ Source_NVME) -> scsi_bus in
match r.s_removable_slot with
| None ->
diff --git a/input/parse_domain_from_vmx.ml b/input/parse_domain_from_vmx.ml
index 730f1177f7..b812edeb81 100644
--- a/input/parse_domain_from_vmx.ml
+++ b/input/parse_domain_from_vmx.ml
@@ -103,6 +103,7 @@ let remote_file_exists uri path =
let rec find_disks vmx vmx_source =
find_scsi_disks vmx vmx_source
+ @ find_nvme_disks vmx vmx_source
@ find_ide_disks vmx vmx_source
(* Find all SCSI hard disks.
@@ -129,6 +130,27 @@ and find_scsi_disks vmx vmx_source =
get_scsi_controller_target is_scsi_controller_target
scsi_device_types scsi_controller
+(* Find all NVMe hard disks.
+ *
+ * In the VMX file:
+ * nvme0.pcislotnumber = "192"
+ * nvme0:0.fileName = "guest.vmdk"
+ *)
+and find_nvme_disks vmx vmx_source =
+ let get_nvme_controller_target ns =
+ sscanf ns "nvme%d:%d" (fun c t -> c, t)
+ in
+ let is_nvme_controller_target ns =
+ try ignore (get_nvme_controller_target ns); true
+ with Scanf.Scan_failure _ | End_of_file | Failure _ -> false
+ in
+ let nvme_device_types = [ None ] in
+ let nvme_controller = Source_NVME in
+
+ find_hdds vmx vmx_source
+ get_nvme_controller_target is_nvme_controller_target
+ nvme_device_types nvme_controller
+
(* Find all IDE hard disks.
*
* In the VMX file:
@@ -153,12 +175,12 @@ and find_ide_disks vmx vmx_source =
and find_hdds vmx vmx_source
get_controller_target is_controller_target
device_types controller =
- (* Find namespaces matching '(ide|scsi)X:Y' with suitable deviceType. *)
+ (* Find namespaces matching '(ide|scsi|nvme)X:Y' with suitable deviceType. *)
let hdds =
Parse_vmx.select_namespaces (
function
| [ns] ->
- (* Check the namespace is '(ide|scsi)X:Y' *)
+ (* Check the namespace is '(ide|scsi|nvme)X:Y' *)
if not (is_controller_target ns) then false
else (
(* Check the deviceType is one we are looking for. *)
diff --git a/lib/types.ml b/lib/types.ml
index 5804c7c79f..92ed0e5251 100644
--- a/lib/types.ml
+++ b/lib/types.ml
@@ -56,7 +56,7 @@ and source_disk = {
s_disk_id : int;
s_controller : s_controller option;
}
-and s_controller = Source_IDE | Source_SATA | Source_SCSI |
+and s_controller = Source_IDE | Source_SATA | Source_SCSI | Source_NVME |
Source_virtio_blk | Source_virtio_SCSI
and source_removable = {
s_removable_type : s_removable_type;
@@ -194,6 +194,7 @@ and string_of_controller = function
| Source_IDE -> "ide"
| Source_SATA -> "sata"
| Source_SCSI -> "scsi"
+ | Source_NVME -> "nvme"
| Source_virtio_blk -> "virtio-blk"
| Source_virtio_SCSI -> "virtio-scsi"
diff --git a/lib/types.mli b/lib/types.mli
index dd2fe5925d..37238cd755 100644
--- a/lib/types.mli
+++ b/lib/types.mli
@@ -103,7 +103,7 @@ and source_disk = {
}
(** A source disk. *)
-and s_controller = Source_IDE | Source_SATA | Source_SCSI |
+and s_controller = Source_IDE | Source_SATA | Source_SCSI | Source_NVME |
Source_virtio_blk | Source_virtio_SCSI
(** Source disk controller. *)
diff --git a/tests/test-v2v-i-vmx-6.expected b/tests/test-v2v-i-vmx-6.expected
new file mode 100644
index 0000000000..1793b6b953
--- /dev/null
+++ b/tests/test-v2v-i-vmx-6.expected
@@ -0,0 +1,22 @@
+
+Source guest information (--print-source option):
+
+ source name: esx6.7-rhel8.6-nvme-disk
+hypervisor type: vmware
+ VM genid:
+ memory: 2147483648 (bytes)
+ nr vCPUs: 1
+ CPU vendor:
+ CPU model:
+ CPU topology:
+ CPU features:
+ firmware: bios
+ display:
+ sound:
+disks:
+ 0 [nvme]
+removable media:
+
+NICs:
+ Network "VM Network" mac: 00:50:56:ac:cf:51 [vmxnet3]
+
diff --git a/tests/test-v2v-i-vmx-6.vmx b/tests/test-v2v-i-vmx-6.vmx
new file mode 100644
index 0000000000..9203c0cfcb
--- /dev/null
+++ b/tests/test-v2v-i-vmx-6.vmx
@@ -0,0 +1,84 @@
+.encoding = "UTF-8"
+config.version = "8"
+virtualHW.version = "14"
+nvram = "esx6.7-rhel8.6-nvme-disk.nvram"
+pciBridge0.present = "TRUE"
+svga.present = "TRUE"
+pciBridge4.present = "TRUE"
+pciBridge4.virtualDev = "pcieRootPort"
+pciBridge4.functions = "8"
+pciBridge5.present = "TRUE"
+pciBridge5.virtualDev = "pcieRootPort"
+pciBridge5.functions = "8"
+pciBridge6.present = "TRUE"
+pciBridge6.virtualDev = "pcieRootPort"
+pciBridge6.functions = "8"
+pciBridge7.present = "TRUE"
+pciBridge7.virtualDev = "pcieRootPort"
+pciBridge7.functions = "8"
+vmci0.present = "TRUE"
+hpet0.present = "TRUE"
+floppy0.present = "FALSE"
+svga.vramSize = "8388608"
+memSize = "2048"
+powerType.powerOff = "default"
+powerType.suspend = "default"
+powerType.reset = "default"
+tools.upgrade.policy = "manual"
+sched.cpu.units = "mhz"
+sched.cpu.affinity = "all"
+vm.createDate = "1648710632879834"
+sata0.present = "TRUE"
+nvme0.present = "TRUE"
+nvme0:0.fileName = "nvme-disk.vmdk"
+sched.nvme0:0.shares = "normal"
+sched.nvme0:0.throughputCap = "off"
+nvme0:0.present = "TRUE"
+ethernet0.virtualDev = "vmxnet3"
+ethernet0.networkName = "VM Network"
+ethernet0.addressType = "vpx"
+ethernet0.generatedAddress = "00:50:56:ac:cf:51"
+ethernet0.uptCompatibility = "TRUE"
+ethernet0.present = "TRUE"
+sata0:0.startConnected = "FALSE"
+sata0:0.deviceType = "cdrom-raw"
+sata0:0.clientDevice = "TRUE"
+sata0:0.fileName = "emptyBackingString"
+sata0:0.present = "TRUE"
+displayName = "esx6.7-rhel8.6-nvme-disk"
+guestOS = "rhel8-64"
+toolScripts.afterPowerOn = "TRUE"
+toolScripts.afterResume = "TRUE"
+toolScripts.beforeSuspend = "TRUE"
+toolScripts.beforePowerOff = "TRUE"
+uuid.bios = "42 2c 17 6d 0b 31 4e 16-82 97 f0 b4 99 0b 86 09"
+vc.uuid = "50 2c fc c7 be fb b1 48-fc 7d 4b 39 b3 f5 de b5"
+migrate.hostLog = "esx6.7-rhel8.6-nvme-disk-7585773e.hlog"
+sched.cpu.min = "0"
+sched.cpu.shares = "normal"
+sched.mem.min = "0"
+sched.mem.minSize = "0"
+sched.mem.shares = "normal"
+migrate.encryptionMode = "opportunistic"
+numa.autosize.cookie = "10001"
+numa.autosize.vcpu.maxPerVirtualNode = "1"
+sched.swap.derivedName =
"/vmfs/volumes/02155034-b2df70e7/esx6.7-rhel8.6-nvme-disk/esx6.7-rhel8.6-nvme-disk-34965f24.vswp"
+uuid.location = "56 4d c5 90 b1 ca f0 32-64 99 32 a7 d8 97 27 3a"
+nvme0:0.redo = ""
+pciBridge0.pciSlotNumber = "17"
+pciBridge4.pciSlotNumber = "21"
+pciBridge5.pciSlotNumber = "22"
+pciBridge6.pciSlotNumber = "23"
+pciBridge7.pciSlotNumber = "24"
+ethernet0.pciSlotNumber = "160"
+vmci0.pciSlotNumber = "32"
+sata0.pciSlotNumber = "33"
+nvme0.pciSlotNumber = "192"
+vmci0.id = "-1727298039"
+monitor.phys_bits_used = "43"
+vmotion.checkpointFBSize = "8388608"
+vmotion.checkpointSVGAPrimarySize = "8388608"
+cleanShutdown = "TRUE"
+softPowerOff = "TRUE"
+svga.guestBackedPrimaryAware = "TRUE"
+tools.syncTime = "FALSE"
\ No newline at end of file
diff --git a/tests/test-v2v-i-vmx.sh b/tests/test-v2v-i-vmx.sh
index db870bea05..d74ddfaaf8 100755
--- a/tests/test-v2v-i-vmx.sh
+++ b/tests/test-v2v-i-vmx.sh
@@ -39,14 +39,14 @@ rm -f test-v2v-i-vmx-*.actual
# For the tests to succeed we need at least the fileName (VMDK input
# files) to exist.
-fns="BZ1308535_21disks.vmdk Fedora-20.vmdk RHEL-7.1-UEFI.vmdk Windows-7-x64.vmdk
MSEdge-Win10_preview.vmdk"
+fns="BZ1308535_21disks.vmdk Fedora-20.vmdk RHEL-7.1-UEFI.vmdk Windows-7-x64.vmdk
MSEdge-Win10_preview.vmdk nvme-disk.vmdk"
for fn in BZ1308535_21disks_{1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20}.vmdk;
do
fns="$fns $fn"
done
for fn in $fns; do qemu-img create -f vmdk $fn 512; done
-for i in 1 2 3 4 5; do
+for i in 1 2 3 4 5 6; do
$VG virt-v2v --debug-gc \
-i vmx test-v2v-i-vmx-$i.vmx \
--print-source > test-v2v-i-vmx-$i.actual
--
2.35.1
9:26 a.m.
On 04/04/22 17:15, Richard W.M. Jones wrote:
We model NVMe devices in the source hypervisor.
We currently assume that no one is using the namespaces feature of
NVMe, ie. that each source device will appear in a Linux guest as
/dev/nvme0n1, /dev/nvme1n1, etc. We could fix this if it is a
problem, but it requires adjusting the current assumption for
removable devices that slots are simple integers.
The devices are mapped to virtio-blk, so in the target the device name
has to change from /dev/nvme0 to /dev/vda (etc.)
Reported-by: Ming Xie
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2070530
---
convert/convert_linux.ml | 28 ++++++-----
convert/target_bus_assignment.ml | 4 +-
input/parse_domain_from_vmx.ml | 26 +++++++++-
lib/types.ml | 3 +-
lib/types.mli | 2 +-
tests/test-v2v-i-vmx-6.expected | 22 +++++++++
tests/test-v2v-i-vmx-6.vmx | 84 ++++++++++++++++++++++++++++++++
tests/test-v2v-i-vmx.sh | 4 +-
8 files changed, 154 insertions(+), 19 deletions(-)
diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
index 58a2a2a84d..d5beb309c8 100644
--- a/convert/convert_linux.ml
+++ b/convert/convert_linux.ml
@@ -1022,17 +1022,23 @@ let convert (g : G.guestfs) source inspect keep_serial_console _
=
let map =
List.mapi (
- fun i disk ->
- let block_prefix_before_conversion =
- match disk.s_controller with
- | Some Source_IDE -> ide_block_prefix
- | Some (Source_virtio_SCSI | Source_SCSI | Source_SATA) ->
"sd"
- | Some Source_virtio_blk -> "vd"
- | None ->
- (* This is basically a guess. It assumes the source used IDE. *)
- ide_block_prefix in
- let source_dev = block_prefix_before_conversion ^ drive_name i in
- let target_dev = block_prefix_after_conversion ^ drive_name i in
+ fun i { s_controller } ->
+ let device_name_before_conversion i =
+ match s_controller with
+ | None
+ (* If we don't have a controller, guess. Assumes the
+ * source used IDE.
+ *)
+ | Some Source_IDE ->
+ ide_block_prefix ^ drive_name i
+ | Some (Source_virtio_SCSI | Source_SCSI | Source_SATA) ->
+ "sd" ^ drive_name i
+ | Some Source_virtio_blk -> "vd" ^ drive_name i
+ | Some Source_NVME ->
+ (* For NVMe assume no one is using namespaces. *)
+ "nvme" ^ drive_name i ^ "n1" in
+ let source_dev = device_name_before_conversion i in
+ let target_dev = device_name_before_conversion i in
(1) I find this change a bit too heavy-weight. Before,
"block_prefix_before_conversion" was a string, and we used that string as a part
of the concatenation in source_dev. Afterwards, "device_name_before_conversion"
is a *function*, which contains the concatenations internally for each pattern, BUT this
function also handles Source_NVME. I think that's what I would have preferred to see
in separation -- first reworking the existing logic from
"block_prefix_before_conversion" (string) to
"device_name_before_conversion" (function), without observable changes in
behavior, then adding the NVME branch. ... I guess if someone is more used to reading
OCaml, doing both things at the same time may not be a big deal.
(2) The computation of "target_dev" looks fishy. First, with the patch applied,
we now only use "block_prefix_after_conversion" for mapping Xen disks (xvd), so
the "block_prefix_after_conversion" name becomes too generic. Second, the effect
on "target_dev" seems to conflict with the commit message (namely the statement
about mapping NVME disks to virtio-blk), PLUS it seems to regress the current logic -- it
would now map (e.g.) SCSI disks to sdX, not vdX (virtio-block). In other words, I think
the "target_dev" calculation should be preserved.
In turn, I think there is no use for the "i" parameter of
"device_name_before_conversion", we could just compute "source_dev" as
a string, using the outer "i". Something like:
fun i { s_controller } ->
let source_dev =
match s_controller with
| None
(* If we don't have a controller, guess. Assumes the
* source used IDE.
*)
| Some Source_IDE ->
ide_block_prefix ^ drive_name i
| Some (Source_virtio_SCSI | Source_SCSI | Source_SATA) ->
"sd" ^ drive_name i
| Some Source_virtio_blk -> "vd" ^ drive_name i
| Some Source_NVME ->
(* For NVMe assume no one is using namespaces. *)
"nvme" ^ drive_name i ^ "n1"
and target_dev = block_prefix_after_conversion ^ drive_name i in
To summarize (1) and (2): I'd likely write a patch first just for sinking (=
multiplying) the
^ drive_name i
operation into the various pattern matches (calculting "source_dev" directly,
without "block_prefix_before_conversion"), not touching "target_dev".
Then a different patch for extending the patterns with NVME.
source_dev, target_dev
) source.s_disks in
diff --git a/convert/target_bus_assignment.ml b/convert/target_bus_assignment.ml
index 4b56a6e171..f8675cf29b 100644
--- a/convert/target_bus_assignment.ml
+++ b/convert/target_bus_assignment.ml
@@ -73,8 +73,8 @@ let rec target_bus_assignment source_disks source_removables guestcaps
=
| None -> ide_bus (* Wild guess, but should be safe. *)
| Some Source_virtio_blk -> virtio_blk_bus
| Some Source_IDE -> ide_bus
- | Some (Source_virtio_SCSI | Source_SCSI | Source_SATA) ->
- scsi_bus in
+ | Some (Source_virtio_SCSI | Source_SCSI | Source_SATA |
+ Source_NVME) -> scsi_bus in
match r.s_removable_slot with
| None ->
(3) Not sure if *all* of the preexistent code is very useful here; I've never seen a
virtio-blk CD-ROM (only IDE/SATA, or (virtio-)SCSI). I also can't really imagine an
NVMe CD-ROM. So I guess the intent of this hunk is just to give the pattern match 100%
coverage?
(Personally I'd be tempted to assert that a CDROM is never on virtio-blk, or NVMe.)
The rest looks OK to me.
Thanks
Laszlo
diff --git a/input/parse_domain_from_vmx.ml
b/input/parse_domain_from_vmx.ml
index 730f1177f7..b812edeb81 100644
--- a/input/parse_domain_from_vmx.ml
+++ b/input/parse_domain_from_vmx.ml
@@ -103,6 +103,7 @@ let remote_file_exists uri path =
let rec find_disks vmx vmx_source =
find_scsi_disks vmx vmx_source
+ @ find_nvme_disks vmx vmx_source
@ find_ide_disks vmx vmx_source
(* Find all SCSI hard disks.
@@ -129,6 +130,27 @@ and find_scsi_disks vmx vmx_source =
get_scsi_controller_target is_scsi_controller_target
scsi_device_types scsi_controller
+(* Find all NVMe hard disks.
+ *
+ * In the VMX file:
+ * nvme0.pcislotnumber = "192"
+ * nvme0:0.fileName = "guest.vmdk"
+ *)
+and find_nvme_disks vmx vmx_source =
+ let get_nvme_controller_target ns =
+ sscanf ns "nvme%d:%d" (fun c t -> c, t)
+ in
+ let is_nvme_controller_target ns =
+ try ignore (get_nvme_controller_target ns); true
+ with Scanf.Scan_failure _ | End_of_file | Failure _ -> false
+ in
+ let nvme_device_types = [ None ] in
+ let nvme_controller = Source_NVME in
+
+ find_hdds vmx vmx_source
+ get_nvme_controller_target is_nvme_controller_target
+ nvme_device_types nvme_controller
+
(* Find all IDE hard disks.
*
* In the VMX file:
@@ -153,12 +175,12 @@ and find_ide_disks vmx vmx_source =
and find_hdds vmx vmx_source
get_controller_target is_controller_target
device_types controller =
- (* Find namespaces matching '(ide|scsi)X:Y' with suitable deviceType. *)
+ (* Find namespaces matching '(ide|scsi|nvme)X:Y' with suitable deviceType. *)
let hdds =
Parse_vmx.select_namespaces (
function
| [ns] ->
- (* Check the namespace is '(ide|scsi)X:Y' *)
+ (* Check the namespace is '(ide|scsi|nvme)X:Y' *)
if not (is_controller_target ns) then false
else (
(* Check the deviceType is one we are looking for. *)
diff --git a/lib/types.ml b/lib/types.ml
index 5804c7c79f..92ed0e5251 100644
--- a/lib/types.ml
+++ b/lib/types.ml
@@ -56,7 +56,7 @@ and source_disk = {
s_disk_id : int;
s_controller : s_controller option;
}
-and s_controller = Source_IDE | Source_SATA | Source_SCSI |
+and s_controller = Source_IDE | Source_SATA | Source_SCSI | Source_NVME |
Source_virtio_blk | Source_virtio_SCSI
and source_removable = {
s_removable_type : s_removable_type;
@@ -194,6 +194,7 @@ and string_of_controller = function
| Source_IDE -> "ide"
| Source_SATA -> "sata"
| Source_SCSI -> "scsi"
+ | Source_NVME -> "nvme"
| Source_virtio_blk -> "virtio-blk"
| Source_virtio_SCSI -> "virtio-scsi"
diff --git a/lib/types.mli b/lib/types.mli
index dd2fe5925d..37238cd755 100644
--- a/lib/types.mli
+++ b/lib/types.mli
@@ -103,7 +103,7 @@ and source_disk = {
}
(** A source disk. *)
-and s_controller = Source_IDE | Source_SATA | Source_SCSI |
+and s_controller = Source_IDE | Source_SATA | Source_SCSI | Source_NVME |
Source_virtio_blk | Source_virtio_SCSI
(** Source disk controller. *)
diff --git a/tests/test-v2v-i-vmx-6.expected b/tests/test-v2v-i-vmx-6.expected
new file mode 100644
index 0000000000..1793b6b953
--- /dev/null
+++ b/tests/test-v2v-i-vmx-6.expected
@@ -0,0 +1,22 @@
+
+Source guest information (--print-source option):
+
+ source name: esx6.7-rhel8.6-nvme-disk
+hypervisor type: vmware
+ VM genid:
+ memory: 2147483648 (bytes)
+ nr vCPUs: 1
+ CPU vendor:
+ CPU model:
+ CPU topology:
+ CPU features:
+ firmware: bios
+ display:
+ sound:
+disks:
+ 0 [nvme]
+removable media:
+
+NICs:
+ Network "VM Network" mac: 00:50:56:ac:cf:51 [vmxnet3]
+
diff --git a/tests/test-v2v-i-vmx-6.vmx b/tests/test-v2v-i-vmx-6.vmx
new file mode 100644
index 0000000000..9203c0cfcb
--- /dev/null
+++ b/tests/test-v2v-i-vmx-6.vmx
@@ -0,0 +1,84 @@
+.encoding = "UTF-8"
+config.version = "8"
+virtualHW.version = "14"
+nvram = "esx6.7-rhel8.6-nvme-disk.nvram"
+pciBridge0.present = "TRUE"
+svga.present = "TRUE"
+pciBridge4.present = "TRUE"
+pciBridge4.virtualDev = "pcieRootPort"
+pciBridge4.functions = "8"
+pciBridge5.present = "TRUE"
+pciBridge5.virtualDev = "pcieRootPort"
+pciBridge5.functions = "8"
+pciBridge6.present = "TRUE"
+pciBridge6.virtualDev = "pcieRootPort"
+pciBridge6.functions = "8"
+pciBridge7.present = "TRUE"
+pciBridge7.virtualDev = "pcieRootPort"
+pciBridge7.functions = "8"
+vmci0.present = "TRUE"
+hpet0.present = "TRUE"
+floppy0.present = "FALSE"
+svga.vramSize = "8388608"
+memSize = "2048"
+powerType.powerOff = "default"
+powerType.suspend = "default"
+powerType.reset = "default"
+tools.upgrade.policy = "manual"
+sched.cpu.units = "mhz"
+sched.cpu.affinity = "all"
+vm.createDate = "1648710632879834"
+sata0.present = "TRUE"
+nvme0.present = "TRUE"
+nvme0:0.fileName = "nvme-disk.vmdk"
+sched.nvme0:0.shares = "normal"
+sched.nvme0:0.throughputCap = "off"
+nvme0:0.present = "TRUE"
+ethernet0.virtualDev = "vmxnet3"
+ethernet0.networkName = "VM Network"
+ethernet0.addressType = "vpx"
+ethernet0.generatedAddress = "00:50:56:ac:cf:51"
+ethernet0.uptCompatibility = "TRUE"
+ethernet0.present = "TRUE"
+sata0:0.startConnected = "FALSE"
+sata0:0.deviceType = "cdrom-raw"
+sata0:0.clientDevice = "TRUE"
+sata0:0.fileName = "emptyBackingString"
+sata0:0.present = "TRUE"
+displayName = "esx6.7-rhel8.6-nvme-disk"
+guestOS = "rhel8-64"
+toolScripts.afterPowerOn = "TRUE"
+toolScripts.afterResume = "TRUE"
+toolScripts.beforeSuspend = "TRUE"
+toolScripts.beforePowerOff = "TRUE"
+uuid.bios = "42 2c 17 6d 0b 31 4e 16-82 97 f0 b4 99 0b 86 09"
+vc.uuid = "50 2c fc c7 be fb b1 48-fc 7d 4b 39 b3 f5 de b5"
+migrate.hostLog = "esx6.7-rhel8.6-nvme-disk-7585773e.hlog"
+sched.cpu.min = "0"
+sched.cpu.shares = "normal"
+sched.mem.min = "0"
+sched.mem.minSize = "0"
+sched.mem.shares = "normal"
+migrate.encryptionMode = "opportunistic"
+numa.autosize.cookie = "10001"
+numa.autosize.vcpu.maxPerVirtualNode = "1"
+sched.swap.derivedName =
"/vmfs/volumes/02155034-b2df70e7/esx6.7-rhel8.6-nvme-disk/esx6.7-rhel8.6-nvme-disk-34965f24.vswp"
+uuid.location = "56 4d c5 90 b1 ca f0 32-64 99 32 a7 d8 97 27 3a"
+nvme0:0.redo = ""
+pciBridge0.pciSlotNumber = "17"
+pciBridge4.pciSlotNumber = "21"
+pciBridge5.pciSlotNumber = "22"
+pciBridge6.pciSlotNumber = "23"
+pciBridge7.pciSlotNumber = "24"
+ethernet0.pciSlotNumber = "160"
+vmci0.pciSlotNumber = "32"
+sata0.pciSlotNumber = "33"
+nvme0.pciSlotNumber = "192"
+vmci0.id = "-1727298039"
+monitor.phys_bits_used = "43"
+vmotion.checkpointFBSize = "8388608"
+vmotion.checkpointSVGAPrimarySize = "8388608"
+cleanShutdown = "TRUE"
+softPowerOff = "TRUE"
+svga.guestBackedPrimaryAware = "TRUE"
+tools.syncTime = "FALSE"
\ No newline at end of file
diff --git a/tests/test-v2v-i-vmx.sh b/tests/test-v2v-i-vmx.sh
index db870bea05..d74ddfaaf8 100755
--- a/tests/test-v2v-i-vmx.sh
+++ b/tests/test-v2v-i-vmx.sh
@@ -39,14 +39,14 @@ rm -f test-v2v-i-vmx-*.actual
# For the tests to succeed we need at least the fileName (VMDK input
# files) to exist.
-fns="BZ1308535_21disks.vmdk Fedora-20.vmdk RHEL-7.1-UEFI.vmdk Windows-7-x64.vmdk
MSEdge-Win10_preview.vmdk"
+fns="BZ1308535_21disks.vmdk Fedora-20.vmdk RHEL-7.1-UEFI.vmdk Windows-7-x64.vmdk
MSEdge-Win10_preview.vmdk nvme-disk.vmdk"
for fn in BZ1308535_21disks_{1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20}.vmdk;
do
fns="$fns $fn"
done
for fn in $fns; do qemu-img create -f vmdk $fn 512; done
-for i in 1 2 3 4 5; do
+for i in 1 2 3 4 5 6; do
$VG virt-v2v --debug-gc \
-i vmx test-v2v-i-vmx-$i.vmx \
--print-source > test-v2v-i-vmx-$i.actual
5:56 a.m.
On Tue, Apr 05, 2022 at 04:26:53PM +0200, Laszlo Ersek wrote:
On 04/04/22 17:15, Richard W.M. Jones wrote:
> We model NVMe devices in the source hypervisor.
>
> We currently assume that no one is using the namespaces feature of
> NVMe, ie. that each source device will appear in a Linux guest as
> /dev/nvme0n1, /dev/nvme1n1, etc. We could fix this if it is a
> problem, but it requires adjusting the current assumption for
> removable devices that slots are simple integers.
>
> The devices are mapped to virtio-blk, so in the target the device name
> has to change from /dev/nvme0 to /dev/vda (etc.)
>
> Reported-by: Ming Xie
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2070530
> ---
> convert/convert_linux.ml | 28 ++++++-----
> convert/target_bus_assignment.ml | 4 +-
> input/parse_domain_from_vmx.ml | 26 +++++++++-
> lib/types.ml | 3 +-
> lib/types.mli | 2 +-
> tests/test-v2v-i-vmx-6.expected | 22 +++++++++
> tests/test-v2v-i-vmx-6.vmx | 84 ++++++++++++++++++++++++++++++++
> tests/test-v2v-i-vmx.sh | 4 +-
> 8 files changed, 154 insertions(+), 19 deletions(-)
>
> diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
> index 58a2a2a84d..d5beb309c8 100644
> --- a/convert/convert_linux.ml
> +++ b/convert/convert_linux.ml
> @@ -1022,17 +1022,23 @@ let convert (g : G.guestfs) source inspect
keep_serial_console _ =
>
> let map =
> List.mapi (
> - fun i disk ->
> - let block_prefix_before_conversion =
> - match disk.s_controller with
> - | Some Source_IDE -> ide_block_prefix
> - | Some (Source_virtio_SCSI | Source_SCSI | Source_SATA) ->
"sd"
> - | Some Source_virtio_blk -> "vd"
> - | None ->
> - (* This is basically a guess. It assumes the source used IDE. *)
> - ide_block_prefix in
> - let source_dev = block_prefix_before_conversion ^ drive_name i in
> - let target_dev = block_prefix_after_conversion ^ drive_name i in
> + fun i { s_controller } ->
> + let device_name_before_conversion i =
> + match s_controller with
> + | None
> + (* If we don't have a controller, guess. Assumes the
> + * source used IDE.
> + *)
> + | Some Source_IDE ->
> + ide_block_prefix ^ drive_name i
> + | Some (Source_virtio_SCSI | Source_SCSI | Source_SATA) ->
> + "sd" ^ drive_name i
> + | Some Source_virtio_blk -> "vd" ^ drive_name i
> + | Some Source_NVME ->
> + (* For NVMe assume no one is using namespaces. *)
> + "nvme" ^ drive_name i ^ "n1" in
> + let source_dev = device_name_before_conversion i in
> + let target_dev = device_name_before_conversion i in
(1) I find this change a bit too heavy-weight. Before,
"block_prefix_before_conversion" was a string, and we used that string as a part
of the concatenation in source_dev. Afterwards, "device_name_before_conversion"
is a *function*, which contains the concatenations internally for each pattern, BUT this
function also handles Source_NVME. I think that's what I would have preferred to see
in separation -- first reworking the existing logic from
"block_prefix_before_conversion" (string) to
"device_name_before_conversion" (function), without observable changes in
behavior, then adding the NVME branch. ... I guess if someone is more used to reading
OCaml, doing both things at the same time may not be a big deal.
(2) The computation of "target_dev" looks fishy. First, with the patch applied,
we now only use "block_prefix_after_conversion" for mapping Xen disks (xvd), so
the "block_prefix_after_conversion" name becomes too generic. Second, the effect
on "target_dev" seems to conflict with the commit message (namely the statement
about mapping NVME disks to virtio-blk), PLUS it seems to regress the current logic -- it
would now map (e.g.) SCSI disks to sdX, not vdX (virtio-block). In other words, I think
the "target_dev" calculation should be preserved.
In turn, I think there is no use for the "i" parameter of
"device_name_before_conversion", we could just compute "source_dev" as
a string, using the outer "i". Something like:
fun i { s_controller } ->
let source_dev =
match s_controller with
| None
(* If we don't have a controller, guess. Assumes the
* source used IDE.
*)
| Some Source_IDE ->
ide_block_prefix ^ drive_name i
| Some (Source_virtio_SCSI | Source_SCSI | Source_SATA) ->
"sd" ^ drive_name i
| Some Source_virtio_blk -> "vd" ^ drive_name i
| Some Source_NVME ->
(* For NVMe assume no one is using namespaces. *)
"nvme" ^ drive_name i ^ "n1"
and target_dev = block_prefix_after_conversion ^ drive_name i in
To summarize (1) and (2): I'd likely write a patch first just for sinking (=
multiplying) the
^ drive_name i
operation into the various pattern matches (calculting "source_dev"
directly, without "block_prefix_before_conversion"), not touching
"target_dev". Then a different patch for extending the patterns with
NVME.
That whole chunk is bogus. As well as the points you make above, it
also uses "a" instead of a number, so it comes up with device names
like "nvmean1". The reason I didn't see that in my test is because
the guest happened to be using UUIDs + LVs.
> source_dev, target_dev
> ) source.s_disks in
>
> diff --git a/convert/target_bus_assignment.ml b/convert/target_bus_assignment.ml
> index 4b56a6e171..f8675cf29b 100644
> --- a/convert/target_bus_assignment.ml
> +++ b/convert/target_bus_assignment.ml
> @@ -73,8 +73,8 @@ let rec target_bus_assignment source_disks source_removables
guestcaps =
> | None -> ide_bus (* Wild guess, but should be safe. *)
> | Some Source_virtio_blk -> virtio_blk_bus
> | Some Source_IDE -> ide_bus
> - | Some (Source_virtio_SCSI | Source_SCSI | Source_SATA) ->
> - scsi_bus in
> + | Some (Source_virtio_SCSI | Source_SCSI | Source_SATA |
> + Source_NVME) -> scsi_bus in
>
> match r.s_removable_slot with
> | None ->
(3) Not sure if *all* of the preexistent code is very useful here; I've never seen a
virtio-blk CD-ROM (only IDE/SATA, or (virtio-)SCSI). I also can't really imagine an
NVMe CD-ROM. So I guess the intent of this hunk is just to give the pattern match 100%
coverage?
Yes.
(Personally I'd be tempted to assert that a CDROM is never on
virtio-blk, or NVMe.)
I think asserting is a bit harsh here, it could mean a conversion
asserts even though it would have succeeded (because in practice no
one cares too much if a CD-ROM ends up in the wrong place).
I'll come up with a new patch that fixes the issues above.
Rich.
The rest looks OK to me.
Thanks
Laszlo
> diff --git a/input/parse_domain_from_vmx.ml b/input/parse_domain_from_vmx.ml
> index 730f1177f7..b812edeb81 100644
> --- a/input/parse_domain_from_vmx.ml
> +++ b/input/parse_domain_from_vmx.ml
> @@ -103,6 +103,7 @@ let remote_file_exists uri path =
>
> let rec find_disks vmx vmx_source =
> find_scsi_disks vmx vmx_source
> + @ find_nvme_disks vmx vmx_source
> @ find_ide_disks vmx vmx_source
>
> (* Find all SCSI hard disks.
> @@ -129,6 +130,27 @@ and find_scsi_disks vmx vmx_source =
> get_scsi_controller_target is_scsi_controller_target
> scsi_device_types scsi_controller
>
> +(* Find all NVMe hard disks.
> + *
> + * In the VMX file:
> + * nvme0.pcislotnumber = "192"
> + * nvme0:0.fileName = "guest.vmdk"
> + *)
> +and find_nvme_disks vmx vmx_source =
> + let get_nvme_controller_target ns =
> + sscanf ns "nvme%d:%d" (fun c t -> c, t)
> + in
> + let is_nvme_controller_target ns =
> + try ignore (get_nvme_controller_target ns); true
> + with Scanf.Scan_failure _ | End_of_file | Failure _ -> false
> + in
> + let nvme_device_types = [ None ] in
> + let nvme_controller = Source_NVME in
> +
> + find_hdds vmx vmx_source
> + get_nvme_controller_target is_nvme_controller_target
> + nvme_device_types nvme_controller
> +
> (* Find all IDE hard disks.
> *
> * In the VMX file:
> @@ -153,12 +175,12 @@ and find_ide_disks vmx vmx_source =
> and find_hdds vmx vmx_source
> get_controller_target is_controller_target
> device_types controller =
> - (* Find namespaces matching '(ide|scsi)X:Y' with suitable deviceType. *)
> + (* Find namespaces matching '(ide|scsi|nvme)X:Y' with suitable
deviceType. *)
> let hdds =
> Parse_vmx.select_namespaces (
> function
> | [ns] ->
> - (* Check the namespace is '(ide|scsi)X:Y' *)
> + (* Check the namespace is '(ide|scsi|nvme)X:Y' *)
> if not (is_controller_target ns) then false
> else (
> (* Check the deviceType is one we are looking for. *)
> diff --git a/lib/types.ml b/lib/types.ml
> index 5804c7c79f..92ed0e5251 100644
> --- a/lib/types.ml
> +++ b/lib/types.ml
> @@ -56,7 +56,7 @@ and source_disk = {
> s_disk_id : int;
> s_controller : s_controller option;
> }
> -and s_controller = Source_IDE | Source_SATA | Source_SCSI |
> +and s_controller = Source_IDE | Source_SATA | Source_SCSI | Source_NVME |
> Source_virtio_blk | Source_virtio_SCSI
> and source_removable = {
> s_removable_type : s_removable_type;
> @@ -194,6 +194,7 @@ and string_of_controller = function
> | Source_IDE -> "ide"
> | Source_SATA -> "sata"
> | Source_SCSI -> "scsi"
> + | Source_NVME -> "nvme"
> | Source_virtio_blk -> "virtio-blk"
> | Source_virtio_SCSI -> "virtio-scsi"
>
> diff --git a/lib/types.mli b/lib/types.mli
> index dd2fe5925d..37238cd755 100644
> --- a/lib/types.mli
> +++ b/lib/types.mli
> @@ -103,7 +103,7 @@ and source_disk = {
> }
> (** A source disk. *)
>
> -and s_controller = Source_IDE | Source_SATA | Source_SCSI |
> +and s_controller = Source_IDE | Source_SATA | Source_SCSI | Source_NVME |
> Source_virtio_blk | Source_virtio_SCSI
> (** Source disk controller. *)
>
> diff --git a/tests/test-v2v-i-vmx-6.expected b/tests/test-v2v-i-vmx-6.expected
> new file mode 100644
> index 0000000000..1793b6b953
> --- /dev/null
> +++ b/tests/test-v2v-i-vmx-6.expected
> @@ -0,0 +1,22 @@
> +
> +Source guest information (--print-source option):
> +
> + source name: esx6.7-rhel8.6-nvme-disk
> +hypervisor type: vmware
> + VM genid:
> + memory: 2147483648 (bytes)
> + nr vCPUs: 1
> + CPU vendor:
> + CPU model:
> + CPU topology:
> + CPU features:
> + firmware: bios
> + display:
> + sound:
> +disks:
> + 0 [nvme]
> +removable media:
> +
> +NICs:
> + Network "VM Network" mac: 00:50:56:ac:cf:51 [vmxnet3]
> +
> diff --git a/tests/test-v2v-i-vmx-6.vmx b/tests/test-v2v-i-vmx-6.vmx
> new file mode 100644
> index 0000000000..9203c0cfcb
> --- /dev/null
> +++ b/tests/test-v2v-i-vmx-6.vmx
> @@ -0,0 +1,84 @@
> +.encoding = "UTF-8"
> +config.version = "8"
> +virtualHW.version = "14"
> +nvram = "esx6.7-rhel8.6-nvme-disk.nvram"
> +pciBridge0.present = "TRUE"
> +svga.present = "TRUE"
> +pciBridge4.present = "TRUE"
> +pciBridge4.virtualDev = "pcieRootPort"
> +pciBridge4.functions = "8"
> +pciBridge5.present = "TRUE"
> +pciBridge5.virtualDev = "pcieRootPort"
> +pciBridge5.functions = "8"
> +pciBridge6.present = "TRUE"
> +pciBridge6.virtualDev = "pcieRootPort"
> +pciBridge6.functions = "8"
> +pciBridge7.present = "TRUE"
> +pciBridge7.virtualDev = "pcieRootPort"
> +pciBridge7.functions = "8"
> +vmci0.present = "TRUE"
> +hpet0.present = "TRUE"
> +floppy0.present = "FALSE"
> +svga.vramSize = "8388608"
> +memSize = "2048"
> +powerType.powerOff = "default"
> +powerType.suspend = "default"
> +powerType.reset = "default"
> +tools.upgrade.policy = "manual"
> +sched.cpu.units = "mhz"
> +sched.cpu.affinity = "all"
> +vm.createDate = "1648710632879834"
> +sata0.present = "TRUE"
> +nvme0.present = "TRUE"
> +nvme0:0.fileName = "nvme-disk.vmdk"
> +sched.nvme0:0.shares = "normal"
> +sched.nvme0:0.throughputCap = "off"
> +nvme0:0.present = "TRUE"
> +ethernet0.virtualDev = "vmxnet3"
> +ethernet0.networkName = "VM Network"
> +ethernet0.addressType = "vpx"
> +ethernet0.generatedAddress = "00:50:56:ac:cf:51"
> +ethernet0.uptCompatibility = "TRUE"
> +ethernet0.present = "TRUE"
> +sata0:0.startConnected = "FALSE"
> +sata0:0.deviceType = "cdrom-raw"
> +sata0:0.clientDevice = "TRUE"
> +sata0:0.fileName = "emptyBackingString"
> +sata0:0.present = "TRUE"
> +displayName = "esx6.7-rhel8.6-nvme-disk"
> +guestOS = "rhel8-64"
> +toolScripts.afterPowerOn = "TRUE"
> +toolScripts.afterResume = "TRUE"
> +toolScripts.beforeSuspend = "TRUE"
> +toolScripts.beforePowerOff = "TRUE"
> +uuid.bios = "42 2c 17 6d 0b 31 4e 16-82 97 f0 b4 99 0b 86 09"
> +vc.uuid = "50 2c fc c7 be fb b1 48-fc 7d 4b 39 b3 f5 de b5"
> +migrate.hostLog = "esx6.7-rhel8.6-nvme-disk-7585773e.hlog"
> +sched.cpu.min = "0"
> +sched.cpu.shares = "normal"
> +sched.mem.min = "0"
> +sched.mem.minSize = "0"
> +sched.mem.shares = "normal"
> +migrate.encryptionMode = "opportunistic"
> +numa.autosize.cookie = "10001"
> +numa.autosize.vcpu.maxPerVirtualNode = "1"
> +sched.swap.derivedName =
"/vmfs/volumes/02155034-b2df70e7/esx6.7-rhel8.6-nvme-disk/esx6.7-rhel8.6-nvme-disk-34965f24.vswp"
> +uuid.location = "56 4d c5 90 b1 ca f0 32-64 99 32 a7 d8 97 27 3a"
> +nvme0:0.redo = ""
> +pciBridge0.pciSlotNumber = "17"
> +pciBridge4.pciSlotNumber = "21"
> +pciBridge5.pciSlotNumber = "22"
> +pciBridge6.pciSlotNumber = "23"
> +pciBridge7.pciSlotNumber = "24"
> +ethernet0.pciSlotNumber = "160"
> +vmci0.pciSlotNumber = "32"
> +sata0.pciSlotNumber = "33"
> +nvme0.pciSlotNumber = "192"
> +vmci0.id = "-1727298039"
> +monitor.phys_bits_used = "43"
> +vmotion.checkpointFBSize = "8388608"
> +vmotion.checkpointSVGAPrimarySize = "8388608"
> +cleanShutdown = "TRUE"
> +softPowerOff = "TRUE"
> +svga.guestBackedPrimaryAware = "TRUE"
> +tools.syncTime = "FALSE"
> \ No newline at end of file
> diff --git a/tests/test-v2v-i-vmx.sh b/tests/test-v2v-i-vmx.sh
> index db870bea05..d74ddfaaf8 100755
> --- a/tests/test-v2v-i-vmx.sh
> +++ b/tests/test-v2v-i-vmx.sh
> @@ -39,14 +39,14 @@ rm -f test-v2v-i-vmx-*.actual
> # For the tests to succeed we need at least the fileName (VMDK input
> # files) to exist.
>
> -fns="BZ1308535_21disks.vmdk Fedora-20.vmdk RHEL-7.1-UEFI.vmdk
Windows-7-x64.vmdk MSEdge-Win10_preview.vmdk"
> +fns="BZ1308535_21disks.vmdk Fedora-20.vmdk RHEL-7.1-UEFI.vmdk
Windows-7-x64.vmdk MSEdge-Win10_preview.vmdk nvme-disk.vmdk"
> for fn in
BZ1308535_21disks_{1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20}.vmdk; do
> fns="$fns $fn"
> done
>
> for fn in $fns; do qemu-img create -f vmdk $fn 512; done
>
> -for i in 1 2 3 4 5; do
> +for i in 1 2 3 4 5 6; do
> $VG virt-v2v --debug-gc \
> -i vmx test-v2v-i-vmx-$i.vmx \
> --print-source > test-v2v-i-vmx-$i.actual
>
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
883
days inactive
887
days old
3 comments
2 participants
participants (2)
-
Laszlo Ersek -
Richard W.M. Jones