On Sat, Nov 13, 2021 at 4:49 PM Laszlo Ersek <[email protected]> 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 > >
_______________________________________________ Libguestfs mailing list [email protected] https://listman.redhat.com/mailman/listinfo/libguestfs
