Add missing DisplayType values #102

dupondje · 2023-05-26T12:27:21Z

In org.ovirt.engine.core.common.businessentities.DisplayType we have the supported DisplayType's. The API Model DisplayType values do not match these values.

The ovirt-engine code contains code to map VNC and SPICE DisplayType from the API to the businessentities.DisplayType values, but never were the real values added to the API Model.

In this commit we deprecate the 'legacy' DisplayType's in favor of the real DisplayType's.

Change-Id: Ibc9d7cbec88effd62172983da6ab705160ad8879

sandrobonazzola · 2023-07-12T13:03:36Z

@dupondje can you move the CI fix to its own PR? Looks like the build is not running at all

dupondje · 2023-07-12T13:11:06Z

Done: #103

In org.ovirt.engine.core.common.businessentities.DisplayType we have the supported DisplayType's. The API Model DisplayType values do not match these values. The ovirt-engine code contains code to map VNC and SPICE DisplayType from the API to the businessentities.DisplayType values, but never were the real values added to the API Model. In this commit we deprecate the 'legacy' DisplayType's in favor of the real DisplayType's. Signed-off-by: Jean-Louis Dupond <[email protected]> Change-Id: Ibc9d7cbec88effd62172983da6ab705160ad8879

mwperina

It looks good to me, but I don't have the expertise on those VM specific details

dupondje · 2023-07-12T13:58:42Z

This was needed to bulk-change a bunch of VM's to VGA-VNC prior to migration to CentOS 9 (Where QXL/SPICE is removed)

mz-pdm

As for the list of the graphics devices, it looks good to me.

ahadas · 2023-07-12T16:40:32Z

src/main/java/types/DisplayType.java

