[pve-devel] [PATCH-SERIES qemu-server 00/11] better handle lost freeze command

[pve-devel] [PATCH-SERIES qemu-server 00/11] better handle lost freeze command

[Fiona Ebner]

The main bit of the series is patch 07/11:

As reported in the enterprise support, it can happen that a guest
agent command is read, but then the guest agent never sends an answer,
because the service in the guest is stopped/killed. For example, if a
guest reboot happens before the command can be successfully executed.
This is usually not problematic, but the fsfreeze-freeze command has a
timeout of 1 hour, so the guest agent socket would be blocked for that
amount of time, waiting on a command that is not being executed
anymore.

Use a lower timeout for the fsfreeze-freeze command, and issue an
fsfreeze-status command afterwards, which will return immediately if
the fsfreeze-freeze command already finished, and which will be queued
if not. This is used as a proxy to determine whether the
fsfreeze-freeze command is still running and to check whether it was
successful. Like this, the time the socket is blocked after a "lost
command" is at most 10 minutes.


The rest of the series is preparation and cleanups.


Fiona Ebner (11):
  agent: drop unused $noerr argument from helpers
  api: agent: improve module imports
  agent: code style: order module imports according to style guide
  agent: avoid dependency on QemuConfig module
  qmp client: remove erroneous comment
  qmp client: add $noerr argument
  agent: implement fsfreeze helper to better handle lost commands
  agent: avoid use of deprecated check_running() function
  agent: move qga_check_running() helper to agent module
  agent: prefer usage of get_qga_key() helper
  agent: move guest agent format and parsing to agent module

 PVE/API2/Qemu.pm          |   9 ++-
 PVE/API2/Qemu/Agent.pm    |  48 ++++++++----
 PVE/CLI/qm.pm             |   8 +-
 PVE/QMPClient.pm          |  13 +++-
 PVE/QemuConfig.pm         |   7 +-
 PVE/QemuMigrate.pm        |   3 +-
 PVE/QemuServer.pm         |  65 +---------------
 PVE/QemuServer/Agent.pm   | 160 +++++++++++++++++++++++++++++++-------
 PVE/QemuServer/Monitor.pm |  11 ++-
 PVE/VZDump/QemuServer.pm  |  10 ++-
 10 files changed, 210 insertions(+), 124 deletions(-)

-- 
2.39.5



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

[pve-devel] [PATCH qemu-server 01/11] agent: drop unused $noerr argument from helpers

[Fiona Ebner]

Both agent_available() and agent_cmd() have no callers that use the
$noerr argument.

The current implementation was not ideal: agent_cmd() did not check
the return value of agent_available() in the $noerr scenario. It
should return early. In agent_available(), failure was silently
ignored in the $noerr scenario. Having a message or warning then would
have been more useful.

The agent_available() function is renamed to assert_agent_available()
and it is not exported anymore, the single caller outside the module
can just call it with the full module path.

The import of 'agent_available' in qm.pm was not used and is dropped.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Qemu/Agent.pm  |  4 ++--
 PVE/CLI/qm.pm           |  2 +-
 PVE/QemuServer/Agent.pm | 26 ++++++++------------------
 3 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/PVE/API2/Qemu/Agent.pm b/PVE/API2/Qemu/Agent.pm
index dceee770..e3102b0a 100644
--- a/PVE/API2/Qemu/Agent.pm
+++ b/PVE/API2/Qemu/Agent.pm
@@ -6,7 +6,7 @@ use warnings;
 use PVE::RESTHandler;
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::QemuServer;
-use PVE::QemuServer::Agent qw(agent_available agent_cmd check_agent_error);
+use PVE::QemuServer::Agent qw(agent_cmd check_agent_error);
 use PVE::QemuServer::Monitor qw(mon_cmd);
 use MIME::Base64 qw(encode_base64 decode_base64);
 use JSON;
@@ -189,7 +189,7 @@ sub register_command {
 
 	    my $conf = PVE::QemuConfig->load_config ($vmid); # check if VM exists
 
-	    agent_available($vmid, $conf);
+	    PVE::QemuServer::Agent::assert_agent_available($vmid, $conf);
 
 	    my $cmd = $param->{command} // $command;
 	    my $res = mon_cmd($vmid, "guest-$cmd");
diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index 3e3a4c91..35e85597 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -32,7 +32,7 @@ use PVE::API2::Qemu;
 use PVE::QemuConfig;
 use PVE::QemuServer::Drive qw(is_valid_drivename);
 use PVE::QemuServer::Helpers;
-use PVE::QemuServer::Agent qw(agent_available);
+use PVE::QemuServer::Agent;
 use PVE::QemuServer::ImportDisk;
 use PVE::QemuServer::Monitor qw(mon_cmd);
 use PVE::QemuServer::QMPHelpers;
diff --git a/PVE/QemuServer/Agent.pm b/PVE/QemuServer/Agent.pm
index 548c020c..945a11c0 100644
--- a/PVE/QemuServer/Agent.pm
+++ b/PVE/QemuServer/Agent.pm
@@ -11,7 +11,6 @@ use base 'Exporter';
 
 our @EXPORT_OK = qw(
 check_agent_error
-agent_available
 agent_cmd
 );
 
@@ -36,32 +35,23 @@ sub check_agent_error {
     return 1;
 }
 
-sub agent_available {
-    my ($vmid, $conf, $noerr) = @_;
+sub assert_agent_available {
+    my ($vmid, $conf) = @_;
 
-    eval {
-	die "No QEMU guest agent configured\n" if !defined($conf->{agent});
-	die "VM $vmid is not running\n" if !PVE::QemuServer::check_running($vmid);
-	die "QEMU guest agent is not running\n" if !PVE::QemuServer::qga_check_running($vmid, 1);
-    };
-
-    if (my $err = $@) {
-	die $err if !$noerr;
-	return;
-    }
-
-    return 1;
+    die "No QEMU guest agent configured\n" if !defined($conf->{agent});
+    die "VM $vmid is not running\n" if !PVE::QemuServer::check_running($vmid);
+    die "QEMU guest agent is not running\n" if !PVE::QemuServer::qga_check_running($vmid, 1);
 }
 
 # loads config, checks if available, executes command, checks for errors
 sub agent_cmd {
-    my ($vmid, $cmd, $params, $errormsg, $noerr) = @_;
+    my ($vmid, $cmd, $params, $errormsg) = @_;
 
     my $conf = PVE::QemuConfig->load_config($vmid); # also checks if VM exists
-    agent_available($vmid, $conf, $noerr);
+    assert_agent_available($vmid, $conf);
 
     my $res = PVE::QemuServer::Monitor::mon_cmd($vmid, "guest-$cmd", %$params);
-    check_agent_error($res, $errormsg, $noerr);
+    check_agent_error($res, $errormsg);
 
     return $res;
 }
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server 02/11] api: agent: improve module imports

[Fiona Ebner]

Order module import according to style guide and add missing
PVE::QemuConfig import.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Qemu/Agent.pm | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/PVE/API2/Qemu/Agent.pm b/PVE/API2/Qemu/Agent.pm
index e3102b0a..c9527070 100644
--- a/PVE/API2/Qemu/Agent.pm
+++ b/PVE/API2/Qemu/Agent.pm
@@ -3,13 +3,16 @@ package PVE::API2::Qemu::Agent;
 use strict;
 use warnings;
 
-use PVE::RESTHandler;
+use JSON;
+use MIME::Base64 qw(encode_base64 decode_base64);
+
 use PVE::JSONSchema qw(get_standard_option);
+use PVE::RESTHandler;
+
+use PVE::QemuConfig;
 use PVE::QemuServer;
 use PVE::QemuServer::Agent qw(agent_cmd check_agent_error);
 use PVE::QemuServer::Monitor qw(mon_cmd);
-use MIME::Base64 qw(encode_base64 decode_base64);
-use JSON;
 
 use base qw(PVE::RESTHandler);
 
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server 03/11] agent: code style: order module imports according to style guide

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer/Agent.pm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/PVE/QemuServer/Agent.pm b/PVE/QemuServer/Agent.pm
index 945a11c0..264c7018 100644
--- a/PVE/QemuServer/Agent.pm
+++ b/PVE/QemuServer/Agent.pm
@@ -3,10 +3,12 @@ package PVE::QemuServer::Agent;
 use strict;
 use warnings;
 
