[pve-devel] [PATCH manager v3] ui: vm console: autodetect novnc or xtermjs

[pve-devel] [PATCH manager v3] 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:
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 | 61 +++++++++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 17 deletions(-)

diff --git a/www/manager6/VNCConsole.js b/www/manager6/VNCConsole.js
index 9057e447..69ced0f8 100644
--- a/www/manager6/VNCConsole.js
+++ b/www/manager6/VNCConsole.js
@@ -27,31 +27,58 @@ 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`,
+		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 v3] ui: vm console: autodetect novnc or xtermjs

[Friedrich Weber]

On 25/02/2025 16:47, Aaron Lauterer wrote:
> 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.

I agree that defaulting to xterm.js in the serial terminal case makes
for a better user experience.

Tested this with a VM with a serial port booting from the PVE ISO,
didn't notice any big issues. See below for some minor comments.

> 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:
> 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 | 61 +++++++++++++++++++++++++++-----------
>  1 file changed, 44 insertions(+), 17 deletions(-)
> 
> diff --git a/www/manager6/VNCConsole.js b/www/manager6/VNCConsole.js
> index 9057e447..69ced0f8 100644
> --- a/www/manager6/VNCConsole.js
> +++ b/www/manager6/VNCConsole.js
> @@ -27,31 +27,58 @@ 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;
> +	    }

I was wondering whether there might a race condition possible here
(i.e., the success callback of API2Request and the `activate` listener
called with unfortunate timing that would prevent the console from
appearing), but thinking about it more, it looks okay -- there is no
threading and both the callback and the listener should normally be
called eventually (in whatever order). We won't get a console if the
/config API request fails, but I guess in that case something is
currently wrong with the setup anyway.

> +	    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") {

nit: missing space after the ===

> +	    Proxmox.Utils.API2Request({
> +		url: `/api2/extjs/nodes/${me.nodename}/qemu/${me.vmid}/config`,

Stumbled upon this by accident: I guess this could should pass
`current=1` to get the currently running config instead of the config
with pending changes applied (because the running config one is the
relevant one for deciding whether to use noVNC or xterm.js).

> +		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();
>  		},
>  	    },
>  	});



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

Re: [pve-devel] [PATCH manager v3] ui: vm console: autodetect novnc or xtermjs

[Aaron Lauterer]

On  2025-03-24  17:57, Friedrich Weber wrote:
> On 25/02/2025 16:47, Aaron Lauterer wrote:
[…]
>> +
>> +	if (me.consoleType ==="kvm") {
> 
> nit: missing space after the ===

thanks for spotting this

> 
>> +	    Proxmox.Utils.API2Request({
>> +		url: `/api2/extjs/nodes/${me.nodename}/qemu/${me.vmid}/config`,
> 
> Stumbled upon this by accident: I guess this could should pass
> `current=1` to get the currently running config instead of the config
> with pending changes applied (because the running config one is the
> relevant one for deciding whether to use noVNC or xterm.js).

good point. I'll send another version that will request the current config


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

Re: [pve-devel] [PATCH manager v3] ui: vm console: autodetect novnc or xtermjs

[Hannes Duerr]

I just noticed that there is a open bugtracker for this issue/feature 
request [0], so you can assign your self and add the bugtracker number 
to the commit message.


[0] https://bugzilla.proxmox.com/show_bug.cgi?id=1926

On 2/25/25 16:47, Aaron Lauterer wrote:
[...]


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

Re: [pve-devel] [PATCH manager v3] ui: vm console: autodetect novnc or xtermjs

[Aaron Lauterer]

I sent out a v4 with the suggested changes by Friedrich and the bug 
number that Hannes found. Thanks!

https://lore.proxmox.com/pve-devel/20250325091854.1051956-1-a.lauterer@proxmox.com/T/#u

On  2025-02-25  16:47, Aaron Lauterer wrote:
> 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:
> 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 | 61 +++++++++++++++++++++++++++-----------
>   1 file changed, 44 insertions(+), 17 deletions(-)
> 
> diff --git a/www/manager6/VNCConsole.js b/www/manager6/VNCConsole.js
> index 9057e447..69ced0f8 100644
> --- a/www/manager6/VNCConsole.js
> +++ b/www/manager6/VNCConsole.js
> @@ -27,31 +27,58 @@ 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`,
> +		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();
>   		},
>   	    },
>   	});



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