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;
+ }
+
if (!(conn = virConnectOpen(cfg->uri)))
goto cleanup;
@@ -749,10 +775,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
@@ -781,11 +819,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;
+
if (virDomainShutdown(domains[i]) < 0) {
VIR_WARN("Unable to request graceful shutdown of '%s': %s",
virDomainGetName(domains[i]),
@@ -801,6 +844,10 @@ virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg)
if (!domains[i])
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;
+
if (virDomainIsActive(domains[i]) == 1) {
anyRunning = true;
} else {
@@ -818,10 +865,15 @@ virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg)
g_timer_destroy(timer);
}
- if (cfg->poweroff) {
+ if (cfg->poweroff != VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE) {
for (i = 0; i < numDomains; i++) {
if (domains[i] == NULL)
continue;
+
+ if ((transient[i] && cfg->poweroff == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_PERSISTENT) ||
+ (!transient[i] && cfg->poweroff == VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_TRANSIENT))
+ continue;
+
/*
* NB might fail if we gave up on waiting for
* virDomainShutdown, but it then completed anyway,
diff --git a/src/hypervisor/domain_driver.h b/src/hypervisor/domain_driver.h
index ff68517edd..6e535ca444 100644
--- a/src/hypervisor/domain_driver.h
+++ b/src/hypervisor/domain_driver.h
@@ -24,6 +24,7 @@
#include "virhostdev.h"
#include "virpci.h"
#include "virdomainobjlist.h"
+#include "virenum.h"
char *
virDomainDriverGenerateRootHash(const char *drivername,
@@ -91,11 +92,22 @@ typedef struct _virDomainDriverAutoStartConfig {
void virDomainDriverAutoStart(virDomainObjList *domains,
virDomainDriverAutoStartConfig *cfg);
+typedef enum {
+ VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE,
+ VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_PERSISTENT,
+ VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_TRANSIENT,
+ VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_ALL,
+
+ VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_LAST,
+} virDomainDriverAutoShutdownScope;
+
+VIR_ENUM_DECL(virDomainDriverAutoShutdownScope);
+
typedef struct _virDomainDriverAutoShutdownConfig {
const char *uri;
- bool trySave;
- bool tryShutdown;
- bool poweroff;
+ virDomainDriverAutoShutdownScope trySave;
+ virDomainDriverAutoShutdownScope tryShutdown;
+ virDomainDriverAutoShutdownScope poweroff;
unsigned int waitShutdownSecs; /* Seconds to wait for VM to shutdown
* before moving onto next action.
* If 0 a default is used (currently 30 secs)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e6fec0a151..5f3aa10371 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1650,6 +1650,8 @@ virDomainCgroupSetupVcpuBW;
# hypervisor/domain_driver.h
virDomainDriverAddIOThreadCheck;
virDomainDriverAutoShutdown;
+virDomainDriverAutoShutdownScopeTypeFromString;
+virDomainDriverAutoShutdownScopeTypeToString;
virDomainDriverAutoStart;
virDomainDriverDelIOThreadCheck;
virDomainDriverGenerateMachineName;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 946f1c7e96..ec389453e7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -947,9 +947,9 @@ qemuStateStop(void)
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(qemu_driver);
virDomainDriverAutoShutdownConfig ascfg = {
.uri = cfg->uri,
- .trySave = true,
- .tryShutdown = false,
- .poweroff = false,
+ .trySave = VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_PERSISTENT,
+ .tryShutdown = VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE,
+ .poweroff = VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE
};
if (!qemu_driver->privileged)
--
2.48.1On 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.
> + }
> +
> if (!(conn = virConnectOpen(cfg->uri)))
> goto cleanup;
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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 :|
© 2016 - 2025 Red Hat, Inc.