[PATCH storage] fix #6184: import: ovf: add CD drives

[PATCH storage] fix #6184: import: ovf: add CD drives

[Michal Fox]

The OVF parser only looks at disk drives, so the CD drives of a guest
are dropped on import. Appliances shipped as OVA sometimes come with
an ISO image in a CD drive, for example to install the guest on first
boot. Such a guest then does not boot after the import, and the user
is not told about the image at all.

Add the CD drives of the manifest as empty drives on their controller,
after the disks in the boot order, like the ESXi import does. As CD
images cannot be imported, report a referenced image with the existing
'cdrom-image-ignored' warning, so that the import wizard tells the user
to configure the drive in the 'Advanced' tab.

As the address only takes the position on the controller into account,
a CD drive can end up with the address of a disk, for example with a
disk on the first IDE controller and the CD drive on the second one.
Such a CD drive gets the next free address on its bus.

Signed-off-by: Michal Fox <me@dualfroz.com>
---
Tested with the OVF tests, extended by a manifest based on the one
attached to the bug report, and with a script that calls
get_import_metadata() for an OVF and an OVA with that manifest: the
CD drive is now part of the create arguments and the boot order, and
the image is reported as 'cdrom-image-ignored'. Without the fix, the
new checks fail. The pve-storage tests pass, except for the ZFS and
LVM plugin tests, which I could not run here.

'qm importovf' only takes the name, memory and cores from the parsed
configuration, so it is not affected.

This touches the same files as my patch for #5998 ("import: avoid
duplicate disk keys with multiple controllers"), which is not applied
yet. Both apply on their own, but applied together, the test hunk for
the boot order conflicts, and the expected boot order of the manifest
added there needs ';sata1' at the end, as it has a CD drive too.

 src/PVE/GuestImport/OVF.pm             |  43 ++++++++-
 src/PVE/Storage/DirPlugin.pm           |  10 +++
 src/test/ovf_manifests/cdrom-image.ovf | 116 +++++++++++++++++++++++++
 src/test/run_ovf_tests.pl              |  37 +++++++-
 4 files changed, 202 insertions(+), 4 deletions(-)
 create mode 100644 src/test/ovf_manifests/cdrom-image.ovf

diff --git a/src/PVE/GuestImport/OVF.pm b/src/PVE/GuestImport/OVF.pm
index a228cd2..f00c36a 100644
--- a/src/PVE/GuestImport/OVF.pm
+++ b/src/PVE/GuestImport/OVF.pm
@@ -385,7 +385,46 @@ ovf:Item[rasd:InstanceID='%s']/rasd:ResourceType", $controller_id,
         $net_count++;
     }
 
-    return { qm => $qm, disks => \@disks, net => $net };
+    # CD drives are added empty, as images cannot be imported, but the referenced image is kept
+    # so the user can be told about it
+    my $cdrom_id = dtmf_name_to_id('CD Drive');
+    my $xpath_find_cdroms =
+        "/ovf:Envelope/ovf:VirtualSystem/ovf:VirtualHardwareSection/ovf:Item[rasd:ResourceType=${cdrom_id}]";
+    my @cdrom_items = $xpc->findnodes($xpath_find_cdroms);
+
+    my $addresses_in_use = { map { $_->{disk_address} => 1 } @disks };
+    my @cdroms;
+    for my $item_node (@cdrom_items) {
+        my $controller_id = $xpc->findvalue('rasd:Parent', $item_node);
+        my $xpath_find_parent_type = sprintf(
+            "/ovf:Envelope/ovf:VirtualSystem/ovf:VirtualHardwareSection/\
+ovf:Item[rasd:InstanceID='%s']/rasd:ResourceType", $controller_id,
+        );
+        my $bus = id_to_pve($xpc->findvalue($xpath_find_parent_type));
+        if (!$bus) {
+            warn "invalid or missing controller for CD drive, skipping\n";
+            next;
+        }
+
+        my $index = $xpc->findvalue('rasd:AddressOnParent', $item_node);
+        $index = 0 if $index !~ m/^\d+$/;
+        $index++ while $addresses_in_use->{"$bus$index"};
+        my $address = "$bus$index";
+        $addresses_in_use->{$address} = 1;
+
+        my $image;
+        my $host_resource = $xpc->findvalue('rasd:HostResource', $item_node);
+        if ($host_resource =~ m|^(?:ovf:)?/file/([[:alnum:]\-\._~]+)$|) {
+            my $xpath_find_image =
+                sprintf("/ovf:Envelope/ovf:References/ovf:File[\@ovf:id='%s']/\@ovf:href", $1);
+            $image = $xpc->findvalue($xpath_find_image) || undef;
+        }
+
+        $qm->{$address} = 'none,media=cdrom';
+        push @cdroms, { disk_address => $address, image => $image };
+    }
+
+    return { qm => $qm, disks => \@disks, net => $net, cdroms => \@cdroms };
 }
 
 my sub resolve_disks {
@@ -436,6 +475,8 @@ my sub resolve_disks {
         push @$boot_order, $pve_disk_address;
     }
 
+    push @$boot_order, map { $_->{disk_address} } $qm->{cdroms}->@*;
+
     $qm->{qm}->{boot} = "order=" . join(';', @$boot_order) if scalar(@$boot_order) > 0;
 
     $qm->{disks} = \@disks;
diff --git a/src/PVE/Storage/DirPlugin.pm b/src/PVE/Storage/DirPlugin.pm
index 80c4a03..723a59a 100644
--- a/src/PVE/Storage/DirPlugin.pm
+++ b/src/PVE/Storage/DirPlugin.pm
@@ -299,6 +299,16 @@ sub get_import_metadata {
         };
     }
 
