From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9])
by lore.proxmox.com (Postfix) with ESMTPS id 8F98C1FF15C
for <inbox@lore.proxmox.com>; Wed, 26 Mar 2025 13:04:40 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 07C3A36165;
Wed, 26 Mar 2025 13:04:34 +0100 (CET)
Message-ID: <871f83ca-1fe8-4ead-8f5b-a65e41e1c794@proxmox.com>
Date: Wed, 26 Mar 2025 13:04:00 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Content-Language: en-US
To: Thomas Lamprecht <t.lamprecht@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
References: <20250325091854.1051956-1-a.lauterer@proxmox.com>
<6e539849-fe12-4e82-a1bc-c826cec4dc6f@proxmox.com>
From: Aaron Lauterer <a.lauterer@proxmox.com>
In-Reply-To: <6e539849-fe12-4e82-a1bc-c826cec4dc6f@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.034 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [PATCH manager v4] fix #1926 ui: vm console:
autodetect novnc or xtermjs
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.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