On Thu, Jan 30, 2025 at 05:44:08PM +0100, Peter Krempa wrote:
> On Wed, Jan 08, 2025 at 19:42:43 +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.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> > ---
> > src/hypervisor/domain_driver.c | 46 +++++++++++++++++++++++++++++++---
> > src/hypervisor/domain_driver.h | 18 ++++++++++---
> > src/libvirt_private.syms | 2 ++
> > src/qemu/qemu_driver.c | 6 ++---
> > 4 files changed, 63 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c
> > index ea3f1cbfcd..4fecaf7e5c 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");
>
> since we have the 'ToString' function ...
>
> > +
> > char *
> > virDomainDriverGenerateRootHash(const char *drivername,
> > const char *root)
> > @@ -715,6 +722,7 @@ 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=%d",
>
> ... consider logging the string here instead of the number.
>
> > @@ -735,6 +743,12 @@ 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 (!(conn = virConnectOpen(cfg->uri)))
> > goto cleanup;
> >
> > @@ -744,10 +758,22 @@ virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg)
> > goto cleanup;
> >
> > VIR_DEBUG("Auto shutdown with %d running domains", numDomains);
> > - if (cfg->trySave) {
> > +
> > + transient = g_new0(bool, numDomains);
> > + for (i = 0; i < numDomains; i++) {
> > + if (virDomainIsPersistent(domains[i]) == 0)
> > + transient[i] = true;
> > + }
> > +
> > + if (cfg->trySave != VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE) {
> > g_autofree unsigned int *flags = g_new0(unsigned int, numDomains);
> > for (i = 0; i < numDomains; i++) {
> > int state;
> > +
> > + if ((transient[i] && cfg->trySave == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_PERSISTENT) ||
> > + (!transient[i] && cfg->trySave == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_TRANSIENT))
> > + continue;
> > +
> > /*
> > * Pause all VMs to make them stop dirtying pages,
> > * so save is quicker. We remember if any VMs were
> > @@ -773,11 +799,16 @@ virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg)
> > }
> > }
> >
> > - if (cfg->tryShutdown) {
> > + if (cfg->tryShutdown != VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE) {
> > GTimer *timer = NULL;
> > for (i = 0; i < numDomains; i++) {
> > if (domains[i] == NULL)
> > continue;
> > +
> > + if ((transient[i] && cfg->tryShutdown == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_PERSISTENT) ||
> > + (!transient[i] && cfg->tryShutdown == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_TRANSIENT))
> > + continue;
>
> Trying to 'managedSave' a transient VM will/should fail as managedSave
> doesn't make sense with transient VMS:
Hmmm, true, we should reject 'trySave == transient' right at the start.
Also if virDomainManagedSave() fails, we need to add a call
to virDomainResume, otherwise the fallthrough to virDomainShutdown
is useless as the VM will be paused still.
>
>
> > +
> > if (virDomainShutdown(domains[i]) < 0) {
> > VIR_WARN("Unable to perform graceful shutdown of '%s': %s",
> > virDomainGetName(domains[i]),
>
> 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 :|
