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

[Aaron Lauterer <a.lauterer@proxmox.com>]

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