[PATCH qemu-server v6 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
- vzdump backup in suspend mode

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.

For vzdump, only suspend mode needs a sync: snapshot mode never pauses
the vCPUs, and in stop mode the guest boots fresh. For the same reason,
vm_resume skips the sync when resuming from 'prelaunch' (e.g. a VM
started paused for a backup) or after a reset of a VM that shut down
during a backup.

Signed-off-by: Jakob Klocker <j.klocker@proxmox.com>
---
 src/PVE/API2/Qemu.pm           | 75 ++++++++++++++++++++++++++++++++--
 src/PVE/CLI/qm.pm              |  2 +-
 src/PVE/QemuConfig.pm          | 12 +++++-
 src/PVE/QemuMigrate.pm         |  3 ++
 src/PVE/QemuServer.pm          | 18 +++++++-
 src/PVE/QemuServer/Agent.pm    | 64 +++++++++++++++++++++++++++++
 src/PVE/QemuServer/RunState.pm | 15 ++++++-
 src/PVE/VZDump/QemuServer.pm   | 25 +++++++++++-
 8 files changed, 203 insertions(+), 11 deletions(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index 7830d09b..df2076a6 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -3599,6 +3599,24 @@ __PACKAGE__->register_method({
                 description => 'Whether to migrate conntrack entries for running VMs.',
             },
             'nets-host-mtu' => get_standard_option('pve-qm-nets-host-mtu'),
+            'sync-time-on-resume' => {
+                type => 'boolean',
+                description =>
+                    "Override the 'sync-time-on-resume' setting from the VM's guest agent (QGA)"
+                    . " config for this start only. Controls whether the guest's clock is"
+                    . " synchronized after the RAM state is restored. The guest agent must be"
+                    . " enabled.",
+                verbose_description =>
+                    "Override the 'sync-time-on-resume' setting from the VM's guest agent (QGA)"
+                    . " config for this start only. When enabled, the 'guest-set-time' QEMU guest"
+                    . " agent command is issued to synchronize the guest's clock to the host's current"
+                    . " time after the RAM state is restored, which happens here when starting a VM"
+                    . " that was hibernated (suspended to disk). Without synchronization the guest's"
+                    . " clock may still reflect the time from when the VM was hibernated. 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.",
+                optional => 1,
+            },
         },
     },
     returns => {
@@ -3614,6 +3632,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]);
@@ -3720,6 +3739,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);
@@ -4204,6 +4224,23 @@ __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 'sync-time-on-resume' setting from the VM's guest agent (QGA)"
+                    . " config for this resume only. Controls whether the guest's clock is"
+                    . " synchronized after the RAM state is restored. The guest agent must be"
+                    . " enabled.",
+                verbose_description =>
+                    "Override the 'sync-time-on-resume' setting from the VM's guest agent (QGA)"
+                    . " config for this resume only. When enabled, the 'guest-set-time' QEMU guest"
+                    . " agent command is issued to synchronize the guest's clock to the host's current"
+                    . " time after resuming the VM from a paused state. Without synchronization the"
+                    . " guest's clock may still reflect the time from when the VM was paused. 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.",
+                optional => 1,
+            },
         },
     },
     returns => {
@@ -4220,6 +4257,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';
@@ -4250,10 +4289,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;
@@ -6467,6 +6510,24 @@ __PACKAGE__->register_method({
                 optional => 1,
                 default => 0,
             },
+            'sync-time-on-resume' => {
+                type => 'boolean',
+                description =>
+                    "Override the 'sync-time-on-resume' setting from the VM's guest agent (QGA)"
+                    . " config for this rollback only. Controls whether the guest's clock is"
+                    . " synchronized after the RAM state is restored. The guest agent must be"
+                    . " enabled.",
+                verbose_description =>
+                    "Override the 'sync-time-on-resume' setting from the VM's guest agent (QGA)"
+                    . " config for this rollback only. When enabled, the 'guest-set-time' QEMU guest"
+                    . " agent command is issued to synchronize the guest's clock to the host's current"
+                    . " time after rolling back to a snapshot that includes RAM. Without"
+                    . " synchronization the guest's clock may still reflect the time from when the"
+                    . " snapshot was taken. 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.",
+                optional => 1,
+            },
         },
     },
     returns => {
@@ -6486,6 +6547,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.
@@ -6494,7 +6557,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 });
@@ -7150,7 +7217,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 f62fe036..ec3793ee 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 c84f10f8..d69ab447 100644
--- a/src/PVE/QemuConfig.pm
+++ b/src/PVE/QemuConfig.pm
@@ -387,6 +387,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 $@;
+            }
         }
     }
 }
@@ -470,8 +475,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
@@ -535,7 +541,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 = {
@@ -543,6 +549,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 8176afe5..e24544a7 100644
--- a/src/PVE/QemuMigrate.pm
+++ b/src/PVE/QemuMigrate.pm
@@ -1778,6 +1778,9 @@ sub phase3_cleanup {
                     $self->{errors} = 1;
                 }
             }
