Re: [pve-devel] [PATCH manager v5 3/4] fix #1926 ui: vm console: autodetect novnc or xtermjs

[Dominik Csapak <d.csapak@proxmox.com>]

tiny nits inline,

with or without that, consider this

Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>

On 4/7/25 18:27, 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 to not load the console right away if
> set to 'kvm'. Instead, the callback from the VM's status/current call
> will run the final steps to load the console. Because then we know if it
> should be noVNC or xtermjs.
> 
> That means that in the VNCConsole component we need to keep track if the
> console has been fully set up and is running. The actual code that loads
> the console into the iframe is moved into a function, so we cann call it
> from the API callback.
> 
> One result is that the behavior changes. Loading the console on a VM
> will take a little bit longer:
> * When then Console is the selected and one switches between VMs, a new
>    status/current call is made and on a good connection, the console
>    should load just a few moments later.
> * When one switches to the console from another submenu of the VM, the
>    console will load once the next API call to status/current finished.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> Overall not really less complex than v4 [0] but with the new display
> property definitely more ready to changes in the future and we save one
> API call.
> 
> [0] https://lore.proxmox.com/pve-devel/20250325091854.1051956-1-a.lauterer@proxmox.com/
> 
> changes since:
> v4:
> * use new status/current display property
> 
> 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  | 52 +++++++++++++++++++++++++------------
>   www/manager6/qemu/Config.js |  8 +++++-
>   2 files changed, 42 insertions(+), 18 deletions(-)
> 
> diff --git a/www/manager6/VNCConsole.js b/www/manager6/VNCConsole.js
> index 9057e447..3371c923 100644
> --- a/www/manager6/VNCConsole.js
> +++ b/www/manager6/VNCConsole.js
> @@ -12,6 +12,36 @@ Ext.define('PVE.noVncConsole', {
>       layout: 'fit',
>       border: false,
>   
> +    setupDone: false, // initial setup is done and the session is running
> +
> +    loadConsole: function(xtermjs, consoleType) {
> +	let me = this;
> +	if (me.setupDone) {
> +	    return;
> +	}
> +
> +	let type = xtermjs ? 'xtermjs' : 'novnc';
> +	let sp = Ext.state.Manager.getProvider();
> +	if (Ext.isFunction(me.beforeLoad)) {
> +	    me.beforeLoad();
> +	}
> +	let queryDict = {
> +	    console: 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'),
> +	};
> +	let box = this.down('[itemid=vncconsole]');
> +	queryDict[type] = 1;
> +	PVE.Utils.cleanEmptyObjectKeys(queryDict);
> +	let url = '/?' + Ext.Object.toQueryString(queryDict);
> +	Proxmox.Utils.setErrorMask(me);
> +	box.load(url);
> +	me.setupDone = true;
> +    },
> +
>       initComponent: function() {
>   	var me = this;
>   
> @@ -29,37 +59,25 @@ Ext.define('PVE.noVncConsole', {
>   
>   	// 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" });
> +	let box = Ext.create('Ext.ux.IFrame', { itemid: "vncconsole" });
>   
> -	var type = me.xtermjs ? 'xtermjs' : 'novnc';
>   	Ext.apply(me, {
>   	    items: box,
>   	    listeners: {
>   		activate: function() {
> -		    let sp = Ext.state.Manager.getProvider();
> -		    if (Ext.isFunction(me.beforeLoad)) {
> -			me.beforeLoad();
> +		    if (me.consoleType !== 'kvm') {
> +			me.loadConsole(me.xtermjs, me.consoleType);
>   		    }
> -		    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);
>   		},
>   	    },
>   	});
>   
> +
>   	me.callParent();
>   
>   	me.on('afterrender', function() {
>   	    me.focus();
> +	    Proxmox.Utils.setErrorMask(me, true);
>   	});
>       },
>   
> diff --git a/www/manager6/qemu/Config.js b/www/manager6/qemu/Config.js
> index 0699175e..03692abe 100644
> --- a/www/manager6/qemu/Config.js
> +++ b/www/manager6/qemu/Config.js
> @@ -5,6 +5,8 @@ Ext.define('PVE.qemu.Config', {
>       onlineHelp: 'chapter_virtual_machines',
>       userCls: 'proxmox-tags-full',
>   
> +    referenceHolder: true,
> +
>       initComponent: function() {
>           var me = this;
>   	var vm = me.pveSelNode.data;
> @@ -283,6 +285,7 @@ Ext.define('PVE.qemu.Config', {
>   		vmid: vmid,
>   		consoleType: 'kvm',
>   		nodename: nodename,
> +		reference: 'submenuConsoleBtn',
>   	    });
>   	}
>   
> @@ -444,7 +447,8 @@ Ext.define('PVE.qemu.Config', {
>   		lock = rec ? rec.data.value : undefined;
>   
>   		spice = !!s.data.get('spice');
> -		xtermjs = !!s.data.get('serial');
> +		rec = s.data.get('display');
> +		xtermjs = rec ? rec.data.value.type.startsWith('serial') : false;

two tiny things here:

* rec is imho not a good name for the display portion, you could just name it 'display'
* for the result of xtermjs you could use something like:

xtermjs = rec?.data.value.type.startsWith('serial') ?? false;

instead of the ternary operator


>   	    }
>   
>   	    rec = s.data.get('tags');
> @@ -467,6 +471,8 @@ Ext.define('PVE.qemu.Config', {
>   	    consoleBtn.setEnableSpice(spice);
>   	    consoleBtn.setEnableXtermJS(xtermjs);
>   
> +	    me.lookup('submenuConsoleBtn')?.loadConsole(xtermjs, 'kvm');
> +
>   	    statusTxt.update({ lock: lock });
>   
>   	    let guest_running = status === 'running' &&



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