On Sat, Nov 13, 2021 at 4:49 PM Laszlo Ersek <lersek@redhat.com> wrote:

Hi!

Please make your email reader's window as wide as possible, for reading this email.

This email is long, but it does contain an actual question. Watch out for the question mark please.

I'm almost ready to post version 1 of the virt-v2v patch set for RHBZ#1961107. The purpose of this BZ is that virt-v2v generate a standard VGA video controller in *all* output (= converted) domains, replacing both Cirrus and QXL. The argument for VGA is captured in both the BZ and the commit messages of my patches.

The missing piece is how to populate the OVF xml fragment that oVirt gets from virt-v2v.

(0) For reference, this is the virt-v2v snippet that produces the fragment (file "lib/create_ovf.ml" at commit ab55f9432e77):

678 (* We always add a qxl device when outputting to RHV. 679 * See RHBZ#1213701 and RHBZ#1211231 for the reasoning 680 * behind that. 681 *) 682 let qxl_resourcetype = 683 match ovf_flavour with 684 | OVirt -> 32768 (* RHBZ#1598715 *) 685 | RHVExportStorageDomain -> 20 in 686 e "Item" [] [ 687 e "rasd:Caption" [] [PCData "Graphical Controller"]; 688 e "rasd:InstanceId" [] [PCData (uuidgen ())]; 689 e "rasd:ResourceType" [] [PCData (string_of_int qxl_resourcetype)]; 690 e "Type" [] [PCData "video"]; 691 e "rasd:VirtualQuantity" [] [PCData "1"]; 692 e "rasd:Device" [] [PCData "qxl"]; 693 ]

Note that the *only* reference to QXL is on the last line, in the <rasd:Device> element.

The "qxl_resourcetype" variable *looks* like it is related to QXL; in fact, it is not. It is instead the generic identifier for the *monitor* (not video controller) resource type in oVirt. In oVirt, there is some compat stuff around this (more on that later); for now, suffice it to say that the values 32768 and 20 are *both* independent of QXL (and standard VGA).

The OVF fragment is parsed by oVirt in the following code snippets. This is my first encounter with the ovirt-engine repository, so please bear with me.

- Repository: https://github.com/oVirt/ovirt-engine.git - Branch: master (at commit daca2ca6cd91)

(1) In file

"backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/VmDeviceType.java", we have the following enum definitions (excerpt):

3 public enum VmDeviceType { 4 FLOPPY("floppy", "14"), 5 DISK("disk", "17"), 6 LUN("lun"), 7 CDROM("cdrom", "15"), 8 INTERFACE("interface", "10"), 9 BRIDGE("bridge", "3"), 10 VIDEO("video", "20"), 11 USB("usb", "23"), 12 CONTROLLER("controller", "23"), 13 REDIR("redir", "23"), 14 SPICEVMC("spicevmc", "23"), 15 QXL("qxl"), 16 CIRRUS("cirrus"), 17 VGA("vga"), 18 SPICE("spice"), 19 VNC("vnc"), 20 SOUND("sound"), 21 ICH6("ich6"), 22 AC97("ac97"), 23 MEMBALLOON("memballoon"), 24 CHANNEL("channel"), 25 SMARTCARD("smartcard"), 26 BALLOON("balloon"), 27 CONSOLE("console"), 28 VIRTIO("virtio"), 29 WATCHDOG("watchdog"), 30 VIRTIOSCSI("virtio-scsi"), 31 VIRTIOSERIAL("virtio-serial"), 32 HOST_DEVICE("hostdev"), 33 MEMORY("memory"), 34 PCI("pci"), 35 IDE("ide"), 36 SATA("sata"), 37 ICH9("ich9"), 38 TPM("tpm"), 39 BOCHS("bochs"), 40 OTHER("other", "0"), 41 UNKNOWN("unknown", "-1"); 42 43 private String name; 44 private String ovfResourceType; 45 46 VmDeviceType(String name) { 47 this.name = name; 48 } 49 50 VmDeviceType(String name, String ovfResourceType) { 51 this.name = name; 52 this.ovfResourceType = ovfResourceType; 53 } 54 55 public String getName() { 56 return name; 57 } 58 59 /** 60 * This method maps OVF Resource Types to oVirt devices. 61 */ 62 public static VmDeviceType getoVirtDevice(int resourceType) { 63 for (VmDeviceType deviceType : values()) { 64 if (deviceType.ovfResourceType != null && Integer.parseInt(deviceType.ovfResourceType) == resourceType) { 65 return deviceType; 66 } 67 } 68 return UNKNOWN; 69 }

(2) In file

"backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DisplayType.java", we have the following enum definitions (excerpt):

5 public enum DisplayType { 6 @Deprecated 7 cirrus(VmDeviceType.CIRRUS), 8 qxl(VmDeviceType.QXL), 9 vga(VmDeviceType.VGA), 10 bochs(VmDeviceType.BOCHS), 11 none(null); // For Headless VM, this type means that the VM will run without any display (VIDEO) device

(3) In file

"backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfHardware.java", we have the following resource type constants (excerpt):

10 /** The OVF specification does not include Monitor devices, 11 * so we use a constant that is supposed to be unused */ 12 public static final String Monitor = "32768"; 13 /** We must keep using '20' for oVirt's OVFs for backward/forward compatibility */ 14 public static final String OVIRT_Monitor = "20";

This is related to my above note about "qxl_resourcetype" in the virt-v2v OCaml source code.

And:

16 public static final String Graphics = "24"; 17 /** In the OVF specification 24 should be used for Graphic devices, but we 18 * must keep using '26' for oVirt's OVFs for backward/forward compatibility */ 19 public static final String OVIRT_Graphics = "26";

However, this resource type seems mostly unused in oVirt, and therefore irrelevant for this discussion. I'm including these enum constants just so I can point that out!

(4) In file

"backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfOvirtReader.java", we have the following override (= specialization) logic:

365 protected String adjustHardwareResourceType(String resourceType) { 366 switch(resourceType) { 367 case OvfHardware.OVIRT_Monitor: 368 return OvfHardware.Monitor;

This is what implements the compatibility resource type mapping discussed above. Again, it is *unrelated* to QXL vs. VGA.

369 case OvfHardware.OVIRT_Graphics: 370 return OvfHardware.Graphics;

And, again, this is irrelevant here; including it just for completeness.

371 default: 372 return super.adjustHardwareResourceType(resourceType); 373 } 374 }

(5) In file

"backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfProperties.java", we many (symbolic) constants for various XML element names in the OVF format; such as (excerpt):

5 String VMD_DEVICE = "Device"; 6 String VMD_TYPE = "Type";

12 String VMD_RESOURCE_TYPE = "rasd:ResourceType"; 13 String VMD_SUB_RESOURCE_TYPE = "rasd:ResourceSubType"; 14 String VMD_VIRTUAL_QUANTITY = "rasd:VirtualQuantity";

22 String VMD_ID = "rasd:InstanceId";

Note that I selected mostly those that cover the OVF snippet generated by virt-v2v, with the code quoted at the top, under bullet (0).

(6) The main oVirt logic that's relevant to us now is in file

"backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfReader.java":

851 private void setDeviceByResource(XmlNode node, VmDevice vmDevice) { 852 String resourceType = selectSingleNode(node, VMD_RESOURCE_TYPE, _xmlNS).innerText; 853 XmlNode resourceSubTypeNode = selectSingleNode(node, VMD_SUB_RESOURCE_TYPE, _xmlNS); 854 if (resourceSubTypeNode == null) { 855 // we need special handling for Monitor to define it as vnc or spice 856 if (OvfHardware.Monitor.equals(adjustHardwareResourceType(resourceType))) { 857 // get number of monitors from VirtualQuantity in OVF 858 if (selectSingleNode(node, VMD_VIRTUAL_QUANTITY, _xmlNS) != null 859 && !StringUtils.isEmpty(selectSingleNode(node, 860 VMD_VIRTUAL_QUANTITY, 861 _xmlNS).innerText)) { 862 int virtualQuantity = 863 Integer.parseInt( 864 selectSingleNode(node, VMD_VIRTUAL_QUANTITY, _xmlNS).innerText); 865 if (virtualQuantity > 1) { 866 vmDevice.setDevice(VmDeviceType.QXL.getName()); 867 } else { 868 // get first supported display device 869 List<Pair<GraphicsType, DisplayType>> supportedGraphicsAndDisplays = 870 osRepository.getGraphicsAndDisplays(vmBase.getOsId(), new Version(getVersion())); 871 if (!supportedGraphicsAndDisplays.isEmpty()) { 872 DisplayType firstDisplayType = supportedGraphicsAndDisplays.get(0).getSecond(); 873 vmDevice.setDevice(firstDisplayType.getDefaultVmDeviceType().getName()); 874 } else { 875 vmDevice.setDevice(VmDeviceType.QXL.getName()); 876 } 877 } 878 } else { // default to spice if quantity not found 879 vmDevice.setDevice(VmDeviceType.QXL.getName()); 880 } 881 } else { 882 vmDevice.setDevice(VmDeviceType.getoVirtDevice(Integer.parseInt(resourceType)).getName()); 883 } 884 } else if (OvfHardware.Network.equals(resourceType)) {

Here's what it does, in English (as far as I can tell):

- If the <rasd:ResourceSubType> element is *present*, then we move to line 884. Not interesting for our case.

- On line 856, the monitor resource type adjustment occurs that I described in bullets (3) and (4). What I want to point out here is that the snippet that virt-v2v generates satisfies this -- in other words, virt-v2v provides a "monitor" resource --, but otherwise it has no bearing on QXL vs. VGA.

- Next, the <rasd:VirtualQuantity> element is consulted. If this element does not exist, or is empty, then oVirt picks QXL (line 879). If the number of displays is larger than 1 (= multi-head setup), the same happens (line 866). In our case, neither case matches, as virt-v2v generates exactly 1 display head (see bullet (0), line 691). So further conditions are checked, below.

- Next, if "OS info" knows anything about the converted OS's supported video controllers, then the first such controller is picked (line 873). Otherwise, oVirt picks QXL (line 875).

- Importantly, line 882 would be reachable if virt-v2v *did not* provide a "monitor" resource at all. However, even in that case, we could not express VGA. That's because the getoVirtDevice() method -- see bullet (1), line 62 -- would have to locate the VGA entry of enum VmDeviceType, and that entry's "ovfResourceType" field is null. See line 17 under bullet (1).

Ultimately, the "rasd:Device" (VMD_DEVICE) element generated by virt-v2v, with contents "qxl" (see bullet (0) line 692), plays *no role* at all, and the current oVirt logic does not allow virt-v2v to choose VGA in any way.

The "qxl_resourcetype" selection in virt-v2v only activates the "monitor" resource handling, but does not affect the video controller.

Nice analysis :)

Here's my proposal (or more precisely, request):

Can the oVirt developers please enhance the setDeviceByResource() method in "OvfReader.java" such that it parse VMD_DEVICE as well, and if that one says "vga", then the method please pick "VmDeviceType.VGA"?

Added Liran that works on dropping QXL on our end (for all guests on latest cluster version of oVirt 4.5 - https://bugzilla.redhat.com/show_bug.cgi?id=1976607, and for RHEL 9 guests on RHV - https://bugzilla.redhat.com/show_bug.cgi?id=1976605) It's tempting to say that this change in virt-v2v only affects oVirt (since RHEL 9 won't be supported for RHV hosts) but with the rhv-upload option in virt-v2v it might happen (though less likely) that also RHV customers will run virt-v2v on RHEL 9 Liran, 1. The proposed change makes sense to me, can you please add that to BZ 1976607? 2. Please take a look at the following comment on the bug Laszlo mentioned above, it's also related to our changes: https://bugzilla.redhat.com/show_bug.cgi?id=1961107#c2

Because, in this case, the virt-v2v change for me would be relatively easy:

- I'd rename the "qxl_resourcetype" variable to "monitor_resourcetype" -- this misnomer should in fact be fixed regardless of the QXL->VGA replacement;

- I'd change the contents of the <rasd:Device> element from "qxl" to "vga", and remove the QXL-related comment too.

(This would likely mean the insertion of two small patches into my virt-v2v series.)

Thank you, Laszlo