+use JSON;
+use MIME::Base64 qw(decode_base64 encode_base64);
+
 use PVE::QemuServer;
 use PVE::QemuServer::Monitor;
-use MIME::Base64 qw(decode_base64 encode_base64);
-use JSON;
+
 use base 'Exporter';
 
 our @EXPORT_OK = qw(
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server 04/11] agent: avoid dependency on QemuConfig module

[Fiona Ebner]

The QemuConfig module uses qga_check_running() which is planned to be
moved to the Agent module and it will use a to-be-added
guest_fsfreeze() helper from the Agent module. Loading the config on
the call-sites of agent_cmd(), qemu_exec() and qemu_exec_status()
makes it possible to avoid introducing that cyclic dependency.

Note that the import for the QemuConfig module was already missing.

Also drops unused variables $write and $res from the 'file-write' API
endpoint implementation.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Qemu/Agent.pm  | 31 ++++++++++++++++++++++++-------
 PVE/CLI/qm.pm           |  6 ++++--
 PVE/QemuServer/Agent.pm | 12 ++++++------
 3 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/PVE/API2/Qemu/Agent.pm b/PVE/API2/Qemu/Agent.pm
index c9527070..5f39e149 100644
--- a/PVE/API2/Qemu/Agent.pm
+++ b/PVE/API2/Qemu/Agent.pm
@@ -250,6 +250,7 @@ __PACKAGE__->register_method({
 	my ($param) = @_;
 
 	my $vmid = $param->{vmid};
+	my $conf = PVE::QemuConfig->load_config($vmid);
 
 	my $crypted = $param->{crypted} // 0;
 	my $args = {
@@ -257,7 +258,7 @@ __PACKAGE__->register_method({
 	    password => encode_base64($param->{password}),
 	    crypted => $crypted ? JSON::true : JSON::false,
 	};
-	my $res = agent_cmd($vmid, "set-user-password", $args, 'cannot set user password');
+	my $res = agent_cmd($vmid, $conf, "set-user-password", $args, 'cannot set user password');
 
 	return { result => $res };
     }});
@@ -305,9 +306,11 @@ __PACKAGE__->register_method({
 	my ($param) = @_;
 
 	my $vmid = $param->{vmid};
+	my $conf = PVE::QemuConfig->load_config($vmid);
+
 	my $cmd = $param->{command};
 
-	my $res = PVE::QemuServer::Agent::qemu_exec($vmid, $param->{'input-data'}, $cmd);
+	my $res = PVE::QemuServer::Agent::qemu_exec($vmid, $conf, $param->{'input-data'}, $cmd);
 	return $res;
     }});
 
@@ -374,9 +377,11 @@ __PACKAGE__->register_method({
 	my ($param) = @_;
 
 	my $vmid = $param->{vmid};
+	my $conf = PVE::QemuConfig->load_config($vmid);
+
 	my $pid = int($param->{pid});
 
-	my $res = PVE::QemuServer::Agent::qemu_exec_status($vmid, $pid);
+	my $res = PVE::QemuServer::Agent::qemu_exec_status($vmid, $conf, $pid);
 
 	return $res;
     }});
