Re: [PATCH qemu-server 7/8] agent: fsfreeze: harmonize checks for guest fs freeze

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 24.03.26 um 14:53 schrieb 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>
> ---
>  src/PVE/QemuConfig.pm          | 14 ++++-------
>  src/PVE/QemuServer/Agent.pm    | 45 ++++++++++++++++++++++++++++++++++
>  src/PVE/QemuServer/BlockJob.pm | 13 ++--------
>  src/PVE/VZDump/QemuServer.pm   | 21 ++++------------
>  4 files changed, 57 insertions(+), 36 deletions(-)
> 
> diff --git a/src/PVE/QemuConfig.pm b/src/PVE/QemuConfig.pm
> index 7656134b..240b8bfd 100644
> --- a/src/PVE/QemuConfig.pm
> +++ b/src/PVE/QemuConfig.pm
> @@ -292,18 +292,14 @@ 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::get_qga_key($config->{agent}, 'enabled')
> -                && PVE::QemuServer::Agent::qga_check_running($vmid)
> -                && (PVE::QemuServer::Agent::get_qga_key($config->{agent}, 'guest-fsfreeze') // 1),
> -        );
> -    } else {
> -        return ($running, 0);
> +        $freeze_needed = PVE::QemuServer::Agent::guest_fsfreeze_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 2c1f580d..12f9f9d5 100644
> --- a/src/PVE/QemuServer/Agent.pm
> +++ b/src/PVE/QemuServer/Agent.pm
> @@ -289,4 +289,49 @@ sub guest_fsthaw {
>      return;
>  }
>  
> +=head3 guest_fsfreeze_applicable
> +
> +    if (guest_fsfreeze_applicable($agent_str, $vmid, $logfunc, $is_backup)) {
> +        guest_fsfreeze($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. In the context of backup tasks, C<$is_backup> must be
> +specified.
> +
> +=cut
> +
> +sub guest_fsfreeze_applicable {
> +    my ($agent_str, $vmid, $logfunc, $is_backup) = @_;

I'm a bit torn here, on one hand I can see where this comes from, but OTOH it
also feels a bit overly opaque. Nicest gain here is probably the centralized
log method, which I can see the benefits for. So probably fine to have...

> +
> +    $logfunc //= sub {
> +        my ($level, $msg) = @_;
> +        chomp($msg);
> +        $level eq 'info' ? print("$msg\n") : warn("$msg\n");

There's a single use of a level other than 'info' in the next patch and the
chomp feels superfluous considering this is a local closure and we fully
control the messages and their trailing newlines here?

And we probably should start to encourage use v5.36+signature migrations
for (smaller) modules soonish, as then we would be able to use (untested): 

$logfunc //= sub ($msg, $level = 'info') { 
    $level eq 'info' ? print("$msg\n") : warn("$msg\n");
}

> +    };
> +
> +    if (!get_qga_key($agent_str, 'enabled')) {
> +        $logfunc->('info', "skipping guest filesystem freeze - agent not configured in VM options");
> +        return;
> +    }
> +
> +    if (!qga_check_running($vmid, 1)) {
> +        $logfunc->('info', "skipping guest filesystem freeze - agent configured but not running?");
> +        return;
> +    }
> +
> +    my $freeze = get_qga_key($agent_str, 'guest-fsfreeze');
> +    $freeze //= get_qga_key($agent_str, 'freeze-fs-on-backup') if $is_backup;

$is_backup could be dropped when aliasing these to mean the same, if
something akin to my series [0] gets applied.

[0]: https://lore.proxmox.com/all/20260325210021.3789748-1-t.lamprecht@proxmox.com/T/#m837483e3ee5f17e5949485f11c7965367c716b03

> +    $freeze //= 1;
> +
> +    if (!$freeze) {
> +        $logfunc->('info', "skipping guest filesystem freeze - disabled in VM options");
> +        return;
> +    }
> +
> +    return 1;
> +}
> +
>  1;