On Thu, Jan 30, 2025 at 06:21:07PM +0100, Peter Krempa wrote:
> On Wed, Jan 08, 2025 at 19:42:44 +0000, Daniel P. Berrangé wrote:
> > Currently automatic VM managed save is only performed in session
> > daemons, on desktop session close, or host OS shutdown request.
> >
> > With this change it is possible to control shutdown behaviour for
> > all daemons. A recommended setup might be:
> >
> > auto_shutdown_try_save = "persistent"
> > auto_shutdown_try_shutdown = "all"
> > auto_shutdown_poweroff = "all"
> >
> > Each setting accepts 'none', 'persistent', 'transient', and 'all'
> > to control what types of guest it applies to.
> >
> > For historical compatibility, for the system daemon, the settings
> > currently default to:
> >
> > auto_shutdown_try_save = "none"
> > auto_shutdown_try_shutdown = "none"
> > auto_shutdown_poweroff = "none"
> >
> > while for the session daemon they currently default to
> >
> > auto_shutdown_try_save = "all"
> > auto_shutdown_try_shutdown = "none"
> > auto_shutdown_poweroff = "none"
> >
> > The system daemon settings should NOT be enabled if the traditional
> > libvirt-guests.service is already enabled.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> > ---
> > src/qemu/libvirtd_qemu.aug | 3 ++
> > src/qemu/qemu.conf.in | 37 ++++++++++++++++++++++
> > src/qemu/qemu_conf.c | 51 ++++++++++++++++++++++++++++++
> > src/qemu/qemu_conf.h | 3 ++
> > src/qemu/qemu_driver.c | 9 +++---
> > src/qemu/test_libvirtd_qemu.aug.in | 3 ++
> > 6 files changed, 101 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
> > index 642093c40b..e465a231fc 100644
> > --- a/src/qemu/libvirtd_qemu.aug
> > +++ b/src/qemu/libvirtd_qemu.aug
> > @@ -98,6 +98,9 @@ module Libvirtd_qemu =
> > | bool_entry "auto_dump_bypass_cache"
> > | bool_entry "auto_start_bypass_cache"
> > | int_entry "auto_start_delay"
> > + | str_entry "auto_shutdown_try_save"
> > + | str_entry "auto_shutdown_try_shutdown"
> > + | str_entry "auto_shutdown_powerdown"
>
> Don't forget to merge in the appropriate hunk from the next patch to
> make tests pass.
>
> >
> > let process_entry = str_entry "hugetlbfs_mount"
> > | str_entry "bridge_helper"
> > diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in
> > index a3e9bbfcf3..d8890c4c32 100644
> > --- a/src/qemu/qemu.conf.in
> > +++ b/src/qemu/qemu.conf.in
> > @@ -638,6 +638,43 @@
> > #
> > #auto_start_delay = 0
> >
> > +# Whether to perform managed save of running VMs if a host OS
> > +# shutdown is requested (system/session daemons), or the desktop
> > +# session terminates (session daemon only). Accepts
> > +#
> > +# * "none" - do not try to save any running VMs
> > +# * "persistent" - only try to save persistent running VMs
> > +# * "transient" - only try to save transient running VMs
> > +# * "all" - try to save all running VMs
> > +#
> > +# Defaults to "all" for session daemons and "none"
> > +# for system daemons
> > +#
> > +# If 'libvirt-guests.service' is enabled, then this must be
> > +# set to 'none' for system daemons to avoid dueling actions
> > +#auto_shutdown_try_save = "all"
>
> As demonstrated in previous patch, the 'transient' and 'all' make no
> sense here, as all you'll get is an error per semantics of managed save.
>
> The only way we could make saving of transient VMs work is to invoke the
> normal save API and dump them in a directory configured for this reason.
>
> But then you get problem of what to do at startup as IIUC the native
> replacement to libvirt-guests will only support startup via 'autostart',
> thus that'd overcomplicate things a bit more.
>
> I'd suggest to simply don't do save for transient VMs.
Yes, I'm going to block 'all' and 'transient' and rephase
these inline config file docs.
> > + if (virConfGetValueString(conf, "auto_shutdown_try_save", &autoShutdownScope) < 0)
> > + return -1;
> > +
> > + if (autoShutdownScope != NULL &&
> > + (cfg->autoShutdownTrySave =
> > + virDomainDriverAutoShutdownScopeTypeFromString(autoShutdownScope)) < 0) {
> > + virReportError(VIR_ERR_INVALID_ARG,
> > + _("unknown auto_shutdown_try_save '%1$s'"),
> > + autoShutdownScope);
> > + return -1;
> > + }
>
> Here it'll need to reject the two unsupported options as well.
yep
> > diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> > index 61a2bdce51..5d9ace6dcc 100644
> > --- a/src/qemu/qemu_conf.h
> > +++ b/src/qemu/qemu_conf.h
> > @@ -201,6 +201,9 @@ struct _virQEMUDriverConfig {
> > bool autoDumpBypassCache;
> > bool autoStartBypassCache;
> > int autoStartDelayMS;
> > + int autoShutdownTrySave;
> > + int autoShutdownTryShutdown;
> > + int autoShutdownPoweroff;
>
> Preferrably use the enum type, although that'll make the parser a bit
> more verbose due to absence of something like virXMLPropEnum.
It is pretty easy actually, so done that too
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
