On Wed, Jan 08, 2025 at 19:42:34 +0000, Daniel P. Berrangé wrote:

We have a way to notify systemd when we're done starting the daemon.

Systemd supports many more notifications, however, and many of them are quite relevant to our needs:

https://www.freedesktop.org/software/systemd/man/latest/sd_notify.html

This renames the existing notification API to better reflect its semantics, and adds new APIs for reporting

* Initiation of config file reload * Initiation of daemon shutdown process * Adhoc progress status messages * Request to extend service shutdown timeout

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 6 +++++- src/rpc/virnetdaemon.c | 2 +- src/util/virsystemd.c | 41 +++++++++++++++++++++++++++++++++++++--- src/util/virsystemd.h | 6 +++++- 4 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 33b93cbd3e..d0c116b78c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3524,7 +3524,11 @@ virSystemdHasResolved; virSystemdHasResolvedResetCachedValue; virSystemdMakeScopeName; virSystemdMakeSliceName; -virSystemdNotifyStartup; +virSystemdNotifyExtendTimeout; +virSystemdNotifyReady; +virSystemdNotifyReload; +virSystemdNotifyStatus; +virSystemdNotifyStopping; virSystemdResolvedRegisterNameServer; virSystemdTerminateMachine;

diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c index e4c6261536..bb7d2ff9a0 100644 --- a/src/rpc/virnetdaemon.c +++ b/src/rpc/virnetdaemon.c @@ -749,7 +749,7 @@ virNetDaemonRun(virNetDaemon *dmn)

/* We are accepting connections now. Notify systemd * so it can start dependent services. */ - virSystemdNotifyStartup(); + virSystemdNotifyReady();

VIR_DEBUG("dmn=%p quit=%d", dmn, dmn->quit); while (!dmn->finished) { diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index 5b772e29dd..2c09b9ab2b 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -624,12 +624,11 @@ int virSystemdTerminateMachine(const char *name) return 0; }

