[PATCH qemu-server v3 3/3] fix #5032: agent: sync guest time via QGA when the clock falls behind

[Jakob Klocker <j.klocker@proxmox.com>]

Add a 'sync-time-on-resume' agent config property (default off) that,
when enabled, issues the guest-set-time QGA command to set the guest
clock to the host's current time after operations that leave the clock
behind:
- resuming from hibernation (suspend-to-disk)
- resuming from pause/suspend
- rolling back to a snapshot that includes RAM
- live migration
- taking a snapshot, during which the guest is briefly frozen

The start, resume and rollback API endpoints gain a
'sync-time-on-resume' parameter (e.g. 'qm start <vmid>
--sync-time-on-resume 0') that overrides the configured agent value for
a single operation, forcing the sync on or off regardless of config.

guest_set_time prints a confirmation line to stdout on success. During
live migration the resume step runs over the migration tunnel, which
also uses stdout, so an unconditional print corrupts the tunnel
protocol. vm_resume therefore takes a 'quiet' parameter to suppress the
print on the migration path; the confirmation is logged by the migration
code instead.

Signed-off-by: Jakob Klocker <j.klocker@proxmox.com>
---
changes since v1:
- adapt warning message in the resume-from-saved-state path
changes since v2:
- add sync on pause/resume
- add override options
- add print task 
- adapt formating and naming

 src/PVE/API2/Qemu.pm           | 46 ++++++++++++++++++++++++++---
 src/PVE/CLI/qm.pm              |  2 +-
 src/PVE/QemuConfig.pm          | 12 ++++++--
 src/PVE/QemuMigrate.pm         |  2 ++
 src/PVE/QemuServer.pm          | 18 +++++++++++-
 src/PVE/QemuServer/Agent.pm    | 54 ++++++++++++++++++++++++++++++++++
 src/PVE/QemuServer/RunState.pm | 11 ++++++-
 7 files changed, 136 insertions(+), 9 deletions(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index 9023c344..f58151e8 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -3510,6 +3510,14 @@ __PACKAGE__->register_method({
                     . ' the migration. A value of 0 means that the host_mtu parameter is to be'
                     . ' avoided for the corresponding device.',
             },
+            'sync-time-on-resume' => {
+                type => 'boolean',
+                description =>
+                    "Override the VM's configured 'sync-time-on-resume' agent setting for this"
+                    . " start only. Controls whether the guest clock is synchronized after the"
+                    . " RAM state is restored. The guest agent must be enabled.",
+                optional => 1,
+            },
         },
     },
     returns => {
@@ -3525,6 +3533,7 @@ __PACKAGE__->register_method({
         my $vmid = extract_param($param, 'vmid');
         my $timeout = extract_param($param, 'timeout');
         my $machine = extract_param($param, 'machine');
+        my $sync_time = extract_param($param, 'sync-time-on-resume');
 
         my $get_root_param = sub {
             my $value = extract_param($param, $_[0]);
@@ -3631,6 +3640,7 @@ __PACKAGE__->register_method({
                     timeout => $timeout,
                     forcecpu => $force_cpu,
                     'nets-host-mtu' => $nets_host_mtu,
+                    'sync-time-on-resume' => $sync_time,
                 };
 
                 PVE::QemuServer::vm_start($storecfg, $vmid, $params, $migrate_opts);
@@ -4115,6 +4125,14 @@ __PACKAGE__->register_method({
                 description => "Do not check whether the VM is running, used"
                     . " internally during migration. Only root may use this option.",
             },
+            'sync-time-on-resume' => {
+                type => 'boolean',
+                description =>
+                    "Override the VM's configured 'sync-time-on-resume' agent setting for this"
+                    . " resume only. Controls whether the guest clock is synchronized after the"
+                    . " RAM state is restored. The guest agent must be enabled.",
+                optional => 1,
+            },
         },
     },
     returns => {
@@ -4131,6 +4149,8 @@ __PACKAGE__->register_method({
 
         my $vmid = extract_param($param, 'vmid');
 
+        my $sync_time = extract_param($param, 'sync-time-on-resume');
+
         my $skiplock = extract_param($param, 'skiplock');
         raise_param_exc({ skiplock => "Only root may use this option." })
             if $skiplock && $authuser ne 'root@pam';
@@ -4161,10 +4181,14 @@ __PACKAGE__->register_method({
             syslog('info', "resume VM $vmid: $upid\n");
 
             if (!$to_disk_suspended) {
-                PVE::QemuServer::RunState::vm_resume($vmid, $skiplock, $nocheck);
+                PVE::QemuServer::RunState::vm_resume($vmid, $skiplock, $nocheck, $sync_time);
             } else {
                 my $storecfg = PVE::Storage::config();
-                PVE::QemuServer::vm_start($storecfg, $vmid, { skiplock => $skiplock });
+                PVE::QemuServer::vm_start(
+                    $storecfg,
+                    $vmid,
+                    { skiplock => $skiplock, 'sync-time-on-resume' => $sync_time },
+                );
             }
 
             return;
@@ -6323,6 +6347,14 @@ __PACKAGE__->register_method({
                 optional => 1,
                 default => 0,
             },
+            'sync-time-on-resume' => {
+                type => 'boolean',
+                description =>
+                    "Override the VM's configured sync-time-on-resume agent setting for this"
+                    . " rollback only. Controls whether the guest clock is synchronized"
+                    . " after the RAM state is restored. The guest agent must be enabled.",
+                optional => 1,
+            },
         },
     },
     returns => {
@@ -6342,6 +6374,8 @@ __PACKAGE__->register_method({
 
         my $snapname = extract_param($param, 'snapname');
 
+        my $sync_time = extract_param($param, 'sync-time-on-resume');
+
         # vm_start is invoked directly from the worker, so its own permissions
         # predicate doesn't fire here - check VM.PowerMgmt up front so a user
         # with only VM.Snapshot.Rollback can't power the VM on as a side effect.
@@ -6350,7 +6384,11 @@ __PACKAGE__->register_method({
 
         my $realcmd = sub {
             PVE::Cluster::log_msg('info', $authuser, "rollback snapshot VM $vmid: $snapname");
-            PVE::QemuConfig->snapshot_rollback($vmid, $snapname);
+            PVE::QemuConfig->snapshot_rollback(
+                $vmid,
+                $snapname,
+                { 'sync-time-on-resume' => $sync_time },
+            );
 
             if ($param->{start} && !PVE::QemuServer::Helpers::vm_running_locally($vmid)) {
                 PVE::API2::Qemu->vm_start({ vmid => $vmid, node => $node });
@@ -6915,7 +6953,7 @@ __PACKAGE__->register_method({
                 },
                 'resume' => sub {
                     if (PVE::QemuServer::Helpers::vm_running_locally($state->{vmid})) {
-                        PVE::QemuServer::RunState::vm_resume($state->{vmid}, 1, 1);
+                        PVE::QemuServer::RunState::vm_resume($state->{vmid}, 1, 1, undef, 1);
                     } else {
                         die "VM $state->{vmid} not running\n";
                     }
diff --git a/src/PVE/CLI/qm.pm b/src/PVE/CLI/qm.pm
index b903c1f1..25351f64 100755
--- a/src/PVE/CLI/qm.pm
+++ b/src/PVE/CLI/qm.pm
@@ -473,7 +473,7 @@ __PACKAGE__->register_method({
                 if (PVE::QemuServer::Helpers::vm_running_locally($vmid)) {
                     # vm_resume with nocheck, since local node might not have processed config
                     # move/rename yet
-                    eval { PVE::QemuServer::RunState::vm_resume($vmid, 1, 1); };
+                    eval { PVE::QemuServer::RunState::vm_resume($vmid, 1, 1, undef, 1); };
                     if ($@) {
                         $tunnel_write->("ERR: resume failed - $@");
                     } else {
diff --git a/src/PVE/QemuConfig.pm b/src/PVE/QemuConfig.pm
index 26f0fda2..e95a8c61 100644
--- a/src/PVE/QemuConfig.pm
+++ b/src/PVE/QemuConfig.pm
@@ -383,6 +383,11 @@ sub __snapshot_create_vol_snapshots_hook {
                     next;
                 }
             }
+            my $conf = $class->load_config($vmid);
+            if (PVE::QemuServer::Agent::should_sync_time_on_resume($conf->{agent})) {
+                eval { PVE::QemuServer::Agent::guest_set_time($vmid); };
+                warn "could not sync guest time after snapshot - $@" if $@;
+            }
         }
     }
 }
@@ -444,8 +449,9 @@ sub __snapshot_rollback_hook {
     my ($class, $vmid, $conf, $snap, $prepare, $data) = @_;
 
     if ($prepare) {
-        # we save the machine of the current config
+        # we save the machine and agent of the current config
         $data->{oldmachine} = $conf->{machine};
+        $data->{agent} = $conf->{agent};
     } else {
         # if we have a 'runningmachine' entry in the snapshot we use that
         # for the forcemachine parameter, else we use the old logic
@@ -509,7 +515,7 @@ sub __snapshot_rollback_vm_stop {
 }
 
 sub __snapshot_rollback_vm_start {
-    my ($class, $vmid, $vmstate, $data) = @_;
+    my ($class, $vmid, $vmstate, $data, $opts) = @_;
 
     my $storecfg = PVE::Storage::config();
     my $params = {
@@ -517,6 +523,8 @@ sub __snapshot_rollback_vm_start {
         forcemachine => $data->{forcemachine},
         forcecpu => $data->{forcecpu},
         'nets-host-mtu' => $data->{'nets-host-mtu'},
+        agent => $data->{agent},
+        'sync-time-on-resume' => $opts->{'sync-time-on-resume'},
     };
     PVE::QemuServer::vm_start($storecfg, $vmid, $params);
 }
diff --git a/src/PVE/QemuMigrate.pm b/src/PVE/QemuMigrate.pm
index 8da6f15d..5846578d 100644
--- a/src/PVE/QemuMigrate.pm
+++ b/src/PVE/QemuMigrate.pm
@@ -1770,6 +1770,8 @@ sub phase3_cleanup {
                     $self->{errors} = 1;
                 }
             }
+            $self->log('info', "synced guest clock via guest agent")
+                if PVE::QemuServer::Agent::should_sync_time_on_resume($conf->{agent});
         }
 
         if (
diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index cdf66e89..e30e89e7 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -5179,7 +5179,10 @@ sub vmconfig_update_agent {
 
     die "skip\n" if !$conf->{$opt};
 
-    my $hotplug_options = { fstrim_cloned_disks => 1 };
+    my $hotplug_options = {
+        fstrim_cloned_disks => 1,
+        'sync-time-on-resume' => 1,
+    };
 
     my $old_agent = parse_guest_agent($conf->{agent});
     my $agent = parse_guest_agent($value);
@@ -6006,6 +6009,19 @@ sub vm_start_nolock {
         );
     }
 
+    # $resume = suspend-to-disk, $statefile = rollback with RAM. Rollback carries the snapshot's
+    # agent setting in params; hibernate resume falls back to the live config
+    if ($resume || ($statefile && !$migratedfrom)) {
+        my $agent = $params->{agent} // $conf->{agent};
+        if (PVE::QemuServer::Agent::should_sync_time_on_resume(
+            $agent,
+            $params->{'sync-time-on-resume'},
+        )) {
+            eval { PVE::QemuServer::Agent::guest_set_time($vmid); 1 };
+            warn "could not sync guest time after resume from saved state - $@" if $@;
+        }
+    }
+
     return $res;
 }
 
diff --git a/src/PVE/QemuServer/Agent.pm b/src/PVE/QemuServer/Agent.pm
index be6df443..77cd2bd6 100644
--- a/src/PVE/QemuServer/Agent.pm
+++ b/src/PVE/QemuServer/Agent.pm
@@ -4,6 +4,7 @@ use v5.36;
 
 use JSON;
 use MIME::Base64 qw(decode_base64 encode_base64);
+use Time::HiRes qw(time);
 
 use PVE::JSONSchema;
 
@@ -52,6 +53,24 @@ our $agent_fmt = {
         optional => 1,
         default => 1,
     },
+    # FIXME: MAJOR VERSION: 10.0.0 - reconsider enabling this option as default
+    'sync-time-on-resume' => {
+        description => "Synchronize the guest clock through QGA after operations that can leave"
+            . " the guest clock behind.",
+        verbose_description =>
+            "Whether to issue the guest-set-time QEMU guest agent command to synchronize the"
+            . " guest clock to the host's current time after operations that can leave the guest"
+            . " clock behind. This happens when resuming from hibernation, resuming from a paused"
+            . " state, taking a snapshot, after a migration, and after rolling back to a snapshot"
+            . " that includes RAM. In these cases the guest's clock may still reflect an earlier"
+            . " time. The time is only synchronized when the QEMU Guest Agent option is enabled"
+            . " in the guest's configuration and the agent is running inside of the guest. For"
+            . " resume, hibernation resume and snapshot rollback, the synchronization can be"
+            . " overridden per operation.",
+        type => 'boolean',
+        optional => 1,
+        default => 0,
+    },
     type => {
         description => "Select the agent type",
         type => 'string',
@@ -332,4 +351,39 @@ sub guest_fs_freeze_applicable($agent_str, $vmid, $logfunc = undef) {
     return 1;
 }
 
+=head3 should_sync_time_on_resume
+
+Returns whether the guest's clock should be synchronized to the host's via the QEMU Guest Agent
+when the VM is resumed from saved state. Requires the guest agent to be enabled. An explicit
+C<$override> (from an API parameter) takes precedence over the C<sync-time-on-resume> agent
+config property when defined; otherwise the config value is used (defaulting to off). Does B<not>
+check whether the agent is actually running.
+
+=cut
+
+sub should_sync_time_on_resume($agent_str, $override = undef) {
+    my $agent = parse_guest_agent($agent_str);
+    return 0 if !$agent->{enabled};
+    return $override ? 1 : 0 if defined($override);
+    return $agent->{'sync-time-on-resume'} // 0;
+}
+
+=head3 guest_set_time
+
+Sets the guest's clock to the current host time via the QEMU Guest Agent's
+C<guest-set-time> command. The host time is passed explicitly (as nanoseconds
+since the UNIX epoch) because the argument-less form reads the time from the
+guest's RTC, which isn't supported on all platforms (e.g. Windows); passing
+the value directly works regardless of guest OS.
+
+=cut
+
+sub guest_set_time($vmid, $quiet = undef) {
+    my $time_ns = Time::HiRes::time() * 1_000_000_000;
+    my $res = PVE::QemuServer::Monitor::mon_cmd($vmid, 'guest-set-time', time => int($time_ns));
+    check_agent_error($res, "unable to set guest time");
+    print "synced guest clock via guest agent\n" if !$quiet;
+    return;
+}
+
 1;
diff --git a/src/PVE/QemuServer/RunState.pm b/src/PVE/QemuServer/RunState.pm
index bbbcc88e..a72aed4d 100644
--- a/src/PVE/QemuServer/RunState.pm
+++ b/src/PVE/QemuServer/RunState.pm
@@ -128,7 +128,7 @@ sub vm_suspend {
 # location of the config file (source or target node) is not deterministic,
 # since migration cannot wait for pmxcfs to process the rename
 sub vm_resume {
-    my ($vmid, $skiplock, $nocheck) = @_;
+    my ($vmid, $skiplock, $nocheck, $sync_time, $quiet) = @_;
 
     PVE::QemuConfig->lock_config(
         $vmid,
@@ -180,6 +180,15 @@ sub vm_resume {
                 if $resume_cmd eq 'cont';
 
             mon_cmd($vmid, $resume_cmd);
+
+            if (
+                $resume_cmd eq 'cont'
+                && PVE::QemuServer::Agent::should_sync_time_on_resume($conf->{agent},
+                    $sync_time)
+            ) {
+                eval { PVE::QemuServer::Agent::guest_set_time($vmid, $quiet); };
+                warn "could not sync guest time after resume - $@" if $@;
+            }
         },
     );
 }
-- 
2.47.3