On Thu, Mar 13, 2025 at 12:18:28PM +0100, Peter Krempa wrote:
> On Wed, Mar 12, 2025 at 17:17:44 +0000, Daniel P. Berrangé wrote:
> > It may be desirable to treat transient VMs differently from persistent
> > VMs. For example, while performing managed save on persistent VMs makes
> > sense, the same not usually true of transient VMs, since by their
> > nature they will have no config to restore from.
> >
> > This also lets us fix a long standing problem with incorrectly
> > attempting to perform managed save on transient VMs.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> > ---
> > src/hypervisor/domain_driver.c | 62 +++++++++++++++++++++++++++++++---
> > src/hypervisor/domain_driver.h | 18 ++++++++--
> > src/libvirt_private.syms | 2 ++
> > src/qemu/qemu_driver.c | 6 ++--
> > 4 files changed, 77 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c
> > index e625726c07..c510e1d2ae 100644
> > --- a/src/hypervisor/domain_driver.c
> > +++ b/src/hypervisor/domain_driver.c
> > @@ -35,6 +35,13 @@
> >
> > VIR_LOG_INIT("hypervisor.domain_driver");
> >
> > +VIR_ENUM_IMPL(virDomainDriverAutoShutdownScope,
> > + VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_LAST,
> > + "none",
> > + "persistent",
> > + "transient",
> > + "all");
> > +
> > char *
> > virDomainDriverGenerateRootHash(const char *drivername,
> > const char *root)
> > @@ -721,9 +728,13 @@ virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg)
> > int numDomains = 0;
> > size_t i;
> > virDomainPtr *domains = NULL;
> > + g_autofree bool *transient = NULL;
> >
> > - VIR_DEBUG("Run autoshutdown uri=%s trySave=%d tryShutdown=%d poweroff=%d waitShutdownSecs=%u",
> > - cfg->uri, cfg->trySave, cfg->tryShutdown, cfg->poweroff,
> > + VIR_DEBUG("Run autoshutdown uri=%s trySave=%s tryShutdown=%s poweroff=%s waitShutdownSecs=%u",
> > + cfg->uri,
> > + virDomainDriverAutoShutdownScopeTypeToString(cfg->trySave),
> > + virDomainDriverAutoShutdownScopeTypeToString(cfg->tryShutdown),
> > + virDomainDriverAutoShutdownScopeTypeToString(cfg->poweroff),
> > cfg->waitShutdownSecs);
> >
> > /*
> > @@ -740,6 +751,21 @@ virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg)
> > if (cfg->waitShutdownSecs <= 0)
> > cfg->waitShutdownSecs = 30;
> >
> > + /* Short-circuit if all actions are disabled */
> > + if (cfg->trySave == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE &&
> > + cfg->tryShutdown == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE &&
> > + cfg->poweroff == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE)
> > + return;
> > +
> > + if (cfg->trySave == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_ALL ||
> > + cfg->trySave == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_TRANSIENT) {
> > + /* virDomainManagedSave will return an error. We'll let
> > + * the code run, since it'll just fall through to the next
> > + * actions, but give a clear warning upfront */
> > + VIR_WARN("Managed save not supported for transient VMs");
> > + return;
>
> This looks like better suited to the config parser. Reporting a
> programming error as warning seems wrong.
>
> This code should IMO at ignore invalid setting for managed-save config
> rather than not do anything at all.
The qemu_conf.c file will validate that trySave != all|transient
too, and raise a fatal error there. So this branch should not
be triggered here - its more of a safety net in case we forget
the same checks when extending it to other drivers.
You are right though, we could just force trySave == persistent
in this case, so at least everything else works.
>
> > + }
> > +
> > if (!(conn = virConnectOpen(cfg->uri)))
> > goto cleanup;
>
> Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
>
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 :|
