[PATCH-SERIES qemu-server v2 0/7] fix #7383: agent: fsfreeze: skip freeze if already frozen

[PATCH-SERIES qemu-server v2 0/7] fix #7383: agent: fsfreeze: skip freeze if already frozen

[Fiona Ebner]

Changes in v2:
* Drop already applied patches.
* Rebase on master, making use of the new should_fs_freeze() function.
* Add patch to switch to v5.36 and use subroutine signatures.
* Add patch renaming guest_fs{freeze,thaw} to guest_fs_{freeze,thaw}.
* Drop patch for changing signature of get_qga_key().
* Rename guest_fsfreeze_applicable() to guest_fs_freeze_applicable().
* Simplify log function (usage) in guest_fsfreeze_applicable().
* Drop $is_backup argument from guest_fs_freeze_applicable() now that
  there is no backup-specific option anymore (is an alias for the
  general one).
* Pass guest agent config for move disk caller of clone_disk() for
  completenetss.

The first patch migrates the agent module to use v5.36 and subroutine
signatures.

The second patch renames guest_fs{freeze,thaw} to
guest_fs_{freeze,thaw} for consistency with should_fs_freeze().

The next four patches lead up to harmonizing the checks for guest
filesystem freeze which previously were duplicated to three different
places and improve encapsulation of the agent module.

The last patch closes bug #7383 and checks if the filesystem is
already frozen before attempting freeze to avoid a confusing error
message.

qemu-server:

Fiona Ebner (7):
  agent: migrate to v5.36 and use subroutine signatures
  agent: rename guest_fs{freeze,thaw} to guest_fs_{freeze,thaw}
  agent: parse: change signature to take property string rather than
    full VM config
  agent: should fs freeze: change signature to take property string
    rather than full VM config
  clone disk/block jobs: change signatures to take guest agent property
    string
  agent: fs freeze: harmonize checks for guest fs freeze
  fix #7383: agent: fsfreeze: skip freeze if already frozen

 src/PVE/API2/Qemu.pm           |  10 +--
 src/PVE/QMPClient.pm           |   2 +-
 src/PVE/QemuConfig.pm          |  18 +++--
 src/PVE/QemuServer.pm          |   8 +--
 src/PVE/QemuServer/Agent.pm    | 121 +++++++++++++++++++++------------
 src/PVE/QemuServer/BlockJob.pm |  19 ++----
 src/PVE/VZDump/QemuServer.pm   |  24 +++----
 7 files changed, 109 insertions(+), 93 deletions(-)


Summary over all repositories:
  7 files changed, 109 insertions(+), 93 deletions(-)

-- 
Generated by git-murpp 0.5.0

[PATCH qemu-server v2 1/7] agent: migrate to v5.36 and use subroutine signatures

[Fiona Ebner]

parse_guest_agent($conf)
get_qga_key($conf, $key)
assert_agent_available($vmid, $conf)
agent_cmd($vmid, $conf, $cmd, $params, $errormsg)
qemu_exec($vmid, $conf, $input_data, $cmd)
qemu_exec_status($vmid, $conf, $pid)
should_fs_freeze($conf)
guest_fsfreeze($vmid)
guest_fsthaw($vmid)

  For these functions, all arguments are declared as mandatory, and
  all existing callers pass all arguments.

qga_check_running($vmid, $nowarn = 0)

  $vmid is mandatory, all existing callers pass the first argument,
  $nowarn is set to 0 by default for compatibility.

check_agent_error($result, $errmsg, $noerr = 0)

  $result and $errmsg are mandatory, all existing callers pass the
  first two arguments, $noerr is set to 0 by default for
  compatibility.

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

New in v2.

 src/PVE/QemuServer/Agent.pm | 47 ++++++++++---------------------------
 1 file changed, 12 insertions(+), 35 deletions(-)

diff --git a/src/PVE/QemuServer/Agent.pm b/src/PVE/QemuServer/Agent.pm
index 5a3eb983..ec3827d6 100644
--- a/src/PVE/QemuServer/Agent.pm
+++ b/src/PVE/QemuServer/Agent.pm
@@ -1,7 +1,6 @@
 package PVE::QemuServer::Agent;
 
-use strict;
-use warnings;
+use v5.36;
 
 use JSON;
 use MIME::Base64 qw(decode_base64 encode_base64);
@@ -62,9 +61,7 @@ our $agent_fmt = {
     },
 };
 
