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