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@redhat.com> --- 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@redhat.com>
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 :|