+            $self->log('info', "guest clock sync requested, will run on target after resume")
+                if !$self->{errors}
+                && PVE::QemuServer::Agent::should_sync_time_on_resume($conf->{agent});
         }
 
         if (
diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 8149184e..55733ee1 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -5047,7 +5047,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);
@@ -5910,6 +5913,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..5aaf76b2 100644
--- a/src/PVE/QemuServer/Agent.pm
+++ b/src/PVE/QemuServer/Agent.pm
@@ -4,6 +4,8 @@ use v5.36;
 
 use JSON;
 use MIME::Base64 qw(decode_base64 encode_base64);
+use Time::HiRes qw(time);
+use POSIX qw(strftime);
 
 use PVE::JSONSchema;
 
@@ -52,6 +54,25 @@ our $agent_fmt = {
         optional => 1,
         default => 1,
     },
+    # FIXME: MAJOR VERSION: 10.0.0 - reconsider setting option on as default
+    'sync-time-on-resume' => {
+        description =>
+            "Synchronize the guest's clock through QGA after operations that can leave"
+            . " the guest's clock behind.",
+        verbose_description =>
+            "Whether to issue the 'guest-set-time' QEMU guest agent command to synchronize the"
+            . " guest's clock to the host's current time after operations that can leave the guest's"
+            . " clock behind. This happens when resuming from hibernation, resuming from a paused"
+            . " state, taking a snapshot, after a migration, after a backup in suspend mode, 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 +353,47 @@ 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.
+
+Returns 1 if the time was set, 0 if it was skipped because the agent is not running.
+Dies if the agent returns an error.
+
+=cut
+
+sub guest_set_time($vmid, $quiet = undef) {
+    if (!qga_check_running($vmid, 1)) {
+        print "skipping guest set time - agent configured but not running?\n" if !$quiet;
+        return 0;
+    }
+    my $time_ns = 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");
+    my $time_str = strftime("%F %T", localtime($time_ns / 1_000_000_000));
+    print "synced guest clock via guest agent to $time_str\n" if !$quiet;
+    return 1;
+}
+
 1;
diff --git a/src/PVE/QemuServer/RunState.pm b/src/PVE/QemuServer/RunState.pm
index 889ddd28..2588dabb 100644
--- a/src/PVE/QemuServer/RunState.pm
+++ b/src/PVE/QemuServer/RunState.pm
@@ -131,7 +131,7 @@ sub vm_suspend($vmid, $skiplock = undef, $includestate = undef, $statestorage =
 # $nocheck is set when called as part of a migration - in this context the
 # 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($vmid, $skiplock = undef, $nocheck = undef) {
+sub vm_resume($vmid, $skiplock = undef, $nocheck = undef, $sync_time = undef, $quiet = undef) {
 
     PVE::QemuConfig->lock_config(
         $vmid,
@@ -183,6 +183,19 @@ sub vm_resume($vmid, $skiplock = undef, $nocheck = undef) {
                 if $resume_cmd eq 'cont';
 
             mon_cmd($vmid, $resume_cmd);
+
+            # A VM in 'prelaunch' never ran, and a VM that shut down during a backup is reset
+            # above. Both boot fresh, so the clock is not behind and the agent is not up yet.
+            my $guest_was_running = !$reset && ($res->{status} // '') ne 'prelaunch';
+            if (
+                $resume_cmd eq 'cont'
+                && $guest_was_running
+                && 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 $@;
+            }
         },
     );
 }
diff --git a/src/PVE/VZDump/QemuServer.pm b/src/PVE/VZDump/QemuServer.pm
index b1240f8f..292e7870 100644
--- a/src/PVE/VZDump/QemuServer.pm
+++ b/src/PVE/VZDump/QemuServer.pm
@@ -1185,20 +1185,41 @@ sub enforce_vm_running_for_backup {
     die $@ if $@;
 }
 
+my sub sync_guest_time_after_resume {
+    my ($self, $vmid) = @_;
+
+    my $agent_str = $self->{vmlist}->{$vmid}->{agent};
+    return if !PVE::QemuServer::Agent::should_sync_time_on_resume($agent_str);
+
+    my $synced = eval { PVE::QemuServer::Agent::guest_set_time($vmid, 1) };
+    if (my $err = $@) {
+        $self->log('warn', "could not sync guest time - $err");
+    } elsif ($synced) {
+        $self->loginfo("synced guest clock via guest agent");
+    } else {
+        $self->loginfo("skipping guest time sync - agent configured but not running?");
+    }
+}
+
 # resume VM again once in a clear state (stop mode backup of running VM)
 sub resume_vm_after_job_start {
     my ($self, $task, $vmid) = @_;
 
     return if !$self->{vm_was_running} || $self->{vm_was_paused};
 
-    if (my $stoptime = $task->{vmstoptime}) {
-        my $delay = time() - $task->{vmstoptime};
+    my $stoptime = $task->{vmstoptime};
+    if ($stoptime) {
+        my $delay = time() - $stoptime;
         $task->{vmstoptime} = undef; # avoid printing 'online after ..' twice
         $self->loginfo("resuming VM again after $delay seconds");
     } else {
         $self->loginfo("resuming VM again");
     }
     mon_cmd($vmid, 'cont', timeout => 45);
+
+    # only after a real vCPU pause: in stop mode the guest just booted, and $stoptime is
+    # cleared after the first call, so the error path does not sync twice
+    sync_guest_time_after_resume($self, $vmid) if $stoptime && $task->{mode} eq 'suspend';
 }
 
 # stop again if VM was not running before
-- 
2.47.3