-sub parse_guest_agent {
-    my ($conf) = @_;
-
+sub parse_guest_agent($conf) {
     return {} if !defined($conf->{agent});
 
     my $res = eval { PVE::JSONSchema::parse_property_string($agent_fmt, $conf->{agent}) };
@@ -75,17 +72,14 @@ sub parse_guest_agent {
     return $res;
 }
 
-sub get_qga_key {
-    my ($conf, $key) = @_;
+sub get_qga_key($conf, $key) {
     return undef if !defined($conf->{agent});
 
     my $agent = parse_guest_agent($conf);
     return $agent->{$key};
 }
 
-sub qga_check_running {
-    my ($vmid, $nowarn) = @_;
-
+sub qga_check_running($vmid, $nowarn = 0) {
     eval { PVE::QemuServer::Monitor::mon_cmd($vmid, "guest-ping", timeout => 3); };
     if ($@) {
         warn "QEMU Guest Agent is not running - $@" if !$nowarn;
@@ -94,10 +88,7 @@ sub qga_check_running {
     return 1;
 }
 
-sub check_agent_error {
-    my ($result, $errmsg, $noerr) = @_;
-
-    $errmsg //= '';
+sub check_agent_error($result, $errmsg, $noerr = 0) {
     my $error = '';
     if (ref($result) eq 'HASH' && $result->{error} && $result->{error}->{desc}) {
         $error = "Agent error: $result->{error}->{desc}\n";
@@ -115,18 +106,14 @@ sub check_agent_error {
     return 1;
 }
 
-sub assert_agent_available {
-    my ($vmid, $conf) = @_;
-
+sub assert_agent_available($vmid, $conf) {
     die "No QEMU guest agent configured\n" if !defined($conf->{agent});
     die "VM $vmid is not running\n" if !PVE::QemuServer::Helpers::vm_running_locally($vmid);
     die "QEMU guest agent is not running\n" if !qga_check_running($vmid, 1);
 }
 
 # loads config, checks if available, executes command, checks for errors
-sub agent_cmd {
-    my ($vmid, $conf, $cmd, $params, $errormsg) = @_;
-
+sub agent_cmd($vmid, $conf, $cmd, $params, $errormsg) {
     assert_agent_available($vmid, $conf);
 
     my $res = PVE::QemuServer::Monitor::mon_cmd($vmid, "guest-$cmd", %$params);
@@ -135,9 +122,7 @@ sub agent_cmd {
     return $res;
 }
 
-sub qemu_exec {
-    my ($vmid, $conf, $input_data, $cmd) = @_;
-
+sub qemu_exec($vmid, $conf, $input_data, $cmd) {
     my $args = {
         'capture-output' => JSON::true,
     };
@@ -165,9 +150,7 @@ sub qemu_exec {
     return $res;
 }
 
-sub qemu_exec_status {
-    my ($vmid, $conf, $pid) = @_;
-
+sub qemu_exec_status($vmid, $conf, $pid) {
     my $res =
         agent_cmd($vmid, $conf, "exec-status", { pid => $pid }, "can't get exec status for '$pid'");
 
@@ -204,9 +187,7 @@ Does B<not> check whether the agent is actually running.
 
 =cut
 
-sub should_fs_freeze {
-    my ($conf) = @_;
-
+sub should_fs_freeze($conf) {
     my $agent = parse_guest_agent($conf);
     return 0 if !$agent->{enabled};
     return $agent->{'freeze-fs'} // 1;
@@ -235,9 +216,7 @@ the time the socket is blocked after a lost command is at most 10 minutes.
 
 =cut
 
-sub guest_fsfreeze {
-    my ($vmid) = @_;
-
+sub guest_fsfreeze($vmid) {
     my $timeout = 10 * 60;
 
     my $result = eval {
@@ -290,9 +269,7 @@ See C<$guest_fsfreeze> for more details.
 
 =cut
 
-sub guest_fsthaw {
-    my ($vmid) = @_;
-
+sub guest_fsthaw($vmid) {
     my $res = PVE::QemuServer::Monitor::mon_cmd($vmid, "guest-fsfreeze-thaw");
     check_agent_error($res, "unable to thaw guest filesystem");
 
-- 
2.47.3

[PATCH qemu-server v2 2/7] agent: rename guest_fs{freeze,thaw} to guest_fs_{freeze,thaw}

[Fiona Ebner]

For better consistency with should_fs_freeze().

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

New in v2.

 src/PVE/QMPClient.pm           |  2 +-
 src/PVE/QemuConfig.pm          |  4 ++--
 src/PVE/QemuServer/Agent.pm    | 14 +++++++-------
 src/PVE/QemuServer/BlockJob.pm |  4 ++--
 src/PVE/VZDump/QemuServer.pm   |  4 ++--
 5 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/src/PVE/QMPClient.pm b/src/PVE/QMPClient.pm
index 723123a6..bb4c5275 100644
--- a/src/PVE/QMPClient.pm
+++ b/src/PVE/QMPClient.pm
@@ -108,7 +108,7 @@ sub cmd {
         } elsif ($cmd->{execute} =~ m/^(eject|change)/) {
             $timeout = 60; # note: cdrom mount command is slow
         } elsif ($cmd->{execute} eq 'guest-fsfreeze-freeze') {
-            # consider using the guest_fsfreeze() helper in Agent.pm
+            # consider using the guest_fs_freeze() helper in Agent.pm
             #
             # freeze syncs all guest FS, if we kill it it stays in an unfreezable
             # locked state with high probability, so use an generous timeout
diff --git a/src/PVE/QemuConfig.pm b/src/PVE/QemuConfig.pm
index 839b6cd0..f93ee5bb 100644
--- a/src/PVE/QemuConfig.pm
+++ b/src/PVE/QemuConfig.pm
@@ -309,10 +309,10 @@ sub __snapshot_freeze {
     my ($class, $vmid, $unfreeze) = @_;
 
     if ($unfreeze) {
-        eval { PVE::QemuServer::Agent::guest_fsthaw($vmid); };
+        eval { PVE::QemuServer::Agent::guest_fs_thaw($vmid); };
         warn "guest-fsfreeze-thaw problems - $@" if $@;
     } else {
-        eval { PVE::QemuServer::Agent::guest_fsfreeze($vmid); };
+        eval { PVE::QemuServer::Agent::guest_fs_freeze($vmid); };
         warn $@ if $@;
     }
 }
diff --git a/src/PVE/QemuServer/Agent.pm b/src/PVE/QemuServer/Agent.pm
index ec3827d6..a68dd8d7 100644
--- a/src/PVE/QemuServer/Agent.pm
+++ b/src/PVE/QemuServer/Agent.pm
@@ -193,9 +193,9 @@ sub should_fs_freeze($conf) {
     return $agent->{'freeze-fs'} // 1;
 }
 
-=head3 guest_fsfreeze
+=head3 guest_fs_freeze
 
-    guest_fsfreeze($vmid);
+    guest_fs_freeze($vmid);
 
 Freeze the file systems of the guest C<$vmid>. Check that the guest agent is enabled and running
 before calling this function. Dies if the file systems cannot be frozen.
@@ -216,7 +216,7 @@ the time the socket is blocked after a lost command is at most 10 minutes.
 
 =cut
 
-sub guest_fsfreeze($vmid) {
+sub guest_fs_freeze($vmid) {
     my $timeout = 10 * 60;
 
     my $result = eval {
@@ -259,17 +259,17 @@ sub guest_fsfreeze($vmid) {
     die "unable to freeze guest fs - unexpected status '$status'\n" if $status ne 'frozen';
 }
 
-=head3 guest_fsthaw
+=head3 guest_fs_thaw
 
-    guest_fsthaw($vmid);
+    guest_fs_thaw($vmid);
 
 Thaws the file systems of the guest C<$vmid>. Dies if the file systems cannot be thawed.
 
-See C<$guest_fsfreeze> for more details.
+See C<guest_fs_freeze()> for more details.
 
 =cut
 
-sub guest_fsthaw($vmid) {
+sub guest_fs_thaw($vmid) {
     my $res = PVE::QemuServer::Monitor::mon_cmd($vmid, "guest-fsfreeze-thaw");
     check_agent_error($res, "unable to thaw guest filesystem");
 
diff --git a/src/PVE/QemuServer/BlockJob.pm b/src/PVE/QemuServer/BlockJob.pm
index d7c78741..2b14a679 100644
--- a/src/PVE/QemuServer/BlockJob.pm
+++ b/src/PVE/QemuServer/BlockJob.pm
@@ -165,7 +165,7 @@ sub monitor {
                     my $should_fsfreeze = $qga && qga_check_running($vmid);
                     if ($should_fsfreeze) {
                         print "issuing guest agent 'guest-fsfreeze-freeze' command\n";
-                        eval { PVE::QemuServer::Agent::guest_fsfreeze($vmid); };
+                        eval { PVE::QemuServer::Agent::guest_fs_freeze($vmid); };
                         warn $@ if $@;
                     } else {
                         if (!$qga) {
@@ -185,7 +185,7 @@ sub monitor {
 
                     if ($should_fsfreeze) {
                         print "issuing guest agent 'guest-fsfreeze-thaw' command\n";
-                        eval { PVE::QemuServer::Agent::guest_fsthaw($vmid); };
+                        eval { PVE::QemuServer::Agent::guest_fs_thaw($vmid); };
                         warn $@ if $@;
                     } else {
                         print "resume vm\n";
diff --git a/src/PVE/VZDump/QemuServer.pm b/src/PVE/VZDump/QemuServer.pm
index 32f92829..ca4a8513 100644
--- a/src/PVE/VZDump/QemuServer.pm
+++ b/src/PVE/VZDump/QemuServer.pm
@@ -1116,7 +1116,7 @@ sub qga_fs_freeze {
     }
 
     $self->loginfo("issuing guest-agent 'fs-freeze' command");
-    eval { PVE::QemuServer::Agent::guest_fsfreeze($vmid); };
+    eval { PVE::QemuServer::Agent::guest_fs_freeze($vmid); };
     $self->logerr($@) if $@;
 
     return 1; # even on error, ensure we always thaw again
@@ -1127,7 +1127,7 @@ sub qga_fs_thaw {
     my ($self, $vmid) = @_;
 
     $self->loginfo("issuing guest-agent 'fs-thaw' command");
-    eval { PVE::QemuServer::Agent::guest_fsthaw($vmid); };
+    eval { PVE::QemuServer::Agent::guest_fs_thaw($vmid); };
     $self->logerr($@) if $@;
 }
 
-- 
2.47.3

[PATCH qemu-server v2 3/7] agent: parse: change signature to take property string rather than full VM config

[Fiona Ebner]

Better encapsulation and in preparation to harmonize checks for freeze
while avoiding the need to pass the full config to block jobs.

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

Changes in v2:
* Also adapt new should_fs_freeze() caller.

 src/PVE/QemuServer.pm       |  6 +++---
 src/PVE/QemuServer/Agent.pm | 10 +++++-----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 2a469fff..ec311160 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -3426,7 +3426,7 @@ sub config_to_command {
 
     push @$cmd, '-k', $conf->{keyboard} if defined($conf->{keyboard});
 
-    my $guest_agent = parse_guest_agent($conf);
+    my $guest_agent = parse_guest_agent($conf->{agent});
 
     if ($guest_agent->{enabled}) {
         my $qgasocket = PVE::QemuServer::Helpers::qmp_socket(
@@ -5163,8 +5163,8 @@ sub vmconfig_update_agent {
 
     my $hotplug_options = { fstrim_cloned_disks => 1 };
 
-    my $old_agent = parse_guest_agent($conf);
-    my $agent = parse_guest_agent({ $opt => $value });
+    my $old_agent = parse_guest_agent($conf->{agent});
+    my $agent = parse_guest_agent($value);
 
     for my $option (keys %$agent) { # added/changed options
         next if defined($hotplug_options->{$option});
diff --git a/src/PVE/QemuServer/Agent.pm b/src/PVE/QemuServer/Agent.pm
index a68dd8d7..8b814edd 100644
--- a/src/PVE/QemuServer/Agent.pm
+++ b/src/PVE/QemuServer/Agent.pm
@@ -61,10 +61,10 @@ our $agent_fmt = {
     },
 };
 
-sub parse_guest_agent($conf) {
-    return {} if !defined($conf->{agent});
+sub parse_guest_agent($value) {
+    return {} if !defined($value);
 
-    my $res = eval { PVE::JSONSchema::parse_property_string($agent_fmt, $conf->{agent}) };
+    my $res = eval { PVE::JSONSchema::parse_property_string($agent_fmt, $value) };
     warn $@ if $@;
 
     # if the agent is disabled ignore the other potentially set properties
@@ -75,7 +75,7 @@ sub parse_guest_agent($conf) {
 sub get_qga_key($conf, $key) {
     return undef if !defined($conf->{agent});
 
-    my $agent = parse_guest_agent($conf);
+    my $agent = parse_guest_agent($conf->{agent});
     return $agent->{$key};
 }
 
@@ -188,7 +188,7 @@ Does B<not> check whether the agent is actually running.
 =cut
 
 sub should_fs_freeze($conf) {
-    my $agent = parse_guest_agent($conf);
+    my $agent = parse_guest_agent($conf->{agent});
     return 0 if !$agent->{enabled};
     return $agent->{'freeze-fs'} // 1;
 }
-- 
2.47.3

[PATCH qemu-server v2 4/7] agent: should fs freeze: change signature to take property string rather than full VM config

[Fiona Ebner]

Better encapsulation and in preparation to harmonize checks for freeze
while avoiding the need to pass the full config to block jobs.

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

New in v2.

 src/PVE/API2/Qemu.pm         | 4 ++--
 src/PVE/QemuConfig.pm        | 2 +-
 src/PVE/QemuServer/Agent.pm  | 4 ++--
 src/PVE/VZDump/QemuServer.pm | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index dadc06b0..e49e8dab 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -376,7 +376,7 @@ my $import_from_volid = sub {
 
         my ($src_storeid) = PVE::Storage::parse_volume_id($src_volid);
 
-        my $fs_freeze = PVE::QemuServer::Agent::should_fs_freeze($src_conf);
+        my $fs_freeze = PVE::QemuServer::Agent::should_fs_freeze($src_conf->{agent});
 
         return PVE::QemuServer::clone_disk(
             $storecfg,
@@ -4606,7 +4606,7 @@ __PACKAGE__->register_method({
                     $dest_info->{efisize} = PVE::QemuServer::get_efivars_size($oldconf)
                         if $opt eq 'efidisk0';
 
-                    my $fs_freeze = PVE::QemuServer::Agent::should_fs_freeze($oldconf);
+                    my $fs_freeze = PVE::QemuServer::Agent::should_fs_freeze($oldconf->{agent});
 
                     my $newdrive = PVE::QemuServer::clone_disk(
                         $storecfg,
diff --git a/src/PVE/QemuConfig.pm b/src/PVE/QemuConfig.pm
index f93ee5bb..eedb34f9 100644
--- a/src/PVE/QemuConfig.pm
+++ b/src/PVE/QemuConfig.pm
@@ -297,7 +297,7 @@ sub __snapshot_check_freeze_needed {
         return (
             $running,
             $running
-                && PVE::QemuServer::Agent::should_fs_freeze($config)
+                && PVE::QemuServer::Agent::should_fs_freeze($config->{agent})
                 && PVE::QemuServer::Agent::qga_check_running($vmid),
         );
     } else {
diff --git a/src/PVE/QemuServer/Agent.pm b/src/PVE/QemuServer/Agent.pm
index 8b814edd..e63b09f2 100644
--- a/src/PVE/QemuServer/Agent.pm
+++ b/src/PVE/QemuServer/Agent.pm
@@ -187,8 +187,8 @@ Does B<not> check whether the agent is actually running.
 
 =cut
 
-sub should_fs_freeze($conf) {
-    my $agent = parse_guest_agent($conf->{agent});
+sub should_fs_freeze($agent_str) {
+    my $agent = parse_guest_agent($agent_str);
     return 0 if !$agent->{enabled};
     return $agent->{'freeze-fs'} // 1;
 }
diff --git a/src/PVE/VZDump/QemuServer.pm b/src/PVE/VZDump/QemuServer.pm
index ca4a8513..9952cff1 100644
--- a/src/PVE/VZDump/QemuServer.pm
+++ b/src/PVE/VZDump/QemuServer.pm
@@ -1110,7 +1110,7 @@ sub qga_fs_freeze {
         return;
     }
 
-    if (!PVE::QemuServer::Agent::should_fs_freeze($conf)) {
+    if (!PVE::QemuServer::Agent::should_fs_freeze($conf->{agent})) {
         $self->loginfo("skipping guest-agent 'fs-freeze', disabled in VM options");
         return;
     }
-- 
2.47.3

[PATCH qemu-server v2 5/7] clone disk/block jobs: change signatures to take guest agent property string

[Fiona Ebner]

In preparation to harmonize checks for guest fs freeze.

Note that the caller of clone_disk() for moving a disk is changed to
pass the guest agent config too for completeness. It doesn't have an
effect since the mirror job does not look at it when the destination
is the same VM.

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

Changes in v2:
* Adapt to new should_fsfreeze() function.
* Pass guest agent config for move disk caller of clone_disk() for
  completenetss.

 src/PVE/API2/Qemu.pm           | 10 +++-------
 src/PVE/QemuServer.pm          |  2 +-
 src/PVE/QemuServer/BlockJob.pm | 10 ++++++----
 3 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index e49e8dab..a2676b3f 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -376,8 +376,6 @@ my $import_from_volid = sub {
 
         my ($src_storeid) = PVE::Storage::parse_volume_id($src_volid);
 
-        my $fs_freeze = PVE::QemuServer::Agent::should_fs_freeze($src_conf->{agent});
-
         return PVE::QemuServer::clone_disk(
             $storecfg,
             $source_info,
@@ -386,7 +384,7 @@ my $import_from_volid = sub {
             $vollist,
             undef,
             undef,
-            $fs_freeze,
+            $src_conf->{agent},
             PVE::Storage::get_bandwidth_limit('clone', [$src_storeid, $dest_info->{storage}]),
         );
     };
@@ -4606,8 +4604,6 @@ __PACKAGE__->register_method({
                     $dest_info->{efisize} = PVE::QemuServer::get_efivars_size($oldconf)
                         if $opt eq 'efidisk0';
 
-                    my $fs_freeze = PVE::QemuServer::Agent::should_fs_freeze($oldconf->{agent});
-
                     my $newdrive = PVE::QemuServer::clone_disk(
                         $storecfg,
                         $source_info,
@@ -4616,7 +4612,7 @@ __PACKAGE__->register_method({
                         $newvollist,
                         $jobs,
                         $completion,
-                        $fs_freeze,
+                        $oldconf->{agent},
                         $clonelimit,
                     );
 
@@ -4894,7 +4890,7 @@ __PACKAGE__->register_method({
                     $newvollist,
                     undef,
                     undef,
-                    undef,
+                    $conf->{agent},
                     $movelimit,
                 );
                 $conf->{$disk} = PVE::QemuServer::print_drive($newdrive);
diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index ec311160..5d7b5743 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -7958,7 +7958,7 @@ sub clone_disk {
             $dest_info->{'zero-initialized'} = 1 if $sparseinit;
             $dest_info->{vmid} = $newvmid if defined($newvmid);
             my $mirror_opts = {};
-            $mirror_opts->{'guest-agent'} = 1 if $qga;
+            $mirror_opts->{'guest-agent'} = $qga;
             $mirror_opts->{bwlimit} = $bwlimit if defined($bwlimit);
             PVE::QemuServer::BlockJob::mirror(
                 $source_info,
diff --git a/src/PVE/QemuServer/BlockJob.pm b/src/PVE/QemuServer/BlockJob.pm
index 2b14a679..93a1ce60 100644
--- a/src/PVE/QemuServer/BlockJob.pm
+++ b/src/PVE/QemuServer/BlockJob.pm
@@ -162,13 +162,14 @@ sub monitor {
                 last if $completion eq 'skip' || $completion eq 'auto';
 
                 if ($vmiddst && $vmiddst != $vmid) {
-                    my $should_fsfreeze = $qga && qga_check_running($vmid);
+                    my $freeze_enabled = PVE::QemuServer::Agent::should_fs_freeze($qga);
+                    my $should_fsfreeze = $freeze_enabled && qga_check_running($vmid);
                     if ($should_fsfreeze) {
                         print "issuing guest agent 'guest-fsfreeze-freeze' command\n";
                         eval { PVE::QemuServer::Agent::guest_fs_freeze($vmid); };
                         warn $@ if $@;
                     } else {
-                        if (!$qga) {
+                        if (!$freeze_enabled) {
                             print "skipping guest-agent 'guest-fsfreeze-freeze', disabled in VM"
                                 . " options\n";
                         } else {
@@ -430,8 +431,9 @@ original drive to use the new target.
 
 =over
 
-=item C<< $options->{'guest-agent'} >>: If the guest agent is configured for the VM. It will be used
-to freeze and thaw the filesystems for consistency when the target belongs to a different VM.
+=item C<< $options->{'guest-agent'} >>: The guest agent configuration of the VM, i.e. the 'agent'
+property string. If enabled by the configuration, freeze and thaw the filesystems for consistency
+when the target belongs to a different VM.
 
 =item C<< $options->{'bwlimit'} >>: The bandwidth limit to use for the mirroring operation, in
 KiB/s.
-- 
2.47.3

[PATCH qemu-server v2 6/7] agent: fs freeze: harmonize checks for guest fs freeze

[Fiona Ebner]

Puts the logic inside the agent module, rather than duplicating it in
three different places.

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

Changes in v2:
* Adapt to new should_fs_freeze() function.
* Use subroutine signature.
* Rename guest_fsfreeze_applicable() to guest_fs_freeze_applicable().
* Simplify log function (usage) in guest_fsfreeze_applicable().
* Drop $is_backup argument from guest_fs_freeze_applicable() now that
  there is no backup-specific option anymore (is an alias for the
  general one).

 src/PVE/QemuConfig.pm          | 14 ++++++--------
 src/PVE/QemuServer/Agent.pm    | 31 +++++++++++++++++++++++++++++++
 src/PVE/QemuServer/BlockJob.pm | 11 ++---------
 src/PVE/VZDump/QemuServer.pm   | 20 +++++++-------------
 4 files changed, 46 insertions(+), 30 deletions(-)

diff --git a/src/PVE/QemuConfig.pm b/src/PVE/QemuConfig.pm
index eedb34f9..5feae68d 100644
--- a/src/PVE/QemuConfig.pm
+++ b/src/PVE/QemuConfig.pm
@@ -292,17 +292,15 @@ sub __snapshot_check_running {
 sub __snapshot_check_freeze_needed {
     my ($class, $vmid, $config, $save_vmstate) = @_;
 
+    my $freeze_needed = 0;
     my $running = $class->__snapshot_check_running($vmid);
+
     if (!$save_vmstate) {
-        return (
-            $running,
-            $running
-                && PVE::QemuServer::Agent::should_fs_freeze($config->{agent})
-                && PVE::QemuServer::Agent::qga_check_running($vmid),
-        );
-    } else {
-        return ($running, 0);
+        $freeze_needed =
+            PVE::QemuServer::Agent::guest_fs_freeze_applicable($config->{agent}, $vmid);
     }
+
+    return ($running, $freeze_needed);
 }
 
 sub __snapshot_freeze {
diff --git a/src/PVE/QemuServer/Agent.pm b/src/PVE/QemuServer/Agent.pm
index e63b09f2..c8cd334f 100644
--- a/src/PVE/QemuServer/Agent.pm
+++ b/src/PVE/QemuServer/Agent.pm
@@ -276,4 +276,35 @@ sub guest_fs_thaw($vmid) {
     return;
 }
 
+=head3 guest_fs_freeze_applicable
+
+    if (guest_fs_freeze_applicable($agent_str, $vmid, $logfunc)) {
+        guest_fs_freeze($vmid);
+    }
+
+Check if the file systems of the guest C<$vmid> should be frozen according to the guest agent
+property string C<$agent_str> and if freezing is actionable in practice, i.e. guest agent running.
+Logs a message if skipped. Using a custom log function via C<$logfunc> is supported. Otherwise,
+C<print()> and C<warn()> will be used.
+
+=cut
+
+sub guest_fs_freeze_applicable($agent_str, $vmid, $logfunc = undef) {
+    $logfunc //= sub($msg, $level = 'info') {
+        $level eq 'info' ? print("$msg\n") : warn("$msg\n");
+    };
+
+    if (!should_fs_freeze($agent_str)) {
+        $logfunc->("skipping guest filesystem freeze - disabled in VM options");
+        return;
+    }
+
+    if (!qga_check_running($vmid, 1)) {
+        $logfunc->("skipping guest filesystem freeze - agent configured but not running?");
+        return;
+    }
+
+    return 1;
+}
+
 1;
diff --git a/src/PVE/QemuServer/BlockJob.pm b/src/PVE/QemuServer/BlockJob.pm
index 93a1ce60..3fc5af3b 100644
--- a/src/PVE/QemuServer/BlockJob.pm
+++ b/src/PVE/QemuServer/BlockJob.pm
@@ -162,20 +162,13 @@ sub monitor {
                 last if $completion eq 'skip' || $completion eq 'auto';
 
                 if ($vmiddst && $vmiddst != $vmid) {
-                    my $freeze_enabled = PVE::QemuServer::Agent::should_fs_freeze($qga);
-                    my $should_fsfreeze = $freeze_enabled && qga_check_running($vmid);
+                    my $should_fsfreeze =
+                        PVE::QemuServer::Agent::guest_fs_freeze_applicable($qga, $vmid);
                     if ($should_fsfreeze) {
                         print "issuing guest agent 'guest-fsfreeze-freeze' command\n";
                         eval { PVE::QemuServer::Agent::guest_fs_freeze($vmid); };
                         warn $@ if $@;
                     } else {
-                        if (!$freeze_enabled) {
-                            print "skipping guest-agent 'guest-fsfreeze-freeze', disabled in VM"
-                                . " options\n";
-                        } else {
-                            print "skipping guest agent 'guest-fsfreeze-freeze' command: the"
-                                . " agent is not running inside of the guest\n";
-                        }
                         print "suspend vm\n";
                         eval { PVE::QemuServer::RunState::vm_suspend($vmid, 1); };
                         warn $@ if $@;
diff --git a/src/PVE/VZDump/QemuServer.pm b/src/PVE/VZDump/QemuServer.pm
index 9952cff1..86909d28 100644
--- a/src/PVE/VZDump/QemuServer.pm
+++ b/src/PVE/VZDump/QemuServer.pm
@@ -1098,20 +1098,14 @@ sub qga_fs_freeze {
         || !$self->{vm_was_running}
         || $self->{vm_was_paused};
 
-    my $conf = $self->{vmlist}->{$vmid};
+    my $agent_str = $self->{vmlist}->{$vmid}->{agent};
 
-    if (!PVE::QemuServer::Agent::get_qga_key($conf, 'enabled')) {
-        $self->loginfo("skipping guest-agent 'fs-freeze', agent not configured in VM options");
-        return;
-    }
-
-    if (!PVE::QemuServer::Agent::qga_check_running($vmid, 1)) {
-        $self->loginfo("skipping guest-agent 'fs-freeze', agent configured but not running?");
-        return;
-    }
-
-    if (!PVE::QemuServer::Agent::should_fs_freeze($conf->{agent})) {
-        $self->loginfo("skipping guest-agent 'fs-freeze', disabled in VM options");
+    my $logfunc = sub {
+        my ($msg, $level) = @_; # Note that msg and level are swapped intentionally here.
+        $level //= 'info';
+        $self->log($level, $msg);
+    };
+    if (!PVE::QemuServer::Agent::guest_fs_freeze_applicable($agent_str, $vmid, $logfunc)) {
         return;
     }
 
-- 
2.47.3

[PATCH qemu-server v2 7/7] fix #7383: agent: fsfreeze: skip freeze if already frozen

[Fiona Ebner]

This avoids the following error message about the command being
disallowed which can be hard to interpret for users:
> Command guest-fsfreeze-freeze has been disabled: the command is not allowed

It's disallowed, because the fs is already frozen, but the error
message does not indicate this. Instead avoid the error by skipping if
already frozen and log it if that is the case.

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

Changes in v2:
* Use subroutine signature.
* Adapt to log function change.

 src/PVE/QemuServer/Agent.pm | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/src/PVE/QemuServer/Agent.pm b/src/PVE/QemuServer/Agent.pm
index c8cd334f..be6df443 100644
--- a/src/PVE/QemuServer/Agent.pm
+++ b/src/PVE/QemuServer/Agent.pm
@@ -276,6 +276,23 @@ sub guest_fs_thaw($vmid) {
     return;
 }
 
+=head3 guest_fs_is_frozen
+
+    if (guest_fs_is_frozen($vmid)) {
+        print "skipping guest fs freeze - already frozen\n";
+    }
+
+Check if the file systems of the guest C<$vmid> is currently frozen.
+
+=cut
+
+sub guest_fs_is_frozen($vmid) {
+    my $status = PVE::QemuServer::Monitor::mon_cmd($vmid, "guest-fsfreeze-status");
+    check_agent_error($status, "unable to check guest filesystem freeze status");
+
+    return $status eq 'frozen';
+}
+
 =head3 guest_fs_freeze_applicable
 
     if (guest_fs_freeze_applicable($agent_str, $vmid, $logfunc)) {
@@ -283,9 +300,9 @@ sub guest_fs_thaw($vmid) {
     }
 
 Check if the file systems of the guest C<$vmid> should be frozen according to the guest agent
-property string C<$agent_str> and if freezing is actionable in practice, i.e. guest agent running.
-Logs a message if skipped. Using a custom log function via C<$logfunc> is supported. Otherwise,
-C<print()> and C<warn()> will be used.
+property string C<$agent_str> and if freezing is actionable in practice, i.e. guest agent running
+and file system not already frozen. Logs a message if skipped. Using a custom log function via
+C<$logfunc> is supported. Otherwise, C<print()> and C<warn()> will be used.
 
 =cut
 
@@ -304,6 +321,14 @@ sub guest_fs_freeze_applicable($agent_str, $vmid, $logfunc = undef) {
         return;
     }
 
+    my $frozen = eval { PVE::QemuServer::Agent::guest_fs_is_frozen($vmid) };
+    $logfunc->("unable to check guest fs freeze status - $@", 'warn') if $@;
+
+    if ($frozen) {
+        $logfunc->("skipping guest filesystem freeze - already frozen");
+        return;
+    }
+
     return 1;
 }
 
-- 
2.47.3