Retrieve current graphic consoles #1420

rszwajko · 2021-04-28T10:53:56Z

This PR depends on patch: https://gerrit.ovirt.org/c/ovirt-engine/+/114302 and fixes bug: https://bugzilla.redhat.com/1940766

use detail=current_graphics_consoles to retrieve the list of
available consoles (for active VMs only)
remove AutoSelect
use ClientModeConsoleDefault to set console priorities

Screenshot (VNC console has the highest priority):

sjd78

Looks ok, just a few questions before approving

sjd78 · 2021-05-07T14:17:29Z

src/sagas/index.js

-
-    yield put(setVmConsoles({ vmId, consolesList: consoles, selectedConsole }))
-  }
+  // for headless VM (no graphics_consoles defined) - API response is `{}`


Is this saga, fetchConsoles, necessary anymore?

we can remove it

sjd78 · 2021-05-07T14:20:48Z

src/components/VmActions/index.js

    const hasRdp = isWindows(vm.getIn(['os', 'type']))
+    const defaultConsole = vm.get('defaultConsole')


Is the defaultConsole being stored with the VM this way because you intend to make it user configurable in future?

Otherwise, it seems unnecessary to keep the system defaultConsole stored in the VM when it can just be accessed in config.

Is the defaultConsole being stored with the VM this way because you intend to make it user configurable in future?

Yep. Per VM settings are the next step here.

sjd78

These changes LGTM. Hopefully the restapi patch can be progressed soon.

Question - and I didn't test this scenario - what happens if this change runs on an engine without the patch? Is the console functionality lost completely? If yes, we should probably put a more specific backend version check at login to ensure things are going to work.

rszwajko · 2021-05-11T14:35:55Z

Question - and I didn't test this scenario - what happens if this change runs on an engine without the patch? Is the console functionality lost completely?

Yes, there will be no consoles on the dashboard. We could use follow=graphics_consoles&current to address this scenario - if the engine supports the new detail then it will ignore follow. Old engine will ignore the detail and return follow. The current flag makes it 2x faster then follow alone.

rszwajko · 2021-05-12T10:47:44Z

@sjd78
The backward compatibility option (follow=graphics_consoles&current) depends on features in patch https://gerrit.ovirt.org/c/ovirt-engine/+/114302
Until the patch is merged I would wait with implementing them in the UI.

1. use detail=current_graphics_consoles to retrieve the list of available consoles (for active VMs only) 2. remove AutoSelect 3. use ClientModeConsoleDefault to set console priorities 4. use additional `current` flag when fetching VM details to retrieve graphic consoles info from vm_dynamic. This is ~2x faster then standard `follow=graphics_consoles. 5. for backward compatibility use `follow=graphics_consoles&current' when (shallow) fetching multiple VMs. If the new detail is supported by the server then those params will be ignored.

rszwajko · 2021-05-13T13:49:30Z

Changes:

use additional current flag when fetching VM details to retrieve
graphic consoles info from vm_dynamic. This is ~2x faster
then standard follow=graphics_consoles.
for backward compatibility use follow=graphics_consoles&current
when (shallow) fetching multiple VMs. If the new detail is supported
by the server then those params will be ignored.

sjd78

The backward compatibility works. I verified against a 4.4.5 engine.

Code LGTM

sgratch

@rszwajko
The changes look good and working well except the following:

For headless Win based VMs, the RDP option is dismissed for both dashboard and VM details views.
The ClientModeVncDefault config parameter is ignored for setting the default console, i.e. in case ClientModeVncDefault is set to NoVnc and ClientModeConsoleDefault is set to vnc or the VM supports only vnc, then the 'VNC console browser' option should be set as a default on dashboard (this is how it behaves on webadmin as well).
For both new engine and backward compatibility for old engines:
It seems that using the vm_dynamic is faster, but we still need to be sure that using ?follow=graphics_consoles&current or the new detail=current_graphics_consoles on dashboard won't re-generate bug https://bugzilla.redhat.com/show_bug.cgi?id=1795457 as was when using ?follow=graphics_consoles. This bug was the whole motivation for the original fix. When checking the code, it seems ok but I can't be sure since we don't know where the sql query: select * from getdisksvmguid is actually generated by the follow=graphics_consoles call.
I suggest asking the performance QE team to verify before marking the BZ as modified. Unless we can verify that in the code.