@@ -39,6 +39,7 @@ public enum DisplayType {
     * @date 23 Apr 2017
     * @status added
     */
+    @Deprecated


why annotating VNC as deprecated?

Because VNC is a GraphicsType, and not a DisplayType. This was used before the introduction of GraphicsType. But as we have both now, we deprecate the DisplayType.VNC as you need to use GraphicsType.VNC.

so let's deprecate DisplayType entirely then..

Well not at this moment I think. On CentOS 8 you would like to have DisplayType.QXL with GraphicsType.VNC for example. Like you can set both in the UI at this moment.

And for headless you might want to return DisplayType.NONE also.

So I wouldn't deprecate DisplayType ..

And for headless you might want to return DisplayType.NONE also.

or keep the current design - you can achieve this by removing the current video devices

And for headless you might want to return DisplayType.NONE also.

or keep the current design - you can achieve this by removing the current video devices

It's not a requirement as far as I see to have DisplayType.NONE indeed.
But does it hurt to have an additional way to make a VM headless by setting the DisplayType to NONE?

ahadas · 2023-07-12T16:41:16Z

src/main/java/types/DisplayType.java

+     * @status added
+     */
+    @Deprecated
+    CIRRUS,


you're mixing video devices and graphic devices here - we intentionally didn't add the video devices to the API because they are represented differently

Back in the early days, oVirt only had DisplayType.
But then the GraphicsType was added: oVirt/ovirt-engine@9cdd6f8

So now oVirt Engine itself has both types:

The DisplayType (QXL/VGA/BOCHS/etc)
https://github.com/oVirt/ovirt-engine/blob/master/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DisplayType.java

And the GraphicsType (VNC/SPICE)
https://github.com/oVirt/ovirt-engine/blob/master/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/GraphicsType.java

But for some reason, you can't set the real DisplayType from the REST API.
While you can just select it from the UI.

So we need to add the real DisplayTypes to the model, so we can then allow the REST API code to set the DisplayType.
Cause without this, you can't set your VM to VGA-VNC for example, as it will always prefer QXL if you pass DisplayType.VNC to the API.

So we need to add the real DisplayTypes to the model, so we can then allow the REST API code to set the DisplayType. Cause without this, you can't set your VM to VGA-VNC for example, as it will always prefer QXL if you pass DisplayType.VNC to the API.

can you please elaborate on that? it should have been handled by making VGA the default video type - what cluster version do you use? how the blank template looks like in terms of video and graphic devices? and how do you provision the vm (from scratch, i.e., from the blank template or from an existing template - in case of the latter, how that template is set in terms of video and graphic devices?)

So we need to add the real DisplayTypes to the model, so we can then allow the REST API code to set the DisplayType. Cause without this, you can't set your VM to VGA-VNC for example, as it will always prefer QXL if you pass DisplayType.VNC to the API.

can you please elaborate on that? it should have been handled by making VGA the default video type

ah no, that's not the right one - @liranr23 do you remember how we managed to set the display type properly? was it by changing the osinfo?

This was not about new VM's.
We had a cluster with +500 VM's on CentOS 8 hypervisors. All QXL - VNC/SPICE.

But to upgrade to CentOS 9, all VM's had to be changed to VGA - VNC (as QXL is gone in CentOS 9).

So when trying to do that, we've noticed there was no way to set this, as there is no 'VGA' DisplayType in the API. You could use DisplayType.VNC, but it preferred QXL over VGA, and didn't change the display to VGA like we needed.

@dupondje yeah, there's no VGA type in DisplayType since DisplayType should not have been used
let me get back to you about how the process should have been done - I don't remember this from the top of my head, I agree that it's not intuitive but I remember we had a good reason for it

So we need to add the real DisplayTypes to the model, so we can then allow the REST API code to set the DisplayType. Cause without this, you can't set your VM to VGA-VNC for example, as it will always prefer QXL if you pass DisplayType.VNC to the API.

can you please elaborate on that? it should have been handled by making VGA the default video type

ah no, that's not the right one - @liranr23 do you remember how we managed to set the display type properly? was it by changing the osinfo?

hard to recall. i think we change osinfo. but maybe we did something also on AsyncDataProvider/UnitVmModel or elsewhere, where we "preferred" something.

https://github.com/oVirt/ovirt-engine/blob/master/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/util/DisplayHelper.java#L129

Here we set the DisplayType depending on the GraphicsType. With a preference for SPICE.
See the note: https://github.com/oVirt/ovirt-engine/blob/master/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/util/DisplayHelper.java#L108

In the UI you can just set the DisplayType, so why would we not want to have the functionality in the API?

ahadas · 2023-07-12T16:42:01Z

src/main/java/types/DisplayType.java

+     * @date 26 May 2023
+     * @status added
+     */
+    NONE;


we had a long thread about this one back then - this couldn't work for specifying that we want a headless vm, we rather need to create the vm and then remove its video devices

This is how it's represented now:
https://github.com/oVirt/ovirt-engine/blob/master/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DisplayType.java#L12

I think this should be fixable from the engine code.
Thing is that before this PR (and the ovirt-engine PR for this change), you couldn't set the DisplayType with the API, the engine code derived the DisplayType from the GraphicsType you passed.

@sgratch is there a chance you can find the discussion we had with Juan back then?

@dupondje @ahadas @liranr23
Trying to restore my memory regarding the discussions and changes for supporting video device types and headless VMs via the rest API ended up with the following:

Juan pushed to a solution of adding a VM 'headless' and a 'video type' attributes for supporting all combinations of video/graphics/headless via the API and we came with a long design thread for adding both, but at the end we decided not to invest on this and stayed with a partial API support https://bugzilla.redhat.com/show_bug.cgi?id=1406394

The solution we ended up with, was based on the old code, as described here: https://github.com/sgratch/ovirt-site/blob/master/source/develop/release-management/features/virt/headless-vm.html.md#rest-api, and included the following main concepts:

We can only set the graphics device type via the API.

The video device type logic is set on backend, based on supported OS combinations of video/graphics

For headless, we should remove all graphics console devices. This will set the video type to DisplayType.none on backend.

For deprecating QXL and replacing it with VGA as the default video device for exising VMs, based on what I understand from https://bugzilla.redhat.com/show_bug.cgi?id=1976607, is that the user is recommended to remove the graphic and video devices from the VM (creating a headless VM) and then adding a VNC graphic device. This can be done via REST in 2 phases of setting to headless and then setting to VNC.

The current API Model DisplayType values represents the graphics info. For supporting and setting the video display type as well, in addition to graphics type , you should ,as a 1st step, to add a new attribute to the VMs general info in addition to the graphics, as part of the Display attribute or as a new attribute under the VM info. Please note that backward compatibility should be kept so you can't just change an existing attribute values.

I remember we decided to have 2 phases to set the VM headless.
Based on packaging/dbscripts/upgrade/04_05_0290_change_default_display.sql , we change the default only for new VMs. I don't think we had a method to existing, other than defaulting in the UI to VGA. Therefore, for API (DisplayMapper), - yes I think you can switch to headless and then setting to VNC.
@sgratch point makes sense to me.

dupondje · 2023-10-19T12:58:14Z

Any chance this can get merged/reviewed? :)