-void -virSystemdNotifyStartup(void) +static void +virSystemdNotify(const char *msg) { #ifndef WIN32 const char *path; - const char *msg = "READY=1"; int fd; struct sockaddr_un un = { .sun_family = AF_UNIX, @@ -644,6 +643,8 @@ virSystemdNotifyStartup(void) .msg_iovlen = 1, };

+ VIR_DEBUG("Notify '%s'", msg); + if (!(path = getenv("NOTIFY_SOCKET"))) { VIR_DEBUG("Skipping systemd notify, not requested"); return; @@ -674,6 +675,40 @@ virSystemdNotifyStartup(void) #endif /* !WIN32 */ }

+void virSystemdNotifyReady(void) +{ + virSystemdNotify("READY=1"); +} + +void virSystemdNotifyReload(void) +{ + virSystemdNotify("RELOAD=1"); +}

Per the man page for 'sd_notify' this should be 'RELOADING=1';

On Wed, Jan 08, 2025 at 19:42:35 +0000, Daniel P. Berrangé wrote:

Switch to the 'notify-reload' service type and send notifications to systemd when reloading configuration.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/libvirtd.service.in | 2 +- src/remote/remote_daemon.c | 2 ++ src/virtd.service.in | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/remote/libvirtd.service.in b/src/remote/libvirtd.service.in index 250b4a6fc3..b0a062e885 100644 --- a/src/remote/libvirtd.service.in +++ b/src/remote/libvirtd.service.in @@ -26,7 +26,7 @@ After=xencommons.service Conflicts=xendomains.service

[Service] -Type=notify +Type=notify-reload

^^^

Environment=LIBVIRTD_ARGS="--timeout 120" EnvironmentFile=-@initconfdir@/libvirtd ExecStart=@sbindir@/libvirtd $LIBVIRTD_ARGS diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index 1d079c7e4b..d44a365000 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -452,9 +452,11 @@ static void daemonReloadHandlerThread(void *opaque G_GNUC_UNUSED) virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", VIR_HOOK_DAEMON_OP_RELOAD, SIGHUP, "SIGHUP", NULL, NULL);

+ virSystemdNotifyReload();

The man page says for 'notify-reload': • Behavior of notify-reload is similar to notify, with one difference: the SIGHUP UNIX process signal is sent to the service's main process when the service is asked to reload and the manager will wait for a notification about the reload being finished. When initiating the reload process the service is expected to reply with a notification message via sd_notify(3) that contains the "RELOADING=1" field in combination with "MONOTONIC_USEC=" set to the current monotonic time (i.e. CLOCK_MONOTONIC in clock_gettime(2)) in μs, formatted as decimal string. Once reloading is complete another notification message must be sent, containing "READY=1". Using this service type and implementing this reload protocol is an efficient alternative to providing an ExecReload= command for reloading of the service's configuration. So IIUC we need to be also sending MONOTONIC_USEC= in addition to RELOADING=1 in order to do the handshake for 'notify-reload' properly.

if (virStateReload() < 0) { VIR_WARN("Error while reloading drivers"); } + virSystemdNotifyReady();

/* Drivers are initialized again. */ g_atomic_int_set(&driversInitialized, 1); diff --git a/src/virtd.service.in b/src/virtd.service.in index 651a8d82d7..7ffb77e339 100644 --- a/src/virtd.service.in +++ b/src/virtd.service.in @@ -15,7 +15,7 @@ After=dbus.service After=apparmor.service

[Service] -Type=notify +Type=notify-reload Environment=@SERVICE@_ARGS="--timeout 120" EnvironmentFile=-@initconfdir@/@service@ ExecStart=@sbindir@/@service@ $@SERVICE@_ARGS -- 2.47.1

On Thu, Jan 30, 2025 at 01:08:00PM +0100, Peter Krempa wrote:

On Wed, Jan 08, 2025 at 19:42:37 +0000, Daniel P. Berrangé wrote:

...

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/bhyve/bhyve_driver.c | 53 ++++++++++++--------------------------- src/libxl/libxl_driver.c | 36 ++++++++------------------- src/lxc/lxc_driver.c | 13 +++++----- src/lxc/lxc_process.c | 18 ++------------ src/lxc/lxc_process.h | 2 ++ src/qemu/qemu_driver.c | 54 ++++++++++++---------------------------- 6 files changed, 53 insertions(+), 123 deletions(-)

diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 8f97ac032c..ec997a38f5 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -71,41 +71,19 @@ VIR_LOG_INIT("bhyve.bhyve_driver"); struct _bhyveConn *bhyve_driver = NULL;

static int -bhyveAutostartDomain(virDomainObj *vm, void *opaque) +bhyveAutostartDomain(virDomainObj *vm, void *opaque G_GNUC_UNUSED) { - const struct bhyveAutostartData *data = opaque; int ret = 0; - VIR_LOCK_GUARD lock = virObjectLockGuard(vm); - - if (vm->autostart && !virDomainObjIsActive(vm)) { - virResetLastError(); - ret = virBhyveProcessStart(data->conn, vm, - VIR_DOMAIN_RUNNING_BOOTED, 0); - if (ret < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to autostart VM '%1$s': %2$s"), - vm->def->name, virGetLastErrorMessage()); - } - } - return ret; -} - -static void -bhyveAutostartDomains(struct _bhyveConn *driver) -{ - /* XXX: Figure out a better way todo this. The domain - * startup code needs a connection handle in order - * to lookup the bridge associated with a virtual - * network - */ - virConnectPtr conn = virConnectOpen("bhyve:///system"); - /* Ignoring NULL conn which is mostly harmless here */

- struct bhyveAutostartData data = { driver, conn }; - - virDomainObjListForEach(driver->domains, false, bhyveAutostartDomain, &data); + ret = virBhyveProcessStart(NULL, vm, + VIR_DOMAIN_RUNNING_BOOTED, 0);

int virBhyveProcessStart(virConnectPtr conn, virDomainObj *vm, virDomainRunningReason reason, unsigned int flags) { struct _bhyveConn *driver = conn->privateData;

The first thing that virBhyveProcessStart() does is to dereference 'conn'. So firstly the note about NULL con being "mostly harmless" is not true and we also can't do this here in the current state.

[...]

The rest looks good. If you plan to fix the bhyve stuff separately (e.g. drop 'conn') and what remains in this patch will be a trivial change, you can add:

Urgh, don't know why I didn't check that more closely. We should just pass 'driver' into virBhyveProcessStart directly, since all the callers have access to it already.

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

Here.

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 :|

On Wed, Jan 08, 2025 at 19:42:38 +0000, Daniel P. Berrangé wrote:

This delay can reduce the CPU/IO load storm when autostarting many guests.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/hypervisor/domain_driver.c | 20 +++++++++++++++++--- src/hypervisor/domain_driver.h | 1 + 2 files changed, 18 insertions(+), 3 deletions(-)

[...]

diff --git a/src/hypervisor/domain_driver.h b/src/hypervisor/domain_driver.h index c27ed9155e..d695e26f90 100644 --- a/src/hypervisor/domain_driver.h +++ b/src/hypervisor/domain_driver.h @@ -84,6 +84,7 @@ typedef struct _virDomainDriverAutoStartConfig { char *stateDir; virDomainDriverAutoStartCallback callback; void *opaque; + int delayMS; /* milliseconds between starting each guest */

As negative delay doesn't make sense consider declaring this as unsigned. Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Thu, Jan 30, 2025 at 01:16:42PM +0100, Peter Krempa wrote:

On Wed, Jan 08, 2025 at 19:42:39 +0000, Daniel P. Berrangé wrote:

...

This allows a user specified delay between autostart of each VM, giving parity with the equivalent feature of libvirt-guests.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf.in | 4 ++++ src/qemu/qemu_conf.c | 2 ++ src/qemu/qemu_conf.h | 1 + src/qemu/qemu_driver.c | 1 + src/qemu/test_libvirtd_qemu.aug.in | 1 + 6 files changed, 10 insertions(+)

[...]

...

diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in index d853136f10..a3e9bbfcf3 100644 --- a/src/qemu/qemu.conf.in +++ b/src/qemu/qemu.conf.in @@ -634,6 +634,10 @@ # #auto_start_bypass_cache = 0

+# Delay in milliseconds between starting each VM, during autostart

I'd suggest you mention that the delay is between kicking off the startup of the autostarted VMs, so that it's obvious that we don't/can't see when the VM actually booted (whatever the definition of 'booted' would be for users).

Yeah, good idea. Side note, for modern Linux guests users can now seen when a VM has fully booted if they add a virtio-vsock device, as systemd is able to notify the host over that. Not something we can/should rely on here though.

...

+# +#auto_start_delay = 0 + # If provided by the host and a hugetlbfs mount point is configured, # a guest may request huge page backing. When this mount point is # unspecified here, determination of a host mount point in /proc/mounts diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8b9fe4e381..0b6b923bcb 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -638,6 +638,8 @@ virQEMUDriverConfigLoadSaveEntry(virQEMUDriverConfig *cfg, return -1; if (virConfGetValueBool(conf, "auto_start_bypass_cache", &cfg->autoStartBypassCache) < 0) return -1; + if (virConfGetValueInt(conf, "auto_start_delay", &cfg->autoStartDelayMS) < 0) + return -1;

return 0; } diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 42cdb6f883..61a2bdce51 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -200,6 +200,7 @@ struct _virQEMUDriverConfig { char *autoDumpPath; bool autoDumpBypassCache; bool autoStartBypassCache; + int autoStartDelayMS;

Once again, preferrably declare this as unsigned. Especially prevent users passing negative values.

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 :|

On Wed, Jan 08, 2025 at 19:42:41 +0000, Daniel P. Berrangé wrote:

Currently the virStateStop method is only wired up to run save for the unprivileged daemons, so there is no functional change.

IOW, session exit, or host OS shutdown will trigger VM managed saved for QEMU session daemon, but not the system daemon.

This changes the daemon code to always run virStateStop for all daemons. Instead the QEMU driver is responsible for skipping its own logic when running privileged...for now.

This means that virStateStop will now be triggered by logind's PrepareForShutdown signal.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_driver.c | 3 ++- src/remote/remote_daemon.c | 28 +++++++++++++++------------- 2 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8c16566ce8..103369ac93 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -949,7 +949,8 @@ qemuStateStop(void) .uri = cfg->uri, };

- virDomainDriverAutoShutdown(&ascfg); + if (!qemu_driver->privileged) + virDomainDriverAutoShutdown(&ascfg);

return 0; } diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index d44a365000..c4b930cb70 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -628,27 +628,29 @@ static void daemonRunStateInit(void *opaque) virStateShutdownPrepare, virStateShutdownWait);

- /* Tie the non-privileged daemons to the session/shutdown lifecycle */ + /* Signal for VM shutdown when desktop session is terminated, in + * unprivileged daemons */ if (!virNetDaemonIsPrivileged(dmn)) { - sessionBus = virGDBusGetSessionBus(); if (sessionBus != NULL) g_dbus_connection_add_filter(sessionBus, handleSessionMessageFunc, dmn, NULL); + }

- systemBus = virGDBusGetSystemBus(); - if (systemBus != NULL) - g_dbus_connection_signal_subscribe(systemBus, - "org.freedesktop.login1", - "org.freedesktop.login1.Manager", - "PrepareForShutdown", - NULL, - NULL, - G_DBUS_SIGNAL_FLAGS_NONE, - handleSystemMessageFunc, + /* Signal for VM shutdown when host OS shutdown is requested, in + * both privileged and unprivileged daemons */ + systemBus = virGDBusGetSystemBus(); + if (systemBus != NULL) + g_dbus_connection_signal_subscribe(systemBus, + "org.freedesktop.login1", + "org.freedesktop.login1.Manager", + "PrepareForShutdown", + NULL, + NULL, + G_DBUS_SIGNAL_FLAGS_NONE, + handleSystemMessageFunc, dmn, NULL);

This patch leaves the code mis-aligned. You later fix it in 23/26. Move the hunk here.

- }

/* Only now accept clients from network */ virNetDaemonUpdateServices(dmn, true); -- 2.47.1

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Wed, Jan 08, 2025 at 19:42:42 +0000, Daniel P. Berrangé wrote:

The auto shutdown code can currently only perform managed save, which may fail in some cases, for example when PCI devices are assigned. On failure, shutdown inhibitors remain in place which may be undesirable.

This expands the logic to try a sequence of operations

* Managed save * Graceful shutdown * Forced poweroff

Each of these operations can be enabled or disabled, but they are always applied in this order.

With the shutdown option, a configurable time is allowed for shutdown to complete, defaulting to 30 seconds, before moving onto the forced poweroff phase.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/hypervisor/domain_driver.c | 113 +++++++++++++++++++++++++++------ src/hypervisor/domain_driver.h | 4 ++ src/qemu/qemu_driver.c | 3 + 3 files changed, 100 insertions(+), 20 deletions(-)

diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index 949e3d01f1..ea3f1cbfcd 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -714,9 +714,26 @@ virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg) g_autoptr(virConnect) conn = NULL; int numDomains = 0; size_t i; - int state; virDomainPtr *domains = NULL; - g_autofree unsigned int *flags = NULL; + + VIR_DEBUG("Run autoshutdown uri=%s trySave=%d tryShutdown=%d poweroff=%d" + "waitShutdownSecs=%d",

Please avoid linebreaks in debug messages. Also this way there's a missing space.

+ cfg->uri, cfg->trySave, cfg->tryShutdown, cfg->poweroff, + cfg->waitShutdownSecs); + + /* + * Ideally guests will shutdown in a few seconds, but it would + * not be unsual for it to take a while longer, especially under + * load, or if the guest OS has inhibitors slowing down shutdown. + * + * If we wait too long, then guests which ignore the shutdown + * request will significantly delay host shutdown. + * + * Pick 30 seconds as a moderately safe default, assuming that + * most guests are well behaved. + */

I'll have to see how this is in the end exposed to the user but ...

+ if (cfg->waitShutdownSecs <= 0) + cfg->waitShutdownSecs = 30;

... I find that this is not the correct place for this kind of logic. All the time it'll look as if the shutdown timer is configured to what the caller intended, but at the very last moment it'll be overridden. Including the debug message above mentioning the old value of waitShutdownSecs=%d.

if (!(conn = virConnectOpen(cfg->uri))) goto cleanup; @@ -726,31 +743,87 @@ virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg) VIR_CONNECT_LIST_DOMAINS_ACTIVE)) < 0) goto cleanup;

- flags = g_new0(unsigned int, numDomains); + VIR_DEBUG("Auto shutdown with %d running domains", numDomains);

This looks like we could do also a VIR_INFO perhaps? So that it's possibly in the system log?