Introduce clear separation between console protocols (VNC, Spice, RDP) and consoles visible in the UI (BrowserVnc, NativeVnc, Spice, RDP). Changes: 1. use defaultUiConsole - a read-only config value derived from defaultConsole and defaultVncMode 2. remove defaultConsole field from the VM and instead directly check the current defaultUiConsole config value

rszwajko · 2021-06-07T11:42:33Z

For headless Win based VMs, the RDP option is dismissed for both dashboard and VM details views.

It's not a regression. This fix was introduced in 148f731

The ClientModeVncDefault config parameter is ignored for setting the default console, i.e. in case ClientModeVncDefault is set to NoVnc and ClientModeConsoleDefault is set to vnc or the VM supports only vnc, then the 'VNC console browser' option should be set as a default on dashboard (this is how it behaves on webadmin as well).

Fixed by introducing a clear separation between protocols and UI consoles available for the end user. The source of the confusion was that VNC protocol maps into 2 UI consoles.

I suggest asking the performance QE team to verify before marking the BZ as modified. Unless we can verify that in the code.

OK. The approach seems safe to me as it is used by Web Admin (with comparable number of VMs). However we could do more research on this.

sgratch

It's not a regression. This fix was introduced in 148f731

Based on webadmin and from current design aspect, we don't really support an option to disable RDP since RDP is always enabled for win vms, even headless ones.
We did miss that for older versions and added this functionality back on #1342.
So if we want to be compatible with webadmin, we need to add this.
Nevertheless since it was not supported till 1.6.8, it's ok for me to leave it as is until someone will complain. Not that important.

sgratch · 2021-06-07T17:12:17Z

