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

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

On Thu Nov 5, 2026 at 9:16 PM CET, Thomas Lamprecht wrote:
> Am 13.08.26 um 15:57 schrieb Jakob Klocker:
>> 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.
>> 
>> A version check was added for migration to display a clear error
>> message in case 'sync-time-on-resume' is enabled and the target doesn't
>> support the 'sync-time-on-resume' the option.
>> 
>> 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         | 13 ++++++
>>  src/PVE/QemuServer.pm          | 18 +++++++-
>>  src/PVE/QemuServer/Agent.pm    | 61 +++++++++++++++++++++++++++
>>  src/PVE/QemuServer/RunState.pm | 11 ++++-
>>  7 files changed, 183 insertions(+), 9 deletions(-)
>
>> diff --git a/src/PVE/QemuMigrate.pm b/src/PVE/QemuMigrate.pm
>> index 8da6f15d..ae40195c 100644
>> --- a/src/PVE/QemuMigrate.pm
>> +++ b/src/PVE/QemuMigrate.pm
>> @@ -312,6 +312,16 @@ sub prepare {
>>          }
>>      }
>>  
>> +    my $target_version = PVE::QemuServer::Helpers::get_node_pvecfg_version($self->{node});
>
> not sure if this is all worth it, we normally require all nodes to be the same level
> and existing VMs do not automatically gain this on upgrade, one has to explicitly
> configure it first. We could also avoid it for now for migration (and probably taking
> a snapshot) those are milliseconds at max anyway (else it would not really be live
> anymore). Could be enabled later for that and then potentially backup snapshots too?
> There's also a brief pause there I think?
>
>

@Daniel mentioned [0] that time sync after taking a snapshot has been
requested in enterprise support before. After checking with another 
colleague, the last request for the snapshot sync in enterprise support 
was just a week ago. Some users report time skews up to a few seconds
when taking snapshots, therefore I'd opt for not deffering it.

If you prefer, we can drop the migration and I'll add it in a follow
up including backup syncs as well. I wouldn't mind sending a
v6 including backups either. Let me know what you prefer.

Ack about dropping the version guards.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=5032#c6

> But if we indeed want to add this check then it needs two changes...
>
>
>> +    if (
>> +        $target_version
>> +        && !PVE::QemuServer::Helpers::pvecfg_min_version($target_version, 9, 2, 5)
>
> ... this new call to PVE::QemuServer::Helpers::pvecfg_min_version(...) could be
> swapped with the new location of that, i.e. PVE::Tools::pvecfg_min_version(...)
> and the version updated (or at least a prominent NOTE in the diffstat section of
> this patch added as reminder to update it on applying.
>
>> +        && PVE::QemuServer::Agent::get_qga_key($conf, 'sync-time-on-resume')
>
> This misses one explicitly setting the property to 0, in which case the other
> node still needs to support it. Would need a definedness check here.
>
>> +    ) {
>> +        die "target node '$self->{node}' is too old to support 'sync-time-on-resume'"
>> +            . " (needs pve-manager >= 9.2.5); disable the option before migrating\n";
>> +    }
>> +
>>      my $vollist = PVE::QemuServer::get_vm_volumes($conf);
>>  
>>      my $storages = {};