[PATCH qemu-server v2 0/4] rework fs-freeze agent property

[PATCH qemu-server v2 0/4] rework fs-freeze agent property

[Thomas Lamprecht]

Sorry for the quick v2 submission, forgot to stage a hunk to complete
the final patch and applied some of Fiona's patches [0] already so a
rebase was required anyway.

Somewhat RFC, albeit I'm pretty sure about this the more I think about
it, even though the transition cost is naturally not great.

Very likely clashes with Fiona's recent QGA-skip-freeze-if-frozen series
[0], but started this already a few days around and figured it might
still be relevant in the current form.

I probably would squash in the deprecation of the intermediate
guest-fsfreeze variant with the commit making the long-time
freeze-fs-on-backup property an alias to the new variant, which would
be named 'freeze-fs' hereafter.

Changes v1 -> v2:
 - rebase on master
 - apply hunk to change guest-fsfreeze to freeze-fs in should_fs_freeze
   too in patch 4/4.

Thomas Lamprecht (4):
  agent: add should_fs_freeze helper
  agent: treat freeze-fs-on-backup as alias for guest-fsfreeze
  tests: cfg2cmd: add agent guest-fsfreeze config tests
  qga: rename guest-fsfreeze to freeze-fs

 src/PVE/API2/Qemu.pm                          | 10 +++---
 src/PVE/QemuConfig.pm                         |  5 ++-
 src/PVE/QemuServer/Agent.pm                   | 35 ++++++++++++++-----
 src/PVE/VZDump/QemuServer.pm                  |  6 +---
 .../cfg2cmd/qga-fs-freeze-backup-legacy.conf  |  2 ++
 .../qga-fs-freeze-backup-legacy.conf.cmd      | 29 +++++++++++++++
 src/test/cfg2cmd/qga-fs-freeze.conf           |  2 ++
 src/test/cfg2cmd/qga-fs-freeze.conf.cmd       | 29 +++++++++++++++
 src/test/cfg2cmd/qga-minimal.conf             |  2 ++
 src/test/cfg2cmd/qga-minimal.conf.cmd         | 29 +++++++++++++++
 10 files changed, 126 insertions(+), 23 deletions(-)
 create mode 100644 src/test/cfg2cmd/qga-fs-freeze-backup-legacy.conf
 create mode 100644 src/test/cfg2cmd/qga-fs-freeze-backup-legacy.conf.cmd
 create mode 100644 src/test/cfg2cmd/qga-fs-freeze.conf
 create mode 100644 src/test/cfg2cmd/qga-fs-freeze.conf.cmd
 create mode 100644 src/test/cfg2cmd/qga-minimal.conf
 create mode 100644 src/test/cfg2cmd/qga-minimal.conf.cmd

-- 
2.47.3

[PATCH qemu-server v2 1/4] agent: add should_fs_freeze helper

[Thomas Lamprecht]

Add a centralized helper that checks whether guest filesystem
freeze/thaw should be attempted, combining the agent-enabled and
guest-fsfreeze setting checks. This replaces duplicated inline logic
and the scattered '// 1' default fallback across call sites.