src/components/VmActions/index.js

    let consoles = []

    consoles = vm.get('consoles').map((c) => ({
      priority: 0,
      protocol: c.get('protocol'),
+      uiConsole: toUiConsole(NATIVE, c.get('protocol')),


This doesn't seem correct to me. I think that in case Spice is not supported and vnc is supported then the default ui conosle type should be based on ClientModeVncDefault as well, i.e in case noVnc is supported then this should be the default.

The current code provides the requested functionality.

Note that we always support both VNC flavors if VNC protocol is present:

NoVnc is added when vncConsole flag is ttrue.

The Native VNC is added by directly mapping the list of available protocols (that's why toUiConsole() is used with first parameter set to NATIVE).

This logic was inherited from previous versions.

@rszwajko the issue raised here is regarding the default console displayed for the VM card in case that the defaultConsole is not supported by that VM.

For reproducing:
set ClientModeConsoleDefault=spice, ClientModeVncDefault=NoVnc and set the VM to support only VNC.

Expected result: the default option should be BrowserVnc.
Actual result: the default option is VNC.
This seems like not supported by the current code but it was supported by the previous version and by webadmin.

We did miss that for older versions and added this functionality back on #1342.
So if we want to be compatible with webadmin, we need to add this.
Nevertheless since it was not supported till 1.6.8, it's ok for me to leave it as is until someone will complain. Not that important.

We can fix it under separate issue.

Sure, please open a separate issue for this

@rszwajko the issue raised here is regarding the default console displayed for the VM card in case that the defaultConsole is not supported by that VM.

For reproducing:
set ClientModeConsoleDefault=spice, ClientModeVncDefault=NoVnc and set the VM to support only VNC.
Expected result: the default option should be BrowserVnc.
Actual result: the default option is VNC.
This seems like not supported by the current code but it was supported by the previous version and by webadmin.

OK. I understand the use case now. However I'm not sure if it applies to the current design. Previous version and Web Admin provide a single button with black-box style logic (no indication what console will be chosen by the algorithm).
In the new design it's transparent to the user:

all available consoles are visible (the default console goes first)

the default global console send from the server (server's default console) is displayed in Account Settings (see below - feature available soon). Due to that there is only one such default (we don't have secondary default for cases like ClientModeConsoleDefault=spice, ClientModeVncDefault=NoVnc )

user can override(completely) the server's default by setting a preferred console in Account Settings (available soon). Once again it's only one value (a limitation of SelectBox UI).

user can override the global console by setting per VM value (available in the next phase)

The goal is to have the logic here simple and transparent that's why only 2 levels are used:

global - either server's default console or console set by the user in Account Settings

per VM (next phase)

If the default is not supported by the given VM (for any reason) then consoles are displayed in default order (as before). Of course we could provide fallback but things will get complex i.e. imagine the case where:

ClientModeConsoleDefault=spice, ClientModeVncDefault=NoVnc

preferred console is RDP

VM is Linux with VNC only

I'm not convinced if the added value justifies the complexity:)

sgratch · 2021-06-07T17:16:01Z

src/constants/console.js

 export const SPICE = 'spice'
 export const RDP = 'rdp'
+
+// VNC modes sent from the backend (ClientConsoleMode)


ClientConsoleMode -> ClientModeVncDefault ?

I was referring to VncConsoleModel.ClientConsoleMode enum but referring to config property is better.

rszwajko · 2021-06-07T18:02:14Z

It's not a regression. This fix was introduced in 148f731

Based on webadmin and from current design aspect, we don't really support an option to disable RDP since RDP is always enabled for win vms, even headless ones.
We did miss that for older versions and added this functionality back on #1342.
So if we want to be compatible with webadmin, we need to add this.
Nevertheless since it was not supported till 1.6.8, it's ok for me to leave it as is until someone will complain. Not that important.

We can fix it under separate issue.

sgratch · 2021-06-10T09:41:48Z

@rszwajko
I agree with all you wrote above regarding the design, except the specific scenario of which the user specifically sets the Vnc mode to noVnc (ClientModeVncDefault=noVnc) and only VNC console type is supported for that VM.
This case is currently working for webadmin and I think that it should work the same for web-ui if we support a default console now.

Previous version and Web Admin provide a single button with black-box style logic (no indication what console will be chosen by the algorithm).

Not accurate. On webadmin you can currently choose the actual console type per VM ,but the default console (console button) is based on two things: the supported consoles and the global config ClientModeVncDefault value. So in case the user sets ClientModeVncDefault=noVnc and if vnc is the only client supported for a VM, this will be the auto default console opened while clicking on console button via webadmin.

Due to that there is only one such default (we don't have secondary default for cases like ClientModeConsoleDefault=spice, ClientModeVncDefault=NoVnc )

We don't need to support a secondary default. But in a very specific case in which:

the VM is configured to run with only VNC console type
ClientModeVncDefault=noVnc
for the new console options settings: the global default is not RDP for Win VMs or VNC)

=> then the default console which appears as the first on list should be noVnc and not VNC.

Of course we could provide fallback but things will get complex i.e. imagine the case where:
ClientModeConsoleDefault=spice, ClientModeVncDefault=NoVnc
preferred console is RDP
VM is Linux with VNC only

in that case, the default should be NoVnc and not VNC.
The same goes If VM is Linux with Spice only then Spice should be the default even if ClientModeConsoleDefault=vnc, ClientModeVncDefault=NoVnc and preferred console is RDP.

We can decide to neglect the default console options on web-ui dashboard and just display the consoles in a default fix order as it was up till now. But if we do support default ordering then let's do it compatible to webadmin.
It shouldn't be too complicated to fix for that specific scenario.

rszwajko · 2021-06-10T10:26:21Z

@sgratch

It shouldn't be too complicated to fix for that specific scenario.

Of course we can address this specific scenario. Please check the newest commit.

Please check the newest code.

sgratch

The code now LGTM.
But there are performance test results that still needed to be examined before merging. They are discussed via mails.

sgratch · 2021-06-14T18:17:20Z

@rszwajko please rebase

sgratch · 2021-06-17T12:11:15Z

The performance test results look ok so we can merge the PR.

rszwajko requested review from bamsalem, sjd78, sgratch and hstastna April 28, 2021 10:54

rszwajko force-pushed the consoles branch from a3e18c2 to 4e9be62 Compare April 30, 2021 11:02

sjd78 reviewed May 7, 2021

View reviewed changes

rszwajko force-pushed the consoles branch from 4e9be62 to acf1c07 Compare May 11, 2021 10:13

rszwajko requested a review from sjd78 May 11, 2021 10:13

sjd78 reviewed May 11, 2021

View reviewed changes

rszwajko force-pushed the consoles branch 2 times, most recently from 29a9f48 to 0dd32bd Compare May 13, 2021 13:25

rszwajko added 2 commits May 13, 2021 15:40

Remove fetchConsoles() logic

5cc7b66

rszwajko force-pushed the consoles branch from 0dd32bd to 5cc7b66 Compare May 13, 2021 13:47

rszwajko requested a review from sjd78 May 13, 2021 13:49

sjd78 approved these changes May 13, 2021

View reviewed changes

sjd78 added the Status: ON_QA status ON_QA (currently being tested) label May 17, 2021

sgratch removed the Status: ON_QA status ON_QA (currently being tested) label Jun 2, 2021

sgratch requested changes Jun 3, 2021

View reviewed changes

rszwajko requested a review from sgratch June 7, 2021 11:42

sgratch previously requested changes Jun 7, 2021

View reviewed changes

Correct the documentation for supported VNC modes

7b9cb75

rszwajko requested a review from sgratch June 7, 2021 18:04

rszwajko mentioned this pull request Jun 9, 2021

RDP console is missing for headless Win based VMs #1445

Closed

Sort VNC consoles based on ClientModeVncDefault

6134c53

rszwajko mentioned this pull request Jun 10, 2021

Console options #1446

Merged

sgratch reviewed Jun 14, 2021

View reviewed changes

rszwajko added 2 commits June 15, 2021 10:34

Merge remote-tracking branch 'origin/master' into consoles

505428a

Merge remote-tracking branch 'origin/master' into consoles

0bc9c60

sgratch merged commit 1078cf8 into oVirt:master Jun 17, 2021

sgratch added the Status: ON_QA status ON_QA (currently being tested) label Jun 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retrieve current graphic consoles #1420

Retrieve current graphic consoles #1420

rszwajko commented Apr 28, 2021 •

edited by sgratch

sjd78 left a comment

sjd78 May 7, 2021

rszwajko May 10, 2021

sjd78 May 7, 2021

rszwajko May 10, 2021

sjd78 left a comment

rszwajko commented May 11, 2021

rszwajko commented May 12, 2021

rszwajko commented May 13, 2021

sjd78 left a comment

sgratch left a comment

rszwajko commented Jun 7, 2021

sgratch left a comment

sgratch Jun 7, 2021 •

edited

rszwajko Jun 7, 2021

sgratch Jun 8, 2021 •

edited

rszwajko Jun 9, 2021 •

edited

sgratch Jun 7, 2021

rszwajko Jun 7, 2021

rszwajko commented Jun 7, 2021

sgratch commented Jun 10, 2021 •

edited

rszwajko commented Jun 10, 2021

sgratch left a comment

sgratch commented Jun 14, 2021

sgratch commented Jun 17, 2021

		const hasRdp = isWindows(vm.getIn(['os', 'type']))
		const defaultConsole = vm.get('defaultConsole')

Retrieve current graphic consoles #1420

Retrieve current graphic consoles #1420

Conversation

rszwajko commented Apr 28, 2021 • edited by sgratch

sjd78 left a comment

Choose a reason for hiding this comment

sjd78 May 7, 2021

Choose a reason for hiding this comment

rszwajko May 10, 2021

Choose a reason for hiding this comment

sjd78 May 7, 2021

Choose a reason for hiding this comment

rszwajko May 10, 2021

Choose a reason for hiding this comment

sjd78 left a comment

Choose a reason for hiding this comment

rszwajko commented May 11, 2021

rszwajko commented May 12, 2021

rszwajko commented May 13, 2021

sjd78 left a comment

Choose a reason for hiding this comment

sgratch left a comment

Choose a reason for hiding this comment

rszwajko commented Jun 7, 2021

sgratch left a comment

Choose a reason for hiding this comment

sgratch Jun 7, 2021 • edited

Choose a reason for hiding this comment

rszwajko Jun 7, 2021

Choose a reason for hiding this comment

sgratch Jun 8, 2021 • edited

Choose a reason for hiding this comment

rszwajko Jun 9, 2021 • edited

Choose a reason for hiding this comment

sgratch Jun 7, 2021

Choose a reason for hiding this comment

rszwajko Jun 7, 2021

Choose a reason for hiding this comment

rszwajko commented Jun 7, 2021

sgratch commented Jun 10, 2021 • edited

rszwajko commented Jun 10, 2021

sgratch left a comment

Choose a reason for hiding this comment

sgratch commented Jun 14, 2021

sgratch commented Jun 17, 2021

rszwajko commented Apr 28, 2021 •

edited by sgratch

sgratch Jun 7, 2021 •

edited

sgratch Jun 8, 2021 •

edited

rszwajko Jun 9, 2021 •

edited

sgratch commented Jun 10, 2021 •

edited