Re: [PATCH qemu-server v2 1/2] fix #5032: qemu: sync guest time on resume and snapshot of saved state

[Fiona Ebner <f.ebner@proxmox.com>]

Am 22.06.26 um 3:47 PM schrieb Jakob Klocker:
> When a VM is resumed from a saved state (hibernation or snapshot with
> RAM), the guest clock may be stale. A time skew can occur when
> creating a snapshot of a running VM.
> 
> Add a new agent option `set-time-on-resume` to automatically
> synchronize the guest time with the host using the QEMU Guest Agent.
> 
> Trigger time synchronization:
> - after restoring a VM from a saved state or snapshot with RAM
> - after taking a snapshot
> 
> This ensures consistent guest time after rollback, restore, or
> snapshot operations when the guest OS clock does not automatically
> correct itself.
> 

It's not limited to snapshots with RAM. A snapshot operation without RAM
will also pause the guest. And I think it also makes sense to do it for
resume after a pause.

For rollback, the agent option from the saved snapshot configuration
will be used, right? So if I created the snapshot originally with
'set-time-on-resume', then it will always be applied upon rollback with
no way to opt-out. And vice-versa if disabled. Should we add an option
to the 'qm rollback' command to allow overriding this? Or
unconditionally use the current value for 'set-time-on-resume' instead
upon rollback? Not sure if that would be more in line with expectations
or more surprising?

Not quite the same for resume from hibernation, but it will also be
impossible to modify the setting if already suspended, because of the
config lock.

And for a paused VM, it would be impossible to change because 'agent'
property changes are not (yet) hot-plugged, but that might make sense to
do actually. But not sure if that would be better done as part of a
major release.

On the other hand, resume (from pause and suspend) could also gain
support for an option to override the 'set-time-on-resume' setting.

Maybe the command options could default to the current guest config
setting if not explicitly specified?

What do you think?

> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=5032
> 
> Signed-off-by: Jakob Klocker <j.klocker@proxmox.com>
> Reviewed-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> Tested-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
> 
> changes since v1:
> - adapt warning message in the resume-from-saved-state path
> 
>  src/PVE/QemuConfig.pm       |  7 ++++++
>  src/PVE/QemuServer.pm       | 10 ++++++++
>  src/PVE/QemuServer/Agent.pm | 47 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 64 insertions(+)
> 
> diff --git a/src/PVE/QemuConfig.pm b/src/PVE/QemuConfig.pm
> index c24eb835..3f0ff663 100644
> --- a/src/PVE/QemuConfig.pm
> +++ b/src/PVE/QemuConfig.pm
> @@ -383,6 +383,13 @@ sub __snapshot_create_vol_snapshots_hook {
>                      next;
>                  }
>              }
> +            if ($snap->{vmstate}) {
> +                my $conf = $class->load_config($vmid);
> +                if (PVE::QemuServer::Agent::should_set_time_on_resume($conf->{agent})) {
> +                    eval { PVE::QemuServer::Agent::guest_set_time($vmid); 1 }
> +                        or warn "could not sync guest time after snapshot - $@";

Style nit: our code base uses the following pattern:
eval { func(); };
warn "msg - $@" if $@;

> +                }
> +            }
>          }
>      }
>  }
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index 55e9f520..daf10904 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -5992,6 +5992,16 @@ sub vm_start_nolock {
>          );
>      }
>  
> +    my $from_saved_state = $resume || ($statefile && !$migratedfrom);

Style nit: I'd kinda prefer to avoid this one-off variable. Maybe add a
short comment near the expression instead?

> +
> +    if (
> +        $from_saved_state
> +        && PVE::QemuServer::Agent::should_set_time_on_resume($conf->{agent})
> +    ) {
> +        eval { PVE::QemuServer::Agent::guest_set_time($vmid); 1 }
> +            or warn "could not sync guest time after resume from saved state - $@";

Same style nit as above.

> +    }
> +
>      return $res;
>  }
>  
> diff --git a/src/PVE/QemuServer/Agent.pm b/src/PVE/QemuServer/Agent.pm
> index be6df443..be8bbae6 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 ();

Stlye nit: I'd use qw() for consistency with the previous line.

>  
>  use PVE::JSONSchema;
>  
> @@ -18,6 +19,8 @@ our @EXPORT_OK = qw(
>      get_qga_key
>      parse_guest_agent
>      qga_check_running
> +    should_set_time_on_resume
> +    guest_set_time

I don't think these are worth exporting. You already call them with the
module prefix and I do think it's helpful to be explicit like that.

>  );
>  
>  our $agent_fmt = {
> @@ -52,6 +55,21 @@ our $agent_fmt = {
>          optional => 1,
>          default => 1,
>      },
> +    'set-time-on-resume' => {

I wonder if 'set' is the best? I'm thinking about 'update' or
'synchronize', but maybe those make it too long? 'sync'? Not sure. What
do you think?

> +        description => "Update the guest clock through QGA after resuming from"
> +            . " hibernation or rolling back to a snapshot with RAM.",
> +        verbose_description =>
> +            "Whether to issue the guest-set-time QEMU guest agent command after the VM"
> +            . " resumes with a restored RAM state, that is, when waking from hibernation"
> +            . " or after rolling back to a snapshot that includes RAM. In these cases the"
> +            . " guest's clock still reflects the time the state was saved. With this"

Nit: s/was saved/was saved at/

The description does not mention that it also happens after a snapshot
create operation.

> +            . " option enabled, the clock is synchronized to the host's current time,"
> +            . " provided the QEMU Guest Agent option is enabled in the guest's"
> +            . " configuration and the agent is running inside of the guest.",
> +        type => 'boolean',
> +        optional => 1,
> +        default => 1,

I don't think we should enable this by default, at least not outside of
a major release. What if something relies on the current behavior and
expects a rolled-back state to have the old time?

> +    },
>      type => {
>          description => "Select the agent type",
>          type => 'string',
> @@ -332,4 +350,33 @@ sub guest_fs_freeze_applicable($agent_str, $vmid, $logfunc = undef) {
>      return 1;
>  }
>  
> +=head3 should_set_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. Does B<not> check whether the agent is actually running.
> +
> +=cut
> +
> +sub should_set_time_on_resume($agent_str) {
> +    my $agent = parse_guest_agent($agent_str);
> +    return 0 if !$agent->{enabled};
> +    return $agent->{'set-time-on-resume'} // 1;
> +}
> +
> +=head3 guest_set_time
> +
> +Sets the guest's clock via the QEMU Guest Agent's C<guest-set-time> command. If C<$time_ns>
> +(nanoseconds since the UNIX epoch, UTC) is not given, the current host time is used. Passing
> +an explicit time is required because the agent's argument-less form reads the guest's RTC,
> +which may itself be stale after a vmstate snapshot or resume.
> +
> +=cut
> +
> +sub guest_set_time($vmid, $time_ns = undef) {

Not sure if adding the time parameter is worth it if it's not used. We
can still add it later if a caller that needs it pops up.

> +    $time_ns //= int(Time::HiRes::time() * 1_000_000_000);
> +    my $res = PVE::QemuServer::Monitor::mon_cmd($vmid, 'guest-set-time', time => $time_ns);

Nit: I'd prefer the coercing to int() to be done here when passing the
param to mon_cmd()

> +    check_agent_error($res, "unable to set guest time");
> +    return;
> +}
> +
>  1;