@@ -420,8 +425,10 @@ __PACKAGE__->register_method({
 	my ($param) = @_;
 
 	my $vmid = $param->{vmid};
+	my $conf = PVE::QemuConfig->load_config($vmid);
 
-	my $qgafh = agent_cmd($vmid, "file-open",  { path => $param->{file} }, "can't open file");
+	my $qgafh = agent_cmd(
+	    $vmid, $conf, "file-open", { path => $param->{file} }, "can't open file");
 
 	my $bytes_left = $MAX_READ_SIZE;
 	my $eof = 0;
@@ -490,12 +497,22 @@ __PACKAGE__->register_method({
 	my ($param) = @_;
 
 	my $vmid = $param->{vmid};
+	my $conf = PVE::QemuConfig->load_config($vmid);
 
 	my $buf = ($param->{encode} // 1) ? encode_base64($param->{content}) : $param->{content};
 
-	my $qgafh = agent_cmd($vmid, "file-open",  { path => $param->{file}, mode => 'wb' }, "can't open file");
-	my $write = agent_cmd($vmid, "file-write", { handle => $qgafh, 'buf-b64' => $buf }, "can't write to file");
-	my $res = agent_cmd($vmid, "file-close", { handle => $qgafh }, "can't close file");
+	my $qgafh = agent_cmd(
+	    $vmid, $conf, "file-open", { path => $param->{file}, mode => 'wb' }, "can't open file");
+
+	agent_cmd(
+	    $vmid,
+	    $conf,
+	    "file-write",
+	    { handle => $qgafh, 'buf-b64' => $buf },
+	    "can't write to file",
+	);
+
+	agent_cmd($vmid, $conf, "file-close", { handle => $qgafh }, "can't close file");
 
 	return;
     }});
diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index 35e85597..9427264f 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -884,7 +884,9 @@ __PACKAGE__->register_method({
 	my $args = $param->{'extra-args'};
 	$args = undef if !$args || !@$args;
 
-	my $res = PVE::QemuServer::Agent::qemu_exec($vmid, $input_data, $args);
+	my $conf = PVE::QemuConfig->load_config($vmid);
+
+	my $res = PVE::QemuServer::Agent::qemu_exec($vmid, $conf, $input_data, $args);
 
 	if ($sync) {
 	    my $pid = $res->{pid};
@@ -892,7 +894,7 @@ __PACKAGE__->register_method({
 	    my $starttime = time();
 
 	    while ($timeout == 0 || (time() - $starttime) < $timeout) {
-		my $out = PVE::QemuServer::Agent::qemu_exec_status($vmid, $pid);
+		my $out = PVE::QemuServer::Agent::qemu_exec_status($vmid, $conf, $pid);
 		if ($out->{exited}) {
 		    $res = $out;
 		    last;
diff --git a/PVE/QemuServer/Agent.pm b/PVE/QemuServer/Agent.pm
index 264c7018..41e615aa 100644
--- a/PVE/QemuServer/Agent.pm
+++ b/PVE/QemuServer/Agent.pm
@@ -47,9 +47,8 @@ sub assert_agent_available {
 
 # loads config, checks if available, executes command, checks for errors
 sub agent_cmd {
-    my ($vmid, $cmd, $params, $errormsg) = @_;
+    my ($vmid, $conf, $cmd, $params, $errormsg) = @_;
 
-    my $conf = PVE::QemuConfig->load_config($vmid); # also checks if VM exists
     assert_agent_available($vmid, $conf);
 
     my $res = PVE::QemuServer::Monitor::mon_cmd($vmid, "guest-$cmd", %$params);
@@ -59,7 +58,7 @@ sub agent_cmd {
 }
 
 sub qemu_exec {
-    my ($vmid, $input_data, $cmd) = @_;
+    my ($vmid, $conf, $input_data, $cmd) = @_;
 
     my $args = {
 	'capture-output' => JSON::true,
@@ -83,15 +82,16 @@ sub qemu_exec {
 	$errmsg .= " (input-data given)";
     }
 
-    my $res = agent_cmd($vmid, "exec", $args, $errmsg);
+    my $res = agent_cmd($vmid, $conf, "exec", $args, $errmsg);
 
     return $res;
 }
 
 sub qemu_exec_status {
-    my ($vmid, $pid) = @_;
+    my ($vmid, $conf, $pid) = @_;
 
-    my $res = agent_cmd($vmid, "exec-status", { pid => $pid }, "can't get exec status for '$pid'");
+    my $res = agent_cmd(
+	$vmid, $conf, "exec-status", { pid => $pid }, "can't get exec status for '$pid'");
 
     if ($res->{'out-data'}) {
 	my $decoded = eval { decode_base64($res->{'out-data'}) };
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server 05/11] qmp client: remove erroneous comment

[Fiona Ebner]

This is most likely a left-over from copy-pasting from an example in
the 'IO::Multiplex' man page. In particular, the method is not called
every second.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QMPClient.pm | 1 -
 1 file changed, 1 deletion(-)

diff --git a/PVE/QMPClient.pm b/PVE/QMPClient.pm
index abed1f01..eefd17a5 100644
--- a/PVE/QMPClient.pm
+++ b/PVE/QMPClient.pm
@@ -460,7 +460,6 @@ sub mux_input {
     &$check_queue($self);
 }
 
-# This gets called every second to update player info, etc...
 sub mux_timeout {
     my ($self, $mux, $fh) = @_;
 
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server 06/11] qmp client: add $noerr argument

[Fiona Ebner]

Using the $noerr argument will allow callers to opt-in to handling
errors themselves.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QMPClient.pm          |  8 +++++---
 PVE/QemuServer/Monitor.pm | 11 ++++++++---
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/PVE/QMPClient.pm b/PVE/QMPClient.pm
index eefd17a5..0f08e678 100644
--- a/PVE/QMPClient.pm
+++ b/PVE/QMPClient.pm
@@ -86,7 +86,7 @@ sub queue_cmd {
 
 # execute a single command
 sub cmd {
-    my ($self, $vmid, $cmd, $timeout) = @_;
+    my ($self, $vmid, $cmd, $timeout, $noerr) = @_;
 
     my $result;
 
@@ -143,8 +143,10 @@ sub cmd {
 
     $self->queue_execute($timeout, 2);
 
-    die "VM $vmid qmp command '$cmd->{execute}' failed - $queue_info->{error}"
-	if defined($queue_info->{error});
+    if (defined($queue_info->{error})) {
+	die "VM $vmid qmp command '$cmd->{execute}' failed - $queue_info->{error}" if !$noerr;
+	$result = { error => $queue_info->{error} };
+    }
 
     return $result;
 };
diff --git a/PVE/QemuServer/Monitor.pm b/PVE/QemuServer/Monitor.pm
index 937006ae..5e16edac 100644
--- a/PVE/QemuServer/Monitor.pm
+++ b/PVE/QemuServer/Monitor.pm
@@ -12,14 +12,19 @@ our @EXPORT_OK = qw(
 mon_cmd
 );
 
+# Supported custom options not in the QMP schema:
+# timeout - wait at most for this amount of time. If there was no actual error, the QMP/QGA command
+#           will still continue to be executed even after the timeout reached.
+# noerr   - do not die when the command gets an error or the timeout is hit. The caller needs to
+#           handle the error that is returned as a structured result.
 sub qmp_cmd {
     my ($vmid, $cmd) = @_;
 
     my $res;
 
-    my $timeout;
+    my ($noerr, $timeout);
     if ($cmd->{arguments}) {
-	$timeout = delete $cmd->{arguments}->{timeout};
+	($noerr, $timeout) = delete($cmd->{arguments}->@{qw(noerr timeout)});
     }
 
     eval {
@@ -28,7 +33,7 @@ sub qmp_cmd {
 	if (-e $sname) { # test if VM is reasonably new and supports qmp/qga
 	    my $qmpclient = PVE::QMPClient->new();
 
-	    $res = $qmpclient->cmd($vmid, $cmd, $timeout);
+	    $res = $qmpclient->cmd($vmid, $cmd, $timeout, $noerr);
 	} else {
 	    die "unable to open monitor socket\n";
 	}
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server 07/11] agent: implement fsfreeze helper to better handle lost commands

[Fiona Ebner]

As reported in the enterprise support, it can happen that a guest
agent command is read, but then the guest agent never sends an answer,
because the service in the guest is stopped/killed. For example, if a
guest reboot happens before the command can be successfully executed.
This is usually not problematic, but the fsfreeze-freeze command has a
timeout of 1 hour, so the guest agent socket would be blocked for that
amount of time, waiting on a command that is not being executed
anymore.

Use a lower timeout for the fsfreeze-freeze command, and issue an
fsfreeze-status command afterwards, which will return immediately if
the fsfreeze-freeze command already finished, and which will be queued
if not. This is used as a proxy to determine whether the
fsfreeze-freeze command is still running and to check whether it was
successful. Like this, the time the socket is blocked after a "lost
command" is at most 10 minutes.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QMPClient.pm         |  4 ++++
 PVE/QemuConfig.pm        |  5 ++--
 PVE/QemuServer.pm        |  3 ++-
 PVE/QemuServer/Agent.pm  | 51 ++++++++++++++++++++++++++++++++++++++++
 PVE/VZDump/QemuServer.pm |  5 ++--
 5 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/PVE/QMPClient.pm b/PVE/QMPClient.pm
index 0f08e678..b3340885 100644
--- a/PVE/QMPClient.pm
+++ b/PVE/QMPClient.pm
@@ -110,6 +110,8 @@ 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
+	    #
 	    # freeze syncs all guest FS, if we kill it it stays in an unfreezable
 	    # locked state with high probability, so use an generous timeout
 	    $timeout = 60*60; # 1 hour
@@ -146,6 +148,7 @@ sub cmd {
     if (defined($queue_info->{error})) {
 	die "VM $vmid qmp command '$cmd->{execute}' failed - $queue_info->{error}" if !$noerr;
 	$result = { error => $queue_info->{error} };
+	$result->{'error-is-timeout'} = 1 if $queue_info->{'error-is-timeout'};
     }
 
     return $result;
@@ -467,6 +470,7 @@ sub mux_timeout {
 
     if (my $queue_info = &$lookup_queue_info($self, $fh)) {
 	$queue_info->{error} = "got timeout\n";
+	$queue_info->{'error-is-timeout'} = 1;
 	$self->{mux}->inbuffer($fh, ''); # clear to avoid warnings
     }
 
diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index 2609542c..e941f093 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -8,6 +8,7 @@ use Scalar::Util qw(blessed);
 use PVE::AbstractConfig;
 use PVE::INotify;
 use PVE::JSONSchema;
+use PVE::QemuServer::Agent;
 use PVE::QemuServer::CPUConfig;
 use PVE::QemuServer::Drive;
 use PVE::QemuServer::Helpers;
@@ -291,8 +292,8 @@ sub __snapshot_freeze {
 	eval { mon_cmd($vmid, "guest-fsfreeze-thaw"); };
 	warn "guest-fsfreeze-thaw problems - $@" if $@;
     } else {
-	eval { mon_cmd($vmid, "guest-fsfreeze-freeze"); };
-	warn "guest-fsfreeze-freeze problems - $@" if $@;
+	eval { PVE::QemuServer::Agent::guest_fsfreeze($vmid); };
+	warn $@ if $@;
     }
 }
 
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 577959a4..317c09f2 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -51,6 +51,7 @@ use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_glob_for
 use PVE::QMPClient;
 use PVE::QemuConfig;
 use PVE::QemuConfig::NoWrite;
+use PVE::QemuServer::Agent;
 use PVE::QemuServer::Helpers qw(config_aware_timeout min_version kvm_user_version windows_version);
 use PVE::QemuServer::Cloudinit;
 use PVE::QemuServer::CGroup;
@@ -8242,7 +8243,7 @@ sub qemu_drive_mirror_monitor {
 		    my $agent_running = $qga && qga_check_running($vmid);
 		    if ($agent_running) {
 			print "freeze filesystem\n";
-			eval { mon_cmd($vmid, "guest-fsfreeze-freeze"); };
+			eval { PVE::QemuServer::Agent::guest_fsfreeze($vmid); };
 			warn $@ if $@;
 		    } else {
 			print "suspend vm\n";
diff --git a/PVE/QemuServer/Agent.pm b/PVE/QemuServer/Agent.pm
index 41e615aa..ef36a6a8 100644
--- a/PVE/QemuServer/Agent.pm
+++ b/PVE/QemuServer/Agent.pm
@@ -119,4 +119,55 @@ sub qemu_exec_status {
     return $res;
 }
 
+# It can happen that a guest agent command is read, but then the guest agent never sends an answer,
+# because the service in the guest is stopped/killed. For example, if a guest reboot happens before
+# the command can be successfully executed. This is usually not problematic, but the fsfreeze-freeze
+# command has a timeout of 1 hour, so the guest agent socket would be blocked for that amount of
+# time, waiting on a command that is not being executed anymore.
+#
+# Use a lower timeout for the fsfreeze-freeze command, and issue an fsfreeze-status command
+# afterwards, which will return immediately if the fsfreeze-freeze command already finished, and
+# which will be queued if not. This is used as a proxy to determine whether the fsfreeze-freeze
+# command is still running and to check whether it was successful. Like this, the time the socket is
+# blocked after a "lost command" is at most 10 minutes.
+sub guest_fsfreeze {
+    my ($vmid) = @_;
+
+    my $timeout = 10 * 60;
+
+    my $result = eval {
+	PVE::QemuServer::Monitor::mon_cmd($vmid, 'guest-fsfreeze-freeze', timeout => $timeout);
+    };
+    if ($result && ref($result) eq 'HASH' && $result->{error}) {
+	my $error = $result->{error}->{desc} // 'unknown';
+	die "unable to freeze guest fs - $error\n";
+    } elsif (defined($result)) {
+	return; # command successful
+    }
+
+    my $status;
+    eval {
+	my ($i, $last_iteration) = (0, 5);
+	while ($i < $last_iteration && !defined($status)) {
+	    print "still waiting on guest fs freeze\n";
+	    $i++;
+
+	    $status = PVE::QemuServer::Monitor::mon_cmd(
+		$vmid, 'guest-fsfreeze-status', timeout => $timeout, noerr => 1);
+
+	    if ($status && ref($status) eq 'HASH' && $status->{'error-is-timeout'}) {
+		$status = undef;
+	    } else {
+		check_agent_error($status, 'unknown error');
+	    }
+	}
+	if (!defined($status)) {
+	    die "timeout after " . ($timeout * ($last_iteration + 1) / 60) . " minutes\n";
+	}
+    };
+    die "querying status after freezing guest fs failed - $@" if $@;
+
+    die "unable to freeze guest fs - unexpected status '$status'\n" if $status ne 'frozen';
+}
+
 1;
diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
index 10514f75..b686da84 100644
--- a/PVE/VZDump/QemuServer.pm
+++ b/PVE/VZDump/QemuServer.pm
@@ -29,6 +29,7 @@ use PVE::Format qw(render_duration render_bytes);
 
 use PVE::QemuConfig;
 use PVE::QemuServer;
+use PVE::QemuServer::Agent;
 use PVE::QemuServer::Drive qw(checked_volume_format);
 use PVE::QemuServer::Helpers;
 use PVE::QemuServer::Machine;
@@ -1069,10 +1070,10 @@ sub qga_fs_freeze {
     }
 
     $self->loginfo("issuing guest-agent 'fs-freeze' command");
-    eval { mon_cmd($vmid, "guest-fsfreeze-freeze") };
+    eval { PVE::QemuServer::Agent::guest_fsfreeze($vmid); };
     $self->logerr($@) if $@;
 
-    return 1; # even on mon command error, ensure we always thaw again
+    return 1; # even on error, ensure we always thaw again
 }
 
 # only call if fs_freeze return 1
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server 08/11] agent: avoid use of deprecated check_running() function

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer/Agent.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer/Agent.pm b/PVE/QemuServer/Agent.pm
index ef36a6a8..92e02645 100644
--- a/PVE/QemuServer/Agent.pm
+++ b/PVE/QemuServer/Agent.pm
@@ -7,6 +7,7 @@ use JSON;
 use MIME::Base64 qw(decode_base64 encode_base64);
 
 use PVE::QemuServer;
+use PVE::QemuServer::Helpers;
 use PVE::QemuServer::Monitor;
 
 use base 'Exporter';
@@ -41,7 +42,7 @@ sub assert_agent_available {
     my ($vmid, $conf) = @_;
 
     die "No QEMU guest agent configured\n" if !defined($conf->{agent});
-    die "VM $vmid is not running\n" if !PVE::QemuServer::check_running($vmid);
+    die "VM $vmid is not running\n" if !PVE::QemuServer::Helpers::vm_running_locally($vmid);
     die "QEMU guest agent is not running\n" if !PVE::QemuServer::qga_check_running($vmid, 1);
 }
 
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server 09/11] agent: move qga_check_running() helper to agent module

[Fiona Ebner]

Removes the dependency on PVE::QemuServer in the agent module. This
makes it possible to use the agent module from nearly everywhere
without introducing a(n indirect) cyclic dependency.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Qemu.pm         |  5 +++--
 PVE/QemuConfig.pm        |  4 ++--
 PVE/QemuServer.pm        | 13 +------------
 PVE/QemuServer/Agent.pm  | 15 +++++++++++++--
 PVE/VZDump/QemuServer.pm |  2 +-
 5 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 626cce45..48a9b75a 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -27,6 +27,7 @@ use PVE::GuestHelpers qw(assert_tag_permissions);
 use PVE::GuestImport;
 use PVE::QemuConfig;
 use PVE::QemuServer;
+use PVE::QemuServer::Agent;
 use PVE::QemuServer::Cloudinit;
 use PVE::QemuServer::CPUConfig;
 use PVE::QemuServer::Drive qw(checked_volume_format checked_parse_volname);
@@ -4436,7 +4437,7 @@ __PACKAGE__->register_method({
 		PVE::QemuConfig->write_config($vmid, $conf);
 
 		my $do_trim = PVE::QemuServer::get_qga_key($conf, 'fstrim_cloned_disks');
-		if ($running && $do_trim && PVE::QemuServer::qga_check_running($vmid)) {
+		if ($running && $do_trim && PVE::QemuServer::Agent::qga_check_running($vmid)) {
 		    eval { mon_cmd($vmid, "guest-fstrim") };
 		}
 
@@ -6212,7 +6213,7 @@ __PACKAGE__->register_method({
 		    return $info;
 		},
 		'fstrim' => sub {
-		    if (PVE::QemuServer::qga_check_running($state->{vmid})) {
+		    if (PVE::QemuServer::Agent::qga_check_running($state->{vmid})) {
 			eval { mon_cmd($state->{vmid}, "guest-fstrim") };
 			warn "fstrim failed: $@\n" if $@;
 		    }
diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index e941f093..2ee615fa 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -8,7 +8,7 @@ use Scalar::Util qw(blessed);
 use PVE::AbstractConfig;
 use PVE::INotify;
 use PVE::JSONSchema;
-use PVE::QemuServer::Agent;
+use PVE::QemuServer::Agent qw(qga_check_running);
 use PVE::QemuServer::CPUConfig;
 use PVE::QemuServer::Drive;
 use PVE::QemuServer::Helpers;
@@ -279,7 +279,7 @@ sub __snapshot_check_freeze_needed {
 
     my $running = $class->__snapshot_check_running($vmid);
     if (!$save_vmstate) {
-	return ($running, $running && PVE::QemuServer::parse_guest_agent($config)->{enabled} && PVE::QemuServer::qga_check_running($vmid));
+	return ($running, $running && PVE::QemuServer::parse_guest_agent($config)->{enabled} && qga_check_running($vmid));
     } else {
 	return ($running, 0);
     }
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 317c09f2..9a81030e 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -51,7 +51,7 @@ use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_glob_for
 use PVE::QMPClient;
 use PVE::QemuConfig;
 use PVE::QemuConfig::NoWrite;
-use PVE::QemuServer::Agent;
+use PVE::QemuServer::Agent qw(qga_check_running);
 use PVE::QemuServer::Helpers qw(config_aware_timeout min_version kvm_user_version windows_version);
 use PVE::QemuServer::Cloudinit;
 use PVE::QemuServer::CGroup;
@@ -7946,17 +7946,6 @@ sub do_snapshots_with_qemu {
     return;
 }
 
-sub qga_check_running {
-    my ($vmid, $nowarn) = @_;
-
-    eval { mon_cmd($vmid, "guest-ping", timeout => 3); };
-    if ($@) {
-	warn "QEMU Guest Agent is not running - $@" if !$nowarn;
-	return 0;
-    }
-    return 1;
-}
-
 =head3 template_create($vmid, $conf [, $disk])
 
 Converts all used disk volumes for the VM with the identifier C<$vmid> and
diff --git a/PVE/QemuServer/Agent.pm b/PVE/QemuServer/Agent.pm
index 92e02645..9f1450ff 100644
--- a/PVE/QemuServer/Agent.pm
+++ b/PVE/QemuServer/Agent.pm
@@ -6,7 +6,6 @@ use warnings;
 use JSON;
 use MIME::Base64 qw(decode_base64 encode_base64);
 
-use PVE::QemuServer;
 use PVE::QemuServer::Helpers;
 use PVE::QemuServer::Monitor;
 
@@ -15,8 +14,20 @@ use base 'Exporter';
 our @EXPORT_OK = qw(
 check_agent_error
 agent_cmd
+qga_check_running
 );
 
+sub qga_check_running {
+    my ($vmid, $nowarn) = @_;
+
+    eval { PVE::QemuServer::Monitor::mon_cmd($vmid, "guest-ping", timeout => 3); };
+    if ($@) {
+	warn "QEMU Guest Agent is not running - $@" if !$nowarn;
+	return 0;
+    }
+    return 1;
+}
+
 sub check_agent_error {
     my ($result, $errmsg, $noerr) = @_;
 
@@ -43,7 +54,7 @@ sub assert_agent_available {
 
     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 !PVE::QemuServer::qga_check_running($vmid, 1);
+    die "QEMU guest agent is not running\n" if !qga_check_running($vmid, 1);
 }
 
 # loads config, checks if available, executes command, checks for errors
diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
index b686da84..80038d8f 100644
--- a/PVE/VZDump/QemuServer.pm
+++ b/PVE/VZDump/QemuServer.pm
@@ -1058,7 +1058,7 @@ sub qga_fs_freeze {
     my ($self, $task, $vmid) = @_;
     return if !$self->{vmlist}->{$vmid}->{agent} || $task->{mode} eq 'stop' || !$self->{vm_was_running} || $self->{vm_was_paused};
 
-    if (!PVE::QemuServer::qga_check_running($vmid, 1)) {
+    if (!PVE::QemuServer::Agent::qga_check_running($vmid, 1)) {
 	$self->loginfo("skipping guest-agent 'fs-freeze', agent configured but not running?");
 	return;
     }
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server 10/11] agent: prefer usage of get_qga_key() helper

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/QemuConfig.pm  | 2 +-
 PVE/QemuMigrate.pm | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index 2ee615fa..a88c12c5 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -279,7 +279,7 @@ sub __snapshot_check_freeze_needed {
 
     my $running = $class->__snapshot_check_running($vmid);
     if (!$save_vmstate) {
-	return ($running, $running && PVE::QemuServer::parse_guest_agent($config)->{enabled} && qga_check_running($vmid));
+	return ($running, $running && PVE::QemuServer::get_qga_key($config, 'enabled') && qga_check_running($vmid));
     } else {
 	return ($running, 0);
     }
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index b7bf2aa3..71c274b4 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -1575,7 +1575,7 @@ sub phase3_cleanup {
 
 	if (
 	    $self->{storage_migration}
-	    && PVE::QemuServer::parse_guest_agent($conf)->{fstrim_cloned_disks}
+	    && PVE::QemuServer::get_qga_key($conf, 'fstrim_cloned_disks')
 	    && $self->{running}
 	) {
 	    if (!$self->{vm_was_paused}) {
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server 11/11] agent: move guest agent format and parsing to agent module

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Qemu.pm         |  4 +--
 PVE/QemuConfig.pm        |  4 +--
 PVE/QemuMigrate.pm       |  3 ++-
 PVE/QemuServer.pm        | 53 ++--------------------------------------
 PVE/QemuServer/Agent.pm  | 53 ++++++++++++++++++++++++++++++++++++++++
 PVE/VZDump/QemuServer.pm |  3 ++-
 6 files changed, 63 insertions(+), 57 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 48a9b75a..0f22c94a 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -3095,7 +3095,7 @@ __PACKAGE__->register_method({
 	    $status->{spice} = 1 if $spice;
 	    $status->{clipboard} = $vga->{clipboard};
 	}
-	$status->{agent} = 1 if PVE::QemuServer::get_qga_key($conf, 'enabled');
+	$status->{agent} = 1 if PVE::QemuServer::Agent::get_qga_key($conf, 'enabled');
 
 	return $status;
     }});
@@ -4436,7 +4436,7 @@ __PACKAGE__->register_method({
 
 		PVE::QemuConfig->write_config($vmid, $conf);
 
-		my $do_trim = PVE::QemuServer::get_qga_key($conf, 'fstrim_cloned_disks');
+		my $do_trim = PVE::QemuServer::Agent::get_qga_key($conf, 'fstrim_cloned_disks');
 		if ($running && $do_trim && PVE::QemuServer::Agent::qga_check_running($vmid)) {
 		    eval { mon_cmd($vmid, "guest-fstrim") };
 		}
diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index a88c12c5..34b55adc 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -8,7 +8,7 @@ use Scalar::Util qw(blessed);
 use PVE::AbstractConfig;
 use PVE::INotify;
 use PVE::JSONSchema;
-use PVE::QemuServer::Agent qw(qga_check_running);
+use PVE::QemuServer::Agent qw(get_qga_key qga_check_running);
 use PVE::QemuServer::CPUConfig;
 use PVE::QemuServer::Drive;
 use PVE::QemuServer::Helpers;
@@ -279,7 +279,7 @@ sub __snapshot_check_freeze_needed {
 
     my $running = $class->__snapshot_check_running($vmid);
     if (!$save_vmstate) {
-	return ($running, $running && PVE::QemuServer::get_qga_key($config, 'enabled') && qga_check_running($vmid));
+	return ($running, $running && get_qga_key($config, 'enabled') && qga_check_running($vmid));
     } else {
 	return ($running, 0);
     }
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 71c274b4..2d59959c 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -25,6 +25,7 @@ use PVE::Tools;
 use PVE::Tunnel;
 
 use PVE::QemuConfig;
+use PVE::QemuServer::Agent qw(get_qga_key);
 use PVE::QemuServer::CPUConfig;
 use PVE::QemuServer::Drive qw(checked_volume_format);
 use PVE::QemuServer::Helpers qw(min_version);
@@ -1575,7 +1576,7 @@ sub phase3_cleanup {
 
 	if (
 	    $self->{storage_migration}
-	    && PVE::QemuServer::get_qga_key($conf, 'fstrim_cloned_disks')
+	    && PVE::QemuServer::Agent::get_qga_key($conf, 'fstrim_cloned_disks')
 	    && $self->{running}
 	) {
 	    if (!$self->{vm_was_paused}) {
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 9a81030e..a636d2ca 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -51,7 +51,7 @@ use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_glob_for
 use PVE::QMPClient;
 use PVE::QemuConfig;
 use PVE::QemuConfig::NoWrite;
-use PVE::QemuServer::Agent qw(qga_check_running);
+use PVE::QemuServer::Agent qw(get_qga_key parse_guest_agent qga_check_running);
 use PVE::QemuServer::Helpers qw(config_aware_timeout min_version kvm_user_version windows_version);
 use PVE::QemuServer::Cloudinit;
 use PVE::QemuServer::CGroup;
@@ -161,34 +161,6 @@ my $watchdog_fmt = {
 };
 PVE::JSONSchema::register_format('pve-qm-watchdog', $watchdog_fmt);
 
-my $agent_fmt = {
-    enabled => {
-	description => "Enable/disable communication with a QEMU Guest Agent (QGA) running in the VM.",
-	type => 'boolean',
-	default => 0,
-	default_key => 1,
-    },
-    fstrim_cloned_disks => {
-	description => "Run fstrim after moving a disk or migrating the VM.",
-	type => 'boolean',
-	optional => 1,
-	default => 0,
-    },
-    'freeze-fs-on-backup' => {
-	description => "Freeze/thaw guest filesystems on backup for consistency.",
-	type => 'boolean',
-	optional => 1,
-	default => 1,
-    },
-    type => {
-	description => "Select the agent type",
-	type => 'string',
-	default => 'virtio',
-	optional => 1,
-	enum => [qw(virtio isa)],
-    },
-};
-
 my $vga_fmt = {
     type => {
 	description => "Select the VGA type. Using type 'cirrus' is not recommended.",
@@ -466,7 +438,7 @@ EODESC
 	optional => 1,
 	description => "Enable/disable communication with the QEMU Guest Agent and its properties.",
 	type => 'string',
-	format => $agent_fmt,
+	format => $PVE::QemuServer::Agent::agent_fmt,
     },
     kvm => {
 	optional => 1,
@@ -1928,27 +1900,6 @@ sub parse_watchdog {
     return $res;
 }
 
-sub parse_guest_agent {
-    my ($conf) = @_;
-
-    return {} if !defined($conf->{agent});
-
-    my $res = eval { parse_property_string($agent_fmt, $conf->{agent}) };
-    warn $@ if $@;
-
-    # if the agent is disabled ignore the other potentially set properties
-    return {} if !$res->{enabled};
-    return $res;
-}
-
-sub get_qga_key {
-    my ($conf, $key) = @_;
-    return undef if !defined($conf->{agent});
-
-    my $agent = parse_guest_agent($conf);
-    return $agent->{$key};
-}
-
 sub parse_vga {
     my ($value) = @_;
 
diff --git a/PVE/QemuServer/Agent.pm b/PVE/QemuServer/Agent.pm
index 9f1450ff..d147c1ae 100644
--- a/PVE/QemuServer/Agent.pm
+++ b/PVE/QemuServer/Agent.pm
@@ -6,6 +6,8 @@ use warnings;
 use JSON;
 use MIME::Base64 qw(decode_base64 encode_base64);
 
+use PVE::JSONSchema;
+
 use PVE::QemuServer::Helpers;
 use PVE::QemuServer::Monitor;
 
@@ -14,9 +16,60 @@ use base 'Exporter';
 our @EXPORT_OK = qw(
 check_agent_error
 agent_cmd
+get_qga_key
+parse_guest_agent
 qga_check_running
 );
 
+our $agent_fmt = {
+    enabled => {
+	description => "Enable/disable communication with a QEMU Guest Agent (QGA) running in the VM.",
+	type => 'boolean',
+	default => 0,
+	default_key => 1,
+    },
+    fstrim_cloned_disks => {
+	description => "Run fstrim after moving a disk or migrating the VM.",
+	type => 'boolean',
+	optional => 1,
+	default => 0,
+    },
+    'freeze-fs-on-backup' => {
+	description => "Freeze/thaw guest filesystems on backup for consistency.",
+	type => 'boolean',
+	optional => 1,
+	default => 1,
+    },
+    type => {
+	description => "Select the agent type",
+	type => 'string',
+	default => 'virtio',
+	optional => 1,
+	enum => [qw(virtio isa)],
+    },
+};
+
+sub parse_guest_agent {
+    my ($conf) = @_;
+
+    return {} if !defined($conf->{agent});
+
+    my $res = eval { PVE::JSONSchema::parse_property_string($agent_fmt, $conf->{agent}) };
+    warn $@ if $@;
+
+    # if the agent is disabled ignore the other potentially set properties
+    return {} if !$res->{enabled};
+    return $res;
+}
+
+sub get_qga_key {
+    my ($conf, $key) = @_;
+    return undef if !defined($conf->{agent});
+
+    my $agent = parse_guest_agent($conf);
+    return $agent->{$key};
+}
+
 sub qga_check_running {
     my ($vmid, $nowarn) = @_;
 
diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
index 80038d8f..b1c10687 100644
--- a/PVE/VZDump/QemuServer.pm
+++ b/PVE/VZDump/QemuServer.pm
@@ -1063,7 +1063,8 @@ sub qga_fs_freeze {
 	return;
     }
 
-    my $freeze = PVE::QemuServer::get_qga_key($self->{vmlist}->{$vmid}, 'freeze-fs-on-backup') // 1;
+    my $freeze =
+	PVE::QemuServer::Agent::get_qga_key($self->{vmlist}->{$vmid}, 'freeze-fs-on-backup') // 1;
     if (!$freeze) {
 	$self->loginfo("skipping guest-agent 'fs-freeze', disabled in VM options");
 	return;
-- 
2.39.5



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

Re: [pve-devel] [PATCH qemu-server 07/11] agent: implement fsfreeze helper to better handle lost commands

[Mira Limbeck]

Thank you for tackling this!

On 5/5/25 14:57, Fiona Ebner wrote:
> As reported in the enterprise support, it can happen that a guest
> agent command is read, but then the guest agent never sends an answer,
> because the service in the guest is stopped/killed. For example, if a
> guest reboot happens before the command can be successfully executed.
> This is usually not problematic, but the fsfreeze-freeze command has a
> timeout of 1 hour, so the guest agent socket would be blocked for that
> amount of time, waiting on a command that is not being executed
> anymore.
> 
> Use a lower timeout for the fsfreeze-freeze command, and issue an
> fsfreeze-status command afterwards, which will return immediately if
> the fsfreeze-freeze command already finished, and which will be queued
> if not. This is used as a proxy to determine whether the
> fsfreeze-freeze command is still running and to check whether it was
> successful. Like this, the time the socket is blocked after a "lost
> command" is at most 10 minutes.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  PVE/QMPClient.pm         |  4 ++++
>  PVE/QemuConfig.pm        |  5 ++--
>  PVE/QemuServer.pm        |  3 ++-
>  PVE/QemuServer/Agent.pm  | 51 ++++++++++++++++++++++++++++++++++++++++
>  PVE/VZDump/QemuServer.pm |  5 ++--
>  5 files changed, 63 insertions(+), 5 deletions(-)
> 
> diff --git a/PVE/QMPClient.pm b/PVE/QMPClient.pm
> index 0f08e678..b3340885 100644
> --- a/PVE/QMPClient.pm
> +++ b/PVE/QMPClient.pm
> @@ -110,6 +110,8 @@ 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
> +	    #
>  	    # freeze syncs all guest FS, if we kill it it stays in an unfreezable
>  	    # locked state with high probability, so use an generous timeout
>  	    $timeout = 60*60; # 1 hour
> @@ -146,6 +148,7 @@ sub cmd {
>      if (defined($queue_info->{error})) {
>  	die "VM $vmid qmp command '$cmd->{execute}' failed - $queue_info->{error}" if !$noerr;
>  	$result = { error => $queue_info->{error} };
> +	$result->{'error-is-timeout'} = 1 if $queue_info->{'error-is-timeout'};
>      }
>  
>      return $result;
> @@ -467,6 +470,7 @@ sub mux_timeout {
>  
>      if (my $queue_info = &$lookup_queue_info($self, $fh)) {
>  	$queue_info->{error} = "got timeout\n";
> +	$queue_info->{'error-is-timeout'} = 1;
>  	$self->{mux}->inbuffer($fh, ''); # clear to avoid warnings
>      }
>  
> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
> index 2609542c..e941f093 100644
> --- a/PVE/QemuConfig.pm
> +++ b/PVE/QemuConfig.pm
> @@ -8,6 +8,7 @@ use Scalar::Util qw(blessed);
>  use PVE::AbstractConfig;
>  use PVE::INotify;
>  use PVE::JSONSchema;
> +use PVE::QemuServer::Agent;
>  use PVE::QemuServer::CPUConfig;
>  use PVE::QemuServer::Drive;
>  use PVE::QemuServer::Helpers;
> @@ -291,8 +292,8 @@ sub __snapshot_freeze {
>  	eval { mon_cmd($vmid, "guest-fsfreeze-thaw"); };
>  	warn "guest-fsfreeze-thaw problems - $@" if $@;
>      } else {
> -	eval { mon_cmd($vmid, "guest-fsfreeze-freeze"); };
> -	warn "guest-fsfreeze-freeze problems - $@" if $@;
> +	eval { PVE::QemuServer::Agent::guest_fsfreeze($vmid); };
> +	warn $@ if $@;
>      }
>  }
>  
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 577959a4..317c09f2 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -51,6 +51,7 @@ use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_glob_for
>  use PVE::QMPClient;
>  use PVE::QemuConfig;
>  use PVE::QemuConfig::NoWrite;
> +use PVE::QemuServer::Agent;
>  use PVE::QemuServer::Helpers qw(config_aware_timeout min_version kvm_user_version windows_version);
>  use PVE::QemuServer::Cloudinit;
>  use PVE::QemuServer::CGroup;
> @@ -8242,7 +8243,7 @@ sub qemu_drive_mirror_monitor {
>  		    my $agent_running = $qga && qga_check_running($vmid);
>  		    if ($agent_running) {
>  			print "freeze filesystem\n";
> -			eval { mon_cmd($vmid, "guest-fsfreeze-freeze"); };
> +			eval { PVE::QemuServer::Agent::guest_fsfreeze($vmid); };
>  			warn $@ if $@;
>  		    } else {
>  			print "suspend vm\n";
> diff --git a/PVE/QemuServer/Agent.pm b/PVE/QemuServer/Agent.pm
> index 41e615aa..ef36a6a8 100644
> --- a/PVE/QemuServer/Agent.pm
> +++ b/PVE/QemuServer/Agent.pm
> @@ -119,4 +119,55 @@ sub qemu_exec_status {
>      return $res;
>  }
>  
> +# It can happen that a guest agent command is read, but then the guest agent never sends an answer,
> +# because the service in the guest is stopped/killed. For example, if a guest reboot happens before
> +# the command can be successfully executed. This is usually not problematic, but the fsfreeze-freeze
> +# command has a timeout of 1 hour, so the guest agent socket would be blocked for that amount of
> +# time, waiting on a command that is not being executed anymore.
> +#
> +# Use a lower timeout for the fsfreeze-freeze command, and issue an fsfreeze-status command
> +# afterwards, which will return immediately if the fsfreeze-freeze command already finished, and
> +# which will be queued if not. This is used as a proxy to determine whether the fsfreeze-freeze
> +# command is still running and to check whether it was successful. Like this, the time the socket is
> +# blocked after a "lost command" is at most 10 minutes.
With the changed logic it's not so clear at first how long it would wait
in the worst case. It's still the same 60 minutes as before, just spread
over the initial fsfreeze command, and then up to 5 iterations of
fsfreese-status, each with a 10 minute timeout.

Maybe that could be documented in the comment and/or the commit message?

> +sub guest_fsfreeze {
> +    my ($vmid) = @_;
> +
> +    my $timeout = 10 * 60;
> +
> +    my $result = eval {
> +	PVE::QemuServer::Monitor::mon_cmd($vmid, 'guest-fsfreeze-freeze', timeout => $timeout);
> +    };
> +    if ($result && ref($result) eq 'HASH' && $result->{error}) {
> +	my $error = $result->{error}->{desc} // 'unknown';
> +	die "unable to freeze guest fs - $error\n";
> +    } elsif (defined($result)) {
> +	return; # command successful
> +    }
> +
> +    my $status;
> +    eval {
> +	my ($i, $last_iteration) = (0, 5);
> +	while ($i < $last_iteration && !defined($status)) {
> +	    print "still waiting on guest fs freeze\n";
> +	    $i++;
> +
> +	    $status = PVE::QemuServer::Monitor::mon_cmd(
> +		$vmid, 'guest-fsfreeze-status', timeout => $timeout, noerr => 1);
> +
> +	    if ($status && ref($status) eq 'HASH' && $status->{'error-is-timeout'}) {
> +		$status = undef;
> +	    } else {
> +		check_agent_error($status, 'unknown error');
> +	    }
> +	}
> +	if (!defined($status)) {
> +	    die "timeout after " . ($timeout * ($last_iteration + 1) / 60) . " minutes\n";
> +	}
> +    };
> +    die "querying status after freezing guest fs failed - $@" if $@;
> +
> +    die "unable to freeze guest fs - unexpected status '$status'\n" if $status ne 'frozen';
> +}
> +
>  1;
> diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
> index 10514f75..b686da84 100644
> --- a/PVE/VZDump/QemuServer.pm
> +++ b/PVE/VZDump/QemuServer.pm
> @@ -29,6 +29,7 @@ use PVE::Format qw(render_duration render_bytes);
>  
>  use PVE::QemuConfig;
>  use PVE::QemuServer;
> +use PVE::QemuServer::Agent;
>  use PVE::QemuServer::Drive qw(checked_volume_format);
>  use PVE::QemuServer::Helpers;
>  use PVE::QemuServer::Machine;
> @@ -1069,10 +1070,10 @@ sub qga_fs_freeze {
>      }
>  
>      $self->loginfo("issuing guest-agent 'fs-freeze' command");
> -    eval { mon_cmd($vmid, "guest-fsfreeze-freeze") };
> +    eval { PVE::QemuServer::Agent::guest_fsfreeze($vmid); };
>      $self->logerr($@) if $@;
>  
> -    return 1; # even on mon command error, ensure we always thaw again
> +    return 1; # even on error, ensure we always thaw again
>  }
>  
>  # only call if fs_freeze return 1



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

Re: [pve-devel] [PATCH qemu-server 07/11] agent: implement fsfreeze helper to better handle lost commands

[Fiona Ebner]

Am 05.05.25 um 15:57 schrieb Mira Limbeck:
> On 5/5/25 14:57, Fiona Ebner wrote:
>> diff --git a/PVE/QemuServer/Agent.pm b/PVE/QemuServer/Agent.pm
>> index 41e615aa..ef36a6a8 100644
>> --- a/PVE/QemuServer/Agent.pm
>> +++ b/PVE/QemuServer/Agent.pm
>> @@ -119,4 +119,55 @@ sub qemu_exec_status {
>>      return $res;
>>  }
>>  
>> +# It can happen that a guest agent command is read, but then the guest agent never sends an answer,
>> +# because the service in the guest is stopped/killed. For example, if a guest reboot happens before
>> +# the command can be successfully executed. This is usually not problematic, but the fsfreeze-freeze
>> +# command has a timeout of 1 hour, so the guest agent socket would be blocked for that amount of
>> +# time, waiting on a command that is not being executed anymore.
>> +#
>> +# Use a lower timeout for the fsfreeze-freeze command, and issue an fsfreeze-status command
>> +# afterwards, which will return immediately if the fsfreeze-freeze command already finished, and
>> +# which will be queued if not. This is used as a proxy to determine whether the fsfreeze-freeze
>> +# command is still running and to check whether it was successful. Like this, the time the socket is
>> +# blocked after a "lost command" is at most 10 minutes.
> With the changed logic it's not so clear at first how long it would wait
> in the worst case. It's still the same 60 minutes as before, just spread
> over the initial fsfreeze command, and then up to 5 iterations of
> fsfreese-status, each with a 10 minute timeout.
> 
> Maybe that could be documented in the comment and/or the commit message?

Sure, will add that in v2. The patch intentionally does not change the
time fsfreeze-freeze is allowed to take if it actually runs for that long.


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