+    for my $cdrom ($res->{cdroms}->@*) {
+        next if !defined($cdrom->{image});
+        push @$warnings,
+            {
+                type => 'cdrom-image-ignored',
+                key => $cdrom->{disk_address},
+                value => $cdrom->{image},
+            };
+    }
+
     if (defined($res->{qm}->{bios}) && $res->{qm}->{bios} eq 'ovmf') {
         $disks->{efidisk0} = 1;
         push @$warnings, { type => 'efi-state-lost', key => 'bios', value => 'ovmf' };
diff --git a/src/test/ovf_manifests/cdrom-image.ovf b/src/test/ovf_manifests/cdrom-image.ovf
new file mode 100644
index 0000000..665461c
--- /dev/null
+++ b/src/test/ovf_manifests/cdrom-image.ovf
@@ -0,0 +1,116 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<Envelope vmw:buildId="build-348481" xmlns="http://schemas.dmtf.org/ovf/envelope/1" xmlns:cim="http://schemas.dmtf.org/wbem/wscim/1/common" xmlns:ovf="http://schemas.dmtf.org/ovf/envelope/1" xmlns:rasd="http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_ResourceAllocationSettingData" xmlns:vmw="http://www.vmware.com/schema/ovf" xmlns:vssd="http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_VirtualSystemSettingData" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
+  <References>
+    <File ovf:href="disk1.vmdk" ovf:id="file1" ovf:size="73216"/>
+    <File ovf:href="installer.iso" ovf:id="file2" ovf:size="650809344"/>
+    <File ovf:href="disk2.vmdk" ovf:id="file3" ovf:size="68096"/>
+  </References>
+  <DiskSection>
+    <Info>Virtual disk information</Info>
+    <Disk ovf:capacity="8" ovf:capacityAllocationUnits="byte * 2^30" ovf:diskId="vmdisk1" ovf:fileRef="file1" ovf:format="http://www.vmware.com/interfaces/specifications/vmdk.html#streamOptimized" ovf:populatedSize="293011456"/>
+    <Disk ovf:capacity="1" ovf:capacityAllocationUnits="byte * 2^30" ovf:diskId="vmdisk2" ovf:fileRef="file3" ovf:format="http://www.vmware.com/interfaces/specifications/vmdk.html#streamOptimized" ovf:populatedSize="0"/>
+  </DiskSection>
+  <NetworkSection>
+    <Info>The list of logical networks</Info>
+    <Network ovf:name="VM Network">
+      <Description>Service Port</Description>
+    </Network>
+  </NetworkSection>
+  <VirtualSystem ovf:id="cdrom-image">
+    <Info>A virtual machine</Info>
+    <Name>cdrom-image</Name>
+    <OperatingSystemSection ovf:id="102" vmw:osType="otherGuest64">
+      <Info>The kind of installed guest operating system</Info>
+    </OperatingSystemSection>
+    <VirtualHardwareSection ovf:transport="iso">
+      <Info>Virtual hardware requirements</Info>
+      <System>
+        <vssd:ElementName>Virtual Hardware Family</vssd:ElementName>
+        <vssd:InstanceID>0</vssd:InstanceID>
+        <vssd:VirtualSystemIdentifier>cdrom-image</vssd:VirtualSystemIdentifier>
+        <vssd:VirtualSystemType>vmx-07</vssd:VirtualSystemType>
+      </System>
+      <Item>
+        <rasd:AllocationUnits>hertz * 10^6</rasd:AllocationUnits>
+        <rasd:Description>Number of Virtual CPUs</rasd:Description>
+        <rasd:ElementName>1 virtual CPU(s)</rasd:ElementName>
+        <rasd:InstanceID>1</rasd:InstanceID>
+        <rasd:ResourceType>3</rasd:ResourceType>
+        <rasd:VirtualQuantity>1</rasd:VirtualQuantity>
+      </Item>
+      <Item>
+        <rasd:AllocationUnits>byte * 2^20</rasd:AllocationUnits>
+        <rasd:Description>Memory Size</rasd:Description>
+        <rasd:ElementName>2048MB of memory</rasd:ElementName>
+        <rasd:InstanceID>2</rasd:InstanceID>
+        <rasd:ResourceType>4</rasd:ResourceType>
+        <rasd:VirtualQuantity>2048</rasd:VirtualQuantity>
+      </Item>
+      <Item>
+        <rasd:Address>0</rasd:Address>
+        <rasd:Description>SCSI Controller</rasd:Description>
+        <rasd:ElementName>SCSI Controller 0</rasd:ElementName>
+        <rasd:InstanceID>3</rasd:InstanceID>
+        <rasd:ResourceSubType>lsilogic</rasd:ResourceSubType>
+        <rasd:ResourceType>6</rasd:ResourceType>
+      </Item>
+      <Item>
+        <rasd:Address>1</rasd:Address>
+        <rasd:Description>IDE Controller</rasd:Description>
+        <rasd:ElementName>VirtualIDEController 1</rasd:ElementName>
+        <rasd:InstanceID>4</rasd:InstanceID>
+        <rasd:ResourceType>5</rasd:ResourceType>
+      </Item>
+      <Item>
+        <rasd:Address>0</rasd:Address>
+        <rasd:Description>IDE Controller</rasd:Description>
+        <rasd:ElementName>VirtualIDEController 0</rasd:ElementName>
+        <rasd:InstanceID>5</rasd:InstanceID>
+        <rasd:ResourceType>5</rasd:ResourceType>
+      </Item>
+      <Item>
+        <rasd:AddressOnParent>0</rasd:AddressOnParent>
+        <rasd:ElementName>Hard Disk 1</rasd:ElementName>
+        <rasd:HostResource>ovf:/disk/vmdisk1</rasd:HostResource>
+        <rasd:InstanceID>6</rasd:InstanceID>
+        <rasd:Parent>3</rasd:Parent>
+        <rasd:ResourceType>17</rasd:ResourceType>
+      </Item>
+      <Item>
+        <rasd:AddressOnParent>0</rasd:AddressOnParent>
+        <rasd:ElementName>Hard Disk 2</rasd:ElementName>
+        <rasd:HostResource>ovf:/disk/vmdisk2</rasd:HostResource>
+        <rasd:InstanceID>7</rasd:InstanceID>
+        <rasd:Parent>5</rasd:Parent>
+        <rasd:ResourceType>17</rasd:ResourceType>
+      </Item>
+      <Item ovf:required="false">
+        <rasd:AddressOnParent>0</rasd:AddressOnParent>
+        <rasd:AutomaticAllocation>false</rasd:AutomaticAllocation>
+        <rasd:Description>Floppy Drive</rasd:Description>
+        <rasd:ElementName>Floppy 1</rasd:ElementName>
+        <rasd:InstanceID>8</rasd:InstanceID>
+        <rasd:ResourceType>14</rasd:ResourceType>
+      </Item>
+      <Item>
+        <rasd:AddressOnParent>0</rasd:AddressOnParent>
+        <rasd:AutomaticAllocation>true</rasd:AutomaticAllocation>
+        <rasd:ElementName>CD-ROM 1</rasd:ElementName>
+        <rasd:HostResource>ovf:/file/file2</rasd:HostResource>
+        <rasd:InstanceID>9</rasd:InstanceID>
+        <rasd:Parent>4</rasd:Parent>
+        <rasd:ResourceType>15</rasd:ResourceType>
+      </Item>
+      <Item>
+        <rasd:AddressOnParent>9</rasd:AddressOnParent>
+        <rasd:AutomaticAllocation>true</rasd:AutomaticAllocation>
+        <rasd:Connection>VM Network</rasd:Connection>
+        <rasd:Description>E1000 ethernet adapter on &quot;VM Network&quot;</rasd:Description>
+        <rasd:ElementName>Ethernet 0</rasd:ElementName>
+        <rasd:InstanceID>10</rasd:InstanceID>
+        <rasd:ResourceSubType>E1000</rasd:ResourceSubType>
+        <rasd:ResourceType>10</rasd:ResourceType>
+      </Item>
+    </VirtualHardwareSection>
+  </VirtualSystem>
+</Envelope>
diff --git a/src/test/run_ovf_tests.pl b/src/test/run_ovf_tests.pl
index 23f0aa6..1215882 100755
--- a/src/test/run_ovf_tests.pl
+++ b/src/test/run_ovf_tests.pl
@@ -45,6 +45,14 @@ if (my $err = $@) {
     fail('parsing XEE.ovf should have failed!');
 }
 
+my $cdromImage = eval { PVE::GuestImport::OVF::parse_ovf("$test_manifests/cdrom-image.ovf") };
+if (my $err = $@) {
+    fail('parse cdrom image');
+    warn("error: $err\n");
+} else {
+    ok('parse cdrom image');
+}
+
 print "testing disks\n";
 
 is(
@@ -101,22 +109,43 @@ is($win10->{net}->{net0}->{model}, 'e1000e', 'win10 has correct nic model');
 is($win10noNs->{net}->{net0}->{model}, 'e1000e',
     'win10 (no default rasd NS) has correct nic model');
 
+print "testing cdroms\n";
+is_deeply(
+    $win2008->{cdroms},
+    [{ disk_address => 'sata1', image => undef }],
+    'win2008 has an empty cdrom',
+);
+is($win2008->{qm}->{sata1}, 'none,media=cdrom', 'win2008 cdrom is added to the config');
+is_deeply(
+    $cdromImage->{cdroms},
+    [{ disk_address => 'ide1', image => 'installer.iso' }],
+    'cdrom with image gets the next free address on its bus',
+);
+is($cdromImage->{qm}->{ide1}, 'none,media=cdrom', 'cdrom with image is added empty to the config');
+is_deeply(
+    [map { $_->{disk_address} } $cdromImage->{disks}->@*],
+    ['scsi0', 'ide0'],
+    'cdrom with image vm has the correct disks',
+);
+
 print "\ntesting vm.conf extraction\n";
 
-is($win2008->{qm}->{boot}, 'order=scsi0;scsi1', 'win2008 VM boot is correct');
+is($win2008->{qm}->{boot}, 'order=scsi0;scsi1;sata1', 'win2008 VM boot is correct');
 is($win2008->{qm}->{name}, 'Win2008-R2x64', 'win2008 VM name is correct');
 is($win2008->{qm}->{memory}, '2048', 'win2008 VM memory is correct');
 is($win2008->{qm}->{cores}, '1', 'win2008 VM cores are correct');
 is($win2008->{qm}->{ostype}, 'win7', 'win2008 VM ostype is correcty');
 
-is($win10->{qm}->{boot}, 'order=scsi0', 'win10 VM boot is correct');
+is($win10->{qm}->{boot}, 'order=scsi0;sata1', 'win10 VM boot is correct');
 is($win10->{qm}->{name}, 'Win10-Liz', 'win10 VM name is correct');
 is($win10->{qm}->{memory}, '6144', 'win10 VM memory is correct');
 is($win10->{qm}->{cores}, '4', 'win10 VM cores are correct');
 # older esxi/ovf standard used 'other' for windows10
 is($win10->{qm}->{ostype}, 'other', 'win10 VM ostype is correct');
 
-is($win10noNs->{qm}->{boot}, 'order=scsi0', 'win10 VM (no default rasd NS) boot is correct');
+is(
+    $win10noNs->{qm}->{boot}, 'order=scsi0;sata1', 'win10 VM (no default rasd NS) boot is correct',
+);
 is($win10noNs->{qm}->{name}, 'Win10-Liz', 'win10 VM (no default rasd NS) name is correct');
 is($win10noNs->{qm}->{memory}, '6144', 'win10 VM (no default rasd NS) memory is correct');
 is($win10noNs->{qm}->{cores}, '4', 'win10 VM (no default rasd NS) cores are correct');
@@ -124,4 +153,6 @@ is($win10noNs->{qm}->{cores}, '4', 'win10 VM (no default rasd NS) cores are corr
 is($win10noNs->{qm}->{ostype}, 'other', 'win10 VM (no default rasd NS) ostype is correct');
 is($win10noNs->{qm}->{bios}, 'ovmf', 'win10 VM (no default rasd NS) bios is correct');
 
+is($cdromImage->{qm}->{boot}, 'order=scsi0;ide0;ide1', 'cdrom with image VM boot is correct');
+
 done_testing();
-- 
2.43.0