[pve-devel] [PATCH qemu-server, manager v8 0/4] fix #1926 autodetect xtermjs or novnc for VM console

[pve-devel] [PATCH qemu-server, manager v8 0/4] fix #1926 autodetect xtermjs or novnc for VM console

[Aaron Lauterer]

Here is a new verson for this fix. Nothing changed since v7 expect
rebasing. Quite a bit happened since then, but the rebase should be fine
by now. I gave it a quick test on my workstation and things seem to work
as expected. A quick test by someone else might be warranted though,
just to be safe. I added all the R-b and T-b tags that were added on v7
direclty into each patch. Hopefully as they should be (chronological).

The old cover letter that explains what is happening below:

After the feedback from Fiona on v6 and some
off-list discussion, we decided to improve the backend part by moving
the "default" logic out of `get_vga_properties` and move it into a
separate public `get_default_vga_type` function. This way we can leave
all the other functions private to the QemuServer.pm module.


We add a new property in the VM status/current API result that includes
the display configurtion of the VM. This way we can check in the
frontend what to do with it.

I chose a nested return value, as that makes it easier to add/move
additional display properties into it.

Patch 1/4 moves the default display logic into its own public function

Patch 2/4 adds the new display property. If not explicitly set in the VM
config, it will return the default value.

Patch 3/4 implements the changes in the UI. The final result isn't
really a lot simpler on the UI side than in V4 where we had the extra
API call to the VM's config directly. Because we still need to wait for
the API call to finish when initially navigating to the VM. But we have
one fewer call.

Patch 4/4 then introduces some changes to make loading of the console
faster if we just navigate in the submenu of a VM itself where we
already have the current status of a VM already cached.


Changes from
v7: rebased
v6: backend only: create new `get_default_vga_type` function.
v5: implement suggestions:

* use get_vga_properties for default VGA
* UI: use helper to determine if serial display

Aaron Lauterer (2):
  add new public get_default_vga_type function
  api: status/current: add display property

 src/PVE/API2/Qemu.pm  | 13 +++++++++++++
 src/PVE/QemuServer.pm | 29 ++++++++++++++++++++++-------
 2 files changed, 35 insertions(+), 7 deletions(-)

-- 
2.47.3



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

[pve-devel] [PATCH qemu-server v8 1/2] add new public get_default_vga_type function

[Aaron Lauterer]

we resolve missing parameters if necessary to make it easier to call
from another module where we likely only have the VM config ready.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
Tested-by: Hannes Duerr <h.duerr@proxmox.com>
Reviewed-by: Michael Köppl <m.koeppl@proxmox.com>
Tested-by: Michael Köppl <m.koeppl@proxmox.com>
---
changes since
v7:
* rebased
v6:
* instead of making get_vga_properties and extract_version public, we
create the new get_default_vga_type function and only make it public.

 src/PVE/QemuServer.pm | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 9f35de2e..adcdac5b 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -3044,6 +3044,27 @@ my sub should_disable_smm {
         && $vga->{type} =~ m/^(serial\d+|none)$/;
 }
 