sgratch · 2023-11-07T11:55:43Z

Any chance this can get merged/reviewed? :)

Please check my comment above #102 (comment)

dupondje requested a review from oliel as a code owner May 26, 2023 12:27

dupondje mentioned this pull request May 26, 2023

Allow setting all DisplayType's via API oVirt/ovirt-engine#857

Open

dupondje force-pushed the fix_displaytype branch from 7ac7628 to 358ee11 Compare May 26, 2023 13:00

sandrobonazzola force-pushed the fix_displaytype branch from 358ee11 to 24dbae8 Compare July 12, 2023 13:44

mwperina requested review from mz-pdm and ahadas July 12, 2023 13:55

mwperina reviewed Jul 12, 2023

View reviewed changes

mz-pdm approved these changes Jul 12, 2023

View reviewed changes

ahadas requested changes Jul 12, 2023

View reviewed changes

sandrobonazzola assigned dupondje May 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing DisplayType values #102

Add missing DisplayType values #102

dupondje commented May 26, 2023

sandrobonazzola commented Jul 12, 2023

dupondje commented Jul 12, 2023

mwperina left a comment

dupondje commented Jul 12, 2023

mz-pdm left a comment

ahadas Jul 12, 2023

dupondje Jul 13, 2023

ahadas Jul 13, 2023

dupondje Jul 13, 2023

ahadas Jul 13, 2023

dupondje Jul 13, 2023

ahadas Jul 12, 2023

dupondje Jul 13, 2023

ahadas Jul 13, 2023

ahadas Jul 13, 2023

dupondje Jul 13, 2023

ahadas Jul 13, 2023 •

edited

Loading

liranr23 Jul 26, 2023

dupondje Aug 21, 2023 •

edited

Loading

ahadas Jul 12, 2023 •

edited

Loading

dupondje Jul 13, 2023

ahadas Jul 13, 2023

sgratch Nov 7, 2023 •

edited

Loading

liranr23 Nov 8, 2023

dupondje commented Oct 19, 2023

sgratch commented Nov 7, 2023

Add missing DisplayType values #102

Are you sure you want to change the base?

Add missing DisplayType values #102

Conversation

dupondje commented May 26, 2023

sandrobonazzola commented Jul 12, 2023

dupondje commented Jul 12, 2023

mwperina left a comment

Choose a reason for hiding this comment

dupondje commented Jul 12, 2023

mz-pdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahadas Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dupondje Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

ahadas Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgratch Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dupondje commented Oct 19, 2023

sgratch commented Nov 7, 2023

ahadas Jul 13, 2023 •

edited

Loading

dupondje Aug 21, 2023 •

edited

Loading

ahadas Jul 12, 2023 •

edited

Loading

sgratch Nov 7, 2023 •

edited

Loading