[pve-devel] [PATCH manager v4] fix #1926 ui: vm console: autodetect novnc or xtermjs

[pve-devel] [PATCH manager v4] fix #1926 ui: vm console: autodetect novnc or xtermjs

[Aaron Lauterer]

Some users configure their VMs to use serial as their display. The big
benefit is that in combination with the xtermjs remote console, copy &
paste works a lot better than via novnc.

While the console button in the top right allows to manually choose the
console type, the Console in the main submenu of a VM does not.

This patch changes the behavior for VMs and will first fetch the VM
config and if the display is set to "serialX", will set the console to
xtermjs. Otherwise it will fall back to regular noVNC.

Since getting the VM config is an async API call, the code had to be
restructured so we can do the actual loading of the console after the
config has been fetched.
Therefore we now have the 'loadConsole()' function which will be called
when the UI item is activated and in the KVM case, after loading and
handling the VM config. It is guarded by the 'activated' and
'configLoaded' variables.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
Another thing that I noticed is that the property we use to decide if we
enable xtermjs for VMs in the top right console button only checks if
the VM has a serial device configured.
PVE::QemuServer::vmstatus() calls `conf_has_serial()`.

A better approach would be to have a vmstatus property that actually
tells us if the VM has serial set as display option. As the display
could be set to something else, even if a serial device exists. There
are other use-cases for a serial device in the VM, like passing through
an actual serial port.
But I did not want to introduce another new property for vmstatus() and
changing the meaning of the current serial property would be a breaking
change.
Therefore, this current UI only implementation.

changes since:
v3:
* fixed spacing issues
* add 'current' parameter when fetching config as the pending might have
  a different display set

v2:
* change approach and do it in the UI alone by fetching the VM
  config before deciding which console to use

v1:
* set 'autodetect' to always true in 'VNCConsole.js'
* add additional checks in pveproxy
  * only if autodetect is enabled and console is set to 'kvm'
  * username exists and has VM.Console permissions for the guest

 www/manager6/VNCConsole.js | 62 +++++++++++++++++++++++++++-----------
 1 file changed, 45 insertions(+), 17 deletions(-)

diff --git a/www/manager6/VNCConsole.js b/www/manager6/VNCConsole.js
index 9057e447..868ee203 100644
--- a/www/manager6/VNCConsole.js
+++ b/www/manager6/VNCConsole.js
@@ -27,31 +27,59 @@ Ext.define('PVE.noVncConsole', {
 	    throw "no VM ID specified";
 	}
 
+	let activated = false;
+	let configLoaded = false;
 	// always use same iframe, to avoid running several noVnc clients
 	// at same time (to avoid performance problems)
 	var box = Ext.create('Ext.ux.IFrame', { itemid: "vncconsole" });
 
-	var type = me.xtermjs ? 'xtermjs' : 'novnc';
+	let loadConsole = function() {
+	    if (!activated || !configLoaded) {
+		return;
+	    }
+	    let type = me.xtermjs ? 'xtermjs' : 'novnc';
+	    let sp = Ext.state.Manager.getProvider();
+	    if (Ext.isFunction(me.beforeLoad)) {
+		me.beforeLoad();
+	    }
+	    let queryDict = {
+		console: me.consoleType, // kvm, lxc, upgrade or shell
+		vmid: me.vmid,
+		node: me.nodename,
+		cmd: me.cmd,
+		'cmd-opts': me.cmdOpts,
+		resize: sp.get('novnc-scaling', 'scale'),
+	    };
+	    queryDict[type] = 1;
+	    PVE.Utils.cleanEmptyObjectKeys(queryDict);
+	    var url = '/?' + Ext.Object.toQueryString(queryDict);
+	    box.load(url);
+	};
+
+	if (me.consoleType === "kvm") {
+	    Proxmox.Utils.API2Request({
+		url: `/api2/extjs/nodes/${me.nodename}/qemu/${me.vmid}/config`,
+		params: { 'current': '1' },
+		method: 'GET',
+		failure: response => Ext.Msg.alert('Error', response.htmlStatus),
+		success: function({ result }, options) {
+		    if (result.data.vga?.startsWith("serial")) {
+			me.xtermjs = true;
+		    }
+		    configLoaded = true;
+		    loadConsole();
+		},
+	    });
+	} else { // don't need to load anything else
+	    configLoaded = true;
+	}
+
 	Ext.apply(me, {
 	    items: box,
 	    listeners: {
 		activate: function() {
-		    let sp = Ext.state.Manager.getProvider();
-		    if (Ext.isFunction(me.beforeLoad)) {
-			me.beforeLoad();
-		    }
-		    let queryDict = {
-			console: me.consoleType, // kvm, lxc, upgrade or shell
-			vmid: me.vmid,
-			node: me.nodename,
-			cmd: me.cmd,
-			'cmd-opts': me.cmdOpts,
-			resize: sp.get('novnc-scaling', 'scale'),
-		    };
-		    queryDict[type] = 1;
-		    PVE.Utils.cleanEmptyObjectKeys(queryDict);
-		    var url = '/?' + Ext.Object.toQueryString(queryDict);
-		    box.load(url);
+		    activated = true;
+		    loadConsole();
 		},
 	    },
 	});
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH manager v4] fix #1926 ui: vm console: autodetect novnc or xtermjs

