Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add missing DisplayType values #102

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add missing DisplayType values #102

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 46 additions & 1 deletion src/main/java/types/DisplayType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ public enum DisplayType {
* @date 23 Apr 2017
* @status added
*/
@Deprecated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why annotating VNC as deprecated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so let's deprecate DisplayType entirely then..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

VNC,

/**
Expand All @@ -49,5 +50,49 @@ public enum DisplayType {
* @date 23 Apr 2017
* @status added
*/
SPICE;
@Deprecated
SPICE,

/**
* Display of type CIRRUS
* @author Jean-Louis Dupond <[email protected]>
* @date 26 May 2023
* @status added
*/
@Deprecated
CIRRUS,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ahadas ahadas Jul 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@dupondje dupondje Aug 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


/**
* Display of type QXL
* @author Jean-Louis Dupond <[email protected]>
* @date 26 May 2023
* @status added
*/
@Deprecated
QXL,

/**
* Display of type VGA
* @author Jean-Louis Dupond <[email protected]>
* @date 26 May 2023
* @status added
*/
VGA,

/**
* Display of type BOCHS
* @author Jean-Louis Dupond <[email protected]>
* @date 26 May 2023
* @status added
*/
BOCHS,

/**
* Display of type NONE
* This is for headless VM's
* @author Jean-Louis Dupond <[email protected]>
* @date 26 May 2023
* @status added
*/
NONE;
Copy link
Member

@ahadas ahadas Jul 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@sgratch sgratch Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

  1. 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

  2. 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.

  3. 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.

  4. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}