+sub get_default_vga_type {
+    my ($conf, $arch, $machine_version, $winversion) = @_;
+
+    $arch //= PVE::QemuServer::Helpers::get_vm_arch($conf);
+    $winversion //= PVE::QemuServer::Helpers::windows_version($conf->{ostype});
+    if (!$machine_version) {
+        my $kvm_binary = PVE::QemuServer::Helpers::get_command_for_arch($arch);
+        my $kvmver = kvm_user_version($kvm_binary);
+        my $machine_type = PVE::QemuServer::Machine::get_vm_machine($conf, undef, $arch);
+        $machine_version = extract_version($machine_type, $kvmver);
+    }
+
+    if ($arch eq 'aarch64') {
+        return 'virtio';
+    } elsif (min_version($machine_version, 2, 9)) {
+        return (!$winversion || $winversion >= 6) ? 'std' : 'cirrus';
+    } else {
+        return ($winversion >= 6) ? 'std' : 'cirrus';
+    }
+}
+
 my sub get_vga_properties {
     my ($conf, $arch, $machine_version, $winversion) = @_;
 
@@ -3053,13 +3074,7 @@ my sub get_vga_properties {
     $vga->{type} = 'qxl' if $qxlnum;
 
     if (!$vga->{type}) {
-        if ($arch eq 'aarch64') {
-            $vga->{type} = 'virtio';
-        } elsif (min_version($machine_version, 2, 9)) {
-            $vga->{type} = (!$winversion || $winversion >= 6) ? 'std' : 'cirrus';
-        } else {
-            $vga->{type} = ($winversion >= 6) ? 'std' : 'cirrus';
-        }
+        $vga->{type} = get_default_vga_type($conf, $arch, $machine_version, $winversion);
     }
 
     return ($vga, $qxlnum);
-- 
2.47.3



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

[pve-devel] [PATCH qemu-server v8 2/2] api: status/current: add display property

[Aaron Lauterer]

This new property returns the configured or default display for a VM.

Instead of a flat property, we use a nested 'type' object that contains
the actual information. This way we can add other properties that belong
to a VM's display in the future without much hassle, to have them all in
one place.
Candidates to be moved into the display property are for example the
spice and clipboard property.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
Tested-by: Hannes Duerr <h.duerr@proxmox.com>
Reviewed-by: Michael Köppl <m.koeppl@proxmox.com>
Tested-by: Michael Köppl <m.koeppl@proxmox.com>
---
changes since
v7:
* rebased
v6:
* use the new QemuServer::get_default_vga_type function, which means we
  can drop a lot of boilerplate code

v5:
* use QemuServer::get_vga_properties to determine the default
* properties.


 src/PVE/API2/Qemu.pm | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index 7fced6c6..d1a4eafb 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -3310,6 +3310,16 @@ __PACKAGE__->register_method({
                 enum => ['vnc'],
                 optional => 1,
             },
+            display => {
+                description => "Display settings",
+                type => 'object',
+                properties => {
+                    type => {
+                        description => "Display type configured",
+                        type => 'string',
+                    },
+                },
+            },
         },
     },
     code => sub {
@@ -3323,8 +3333,11 @@ __PACKAGE__->register_method({
 
         $status->{ha} = PVE::HA::Config::get_service_status("vm:$param->{vmid}");
 
+        $status->{display}->{type} = PVE::QemuServer::get_default_vga_type($conf);
         if ($conf->{vga}) {
             my $vga = PVE::QemuServer::parse_vga($conf->{vga});
+            $status->{display}->{type} = $vga->{type} if defined($vga->{type});
+
             my $spice = defined($vga->{type}) && $vga->{type} =~ /^virtio/;
             $spice ||= PVE::QemuServer::vga_conf_has_spice($conf->{vga});
             $status->{spice} = 1 if $spice;
-- 
2.47.3



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

[pve-devel] [PATCH manager v8 3/4] 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 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>
Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Hannes Duerr <h.duerr@proxmox.com>
Reviewed-by: Michael Köppl <m.koeppl@proxmox.com>
Tested-by: Michael Köppl <m.koeppl@proxmox.com>
---
changes since:
v7:
* rebased
v6: none
v5:
* introduce PVE.Utils.isSerialDisplay helper
* avoid 'rec' assignment in callback when setting the 'xtermjs' variable
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/Utils.js       |  4 +++
 www/manager6/VNCConsole.js  | 51 ++++++++++++++++++++++++-------------
 www/manager6/qemu/Config.js |  9 ++++++-
 3 files changed, 46 insertions(+), 18 deletions(-)

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index c48ee0b2..03eca22d 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -2051,6 +2051,10 @@ Ext.define('PVE.Utils', {
         },
     },
 
+    isSerialDisplay: function (display) {
+        return display?.startsWith('serial') ?? false;
+    },
+
     singleton: true,
     constructor: function () {
         var me = this;
diff --git a/www/manager6/VNCConsole.js b/www/manager6/VNCConsole.js
index e4f22cf9..d903f2d5 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,29 +59,15 @@ 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);
                 },
             },
         });
@@ -60,6 +76,7 @@ Ext.define('PVE.noVncConsole', {
 
         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 842d35de..ffec14de 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;
@@ -311,6 +313,7 @@ Ext.define('PVE.qemu.Config', {
                 vmid: vmid,
                 consoleType: 'kvm',
                 nodename: nodename,
+                reference: 'submenuConsoleBtn',
             });
         }
 
@@ -474,7 +477,7 @@ Ext.define('PVE.qemu.Config', {
                 lock = rec ? rec.data.value : undefined;
 
                 spice = !!s.data.get('spice');
-                xtermjs = !!s.data.get('serial');
+                xtermjs = PVE.Utils.isSerialDisplay(s.data.get('display')?.data.value.type);
             }
 
             rec = s.data.get('tags');
@@ -486,6 +489,10 @@ Ext.define('PVE.qemu.Config', {
 
             var resume = ['prelaunch', 'paused', 'suspended'].indexOf(qmpstatus) !== -1;
 
+            me.lookup('submenuConsoleBtn')?.loadConsole(xtermjs, 'kvm');
+
+            statusTxt.update({ lock: lock });
+
             if (resume || lock === 'suspended') {
                 startBtn.setVisible(false);
                 resumeBtn.setVisible(true);
-- 
2.47.3



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

[pve-devel] [PATCH manager v8 4/4] ui: console: check on activate if display info for VMs is present

[Aaron Lauterer]

If we already have the display information for a VM, we can proceed
loading the correct console (noVNC or xtermjs).

This way, we don't need to wait for the callback of the VM's
status/current API call to finish setting up the console.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
Tested-by: Hannes Duerr <h.duerr@proxmox.com>
Reviewed-by: Michael Köppl <m.koeppl@proxmox.com>
Tested-by: Michael Köppl <m.koeppl@proxmox.com>
---
changes since:
v7:
* rebased
v6: none
v5:
* introduce PVE.Utils.isSerialDisplay helper
* avoid 'rec' assignment in callback when setting the 'xtermjs' variable
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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/www/manager6/VNCConsole.js b/www/manager6/VNCConsole.js
index d903f2d5..b7f90bda 100644
--- a/www/manager6/VNCConsole.js
+++ b/www/manager6/VNCConsole.js
@@ -67,6 +67,14 @@ Ext.define('PVE.noVncConsole', {
                 activate: function () {
                     if (me.consoleType !== 'kvm') {
                         me.loadConsole(me.xtermjs, me.consoleType);
+                    } else {
+                        let display = me.up().statusStore.getById('display');
+                        if (PVE.Utils.isSerialDisplay(display?.data.value.type)) {
+                            me.xtermjs = true;
+                        }
+                        if (display) {
+                            me.loadConsole(me.xtermjs, me.consoleType);
+                        }
                     }
                 },
             },
-- 
2.47.3



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