[Thomas Lamprecht]

Am 25.03.25 um 10:18 schrieb Aaron Lauterer:
> Some users configure their VMs to use serial as their display. The big
> benefit is that in combination with the xtermjs remote console, copy &
> paste works a lot better than via novnc.
> 
> While the console button in the top right allows to manually choose the
> console type, the Console in the main submenu of a VM does not.
> 
> This patch changes the behavior for VMs and will first fetch the VM
> config and if the display is set to "serialX", will set the console to
> xtermjs. Otherwise it will fall back to regular noVNC.
> 
> Since getting the VM config is an async API call, the code had to be
> restructured so we can do the actual loading of the console after the
> config has been fetched.

> Therefore we now have the 'loadConsole()' function which will be called
> when the UI item is activated and in the KVM case, after loading and
> handling the VM config. It is guarded by the 'activated' and
> 'configLoaded' variables.

A bit late in, so sorry for that, but FWIW, one can also use our async API
request wrapper for that, i.e. mark the function this is in as async and
then use something like:

let config = await Proxmox.Async.api2({
    url: `/api2/extjs/nodes/${me.nodename}/qemu/${me.vmid}/config?current=1`
});

That would probably reduce the amount of changes required and also
keep the code a bit more linear.

Btw. don't we have the config available in some call-sites? I.e., would
it make sense to allow overriding it already now? Because if we can
reduce the need for an extra API call due to information being already
available then I'd always favor that, having experienced working over
some high latency links and/or spotty & congested (wifi) connections.

We can improve this later on, but would be good to evaluate the status
quo already; FWIW I'd be open to look into querying the respective
properties in the cluster resource API endpoint, which would then be
available in the in-memory resource store and really cheap to query
from the frontend. As we already get some properties the backend side
would not really get more expensive either. Again, that does not has
to be a prerequisite for applying something like this, but IMO worth
to evaluate at some (soonish) point.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH manager v4] fix #1926 ui: vm console: autodetect novnc or xtermjs

[Aaron Lauterer]

