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.

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

UI: Default VGA when backing from headless #470

Merged
merged 1 commit into from
Jun 21, 2022

Conversation

liranr23
Copy link
Member

When switching back from headless VM, the default is SPICE/QXL.
This patch changes it to VNC/VGA.

Change-Id: Ic63dbf22e3cd199fc782dc8e8963c8899478a6ae
Signed-off-by: Liran Rotenberg lrotenbe@redhat.com

@ljelinkova
Copy link
Contributor

I am starting to be confused about our defaults - is the new default VGA + VNC or Bochs + VNC? Does it depend on the VM type and compatibility version? We are handling the defaults on too many places in the UI..

@michalskrivanek
Copy link
Member

AFAICT - currently it's supposed to be VNC/VGA. But with https://bugzilla.redhat.com/1936163 it did become more complicated and IIUC it should be VNC/Bochs for Server+UEFI instead.

@ahadas
Copy link
Member

ahadas commented Jun 17, 2022

AFAICT - currently it's supposed to be VNC/VGA. But with https://bugzilla.redhat.com/1936163 it did become more complicated and IIUC it should be VNC/Bochs for Server+UEFI instead.

as I filed that bug I can explain what it meant.
it doesn't mean that every vm that is of type server and has UEFI firmware should be set with bochs. if you have such a vm that is based on template with a different video device, we should default to the video type of the template. if it's a new vm, then the initial settings in the dialog, including the video device, should be set according to the blank template. I swear I didn't mean to change those mechanisms we have for years.
what I meant is that when it comes to vm type, that essentially represents some predefined settings associated with each value, then the 'server' type should set bochs by default for a vm with UEFI firmware. as far as I can tell we do that now so I'm pleased with the current state

@michalskrivanek
Copy link
Member

but then what was the goal of that bug? Every VM creation in oVirt is based on a template. There's no way how to create a VM entirely from scratch, the only "defaulting" we have is the Blank template, and that's also why it's treated as special, cluster independent, hardcoded uuid, etc. The "Optimized for" also comes from the template.

So where would we use that Bochs default, in which flow?

When switching back from headless VM, the default is SPICE/QXL.
This patch changes it to VNC/VGA.

Change-Id: Ic63dbf22e3cd199fc782dc8e8963c8899478a6ae
Signed-off-by: Liran Rotenberg <lrotenbe@redhat.com>
@liranr23
Copy link
Member Author

but then what was the goal of that bug? Every VM creation in oVirt is based on a template. There's no way how to create a VM entirely from scratch, the only "defaulting" we have is the Blank template, and that's also why it's treated as special, cluster independent, hardcoded uuid, etc. The "Optimized for" also comes from the template.

So where would we use that Bochs default, in which flow?

Well, there is no bug for it to start with :) , to me it was missing out from the switch to default VNC.
from my testing of this PR, when the VM is headless and it's UEFI, server, by switching it to have console the UI will have Bochs (well of course if it's existing in the relevant place in osinfo).

@michalskrivanek
Copy link
Member

Well, there is no bug for it to start with :) , to me it was missing out from the switch to default VNC. from my testing of this PR, when the VM is headless and it's UEFI, server, by switching it to have console the UI will have Bochs (well of course if it's existing in the relevant place in osinfo).

So that's the inconsistency - Why does it select Bochs when you switch (e.g from headless back to console) and it doesn't when you open the Create VM dialog for the first time, UEFI+Server is preselected.

@liranr23
Copy link
Member Author

Well, there is no bug for it to start with :) , to me it was missing out from the switch to default VNC. from my testing of this PR, when the VM is headless and it's UEFI, server, by switching it to have console the UI will have Bochs (well of course if it's existing in the relevant place in osinfo).

So that's the inconsistency - Why does it select Bochs when you switch (e.g from headless back to console) and it doesn't when you open the Create VM dialog for the first time, UEFI+Server is preselected.

Moving from headless to console doesn't seem to respect the template, now or before. e.g. before this PR even with the blank having the VNC as default video device, it will result you in QXL/SPICE. When it's UEFI+Server you will get Bochs.

@smelamud
Copy link
Member

So that's the inconsistency - Why does it select Bochs when you switch (e.g from headless back to console) and it doesn't when you open the Create VM dialog for the first time, UEFI+Server is preselected.

If there is a conflict between a user's choice and a default, we prefer user's choice. Otherwise, we can use a default that fits better.

In the case of Create VM, when Blank template and Server are preselected, there is no user's choice here, so we prefer Bochs.

In the case of creating a VM from a template, the template is the user's choice, so we use the video device from the template. But if user switches VM type after that, we can change the video device accordingly, because the user has chosen that.

In the case of headless VM, there is no video device present. When the user moves from headless to console, there is no user's choice for video device, so we can use the default that comes from VM type, i.e. Bochs.

@ahadas
Copy link
Member

ahadas commented Jun 21, 2022

in an ideal situation, Michal is right - when the blank template is set with vm type X, its settings should be aligned with the defaults for vm type X and then when creating a new vm, that by default is based on the blank template, you get the settings of vm type X (for 'server', we would expect it not to have a sound device, the disks should be cloned (rather than thin provisioned), etc)

the problem in this specific case is that it is problematic to set the blank template with Bochs due to lower cluster levels.

this PR is about some really corner case of switching between headless and non-headless setting, not so interesting - but if it was implemented already I have no objection to get it in. the more important part is to set Bochs display when things change and Bochs device should be set ('server' type + UEFI + compatibility level in which Bochs is supported) - and that was verified already. about the blank template - I think that changing it to pick Bochs on CL>4.5 and VGA on CL<4.6 is not trivial and doesn't worth the effort (and I don't think that enabling Bochs on all compatibility levels is a smart move now).

so yeah, not ideal but also not that bad - after all the benefit of Bochs over VGA is not that significant, if someone really wants their new VMs that are created from scratch (i.e., from the blank template) to have video device = Bochs, they can achieve that

@ahadas
Copy link
Member

ahadas commented Jun 21, 2022

this PR is about some really corner case of switching between headless and non-headless setting, not so interesting - but if it was implemented already I have no objection to get it in.

that said, if someone insists on setting Bochs, when applicable, in this flow as well - I think it doesn't worth our time (which is true for this PR in general), but it does make sense to me..

Copy link
Contributor

@ljelinkova ljelinkova left a comment

Choose a reason for hiding this comment

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

The code looks OK.

@liranr23
Copy link
Member Author

liranr23 commented Jun 21, 2022

@michalskrivanek are we good to push it? any objection?

@ahadas
Copy link
Member

ahadas commented Jun 21, 2022

this PR is about some really corner case of switching between headless and non-headless setting, not so interesting - but if it was implemented already I have no objection to get it in.

that said, if someone insists on setting Bochs, when applicable, in this flow as well - I think it doesn't worth our time (which is true for this PR in general), but it does make sense to me..

apparently no one does, great

@ahadas
Copy link
Member

ahadas commented Jun 21, 2022

/ost

@ahadas ahadas merged commit ba24e65 into oVirt:master Jun 21, 2022
@liranr23 liranr23 deleted the headless_vga_ui branch June 21, 2022 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants