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 25.06.26 um 9:45 AM schrieb Jakob Klocker:
> On 6/24/26 1:31 PM, Fiona Ebner wrote:
>> 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.
>>
> Thanks for pointing that out, I missed that. I'll make sure to sync
> the time for snapshots without RAM as well.
> 
> I will look into time sync for resume and add it in this series. I
> question whether we want one agent option for all the time syncs (on
> snapshot, rollback, pause/start) or make it more fine-grained. I'd
> lean toward keeping a single option.
>> 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?
> Exactly, the config from the snapshot will be used. I've discussed
> this offlist with colleagues before sending the patch, since I also
> disliked that there is no way to disable the time sync for a snapshot
> once it was taken. We didn't come up with any real use cases where
> someone would want this behavior and therefore I didn't look into this
> further. I forgot that someone might want to enable it for older
> snapshots, so the override would definitely make sense!
> 
> I do agree that it feels counterintuitive, I'd expect that if the
> current config has sync time disabled, the rollback wouldn't use this
> option. I'd opt for using the current guest config, this would feel
> most intuitive to me, but am open to suggestions.
> I'll gladly add it in a v3.

As discussed off-list today, we could add a new 'sync-time-on-resume'
option to the resume command (for resume from disk as well as from
paused) and the snapshot rollback command. The default value for these
would be the current configuration setting. And if the option is
explicitly set for the operation, the explicitly set value is used. This
should give users all the necessary flexibility and like that, it also
seems natural that the current configuration value serves as a fallback
(rather than the value in the snapshot config).

As for snapshot create, I would use the current configuration value to
decide whether the time should be synced after the operation and not add
a command option. If somebody has a use case where that would actually
be required, we could still add it later, but it's a bit hard to imagine
right now.

>>> +            . " 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?
>>
> I'll change the default to disabled, but would like this reconsidered
> before a major release. A VM whose clock is always in sync feels more
> intuitive to me.>> +    },

Feel free to add a reminder comment that this should be considered with
the usual '# FIXME: MAJOR VERSION: xyz' comment template.