On  2025-03-25  18:09, Thomas Lamprecht wrote:
> Am 25.03.25 um 10:18 schrieb Aaron Lauterer:
>> Some users configure their VMs to use serial as their display. The big
>> benefit is that in combination with the xtermjs remote console, copy &
>> paste works a lot better than via novnc.
>>
>> While the console button in the top right allows to manually choose the
>> console type, the Console in the main submenu of a VM does not.
>>
>> This patch changes the behavior for VMs and will first fetch the VM
>> config and if the display is set to "serialX", will set the console to
>> xtermjs. Otherwise it will fall back to regular noVNC.
>>
>> Since getting the VM config is an async API call, the code had to be
>> restructured so we can do the actual loading of the console after the
>> config has been fetched.
> 
>> Therefore we now have the 'loadConsole()' function which will be called
>> when the UI item is activated and in the KVM case, after loading and
>> handling the VM config. It is guarded by the 'activated' and
>> 'configLoaded' variables.
> 
> A bit late in, so sorry for that, but FWIW, one can also use our async API
> request wrapper for that, i.e. mark the function this is in as async and
> then use something like:
> 
> let config = await Proxmox.Async.api2({
>      url: `/api2/extjs/nodes/${me.nodename}/qemu/${me.vmid}/config?current=1`
> });
> 
> That would probably reduce the amount of changes required and also
> keep the code a bit more linear.
> 
> Btw. don't we have the config available in some call-sites? I.e., would
> it make sense to allow overriding it already now? Because if we can
> reduce the need for an extra API call due to information being already
> available then I'd always favor that, having experienced working over
> some high latency links and/or spotty & congested (wifi) connections.
> 
> We can improve this later on, but would be good to evaluate the status
> quo already; FWIW I'd be open to look into querying the respective
> properties in the cluster resource API endpoint, which would then be
> available in the in-memory resource store and really cheap to query
> from the frontend. As we already get some properties the backend side
> would not really get more expensive either. Again, that does not has
> to be a prerequisite for applying something like this, but IMO worth
> to evaluate at some (soonish) point.

I did not find if we already have the full VM config already. AFAICT we 
go from `qemu/Config.js` -> `VNCConsole.js`.

Only the status of the VM. As I mentioned in the comment below the 
commit msg, the backend does check against the wrong config property for 
this use-case.

So if we actually have the config already and I just couldn't find it, 
point me to it :)

Otherwise, to avoid additional API calls, the other options we have are:

* change the backend check that populates `serial` in the status. It 
currently checks against the presence of a serial device. But we need to 
know if the display is set to serial, otherwise we get a false positive 
if the serial device is used for a real physical serial device.
But I don't know where else (externally?) that might be used, therefore 
I consider this a breaking change.

* extend the vm status to have the infos we need.
** property like "serialdisplay"
** a "display" property that contains the configured display option?

If we introduce a new property to vmstatus, the display one would most 
likely be more generic and could be more useful in the future?
Initially we decided against that to keep in lean.

But if you think that having a bit more info in the vmstatus is better 
than another API call, I can send another patch that also extends the 
backend.

If we decide to keep the additional API call, I will send another one 
trying the async wrapper.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH manager v4] fix #1926 ui: vm console: autodetect novnc or xtermjs

[Thomas Lamprecht]

Am 26.03.25 um 13:04 schrieb Aaron Lauterer:
> I did not find if we already have the full VM config already. AFAICT we 
> go from `qemu/Config.js` -> `VNCConsole.js`.
> 
> Only the status of the VM. As I mentioned in the comment below the 
> commit msg, the backend does check against the wrong config property for 
> this use-case.
> 
> So if we actually have the config already and I just couldn't find it, 
> point me to it :)
> 
> Otherwise, to avoid additional API calls, the other options we have are:
> 
> * change the backend check that populates `serial` in the status. It 
> currently checks against the presence of a serial device. But we need to 
> know if the display is set to serial, otherwise we get a false positive 
> if the serial device is used for a real physical serial device.
> But I don't know where else (externally?) that might be used, therefore 
> I consider this a breaking change.
> 
> * extend the vm status to have the infos we need.
> ** property like "serialdisplay"
> ** a "display" property that contains the configured display option?
As vm_status already has all information parsed that required for this and
already has a 'spice` boolean flag, it seems fine to handle that in
vm_status. But it might be better to add a new slightly more general
property where we can absorb the spice flag in the long run, like:

display: (serial;qxl;...)

or already default to a property format-string now, but 

display: type=[serial;qxl;...]

but we can transform it to that later one too if we're unsure about
potential additional data added here, besides maybe merging in the
clipboard too – then it might be better to have something like:

user-interface: display=...[,clipboard=...][,...?]

But no hard feelings on that, maybe someone else has input here, else
I probably would go for `display: (serial;qxl;...)` for now. We need
to handle "allow-spice" (as spice virt-viewer can be used for more
than just QXL) then in the frontend though once we drop the spice
flag then.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel