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

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

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?

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 = {};