+ if (cfg->trySave) { + g_autofree unsigned int *flags = g_new0(unsigned int, numDomains); + for (i = 0; i < numDomains; i++) { + int state; + /* + * Pause all VMs to make them stop dirtying pages, + * so save is quicker. We remember if any VMs were + * paused so we can restore that on resume. + */ + flags[i] = VIR_DOMAIN_SAVE_RUNNING; + if (virDomainGetState(domains[i], &state, NULL, 0) == 0) { + if (state == VIR_DOMAIN_PAUSED) + flags[i] = VIR_DOMAIN_SAVE_PAUSED; + } + virDomainSuspend(domains[i]); + } + + for (i = 0; i < numDomains; i++) { + if (virDomainManagedSave(domains[i], flags[i]) < 0) { + VIR_WARN("Unable to perform managed save of '%s': %s", + virDomainGetName(domains[i]), + virGetLastErrorMessage()); + continue; + }

In similar spirit of adding log entries, should we perhaps add an entry about the final state/decision that was used for given VM?

+ virObjectUnref(domains[i]); + domains[i] = NULL; + } + } + + if (cfg->tryShutdown) { + GTimer *timer = NULL; + for (i = 0; i < numDomains; i++) { + if (domains[i] == NULL) + continue; + if (virDomainShutdown(domains[i]) < 0) { + VIR_WARN("Unable to perform graceful shutdown of '%s': %s", + virDomainGetName(domains[i]), + virGetLastErrorMessage()); + break; + } + } + + timer = g_timer_new(); + while (1) { + bool anyRunning = false; + for (i = 0; i < numDomains; i++) { + if (!domains[i]) + continue;

- /* First we pause all VMs to make them stop dirtying - pages, etc. We remember if any VMs were paused so - we can restore that on resume. */ - for (i = 0; i < numDomains; i++) { - flags[i] = VIR_DOMAIN_SAVE_RUNNING; - if (virDomainGetState(domains[i], &state, NULL, 0) == 0) { - if (state == VIR_DOMAIN_PAUSED) - flags[i] = VIR_DOMAIN_SAVE_PAUSED; + if (virDomainIsActive(domains[i]) == 1) { + anyRunning = true; + } else {

Here too possibly.

+ virObjectUnref(domains[i]); + domains[i] = NULL; + } + } + + if (!anyRunning) + break; + if (g_timer_elapsed(timer, NULL) > cfg->waitShutdownSecs) + break; + g_usleep(1000*500); } - virDomainSuspend(domains[i]); + g_timer_destroy(timer); }

- /* Then we save the VMs to disk */ - for (i = 0; i < numDomains; i++) - if (virDomainManagedSave(domains[i], flags[i]) < 0) - VIR_WARN("Unable to perform managed save of '%s': %s", - virDomainGetName(domains[i]), - virGetLastErrorMessage()); + if (cfg->poweroff) { + for (i = 0; i < numDomains; i++) { + if (domains[i] == NULL) + continue; + virDomainDestroy(domains[i]); + } + }

cleanup: if (domains) { - for (i = 0; i < numDomains; i++) - virObjectUnref(domains[i]); + for (i = 0; i < numDomains; i++) { + if (domains[i]) + virObjectUnref(domains[i]);

virObjectUnref should be NULL-safe.

+ } VIR_FREE(domains); } } diff --git a/src/hypervisor/domain_driver.h b/src/hypervisor/domain_driver.h index d1588ea712..25c4b0c664 100644 --- a/src/hypervisor/domain_driver.h +++ b/src/hypervisor/domain_driver.h @@ -92,6 +92,10 @@ void virDomainDriverAutoStart(virDomainObjList *domains,

typedef struct _virDomainDriverAutoShutdownConfig { const char *uri; + bool trySave; + bool tryShutdown; + bool poweroff; + int waitShutdownSecs;

As before; consider 'unsigned' as negative shutdown wait time doesn't seem to make sense. Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Thu, Jan 30, 2025 at 15:10:55 +0000, Daniel P. Berrangé wrote:

On Thu, Jan 30, 2025 at 04:02:46PM +0100, Peter Krempa wrote:

...

On Wed, Jan 08, 2025 at 19:42:42 +0000, Daniel P. Berrangé wrote:

...

The auto shutdown code can currently only perform managed save, which may fail in some cases, for example when PCI devices are assigned. On failure, shutdown inhibitors remain in place which may be undesirable.

This expands the logic to try a sequence of operations

* Managed save * Graceful shutdown * Forced poweroff

Each of these operations can be enabled or disabled, but they are always applied in this order.

With the shutdown option, a configurable time is allowed for shutdown to complete, defaulting to 30 seconds, before moving onto the forced poweroff phase.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/hypervisor/domain_driver.c | 113 +++++++++++++++++++++++++++------ src/hypervisor/domain_driver.h | 4 ++ src/qemu/qemu_driver.c | 3 + 3 files changed, 100 insertions(+), 20 deletions(-)

diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index 949e3d01f1..ea3f1cbfcd 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -714,9 +714,26 @@ virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg) g_autoptr(virConnect) conn = NULL; int numDomains = 0; size_t i; - int state; virDomainPtr *domains = NULL; - g_autofree unsigned int *flags = NULL; + + VIR_DEBUG("Run autoshutdown uri=%s trySave=%d tryShutdown=%d poweroff=%d" + "waitShutdownSecs=%d",

Please avoid linebreaks in debug messages. Also this way there's a missing space.

...

+ cfg->uri, cfg->trySave, cfg->tryShutdown, cfg->poweroff, + cfg->waitShutdownSecs); + + /* + * Ideally guests will shutdown in a few seconds, but it would + * not be unsual for it to take a while longer, especially under + * load, or if the guest OS has inhibitors slowing down shutdown. + * + * If we wait too long, then guests which ignore the shutdown + * request will significantly delay host shutdown. + * + * Pick 30 seconds as a moderately safe default, assuming that + * most guests are well behaved. + */

I'll have to see how this is in the end exposed to the user but ...

...

+ if (cfg->waitShutdownSecs <= 0) + cfg->waitShutdownSecs = 30;

... I find that this is not the correct place for this kind of logic. All the time it'll look as if the shutdown timer is configured to what the caller intended, but at the very last moment it'll be overridden. Including the debug message above mentioning the old value of waitShutdownSecs=%d.

I did it here because this is common code for all drivers.

This series wires up QEMU, but we should also wire up bhyve, libxl, lxc, ch too with the same APIs.

The qemu.conf settnig introduced ina later patch makes it clear that the default value will be '30' if unset, so this shouldn't really be a surprise, especially since '0' is clearly not a setting you would otherwise pick.

Fair enough; but consider moving the VIR_DEBUG statement after this tweak.

...

...

@@ -726,31 +743,87 @@ virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg) VIR_CONNECT_LIST_DOMAINS_ACTIVE)) < 0) goto cleanup;

- flags = g_new0(unsigned int, numDomains); + VIR_DEBUG("Auto shutdown with %d running domains", numDomains);

This looks like we could do also a VIR_INFO perhaps? So that it's possibly in the system log?

Later in the series (last patch), I wire up systemd notifiers for sending status messages. This reminds me though, I was kinda hoping those would end up in the journal, but they didn't seem to and I've not yet figured out what systemd does with them.

If they don't, we could make the virSystemdNotifyStatus API itself issue a VIR_INFO though.

Ah, those messages are exactly what I was looking for. I'd just note that any failures should also be delivered via the same mechanism (e.g. if suspend fails, I'd expect to also use the notification instead of it being recorded in the libvirt-related logs via VIR_WARN

On Wed, Jan 08, 2025 at 19:42:42 +0000, Daniel P. Berrangé wrote:

The auto shutdown code can currently only perform managed save, which may fail in some cases, for example when PCI devices are assigned. On failure, shutdown inhibitors remain in place which may be undesirable.

This expands the logic to try a sequence of operations

* Managed save * Graceful shutdown * Forced poweroff

Each of these operations can be enabled or disabled, but they are always applied in this order.

With the shutdown option, a configurable time is allowed for shutdown to complete, defaulting to 30 seconds, before moving onto the forced poweroff phase.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/hypervisor/domain_driver.c | 113 +++++++++++++++++++++++++++------ src/hypervisor/domain_driver.h | 4 ++ src/qemu/qemu_driver.c | 3 + 3 files changed, 100 insertions(+), 20 deletions(-)

diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index 949e3d01f1..ea3f1cbfcd 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c

Noticed while reviewing last patch:

@@ -726,31 +743,87 @@ virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg) VIR_CONNECT_LIST_DOMAINS_ACTIVE)) < 0) goto cleanup;

- flags = g_new0(unsigned int, numDomains); + VIR_DEBUG("Auto shutdown with %d running domains", numDomains); + if (cfg->trySave) { + g_autofree unsigned int *flags = g_new0(unsigned int, numDomains); + for (i = 0; i < numDomains; i++) { + int state; + /* + * Pause all VMs to make them stop dirtying pages, + * so save is quicker. We remember if any VMs were + * paused so we can restore that on resume. + */ + flags[i] = VIR_DOMAIN_SAVE_RUNNING; + if (virDomainGetState(domains[i], &state, NULL, 0) == 0) { + if (state == VIR_DOMAIN_PAUSED) + flags[i] = VIR_DOMAIN_SAVE_PAUSED; + } + virDomainSuspend(domains[i]);

Any VM where we attempt 'save' is paused ...

+ } + + for (i = 0; i < numDomains; i++) { + if (virDomainManagedSave(domains[i], flags[i]) < 0) { + VIR_WARN("Unable to perform managed save of '%s': %s", + virDomainGetName(domains[i]), + virGetLastErrorMessage()); + continue;

... but not unpaused if saving fails ...

+ } + virObjectUnref(domains[i]); + domains[i] = NULL; + } + } + + if (cfg->tryShutdown) { + GTimer *timer = NULL; + for (i = 0; i < numDomains; i++) { + if (domains[i] == NULL) + continue; + if (virDomainShutdown(domains[i]) < 0) {

... so if we then request a graceful shutdown the guest OS can't respond to it.

+ VIR_WARN("Unable to perform graceful shutdown of '%s': %s", + virDomainGetName(domains[i]), + virGetLastErrorMessage()); + break; + } + }

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@redhat.com> --- 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: static int qemuDomainManagedSaveHelper(virQEMUDriver *driver, virDomainObj *vm, const char *dxml, unsigned int flags) { g_autoptr(virQEMUDriverConfig) cfg = NULL; g_autoptr(virCommand) compressor = NULL; g_autofree char *path = NULL; int format; if (virDomainObjCheckActive(vm) < 0) return -1; if (!vm->persistent) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot do managed save for transient domain")); return -1; }

+ if (virDomainShutdown(domains[i]) < 0) { VIR_WARN("Unable to perform graceful shutdown of '%s': %s", virDomainGetName(domains[i]),

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

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

On Wed, Jan 08, 2025 at 19:42:45 +0000, Daniel P. Berrangé wrote:

Currently the session daemon will try a managed save on all VMs, leaving them running if that fails.

This limits the managed save just to persistent VMs, as there will usually not be any way to restore transient VMs later.

It also enables graceful shutdown and then forced poweroff, should save fail for some reason.

These new defaults can be overridden in the config file if needed.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/libvirtd_qemu.aug | 2 +- src/qemu/qemu.conf.in | 14 ++++++++------ src/qemu/qemu_conf.c | 6 +++--- src/qemu/test_libvirtd_qemu.aug.in | 6 +++--- 4 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index e465a231fc..605604a01a 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -100,7 +100,7 @@ module Libvirtd_qemu = | int_entry "auto_start_delay" | str_entry "auto_shutdown_try_save" | str_entry "auto_shutdown_try_shutdown" - | str_entry "auto_shutdown_powerdown" + | str_entry "auto_shutdown_poweroff"

let process_entry = str_entry "hugetlbfs_mount" | str_entry "bridge_helper"

This hunk belongs to previous patch.

diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in index d8890c4c32..82eae2eecd 100644 --- a/src/qemu/qemu.conf.in +++ b/src/qemu/qemu.conf.in @@ -647,33 +647,35 @@ # * "transient" - only try to save transient running VMs # * "all" - try to save all running VMs # -# Defaults to "all" for session daemons and "none" +# Defaults to "persistent" 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" +#auto_shutdown_try_save = "persistent"

As noted 'all' doesn't make sense for 'save' operation so I'd call that a bugfix. Perhaps even worhty of a separate patch.

# As above, but with a graceful shutdown action instead of # managed save. If managed save is enabled, shutdown will # be tried only on failure to perform managed save. # -# Defaults to "none" +# 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_shutdown = "none" +#auto_shutdown_try_shutdown = "all"

# As above, but with a forced poweroff instead of managed # save. If managed save or graceful shutdown are enabled, # forced poweroff will be tried only on failure of the # other options. # -# Defaults to "none" +# 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_powerdown = "none" +#auto_shutdown_poweroff = "all"

# If provided by the host and a hugetlbfs mount point is configured, # a guest may request huge page backing. When this mount point is diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index a57eccf569..76bb3bd888 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -313,9 +313,9 @@ virQEMUDriverConfig *virQEMUDriverConfigNew(bool privileged, cfg->autoShutdownTryShutdown = VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE; cfg->autoShutdownPoweroff = VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE; } else { - cfg->autoShutdownTrySave = VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_ALL; - cfg->autoShutdownTryShutdown = VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE; - cfg->autoShutdownPoweroff = VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE; + cfg->autoShutdownTrySave = VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_PERSISTENT; + cfg->autoShutdownTryShutdown = VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_ALL; + cfg->autoShutdownPoweroff = VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_ALL; }

return g_steal_pointer(&cfg); diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index e7137f686f..3bc8104d7a 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -76,9 +76,9 @@ module Test_libvirtd_qemu = { "auto_dump_bypass_cache" = "0" } { "auto_start_bypass_cache" = "0" } { "auto_start_delay" = "0" } -{ "auto_shutdown_try_save" = "all" } -{ "auto_shutdown_try_shutdown" = "none" } -{ "auto_shutdown_poweroff" = "none" } +{ "auto_shutdown_try_save" = "persistent" } +{ "auto_shutdown_try_shutdown" = "all" } +{ "auto_shutdown_poweroff" = "all" } { "hugetlbfs_mount" = "/dev/hugepages" } { "bridge_helper" = "qemu-bridge-helper" } { "set_process_name" = "1" }

For the rest: Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Wed, Jan 08, 2025 at 19:42:50 +0000, Daniel P. Berrangé wrote:

This is maintained in the same way as the autostart flag, using a symlink. The difference is that instead of '.xml', the symlink suffix is '.xml.once'. The link is also deleted immediately after it has been read.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_conf.c | 6 +++++- src/conf/domain_conf.h | 1 + src/conf/virdomainobjlist.c | 7 ++++++- 3 files changed, 12 insertions(+), 2 deletions(-)

[...]

diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 72207450c5..ba2f9f544d 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c

[...]

@@ -500,13 +501,17 @@ virDomainObjListLoadConfig(virDomainObjList *doms, return NULL;

autostartLink = virDomainConfigFile(autostartDir, name); + autostartOnceLink = g_strdup_printf("%s.once", autostartLink);

autostart = virFileLinkPointsTo(autostartLink, configFile); + autostartOnce = virFileLinkPointsTo(autostartOnceLink, configFile); + unlink(autostartOnceLink);

Actually this will not work properly if you restart the daemon before rebooting the host as configs will be loaded, thus the file unlinked but then the only place this is recorded is in memory. The unlinking of the file needs to happen only after autostart was actually attempted, which should be reasonably doable now that's centralised.

On Wed, Jan 08, 2025 at 19:42:51 +0000, Daniel P. Berrangé wrote:

When performing auto-shutdown of running domains, there is now the option to mark them as "autostart once", so that their state is restored on next boot. This applies on top of the traditional autostart flag.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/hypervisor/domain_driver.c | 27 ++++++++++++++++++++++----- src/hypervisor/domain_driver.h | 1 + 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index 867ee1ae2a..2f89b8c841 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -677,10 +677,11 @@ virDomainDriverAutoStartOne(virDomainObj *vm, virObjectLock(vm); virObjectRef(vm);

- VIR_DEBUG("Autostart %s: autostart=%d", - vm->def->name, vm->autostart); + VIR_DEBUG("Autostart %s: autostart=%d autostartOnce=%d", + vm->def->name, vm->autostart, vm->autostartOnce);

- if (vm->autostart && !virDomainObjIsActive(vm)) { + if ((vm->autostart || vm->autostartOnce) + && !virDomainObjIsActive(vm)) { virResetLastError(); if (state->cfg->delayMS) { if (!state->first) { @@ -691,6 +692,7 @@ virDomainDriverAutoStartOne(virDomainObj *vm, }

state->cfg->callback(vm, state->cfg->opaque); + vm->autostartOnce = 0;

Per comment in previous patch the '.once' file needs to be unlinked here instead.

}

virDomainObjEndAPI(&vm);

Also both of the hunks above really look like they belong to the previous patch as they implement the auto-start-once feature while this commit is now using that feature to do 'restore' of the state at shutdown.

@@ -725,9 +727,9 @@ virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg) g_autofree bool *transient = NULL;

VIR_DEBUG("Run autoshutdown uri=%s trySave=%d tryShutdown=%d poweroff=%d" - "waitShutdownSecs=%d saveBypassCache=%d", + "waitShutdownSecs=%d saveBypassCache=%d autoRestore=%d", cfg->uri, cfg->trySave, cfg->tryShutdown, cfg->poweroff, - cfg->waitShutdownSecs, cfg->saveBypassCache); + cfg->waitShutdownSecs, cfg->saveBypassCache, cfg->autoRestore);

/* * Ideally guests will shutdown in a few seconds, but it would @@ -763,6 +765,21 @@ virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg) for (i = 0; i < numDomains; i++) { if (virDomainIsPersistent(domains[i]) == 0) transient[i] = true; + + if (cfg->autoRestore) { + if (transient[i]) { + VIR_DEBUG("Cannot auto-restore transient VM %s", + virDomainGetName(domains[i])); + } else { + VIR_DEBUG("Mark %s for autostart on next boot", + virDomainGetName(domains[i])); + if (virDomainSetAutostartOnce(domains[i], 1) < 0) { + VIR_WARN("Unable to mark domain '%s' for auto restore: %s", + virDomainGetName(domains[i]), + virGetLastErrorMessage()); + } + } + } }

if (cfg->trySave != VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE) { diff --git a/src/hypervisor/domain_driver.h b/src/hypervisor/domain_driver.h index 16832f2449..72152f8054 100644 --- a/src/hypervisor/domain_driver.h +++ b/src/hypervisor/domain_driver.h @@ -109,6 +109,7 @@ typedef struct _virDomainDriverAutoShutdownConfig { virDomainDriverAutoShutdownScope poweroff; int waitShutdownSecs; bool saveBypassCache; + bool autoRestore; } virDomainDriverAutoShutdownConfig;

void virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg);

Wit the comments above addressed: Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Wed, Jan 08, 2025 at 19:42:52 +0000, Daniel P. Berrangé wrote:

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_driver.c | 95 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8729b0fab0..84bde503dd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7778,6 +7778,99 @@ static int qemuDomainSetAutostart(virDomainPtr dom, }

+static int qemuDomainGetAutostartOnce(virDomainPtr dom, + int *autostart)

Return type on separate line please.

+{ + virDomainObj *vm; + int ret = -1; + + if (!(vm = qemuDomainObjFromDomain(dom))) + goto cleanup; + + if (virDomainGetAutostartOnceEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + *autostart = vm->autostartOnce; + ret = 0; + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + +static int qemuDomainSetAutostartOnce(virDomainPtr dom, + int autostart)

Also here.

+{ + virQEMUDriver *driver = dom->conn->privateData; + virDomainObj *vm; + g_autofree char *configFile = NULL; + g_autofree char *autostartLink = NULL; + g_autofree char *autostartOnceLink = NULL; + int ret = -1; + g_autoptr(virQEMUDriverConfig) cfg = NULL; + + if (!(vm = qemuDomainObjFromDomain(dom))) + return -1; + + cfg = virQEMUDriverGetConfig(driver); + + if (virDomainSetAutostartOnceEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (!vm->persistent) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cannot set autostart for transient domain")); + goto cleanup; + } + + autostart = (autostart != 0); + + if (vm->autostartOnce != autostart) { + if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0) + goto cleanup; + + configFile = virDomainConfigFile(cfg->configDir, vm->def->name); + autostartLink = virDomainConfigFile(cfg->autostartDir, vm->def->name); + autostartOnceLink = g_strdup_printf("%s.once", autostartLink); + + if (autostart) { + if (g_mkdir_with_parents(cfg->autostartDir, 0777) < 0) { + virReportSystemError(errno, + _("cannot create autostart directory %1$s"), + cfg->autostartDir); + goto endjob; + } + + if (symlink(configFile, autostartOnceLink) < 0) { + virReportSystemError(errno, + _("Failed to create symlink '%1$s' to '%2$s'"), + autostartOnceLink, configFile); + goto endjob; + } + } else { + if (unlink(autostartOnceLink) < 0 && + errno != ENOENT && + errno != ENOTDIR) { + virReportSystemError(errno, + _("Failed to delete symlink '%1$s'"), + autostartOnceLink); + goto endjob; + } + } + + vm->autostartOnce = autostart; + + endjob: + virDomainObjEndJob(vm); + } + ret = 0; + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + + static char *qemuDomainGetSchedulerType(virDomainPtr dom, int *nparams) { @@ -20257,6 +20350,8 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainSetLaunchSecurityState = qemuDomainSetLaunchSecurityState, /* 8.0.0 */ .domainFDAssociate = qemuDomainFDAssociate, /* 9.0.0 */ .domainGraphicsReload = qemuDomainGraphicsReload, /* 10.2.0 */ + .domainGetAutostartOnce = qemuDomainGetAutostartOnce, /* 11.0.0 */ + .domainSetAutostartOnce = qemuDomainSetAutostartOnce, /* 11.0.0 */

11.1.0

};

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Wed, Jan 08, 2025 at 19:42:53 +0000, Daniel P. Berrangé wrote:

If shutting down running VMs at host shutdown, it can be useful to automatically start them again on next boot. This adds a config parameter 'auto_shutdown_restore', which defaults to enabled, which leverages the autostart once feature.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf.in | 4 ++++ src/qemu/qemu_conf.c | 3 +++ src/qemu/qemu_conf.h | 1 + src/qemu/qemu_driver.c | 1 + src/qemu/test_libvirtd_qemu.aug.in | 1 + 6 files changed, 11 insertions(+)

[...]

diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in index d26ab4cb57..ace4b20a0c 100644 --- a/src/qemu/qemu.conf.in +++ b/src/qemu/qemu.conf.in @@ -681,6 +681,10 @@ # when 'auto_shutdown_try_shutdown' is enabled #auto_shutdown_wait = 30

+# Whether VMs that are automatically shutdown are set to

Consider: shutdown or saved

+# automatically restore on next boot +#auto_shutdown_restore = 1 + # When a domain is configured to be auto-saved on shutdown, enabling # this flag has the same effect as using the VIR_DOMAIN_SAVE_BYPASS_CACHE # flag with the virDomainManagedSave API. That is, the system will

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Wed, Jan 08, 2025 at 19:42:54 +0000, Daniel P. Berrangé wrote:

Currently the remote daemon code is responsible for calling virStateStop in a background thread. The virNetDaemon code wants to synchronize with this during shutdown, however, so the virThreadPtr must be passed over.

Even the limited synchronization done currently, however, is flawed and to fix this requires the virNetDaemon code to be responsible for calling virStateStop in a thread more directly.

Thus the logic is moved over into virStateStop via a further callback to be registered by the remote daemon.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_remote.syms | 2 +- src/remote/remote_daemon.c | 40 ++------------------------ src/rpc/virnetdaemon.c | 58 ++++++++++++++++++++++++++++---------- src/rpc/virnetdaemon.h | 20 +++++++++++-- 4 files changed, 64 insertions(+), 56 deletions(-)

diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms index f0f90815cf..7e87b0bd2a 100644 --- a/src/libvirt_remote.syms +++ b/src/libvirt_remote.syms @@ -90,12 +90,12 @@ virNetDaemonIsPrivileged; virNetDaemonNew; virNetDaemonNewPostExecRestart; virNetDaemonPreExecRestart; +virNetDaemonPreserve; virNetDaemonQuit; virNetDaemonQuitExecRestart; virNetDaemonRemoveShutdownInhibition; virNetDaemonRun; virNetDaemonSetShutdownCallbacks; -virNetDaemonSetStateStopWorkerThread; virNetDaemonUpdateServices;

diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index c4b930cb70..bf91ee5772 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -514,41 +514,6 @@ static void daemonInhibitCallback(bool inhibit, void *opaque) static GDBusConnection *sessionBus; static GDBusConnection *systemBus;

-static void daemonStopWorker(void *opaque) -{ - virNetDaemon *dmn = opaque; - - VIR_DEBUG("Begin stop dmn=%p", dmn); - - ignore_value(virStateStop()); - - VIR_DEBUG("Completed stop dmn=%p", dmn); - - /* Exit daemon cleanly */ - virNetDaemonQuit(dmn); -} - - -/* We do this in a thread to not block the main loop */ -static void daemonStop(virNetDaemon *dmn) -{ - virThread *thr; - virObjectRef(dmn); - - thr = g_new0(virThread, 1); - - if (virThreadCreateFull(thr, true, - daemonStopWorker, - "daemon-stop", false, dmn) < 0) { - virObjectUnref(dmn); - g_free(thr); - return; - } - - virNetDaemonSetStateStopWorkerThread(dmn, &thr); -} - - static GDBusMessage * handleSessionMessageFunc(GDBusConnection *connection G_GNUC_UNUSED, GDBusMessage *message, @@ -562,7 +527,7 @@ handleSessionMessageFunc(GDBusConnection *connection G_GNUC_UNUSED, if (virGDBusMessageIsSignal(message, "org.freedesktop.DBus.Local", "Disconnected")) - daemonStop(dmn); + virNetDaemonPreserve(dmn);

return message; } @@ -581,7 +546,7 @@ handleSystemMessageFunc(GDBusConnection *connection G_GNUC_UNUSED,

VIR_DEBUG("dmn=%p", dmn);

- daemonStop(dmn); + virNetDaemonPreserve(dmn); }

@@ -625,6 +590,7 @@ static void daemonRunStateInit(void *opaque) g_atomic_int_set(&driversInitialized, 1);

virNetDaemonSetShutdownCallbacks(dmn, + virStateStop,

The name 'virStateStop' is starting to be confusing in the context it's being used now. I suggest you at least document what the expectations from the hypervisor drivers are? Or better when it's invoked.

virStateShutdownPrepare, virStateShutdownWait);

diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c index bb7d2ff9a0..19b19ff834 100644 --- a/src/rpc/virnetdaemon.c +++ b/src/rpc/virnetdaemon.c @@ -65,9 +65,10 @@ struct _virNetDaemon { GHashTable *servers; virJSONValue *srvObject;

+ virNetDaemonShutdownCallback shutdownPreserveCb; virNetDaemonShutdownCallback shutdownPrepareCb; virNetDaemonShutdownCallback shutdownWaitCb; - virThread *stateStopThread; + virThread *shutdownPreserveThread; int finishTimer; bool quit; bool finished; @@ -107,7 +108,7 @@ virNetDaemonDispose(void *obj) virEventRemoveHandle(dmn->sigwatch); #endif /* !WIN32 */

- g_free(dmn->stateStopThread); + g_free(dmn->shutdownPreserveThread);

g_clear_pointer(&dmn->servers, g_hash_table_unref);

@@ -705,8 +706,8 @@ daemonShutdownWait(void *opaque)

virHashForEach(dmn->servers, daemonServerShutdownWait, NULL); if (!dmn->shutdownWaitCb || dmn->shutdownWaitCb() >= 0) { - if (dmn->stateStopThread) - virThreadJoin(dmn->stateStopThread); + if (dmn->shutdownPreserveThread) + virThreadJoin(dmn->shutdownPreserveThread);

graceful = true; } @@ -801,17 +802,6 @@ virNetDaemonRun(virNetDaemon *dmn) }

-void -virNetDaemonSetStateStopWorkerThread(virNetDaemon *dmn, - virThread **thr) -{ - VIR_LOCK_GUARD lock = virObjectLockGuard(dmn); - - VIR_DEBUG("Setting state stop worker thread on dmn=%p to thr=%p", dmn, thr); - dmn->stateStopThread = g_steal_pointer(thr); -} - - void virNetDaemonQuit(virNetDaemon *dmn) { @@ -833,6 +823,42 @@ virNetDaemonQuitExecRestart(virNetDaemon *dmn) }

+static void virNetDaemonPreserveWorker(void *opaque) +{ + virNetDaemon *dmn = opaque; + + VIR_DEBUG("Begin stop dmn=%p", dmn); + + dmn->shutdownPreserveCb(); + + VIR_DEBUG("Completed stop dmn=%p", dmn); + + virNetDaemonQuit(dmn); + virObjectUnref(dmn); +} + + +void virNetDaemonPreserve(virNetDaemon *dmn) +{ + VIR_LOCK_GUARD lock = virObjectLockGuard(dmn); + + if (!dmn->shutdownPreserveCb || + dmn->shutdownPreserveThread) + return; + + virObjectRef(dmn); + dmn->shutdownPreserveThread = g_new0(virThread, 1); + + if (virThreadCreateFull(dmn->shutdownPreserveThread, true, + virNetDaemonPreserveWorker, + "daemon-stop", false, dmn) < 0) { + virObjectUnref(dmn); + g_clear_pointer(&dmn->shutdownPreserveThread, g_free); + return; + } +}

Please use the new function header formatting style.

+ + static int daemonServerClose(void *payload, const char *key G_GNUC_UNUSED, @@ -870,11 +896,13 @@ virNetDaemonHasClients(virNetDaemon *dmn)

void virNetDaemonSetShutdownCallbacks(virNetDaemon *dmn, + virNetDaemonShutdownCallback preserveCb, virNetDaemonShutdownCallback prepareCb, virNetDaemonShutdownCallback waitCb) { VIR_LOCK_GUARD lock = virObjectLockGuard(dmn);

+ dmn->shutdownPreserveCb = preserveCb; dmn->shutdownPrepareCb = prepareCb; dmn->shutdownWaitCb = waitCb; }

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Mon, Feb 03, 2025 at 10:52:23 +0100, Peter Krempa wrote:

On Wed, Jan 08, 2025 at 19:42:54 +0000, Daniel P. Berrangé wrote:

...

Currently the remote daemon code is responsible for calling virStateStop in a background thread. The virNetDaemon code wants to synchronize with this during shutdown, however, so the virThreadPtr must be passed over.

Even the limited synchronization done currently, however, is flawed and to fix this requires the virNetDaemon code to be responsible for calling virStateStop in a thread more directly.

Thus the logic is moved over into virStateStop via a further callback to be registered by the remote daemon.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_remote.syms | 2 +- src/remote/remote_daemon.c | 40 ++------------------------ src/rpc/virnetdaemon.c | 58 ++++++++++++++++++++++++++++---------- src/rpc/virnetdaemon.h | 20 +++++++++++-- 4 files changed, 64 insertions(+), 56 deletions(-)

[...]

...

daemonServerClose(void *payload, const char *key G_GNUC_UNUSED, @@ -870,11 +896,13 @@ virNetDaemonHasClients(virNetDaemon *dmn)

void virNetDaemonSetShutdownCallbacks(virNetDaemon *dmn, + virNetDaemonShutdownCallback preserveCb, virNetDaemonShutdownCallback prepareCb, virNetDaemonShutdownCallback waitCb) { VIR_LOCK_GUARD lock = virObjectLockGuard(dmn);

+ dmn->shutdownPreserveCb = preserveCb;

And this setter technically makes it non-immutable.

While it most likely will not be a problem I don't think we should access this function pointer outside of a lock. That is unless you document it as being/make it immutable.

Upcoming patches also add code where virNetDaemonPreserveWorker will access dmn->quit which definitely isn't immutable so it's likely a more complete solution will be needed.

Disregard this paragraph, I didn't notice the lock guard in the patch adding the access to dmn->quit. Thus declaring 'shutdownPreserveCb' to be immutable should be good enough.

On Wed, Jan 08, 2025 at 19:42:56 +0000, Daniel P. Berrangé wrote:

The preserving of state (ie running VMs) requires a fully functional daemon and hypervisor driver. If any part has started shutting down then saving state may fail, or worse, hang.

The current shutdown sequence does not guarantee safe ordering, as we synchronize with the state saving thread only after the hypervisor driver has had its 'shutdownPrepare' callback invoked. In the case of QEMU this means that worker threads processing monitor events may well have been stopped.

This implements a full state machine that has a well defined ordering that an earlier commit documented as the desired semantics.

With this change, nothing will start shutting down if the state saving thread is still running.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/remote_daemon.c | 4 +- src/rpc/virnetdaemon.c | 103 ++++++++++++++++++++++++++----------- 2 files changed, 76 insertions(+), 31 deletions(-)

diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index e03ee1de5a..f64ecad7f5 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -617,8 +617,8 @@ static void daemonRunStateInit(void *opaque) NULL, G_DBUS_SIGNAL_FLAGS_NONE, handleSystemMessageFunc, - dmn, - NULL); + dmn, + NULL);

/* Only now accept clients from network */ virNetDaemonUpdateServices(dmn, true);

This belongs to a different commit.

diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c index 7f14c5420a..d1d7bae569 100644 --- a/src/rpc/virnetdaemon.c +++ b/src/rpc/virnetdaemon.c @@ -39,6 +39,15 @@

VIR_LOG_INIT("rpc.netdaemon");

+typedef enum { + VIR_NET_DAEMON_QUIT_NONE, + VIR_NET_DAEMON_QUIT_REQUESTED, + VIR_NET_DAEMON_QUIT_PRESERVING, + VIR_NET_DAEMON_QUIT_READY, + VIR_NET_DAEMON_QUIT_WAITING, + VIR_NET_DAEMON_QUIT_COMPLETED, +} virNetDaemonQuitPhase;

[...]

@@ -753,7 +762,7 @@ virNetDaemonRun(virNetDaemon *dmn) virSystemdNotifyReady();

VIR_DEBUG("dmn=%p quit=%d", dmn, dmn->quit); - while (!dmn->finished) { + while (dmn->quit != VIR_NET_DAEMON_QUIT_COMPLETED) { virNetDaemonShutdownTimerUpdate(dmn);

virObjectUnlock(dmn); @@ -767,17 +776,30 @@ virNetDaemonRun(virNetDaemon *dmn) virHashForEach(dmn->servers, daemonServerProcessClients, NULL);

/* don't shutdown services when performing an exec-restart */ - if (dmn->quit && dmn->execRestart) + if (dmn->quit == VIR_NET_DAEMON_QUIT_REQUESTED && dmn->execRestart) goto cleanup;

- if (dmn->quit && dmn->finishTimer == -1) { + if (dmn->quit == VIR_NET_DAEMON_QUIT_REQUESTED) { + VIR_DEBUG("Process quit request"); virHashForEach(dmn->servers, daemonServerClose, NULL); + + if (dmn->shutdownPreserveThread) { + VIR_DEBUG("Shutdown preserve thread running"); + dmn->quit = VIR_NET_DAEMON_QUIT_PRESERVING; + } else { + VIR_DEBUG("Ready to shutdown"); + dmn->quit = VIR_NET_DAEMON_QUIT_READY; + } + } + + if (dmn->quit == VIR_NET_DAEMON_QUIT_READY) { + VIR_DEBUG("Starting shutdown, running prepare"); if (dmn->shutdownPrepareCb && dmn->shutdownPrepareCb() < 0) break;

- if ((dmn->finishTimer = virEventAddTimeout(30 * 1000, - virNetDaemonFinishTimer, - dmn, NULL)) < 0) { + if ((dmn->quitTimer = virEventAddTimeout(30 * 1000, + virNetDaemonQuitTimer, + dmn, NULL)) < 0) { VIR_WARN("Failed to register finish timer."); break; } @@ -787,6 +809,9 @@ virNetDaemonRun(virNetDaemon *dmn) VIR_WARN("Failed to register join thread."); break; } + + VIR_DEBUG("Waiting for shutdown completion"); + dmn->quit = VIR_NET_DAEMON_QUIT_WAITING;

[1]

@@ -808,7 +833,7 @@ virNetDaemonQuit(virNetDaemon *dmn) VIR_LOCK_GUARD lock = virObjectLockGuard(dmn);

VIR_DEBUG("Quit requested %p", dmn); - dmn->quit = true; + dmn->quit = VIR_NET_DAEMON_QUIT_REQUESTED; }

@@ -818,7 +843,7 @@ virNetDaemonQuitExecRestart(virNetDaemon *dmn) VIR_LOCK_GUARD lock = virObjectLockGuard(dmn);

VIR_DEBUG("Exec-restart requested %p", dmn); - dmn->quit = true; + dmn->quit = VIR_NET_DAEMON_QUIT_REQUESTED; dmn->execRestart = true; }

These two seem to be apart from other cases also be registered in various signal (SIGTERM/SIGINT/etc) handlers. As the signal handler code we use doesn't seem to interlock multiple deliveries of said signals it seems that these will be able to mess up the state of the state machine if a 'well-timed' signal hits. Specifically they could clear VIR_NET_DAEMON_QUIT_PRESERVING or VIR_NET_DAEMON_QUIT_WAITING. When VIR_NET_DAEMON_QUIT_PRESERVING will be overwritten it will get fixed in the next iteration of the worker loop in virNetDaemonRun(), but will re-notify systemd about the shutdown and re-stop the 'server's. What's most likely worse is that [1] VIR_NET_DAEMON_QUIT_WAITING can be lost as well, which would re-register the timer and start another shutdown thread.

@@ -827,12 +852,21 @@ static void virNetDaemonPreserveWorker(void *opaque) { virNetDaemon *dmn = opaque;

- VIR_DEBUG("Begin stop dmn=%p", dmn); + VIR_DEBUG("Begin preserve dmn=%p", dmn);

dmn->shutdownPreserveCb();

- VIR_DEBUG("Completed stop dmn=%p", dmn); + VIR_DEBUG("Completed preserve dmn=%p", dmn);

+ VIR_WITH_OBJECT_LOCK_GUARD(dmn) { + if (dmn->quit == VIR_NET_DAEMON_QUIT_PRESERVING) { + VIR_DEBUG("Marking shutdown as ready"); + dmn->quit = VIR_NET_DAEMON_QUIT_READY; + } + g_clear_pointer(&dmn->shutdownPreserveThread, g_free); + } + + VIR_DEBUG("End preserve dmn=%p", dmn); virObjectUnref(dmn); }

@@ -840,15 +874,26 @@ static void virNetDaemonPreserveWorker(void *opaque) void virNetDaemonPreserve(virNetDaemon *dmn) { VIR_LOCK_GUARD lock = virObjectLockGuard(dmn); + VIR_DEBUG("Preserve state request");

- if (!dmn->shutdownPreserveCb || - dmn->shutdownPreserveThread) + if (!dmn->shutdownPreserveCb) { + VIR_DEBUG("No preserve callback registered"); return; + } + if (dmn->shutdownPreserveThread) { + VIR_DEBUG("Preserve state thread already running"); + return; + } + + if (dmn->quit != VIR_NET_DAEMON_QUIT_NONE) { + VIR_WARN("Already initiated shutdown sequence, unable to preserve state"); + return; + }

virObjectRef(dmn); dmn->shutdownPreserveThread = g_new0(virThread, 1);

- if (virThreadCreateFull(dmn->shutdownPreserveThread, true, + if (virThreadCreateFull(dmn->shutdownPreserveThread, false,

This looks like a bugfix to a previous commit as the thread was never actually joined.

virNetDaemonPreserveWorker, "daemon-stop", false, dmn) < 0) { virObjectUnref(dmn);

On Wed, Jan 08, 2025 at 19:42:57 +0000, Daniel P. Berrangé wrote:

The daemons are wired up to shutdown in responsible to UNIX process signals, as well as in response to login1 dbus signals, or loss of desktop session. The latter two options can optionally preserve state (ie running VMs).

In non-systemd environments, as well as for testing, it would be useful to have a way to trigger shutdown with state preservation more directly.

Thus a new admin protocol API is introduced

virAdmConnectDaemonShutdown

which will trigger a daemon shutdown, and preserve running VMs if the VIR_DAEMON_SHUTDOWN_PRESERVE flag is set.

It has a corresponding 'virt-admin daemon-shutdown [--preserve]' command binding.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- include/libvirt/libvirt-admin.h | 13 +++++++++ src/admin/admin_protocol.x | 11 +++++++- src/admin/admin_server_dispatch.c | 13 +++++++++ src/admin/libvirt-admin.c | 33 +++++++++++++++++++++++ src/admin/libvirt_admin_public.syms | 5 ++++ src/rpc/virnetdaemon.c | 4 +++ tools/virt-admin.c | 41 +++++++++++++++++++++++++++++

Missing man-page addition for the 'virt-admin' tool.

7 files changed, 119 insertions(+), 1 deletion(-)

diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h index ae4703f89b..1926202e97 100644 --- a/include/libvirt/libvirt-admin.h +++ b/include/libvirt/libvirt-admin.h @@ -484,6 +484,19 @@ int virAdmConnectSetDaemonTimeout(virAdmConnectPtr conn, unsigned int timeout, unsigned int flags);

+/** + * virAdmConnectDaemonShutdownFlags: + * + * Since: 11.0.0 + */ +typedef enum { + /* Preserve state before shutting down daemon (Since: 11.0.0) */ + VIR_DAEMON_SHUTDOWN_PRESERVE = (1 << 0), +} virAdmConnectDaemonShutdownFlags;

Don't forget to fix version numbers. Especially in the public.syms file.

diff --git a/src/admin/libvirt_admin_public.syms b/src/admin/libvirt_admin_public.syms index 17930e4fac..d39656bc53 100644 --- a/src/admin/libvirt_admin_public.syms +++ b/src/admin/libvirt_admin_public.syms @@ -53,3 +53,8 @@ LIBVIRT_ADMIN_8.6.0 { global: virAdmConnectSetDaemonTimeout; } LIBVIRT_ADMIN_3.0.0; + +LIBVIRT_ADMIN_11.0.0 {

^^^

+ global: + virAdmConnectDaemonShutdown; +} LIBVIRT_ADMIN_8.6.0;

diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c index d1d7bae569..8cc7af1182 100644 --- a/src/rpc/virnetdaemon.c +++ b/src/rpc/virnetdaemon.c @@ -815,8 +815,10 @@ virNetDaemonRun(virNetDaemon *dmn) } }

+ VIR_DEBUG("Main loop exited"); if (dmn->graceful) { virThreadJoin(&shutdownThread); + VIR_DEBUG("Graceful shutdown complete"); } else { VIR_WARN("Make forcefull daemon shutdown"); exit(EXIT_FAILURE); @@ -946,6 +948,8 @@ virNetDaemonSetShutdownCallbacks(virNetDaemon *dmn, { VIR_LOCK_GUARD lock = virObjectLockGuard(dmn);

+ VIR_DEBUG("Shutdown callbacks preserve=%p prepare=%p wait=%p", + preserveCb, prepareCb, waitCb); dmn->shutdownPreserveCb = preserveCb; dmn->shutdownPrepareCb = prepareCb; dmn->shutdownWaitCb = waitCb;

These debug statements don't seem to belong to this patch. Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Wed, Jan 08, 2025 at 19:42:58 +0000, Daniel P. Berrangé wrote:

The service unit "TimeoutStopSec" setting controls how long systemd waits for a service to stop before aggressively killing it, defaulting to 30 seconds if not set.

When we're processing shutdown of VMs in response to OS shutdown, we very likely need more than 30 seconds to complete this job, and can not stop the daemon during this time.

To avoid being prematurely killed, setup a timer that repeatedly extends the "TimeoutStopSec" value while stop of running VMs is arranged.

This does mean if libvirt hangs while stoppping VMs, systemd won't get to kill the libvirt daemon, but this is considered less harmful that forcefully killing running VMs.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/rpc/virnetdaemon.c | 62 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+)

diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c index 8cc7af1182..3ddb9b5404 100644 --- a/src/rpc/virnetdaemon.c +++ b/src/rpc/virnetdaemon.c @@ -78,6 +78,9 @@ struct _virNetDaemon { virNetDaemonShutdownCallback shutdownPrepareCb; virNetDaemonShutdownCallback shutdownWaitCb; virThread *shutdownPreserveThread; + unsigned long long preserveStart; + unsigned int preserveExtended; + int preserveTimer; int quitTimer; virNetDaemonQuitPhase quit; bool graceful; @@ -93,6 +96,14 @@ struct _virNetDaemon {

static virClass *virNetDaemonClass;

+/* + * The minimum additional shutdown time (secs) we should ask + * systemd to allow, while state preservation operations + * are running. A timer will run every 5 seconds, and + * ensure at least this much extra time is requested + */ +#define VIR_NET_DAEMON_PRESERVE_MIN_TIME 30 + static int daemonServerClose(void *payload, const char *key G_GNUC_UNUSED, @@ -162,6 +173,7 @@ virNetDaemonNew(void) if (virEventRegisterDefaultImpl() < 0) goto error;

+ dmn->preserveTimer = -1; dmn->autoShutdownTimerID = -1;

#ifndef WIN32 @@ -727,6 +739,42 @@ daemonShutdownWait(void *opaque) } }

+static void +virNetDaemonPreserveTimer(int timerid G_GNUC_UNUSED, + void *opaque) +{ + virNetDaemon *dmn = opaque; + VIR_LOCK_GUARD lock = virObjectLockGuard(dmn); + unsigned long long now = g_get_monotonic_time(); + unsigned long long delta; + + if (dmn->quit != VIR_NET_DAEMON_QUIT_PRESERVING) + return; + + VIR_DEBUG("Started at %llu now %llu extended %u", + dmn->preserveStart, now, dmn->preserveExtended); + + /* Time since start of preserving state in usec */ + delta = now - dmn->preserveStart; + /* Converts to secs */ + delta /= (1000ull * 1000ull); + + /* Want extra seconds grace to ensure this timer fires + * again before system timeout expires, under high + * load conditions */ + delta += VIR_NET_DAEMON_PRESERVE_MIN_TIME; + + /* Deduct any extension we've previously asked for */ + delta -= dmn->preserveExtended; + + /* Tell systemd how much more we need to extend by */ + virSystemdNotifyExtendTimeout(delta); + dmn->preserveExtended += delta; + + VIR_DEBUG("Extended by %llu", delta); +}

Assume: preserveStart = 0; preserveExtend = 30; /* from the initialization */ Calls: by assumption of starting time above above (all in seconds) the notify timeout simplifies to: notify(delta) = now + 30 - preserveExtend Iteration 0 is the direct call, all others are via timer iter now() notify(delta) preserveExtend 0 0 30 (hardcoded) 30 1 5 5 35 2 10 5 40 3 15 5 45 4 20 5 50 [...] 'man sd_notify' says for EXTEND_TIMEOUT_USEC=... The value specified is a time in microseconds during which the service must send a new message. So this means: - initial iteration indeed extends by 30 seconds - any subsequent iteration extends only by 5 - the timer is also 5 seconds, so it's likely to instantly timeout on second iteration - the complicated algorithm doesn't seem to be necessary, all you want is a constant which is 'timer_interval+tolerance'

static void virNetDaemonQuitTimer(int timerid G_GNUC_UNUSED, void *opaque) @@ -781,11 +829,21 @@ virNetDaemonRun(virNetDaemon *dmn)

if (dmn->quit == VIR_NET_DAEMON_QUIT_REQUESTED) { VIR_DEBUG("Process quit request"); + virSystemdNotifyStopping(); virHashForEach(dmn->servers, daemonServerClose, NULL);

if (dmn->shutdownPreserveThread) { VIR_DEBUG("Shutdown preserve thread running"); dmn->quit = VIR_NET_DAEMON_QUIT_PRESERVING; + dmn->preserveStart = g_get_monotonic_time(); + dmn->preserveExtended = VIR_NET_DAEMON_PRESERVE_MIN_TIME;

^^^ assumptions

+ virSystemdNotifyExtendTimeout(dmn->preserveExtended);

iter 0

+ if ((dmn->preserveTimer = virEventAddTimeout(5 * 1000, + virNetDaemonPreserveTimer, + dmn, NULL)) < 0) {

subsequent iters via timer

+ VIR_WARN("Failed to register preservation timer"); + /* hope for the best */ + } } else { VIR_DEBUG("Ready to shutdown"); dmn->quit = VIR_NET_DAEMON_QUIT_READY; @@ -866,6 +924,10 @@ static void virNetDaemonPreserveWorker(void *opaque) dmn->quit = VIR_NET_DAEMON_QUIT_READY; } g_clear_pointer(&dmn->shutdownPreserveThread, g_free); + if (dmn->preserveTimer != -1) { + virEventRemoveTimeout(dmn->preserveTimer); + dmn->preserveTimer = -1; + } }

VIR_DEBUG("End preserve dmn=%p", dmn); -- 2.47.1