No behavioral change intended, the helper returns the same result as
the previous inline expressions.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/PVE/API2/Qemu.pm        | 10 ++++------
 src/PVE/QemuConfig.pm       |  5 ++---
 src/PVE/QemuServer/Agent.pm | 15 +++++++++++++++
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index 1752f46a..020c68b2 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -374,8 +374,7 @@ my $import_from_volid = sub {
 
         my ($src_storeid) = PVE::Storage::parse_volume_id($src_volid);
 
-        my $qga = PVE::QemuServer::Agent::get_qga_key($src_conf, 'enabled')
-            && (PVE::QemuServer::Agent::get_qga_key($src_conf, 'guest-fsfreeze') // 1);
+        my $fs_freeze = PVE::QemuServer::Agent::should_fs_freeze($src_conf);
 
         return PVE::QemuServer::clone_disk(
             $storecfg,
@@ -385,7 +384,7 @@ my $import_from_volid = sub {
             $vollist,
             undef,
             undef,
-            $qga,
+            $fs_freeze,
             PVE::Storage::get_bandwidth_limit('clone', [$src_storeid, $dest_info->{storage}]),
         );
     };
@@ -4562,8 +4561,7 @@ __PACKAGE__->register_method({
                     $dest_info->{efisize} = PVE::QemuServer::get_efivars_size($oldconf)
                         if $opt eq 'efidisk0';
 
-                    my $qga = PVE::QemuServer::Agent::get_qga_key($oldconf, 'enabled')
-                        && (PVE::QemuServer::Agent::get_qga_key($oldconf, 'guest-fsfreeze') // 1);
+                    my $fs_freeze = PVE::QemuServer::Agent::should_fs_freeze($oldconf);
 
                     my $newdrive = PVE::QemuServer::clone_disk(
                         $storecfg,
@@ -4573,7 +4571,7 @@ __PACKAGE__->register_method({
                         $newvollist,
                         $jobs,
                         $completion,
-                        $qga,
+                        $fs_freeze,
                         $clonelimit,
                     );
 
diff --git a/src/PVE/QemuConfig.pm b/src/PVE/QemuConfig.pm
index 462d1d6d..839b6cd0 100644
--- a/src/PVE/QemuConfig.pm
+++ b/src/PVE/QemuConfig.pm
@@ -297,9 +297,8 @@ sub __snapshot_check_freeze_needed {
         return (
             $running,
             $running
-                && PVE::QemuServer::Agent::get_qga_key($config, 'enabled')
-                && PVE::QemuServer::Agent::qga_check_running($vmid)
-                && (PVE::QemuServer::Agent::get_qga_key($config, 'guest-fsfreeze') // 1),
+                && PVE::QemuServer::Agent::should_fs_freeze($config)
+                && PVE::QemuServer::Agent::qga_check_running($vmid),
         );
     } else {
         return ($running, 0);
diff --git a/src/PVE/QemuServer/Agent.pm b/src/PVE/QemuServer/Agent.pm
index 1e7b1895..7520dd6e 100644
--- a/src/PVE/QemuServer/Agent.pm
+++ b/src/PVE/QemuServer/Agent.pm
@@ -202,6 +202,21 @@ sub qemu_exec_status {
     return $res;
 }
 
+=head3 should_fs_freeze
+
+Returns whether guest filesystem freeze/thaw should be attempted based on the agent configuration.
+Does B<not> check whether the agent is actually running.
+
+=cut
+
+sub should_fs_freeze {
+    my ($conf) = @_;
+
+    my $agent = parse_guest_agent($conf);
+    return 0 if !$agent->{enabled};
+    return $agent->{'guest-fsfreeze'} // 1;
+}
+
 =head3 guest_fsfreeze
 
     guest_fsfreeze($vmid);
-- 
2.47.3

[PATCH qemu-server v2 2/4] agent: treat freeze-fs-on-backup as alias for guest-fsfreeze

[Thomas Lamprecht]

Alias the deprecated key to 'guest-fsfreeze' through the JSONSchema
alias key support so consumers see a single, consistent value.
Previously only the backup code path fell back to it, while snapshots,
replications, clones and imports silently ignored it.

Note that while alias support for top-level JSON schema has a bug [0],
the one for property strings - which this here is - works out fine.

[0]: https://lore.proxmox.com/all/20251205100317.1101549-1-d.csapak@proxmox.com/

The key stays in the schema indefinitely for old backup restore
compatibility rather than being removed in a future major release.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/PVE/QemuServer/Agent.pm  | 15 ++++++---------
 src/PVE/VZDump/QemuServer.pm |  6 +-----
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/src/PVE/QemuServer/Agent.pm b/src/PVE/QemuServer/Agent.pm
index 7520dd6e..b0abd7c0 100644
--- a/src/PVE/QemuServer/Agent.pm
+++ b/src/PVE/QemuServer/Agent.pm
@@ -35,25 +35,21 @@ our $agent_fmt = {
         optional => 1,
         default => 0,
     },
-    # TODO Remove for Proxmox VE 10
     'freeze-fs-on-backup' => {
-        description => "Deprecated: Use 'guest-fsfreeze' instead.\n\n"
-            . "Freeze/thaw guest filesystems on backup for consistency.",
         type => 'boolean',
         optional => 1,
-        default => 1,
+        alias => 'guest-fsfreeze', # keep for old backup restore compatibility
     },
     'guest-fsfreeze' => {
-        description =>
-            "Whether to issue the guest-fsfreeze-freeze and guest-fsfreeze-thaw QEMU guest agent"
-            . " commands.",
+        description => "Freeze guest filesystems through QGA for consistent disk state on"
+            . " operations such as snapshots, backups, replications and clones.",
         verbose_description =>
             "Whether to issue the guest-fsfreeze-freeze and guest-fsfreeze-thaw QEMU guest agent"
             . " commands. Backups in snapshot mode, clones, snapshots without RAM, importing"
             . " disks from a running guest, and replications normally issue a guest-fsfreeze-freeze"
             . " and a respective thaw command when the QEMU Guest agent option is enabled in the"
-            . " guest's configuration and the agent is running inside of the guest.\n\nWhen set, it"
-            . " will take precedence over 'freeze-fs-on-backup'.",
+            . " guest's configuration and the agent is running inside of the guest.\n\nThe deprecated"
+            . " 'freeze-fs-on-backup' setting is treated as an alias for this setting.",
         type => 'boolean',
         optional => 1,
         default => 1,
@@ -77,6 +73,7 @@ sub parse_guest_agent {
 
     # if the agent is disabled ignore the other potentially set properties
     return {} if !$res->{enabled};
+
     return $res;
 }
 
diff --git a/src/PVE/VZDump/QemuServer.pm b/src/PVE/VZDump/QemuServer.pm
index 0681661d..9f3cd191 100644
--- a/src/PVE/VZDump/QemuServer.pm
+++ b/src/PVE/VZDump/QemuServer.pm
@@ -1108,11 +1108,7 @@ sub qga_fs_freeze {
         return;
     }
 
-    my $freeze = PVE::QemuServer::Agent::get_qga_key($conf, 'guest-fsfreeze');
-    $freeze //= PVE::QemuServer::Agent::get_qga_key($conf, 'freeze-fs-on-backup');
-    $freeze //= 1;
-
-    if (!$freeze) {
+    if (!PVE::QemuServer::Agent::should_fs_freeze($self->{vmlist}->{$vmid})) {
         $self->loginfo("skipping guest-agent 'fs-freeze', disabled in VM options");
         return;
     }
-- 
2.47.3

[PATCH qemu-server v2 3/4] tests: cfg2cmd: add agent guest-fsfreeze config tests

[Thomas Lamprecht]

Ensure the different agent sub-property variants, including the
deprecated freeze-fs-on-backup key, are parsed without affecting
the QEMU command line.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---

Pondered about using a symlink for the expected .cmd file,  as they are
all the same, or alternatively add a new "these configs must be parsed
OK" test harness, but the files are rather small, git will deduplicate
and we can add such a harness anytime (out of scope here).

 .../cfg2cmd/qga-fs-freeze-backup-legacy.conf  |  2 ++
 .../qga-fs-freeze-backup-legacy.conf.cmd      | 29 +++++++++++++++++++
 src/test/cfg2cmd/qga-fs-freeze.conf           |  2 ++
 src/test/cfg2cmd/qga-fs-freeze.conf.cmd       | 29 +++++++++++++++++++
 src/test/cfg2cmd/qga-minimal.conf             |  2 ++
 src/test/cfg2cmd/qga-minimal.conf.cmd         | 29 +++++++++++++++++++
 6 files changed, 93 insertions(+)
 create mode 100644 src/test/cfg2cmd/qga-fs-freeze-backup-legacy.conf
 create mode 100644 src/test/cfg2cmd/qga-fs-freeze-backup-legacy.conf.cmd
 create mode 100644 src/test/cfg2cmd/qga-fs-freeze.conf
 create mode 100644 src/test/cfg2cmd/qga-fs-freeze.conf.cmd
 create mode 100644 src/test/cfg2cmd/qga-minimal.conf
 create mode 100644 src/test/cfg2cmd/qga-minimal.conf.cmd

diff --git a/src/test/cfg2cmd/qga-fs-freeze-backup-legacy.conf b/src/test/cfg2cmd/qga-fs-freeze-backup-legacy.conf
new file mode 100644
index 00000000..14ef474d
--- /dev/null
+++ b/src/test/cfg2cmd/qga-fs-freeze-backup-legacy.conf
@@ -0,0 +1,2 @@
+# TEST: Ensure the deprecated freeze-fs-on-backup agent key is still parsed.
+agent: enabled=1,freeze-fs-on-backup=0
diff --git a/src/test/cfg2cmd/qga-fs-freeze-backup-legacy.conf.cmd b/src/test/cfg2cmd/qga-fs-freeze-backup-legacy.conf.cmd
new file mode 100644
index 00000000..41b185e0
--- /dev/null
+++ b/src/test/cfg2cmd/qga-fs-freeze-backup-legacy.conf.cmd
@@ -0,0 +1,29 @@
+/usr/bin/kvm \
+  -id 8006 \
+  -name 'vm8006,debug-threads=on' \
+  -no-shutdown \
+  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server=on,wait=off' \
+  -mon 'chardev=qmp,mode=control' \
+  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect-ms=5000' \
+  -mon 'chardev=qmp-event,mode=control' \
+  -pidfile /var/run/qemu-server/8006.pid \
+  -daemonize \
+  -smp '1,sockets=1,cores=1,maxcpus=1' \
+  -nodefaults \
+  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
+  -vnc 'unix:/var/run/qemu-server/8006.vnc,password=on' \
+  -cpu kvm64,enforce,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep \
+  -m 512 \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1' \
+  -device 'pci-bridge,id=pci.1,chassis_nr=1,bus=pci.0,addr=0x1e' \
+  -device 'pci-bridge,id=pci.2,chassis_nr=2,bus=pci.0,addr=0x1f' \
+  -device 'piix3-usb-uhci,id=uhci,bus=pci.0,addr=0x1.0x2' \
+  -device 'usb-tablet,id=tablet,bus=uhci.0,port=1' \
+  -device 'VGA,id=vga,bus=pci.0,addr=0x2' \
+  -chardev 'socket,path=/var/run/qemu-server/8006.qga,server=on,wait=off,id=qga0' \
+  -device 'virtio-serial,id=qga0,bus=pci.0,addr=0x8' \
+  -device 'virtserialport,chardev=qga0,name=org.qemu.guest_agent.0' \
+  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3,free-page-reporting=on' \
+  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
+  -machine 'type=pc+pve0'
diff --git a/src/test/cfg2cmd/qga-fs-freeze.conf b/src/test/cfg2cmd/qga-fs-freeze.conf
new file mode 100644
index 00000000..c32e143e
--- /dev/null
+++ b/src/test/cfg2cmd/qga-fs-freeze.conf
@@ -0,0 +1,2 @@
+# TEST: Ensure agent sub-properties do not affect the QEMU command line.
+agent: 1,guest-fsfreeze=1
diff --git a/src/test/cfg2cmd/qga-fs-freeze.conf.cmd b/src/test/cfg2cmd/qga-fs-freeze.conf.cmd
new file mode 100644
index 00000000..41b185e0
--- /dev/null
+++ b/src/test/cfg2cmd/qga-fs-freeze.conf.cmd
@@ -0,0 +1,29 @@
+/usr/bin/kvm \
+  -id 8006 \
+  -name 'vm8006,debug-threads=on' \
+  -no-shutdown \
+  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server=on,wait=off' \
+  -mon 'chardev=qmp,mode=control' \
+  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect-ms=5000' \
+  -mon 'chardev=qmp-event,mode=control' \
+  -pidfile /var/run/qemu-server/8006.pid \
+  -daemonize \
+  -smp '1,sockets=1,cores=1,maxcpus=1' \
+  -nodefaults \
+  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
+  -vnc 'unix:/var/run/qemu-server/8006.vnc,password=on' \
+  -cpu kvm64,enforce,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep \
+  -m 512 \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1' \
+  -device 'pci-bridge,id=pci.1,chassis_nr=1,bus=pci.0,addr=0x1e' \
+  -device 'pci-bridge,id=pci.2,chassis_nr=2,bus=pci.0,addr=0x1f' \
+  -device 'piix3-usb-uhci,id=uhci,bus=pci.0,addr=0x1.0x2' \
+  -device 'usb-tablet,id=tablet,bus=uhci.0,port=1' \
+  -device 'VGA,id=vga,bus=pci.0,addr=0x2' \
+  -chardev 'socket,path=/var/run/qemu-server/8006.qga,server=on,wait=off,id=qga0' \
+  -device 'virtio-serial,id=qga0,bus=pci.0,addr=0x8' \
+  -device 'virtserialport,chardev=qga0,name=org.qemu.guest_agent.0' \
+  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3,free-page-reporting=on' \
+  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
+  -machine 'type=pc+pve0'
diff --git a/src/test/cfg2cmd/qga-minimal.conf b/src/test/cfg2cmd/qga-minimal.conf
new file mode 100644
index 00000000..86560065
--- /dev/null
+++ b/src/test/cfg2cmd/qga-minimal.conf
@@ -0,0 +1,2 @@
+# TEST: Ensure agent sub-properties do not affect the QEMU command line.
+agent: 1
diff --git a/src/test/cfg2cmd/qga-minimal.conf.cmd b/src/test/cfg2cmd/qga-minimal.conf.cmd
new file mode 100644
index 00000000..41b185e0
--- /dev/null
+++ b/src/test/cfg2cmd/qga-minimal.conf.cmd
@@ -0,0 +1,29 @@
+/usr/bin/kvm \
+  -id 8006 \
+  -name 'vm8006,debug-threads=on' \
+  -no-shutdown \
+  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server=on,wait=off' \
+  -mon 'chardev=qmp,mode=control' \
+  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect-ms=5000' \
+  -mon 'chardev=qmp-event,mode=control' \
+  -pidfile /var/run/qemu-server/8006.pid \
+  -daemonize \
+  -smp '1,sockets=1,cores=1,maxcpus=1' \
+  -nodefaults \
+  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
+  -vnc 'unix:/var/run/qemu-server/8006.vnc,password=on' \
+  -cpu kvm64,enforce,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep \
+  -m 512 \
+  -global 'PIIX4_PM.disable_s3=1' \
+  -global 'PIIX4_PM.disable_s4=1' \
+  -device 'pci-bridge,id=pci.1,chassis_nr=1,bus=pci.0,addr=0x1e' \
+  -device 'pci-bridge,id=pci.2,chassis_nr=2,bus=pci.0,addr=0x1f' \
+  -device 'piix3-usb-uhci,id=uhci,bus=pci.0,addr=0x1.0x2' \
+  -device 'usb-tablet,id=tablet,bus=uhci.0,port=1' \
+  -device 'VGA,id=vga,bus=pci.0,addr=0x2' \
+  -chardev 'socket,path=/var/run/qemu-server/8006.qga,server=on,wait=off,id=qga0' \
+  -device 'virtio-serial,id=qga0,bus=pci.0,addr=0x8' \
+  -device 'virtserialport,chardev=qga0,name=org.qemu.guest_agent.0' \
+  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3,free-page-reporting=on' \
+  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
+  -machine 'type=pc+pve0'
-- 
2.47.3

[PATCH qemu-server v2 4/4] qga: rename guest-fsfreeze to freeze-fs

[Thomas Lamprecht]

These settings are already in the (QEMU guest) agent property, and we
do not use the guest- prefix for any of the existing properties
(fstrim or freeze-fs-on-backup), and moving freeze-fs-on-backup to a
generic variant is less confusing if one just drops the "-on-backup"
part, i.e. no point in inventing a new name schema just for the sake
of it.

We already rolled this the guest-fsfreeze name to pve-test, so lets
add an alias, but as we do not provide API stability guarantees for
test packages this is really just for convenience for anybody that
tested this, I'm more than fine with dropping such a (never stable)
released key again on a major release with the respective upgrade
check in our pveXtoY tool.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/PVE/QemuServer/Agent.pm         | 9 +++++++--
 src/test/cfg2cmd/qga-fs-freeze.conf | 2 +-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/src/PVE/QemuServer/Agent.pm b/src/PVE/QemuServer/Agent.pm
index b0abd7c0..23dbb3ff 100644
--- a/src/PVE/QemuServer/Agent.pm
+++ b/src/PVE/QemuServer/Agent.pm
@@ -38,9 +38,14 @@ our $agent_fmt = {
     'freeze-fs-on-backup' => {
         type => 'boolean',
         optional => 1,
-        alias => 'guest-fsfreeze', # keep for old backup restore compatibility
+        alias => 'freeze-fs', # keep for old backup restore compatibility
     },
     'guest-fsfreeze' => {
+        type => 'boolean',
+        optional => 1,
+        alias => 'freeze-fs', # TODO: was only on test repo, drop with PVE 10.
+    },
+    'freeze-fs' => {
         description => "Freeze guest filesystems through QGA for consistent disk state on"
             . " operations such as snapshots, backups, replications and clones.",
         verbose_description =>
@@ -211,7 +216,7 @@ sub should_fs_freeze {
 
     my $agent = parse_guest_agent($conf);
     return 0 if !$agent->{enabled};
-    return $agent->{'guest-fsfreeze'} // 1;
+    return $agent->{'freeze-fs'} // 1;
 }
 
 =head3 guest_fsfreeze
diff --git a/src/test/cfg2cmd/qga-fs-freeze.conf b/src/test/cfg2cmd/qga-fs-freeze.conf
index c32e143e..809b7d43 100644
--- a/src/test/cfg2cmd/qga-fs-freeze.conf
+++ b/src/test/cfg2cmd/qga-fs-freeze.conf
@@ -1,2 +1,2 @@
 # TEST: Ensure agent sub-properties do not affect the QEMU command line.
-agent: 1,guest-fsfreeze=1
+agent: 1,freeze-fs=1
-- 
2.47.3

Re: [PATCH qemu-server v2 4/4] qga: rename guest-fsfreeze to freeze-fs

[Maximiliano Sandoval]

Thomas Lamprecht <t.lamprecht@proxmox.com> writes:

> These settings are already in the (QEMU guest) agent property, and we
> do not use the guest- prefix for any of the existing properties
> (fstrim or freeze-fs-on-backup), and moving freeze-fs-on-backup to a
> generic variant is less confusing if one just drops the "-on-backup"
> part, i.e. no point in inventing a new name schema just for the sake
> of it.
>
> We already rolled this the guest-fsfreeze name to pve-test, so lets
> add an alias, but as we do not provide API stability guarantees for
> test packages this is really just for convenience for anybody that
> tested this, I'm more than fine with dropping such a (never stable)
> released key again on a major release with the respective upgrade
> check in our pveXtoY tool.
>
> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>

For the sake of documenting, the rationale behind the "guest-fsfreeze"
name was using the guest agent command names, namely
"guest-fsfreeze-freeze" and "guest-fsfreeze-thaw", as a base.

For a long time I struggled to find the actual name of these commands
and their documentation since they are named ever-so-slightly
differently each time they appear on the docs or UI: "fsfreeze",
"freeze", "fs-freeze", etc.

If anything, I personally found the old name to be a source of confusion.

> ---
>  src/PVE/QemuServer/Agent.pm         | 9 +++++++--
>  src/test/cfg2cmd/qga-fs-freeze.conf | 2 +-
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/src/PVE/QemuServer/Agent.pm b/src/PVE/QemuServer/Agent.pm
> index b0abd7c0..23dbb3ff 100644
> --- a/src/PVE/QemuServer/Agent.pm
> +++ b/src/PVE/QemuServer/Agent.pm
> @@ -38,9 +38,14 @@ our $agent_fmt = {
>      'freeze-fs-on-backup' => {
>          type => 'boolean',
>          optional => 1,
> -        alias => 'guest-fsfreeze', # keep for old backup restore compatibility
> +        alias => 'freeze-fs', # keep for old backup restore compatibility
>      },
>      'guest-fsfreeze' => {
> +        type => 'boolean',
> +        optional => 1,
> +        alias => 'freeze-fs', # TODO: was only on test repo, drop with PVE 10.
> +    },
> +    'freeze-fs' => {
>          description => "Freeze guest filesystems through QGA for consistent disk state on"
>              . " operations such as snapshots, backups, replications and clones.",
>          verbose_description =>
> @@ -211,7 +216,7 @@ sub should_fs_freeze {
>  
>      my $agent = parse_guest_agent($conf);
>      return 0 if !$agent->{enabled};
> -    return $agent->{'guest-fsfreeze'} // 1;
> +    return $agent->{'freeze-fs'} // 1;
>  }
>  
>  =head3 guest_fsfreeze
> diff --git a/src/test/cfg2cmd/qga-fs-freeze.conf b/src/test/cfg2cmd/qga-fs-freeze.conf
> index c32e143e..809b7d43 100644
> --- a/src/test/cfg2cmd/qga-fs-freeze.conf
> +++ b/src/test/cfg2cmd/qga-fs-freeze.conf
> @@ -1,2 +1,2 @@
>  # TEST: Ensure agent sub-properties do not affect the QEMU command line.
> -agent: 1,guest-fsfreeze=1
> +agent: 1,freeze-fs=1

-- 
Maximiliano

Re: [PATCH qemu-server v2 1/4] agent: add should_fs_freeze helper

[Fiona Ebner]

I feel like we should either rename the existing guest_fs{freeze,thaw}()
functions or this one for consistency ('fs_freeze' versus 'fsfreeze').

Am 25.03.26 um 10:33 PM schrieb Thomas Lamprecht:
> Add a centralized helper that checks whether guest filesystem
> freeze/thaw should be attempted, combining the agent-enabled and
> guest-fsfreeze setting checks. This replaces duplicated inline logic
> and the scattered '// 1' default fallback across call sites.
> 
> No behavioral change intended, the helper returns the same result as
> the previous inline expressions.
> 
> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

Re: [PATCH qemu-server v2 2/4] agent: treat freeze-fs-on-backup as alias for guest-fsfreeze

[Fiona Ebner]

Am 25.03.26 um 10:34 PM schrieb Thomas Lamprecht:
> Alias the deprecated key to 'guest-fsfreeze' through the JSONSchema
> alias key support so consumers see a single, consistent value.
> Previously only the backup code path fell back to it, while snapshots,
> replications, clones and imports silently ignored it.
> 
> Note that while alias support for top-level JSON schema has a bug [0],
> the one for property strings - which this here is - works out fine.
> 
> [0]: https://lore.proxmox.com/all/20251205100317.1101549-1-d.csapak@proxmox.com/
> 
> The key stays in the schema indefinitely for old backup restore
> compatibility rather than being removed in a future major release.
> 

One potentially surprising thing (I guess another bug in JSONSchema?) is
that it's possible to set
qm set 100 --agent 1,freeze-fs=0,freeze-fs-on-backup=1
and the second option (which in this case is the deprecated one) will
then override the first when parsing. The check for duplicate keys
precedes the resolution of the alias in parse_property_string():

https://git.proxmox.com/?p=pve-common.git;a=blob;f=src/PVE/JSONSchema.pm;h=17e7126a6c42f8326a1da890680af10b6106d906;hb=17c6c3bdcf018756b4b48589b5485663103a9843#l1069

> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

with two nits below:

---snip---

> @@ -77,6 +73,7 @@ sub parse_guest_agent {
>  
>      # if the agent is disabled ignore the other potentially set properties
>      return {} if !$res->{enabled};
> +
>      return $res;
>  }

Nit: unrelated change

>  
> diff --git a/src/PVE/VZDump/QemuServer.pm b/src/PVE/VZDump/QemuServer.pm
> index 0681661d..9f3cd191 100644
> --- a/src/PVE/VZDump/QemuServer.pm
> +++ b/src/PVE/VZDump/QemuServer.pm
> @@ -1108,11 +1108,7 @@ sub qga_fs_freeze {
>          return;
>      }
>  
> -    my $freeze = PVE::QemuServer::Agent::get_qga_key($conf, 'guest-fsfreeze');
> -    $freeze //= PVE::QemuServer::Agent::get_qga_key($conf, 'freeze-fs-on-backup');
> -    $freeze //= 1;
> -
> -    if (!$freeze) {
> +    if (!PVE::QemuServer::Agent::should_fs_freeze($self->{vmlist}->{$vmid})) {

Nit: could re-use $conf

>          $self->loginfo("skipping guest-agent 'fs-freeze', disabled in VM options");
>          return;
>      }

Re: [PATCH qemu-server v2 3/4] tests: cfg2cmd: add agent guest-fsfreeze config tests

[Fiona Ebner]

Am 25.03.26 um 10:34 PM schrieb Thomas Lamprecht:
> Ensure the different agent sub-property variants, including the
> deprecated freeze-fs-on-backup key, are parsed without affecting
> the QEMU command line.
> 
> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

Re: [PATCH qemu-server v2 4/4] qga: rename guest-fsfreeze to freeze-fs

[Fiona Ebner]

Am 25.03.26 um 10:34 PM schrieb Thomas Lamprecht:
> These settings are already in the (QEMU guest) agent property, and we
> do not use the guest- prefix for any of the existing properties
> (fstrim or freeze-fs-on-backup), and moving freeze-fs-on-backup to a
> generic variant is less confusing if one just drops the "-on-backup"
> part, i.e. no point in inventing a new name schema just for the sake
> of it.

There's an inconsistency in naming between fstrim and freeze-fs too, but
maybe we shouldn't care.

> 
> We already rolled this the guest-fsfreeze name to pve-test, so lets
> add an alias, but as we do not provide API stability guarantees for
> test packages this is really just for convenience for anybody that
> tested this, I'm more than fine with dropping such a (never stable)
> released key again on a major release with the respective upgrade
> check in our pveXtoY tool.
> 
> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

Re: [PATCH qemu-server v2 4/4] qga: rename guest-fsfreeze to freeze-fs

[Thomas Lamprecht]

Am 26.03.26 um 10:07 schrieb Maximiliano Sandoval:
> Thomas Lamprecht <t.lamprecht@proxmox.com> writes:
> 
>> These settings are already in the (QEMU guest) agent property, and we
>> do not use the guest- prefix for any of the existing properties
>> (fstrim or freeze-fs-on-backup), and moving freeze-fs-on-backup to a
>> generic variant is less confusing if one just drops the "-on-backup"
>> part, i.e. no point in inventing a new name schema just for the sake
>> of it.
>>
>> We already rolled this the guest-fsfreeze name to pve-test, so lets
>> add an alias, but as we do not provide API stability guarantees for
>> test packages this is really just for convenience for anybody that
>> tested this, I'm more than fine with dropping such a (never stable)
>> released key again on a major release with the respective upgrade
>> check in our pveXtoY tool.
>>
>> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> For the sake of documenting, the rationale behind the "guest-fsfreeze"
> name was using the guest agent command names, namely
> "guest-fsfreeze-freeze" and "guest-fsfreeze-thaw", as a base.

I got that, but the specific low-level QMP QGA commands are a
implementation detail, leaking that is IMO not the best way to reduce
any confusion, which IMO is rather unlikely to affect end users much,
as those don't really care what the underlying commands are named after.

> For a long time I struggled to find the actual name of these commands
> and their documentation since they are named ever-so-slightly
> differently each time they appear on the docs or UI: "fsfreeze",
> "freeze", "fs-freeze", etc.

If, then this makes it sound like the documentation should improve, if
this is really something our users struggle with too - which I'd find
slightly surprising, tbh.

> If anything, I personally found the old name to be a source of confusion.

For what exactly? And wouldn't we then have to encode both, the freeze
and thaw also in the name? Like guest-fsfreeze-freeze-and-thaw?

Re: [PATCH qemu-server v2 2/4] agent: treat freeze-fs-on-backup as alias for guest-fsfreeze

[Thomas Lamprecht]

Am 26.03.26 um 13:55 schrieb Fiona Ebner:
> Am 25.03.26 um 10:34 PM schrieb Thomas Lamprecht:
>> Alias the deprecated key to 'guest-fsfreeze' through the JSONSchema
>> alias key support so consumers see a single, consistent value.
>> Previously only the backup code path fell back to it, while snapshots,
>> replications, clones and imports silently ignored it.
>>
>> Note that while alias support for top-level JSON schema has a bug [0],
>> the one for property strings - which this here is - works out fine.
>>
>> [0]: https://lore.proxmox.com/all/20251205100317.1101549-1-d.csapak@proxmox.com/
>>
>> The key stays in the schema indefinitely for old backup restore
>> compatibility rather than being removed in a future major release.
>>
> 
> One potentially surprising thing (I guess another bug in JSONSchema?) is
> that it's possible to set
> qm set 100 --agent 1,freeze-fs=0,freeze-fs-on-backup=1
> and the second option (which in this case is the deprecated one) will
> then override the first when parsing. The check for duplicate keys
> precedes the resolution of the alias in parse_property_string():
> 
> https://git.proxmox.com/?p=pve-common.git;a=blob;f=src/PVE/JSONSchema.pm;h=17e7126a6c42f8326a1da890680af10b6106d906;hb=17c6c3bdcf018756b4b48589b5485663103a9843#l1069

Ah, right, that pre-existing issue should be fixed in latest pve-common [0].

[0]: https://git.proxmox.com/?p=pve-common.git;a=commitdiff;h=5d843877321a69255dd55b49cd1c8e13222dd914

applied: [PATCH qemu-server v2 0/4] rework fs-freeze agent property

[Thomas Lamprecht]

Am 25.03.26 um 22:34 schrieb Thomas Lamprecht:
> Sorry for the quick v2 submission, forgot to stage a hunk to complete
> the final patch and applied some of Fiona's patches [0] already so a
> rebase was required anyway.
> 
> Somewhat RFC, albeit I'm pretty sure about this the more I think about
> it, even though the transition cost is naturally not great.
> 
> Very likely clashes with Fiona's recent QGA-skip-freeze-if-frozen series
> [0], but started this already a few days around and figured it might
> still be relevant in the current form.
> 
> I probably would squash in the deprecation of the intermediate
> guest-fsfreeze variant with the commit making the long-time
> freeze-fs-on-backup property an alias to the new variant, which would
> be named 'freeze-fs' hereafter.
> 
> Changes v1 -> v2:
>  - rebase on master
>  - apply hunk to change guest-fsfreeze to freeze-fs in should_fs_freeze
>    too in patch 4/4.
> 
> Thomas Lamprecht (4):
>   agent: add should_fs_freeze helper
>   agent: treat freeze-fs-on-backup as alias for guest-fsfreeze
>   tests: cfg2cmd: add agent guest-fsfreeze config tests
>   qga: rename guest-fsfreeze to freeze-fs
> 
>  src/PVE/API2/Qemu.pm                          | 10 +++---
>  src/PVE/QemuConfig.pm                         |  5 ++-
>  src/PVE/QemuServer/Agent.pm                   | 35 ++++++++++++++-----
>  src/PVE/VZDump/QemuServer.pm                  |  6 +---
>  .../cfg2cmd/qga-fs-freeze-backup-legacy.conf  |  2 ++
>  .../qga-fs-freeze-backup-legacy.conf.cmd      | 29 +++++++++++++++
>  src/test/cfg2cmd/qga-fs-freeze.conf           |  2 ++
>  src/test/cfg2cmd/qga-fs-freeze.conf.cmd       | 29 +++++++++++++++
>  src/test/cfg2cmd/qga-minimal.conf             |  2 ++
>  src/test/cfg2cmd/qga-minimal.conf.cmd         | 29 +++++++++++++++
>  10 files changed, 126 insertions(+), 23 deletions(-)
>  create mode 100644 src/test/cfg2cmd/qga-fs-freeze-backup-legacy.conf
>  create mode 100644 src/test/cfg2cmd/qga-fs-freeze-backup-legacy.conf.cmd
>  create mode 100644 src/test/cfg2cmd/qga-fs-freeze.conf
>  create mode 100644 src/test/cfg2cmd/qga-fs-freeze.conf.cmd
>  create mode 100644 src/test/cfg2cmd/qga-minimal.conf
>  create mode 100644 src/test/cfg2cmd/qga-minimal.conf.cmd
> 


applied with Fiona's findings and nits addressed and R-b added

Re: [PATCH qemu-server v2 1/4] agent: add should_fs_freeze helper

[Thomas Lamprecht]

Am 26.03.26 um 13:54 schrieb Fiona Ebner:
> I feel like we should either rename the existing guest_fs{freeze,thaw}()
> functions or this one for consistency ('fs_freeze' versus 'fsfreeze').

Agree, that would be nicer to read too besides consistency.

Re: [PATCH qemu-server v2 2/4] agent: treat freeze-fs-on-backup as alias for guest-fsfreeze

[Fiona Ebner]

Am 27.03.26 um 12:04 AM schrieb Thomas Lamprecht:
> Am 26.03.26 um 13:55 schrieb Fiona Ebner:
>> Am 25.03.26 um 10:34 PM schrieb Thomas Lamprecht:
>>> Alias the deprecated key to 'guest-fsfreeze' through the JSONSchema
>>> alias key support so consumers see a single, consistent value.
>>> Previously only the backup code path fell back to it, while snapshots,
>>> replications, clones and imports silently ignored it.
>>>
>>> Note that while alias support for top-level JSON schema has a bug [0],
>>> the one for property strings - which this here is - works out fine.
>>>
>>> [0]: https://lore.proxmox.com/all/20251205100317.1101549-1-d.csapak@proxmox.com/
>>>
>>> The key stays in the schema indefinitely for old backup restore
>>> compatibility rather than being removed in a future major release.
>>>
>>
>> One potentially surprising thing (I guess another bug in JSONSchema?) is
>> that it's possible to set
>> qm set 100 --agent 1,freeze-fs=0,freeze-fs-on-backup=1
>> and the second option (which in this case is the deprecated one) will
>> then override the first when parsing. The check for duplicate keys
>> precedes the resolution of the alias in parse_property_string():
>>
>> https://git.proxmox.com/?p=pve-common.git;a=blob;f=src/PVE/JSONSchema.pm;h=17e7126a6c42f8326a1da890680af10b6106d906;hb=17c6c3bdcf018756b4b48589b5485663103a9843#l1069
> 
> Ah, right, that pre-existing issue should be fixed in latest pve-common [0].
> 
> [0]: https://git.proxmox.com/?p=pve-common.git;a=commitdiff;h=5d843877321a69255dd55b49cd1c8e13222dd914

Thanks!