Autoshutdown #7
Merged
Autoshutdown #7
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a move of the code that currently exists in the QEMU driver, into the common layer that can be used by multiple drivers. The code currently supports performing managed save of all running guests, ignoring any failures. Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
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. Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
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. Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
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. Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
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 = "persistent" 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. Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
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. Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Allow users to control how many seconds libvirt waits for QEMU shutdown before force powering off a guest. Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Bypassing cache can make save performance more predictable and avoids trashing the OS cache with data that will not be read again. Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
When doing managed save of VMs, triggered by OS shutdown, it may be desirable to control cache usage. Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
When a domain is marked for autostart, it will be started on every subsequent host OS boot. There may be times when it is desirable to mark a domain to be autostarted, on the next boot only. Thus we add virDomainSetAutostartOnce / virDomainGetAutostartOnce. An alternative would have been to overload the existing virDomainSetAutostart method, to accept values '1' or '2' for the autostart flag. This was not done because it is expected that language bindings will have mapped the current autostart flag to a boolean, and thus turning it into an enum would create a compatibility problem. A further alternative would have been to create a new method virDomainSetAutostartFlags, with a VIR_DOMAIN_AUTOSTART_ONCE flag defined. This was not done because it is felt desirable to clearly separate the two flags. Setting the "once" flag should not interfere with existing autostart setting, whether it is enabled or disabled currently. The 'virsh autostart' command, however, is still overloaded by just adding a --once flag, while current state is added to 'virsh dominfo'. No ability to filter by 'autostart once' status is added to the domain list APIs. The most common use of autostart once will be to automatically set it on host shutdown, and it be cleared on host startup. Thus there would rarely be scenarios in which a running app will need to filter on this new flag. Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
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'. Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
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. Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
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. Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
It is not documented what the various virStateNNN methods are each responsible for doing and the names give little guidance either. Provide some useful documentation comments to explain the intended usage of each. Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The next patch will be introducing a new callback, so rename the method to virNetDaemonSetLifecycleCallbacks to reflect the more general usage. Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
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. Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The call to preserve state (ie running VMs) is triggered in response to the desktop session dbus terminating (session daemon), or logind sending a "PrepareForShutdown" signal. In the case of the latter, daemons should only save their state, not actually exit yet. Other things on the system may still expect the daemon to be running at this stage. Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
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, n 8000 othing will start shutting down if the state saving thread is still running. Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
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. Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
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. Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Since processing running VMs on OS shutdown can take a while, it is beneficial to send systemd status messages about the progress. The systemd status is a point-in-time message, with no ability to look at the history of received messages. So in the systemd status we include the progress information. For the same reason there is no benefit in sending failure messages, as they'll disappear as soon as a status is sent for the subsequent VM to be processed. The libvirt log statements can be viewed as a complete log record so don't need progress info, but do include warnings about failures (present from earlier commits). Reviewed-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewer's Guide by SourceryThis pull request introduces a comprehensive auto-shutdown mechanism for libvirt, replacing the No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @berrange - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a comment about the upgrade story with libvirt-guests.service to the documentation.
- The commit message mentions virt-admin, but there are no tests for the new virt-admin command.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @berrange - I've reviewed your changes - here's some feedback:
Overall Comments:
- The introduction of
virNetDaemonQuitPhaseand the refactoring of the shutdown process invirNetDaemonRunlooks good. - Consider adding a comment to explain why the
stopCbis only called ifquit != VIR_NET_DAEMON_QUIT_NONEinvirNetDaemonStop.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| Instruct the daemon to exit gracefully. If the ``--preserve`` flag is given, | ||
| it will save state in the same manner that would be done on a host OS shutdown | ||
| (privileged daemons) or a login session quit (unprivileged daemons). |
There was a problem hiding this comment.
nitpick (typo): Minor typo: 'host OS shutdown'
It seems like there's a small typo. Should it be 'host OS shutdown' instead of just 'OS shutdown'?
Suggested implementation:
it will save state in the same manner that would be done on a host OS shutdown
Ensure that if there are any other occurrences of "OS shutdown" that lack the "host" prefix, they are updated similarly.
There was a problem hiding this comment.
This is nonsense. The code is already written the way you've suggested to write it.
There was a problem hiding this comment.
I apologize for my previous review comment. Upon reviewing the code and the conversation, it's clear that my suggestion was unnecessary and incorrect. The original text is already correct and precise.
In this case, the best response is to acknowledge the maintainer's feedback and recognize that no change is needed. I'll refrain from making further comments on this specific detail.
| @@ -713,3 +729,231 @@ virDomainDriverAutoStart(virDomainObjList *domains, | |||
|
|
|||
| virDomainObjListForEach(domains, false, virDomainDriverAutoStartOne, &state); | |||
| } | |||
|
|
|||
|
|
|||
| void | |||
There was a problem hid 6D40 ing this comment.
issue (complexity): Consider extracting the shutdown actions into separate helper functions to improve readability and maintainability of the code in virDomainDriverAutoShutdown function, by reducing its cyclomatic complexity and improving its structure..
Consider extracting the different shutdown actions into separate helper functions. This will simplify the nested loops and conditionals in virDomainDriverAutoShutdown while keeping the functionality intact. For example, you might create three helpers, one for each action:
static void perform_managed_save(virDomainPtr *domains, bool *transient, size_t numDomains,
virDomainDriverAutoShutdownConfig *cfg) {
unsigned int *flags = g_new0(unsigned int, numDomains);
for (size_t 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;
// ... Prepare flags, pause domains, etc.
}
for (size_t i = 0; i < numDomains; i++) {
// ... Perform managed save, handle failures, etc.
}
}Similarly, extract shutdown and poweroff actions:
static void perform_shutdown(virDomainPtr *domains, bool *transient, size_t numDomains,
virDomainDriverAutoShutdownConfig *cfg) {
for (size_t i = 0; i < numDomains; i++) {
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;
// ... Initiate shutdown request and log messages
}
// Extract and use a helper for the waiting loop if desired.
}
static void perform_poweroff(virDomainPtr *domains, bool *transient, size_t numDomains,
virDomainDriverAutoShutdownConfig *cfg) {
for (size_t i = 0; i < numDomains; i++) {
if (!domains[i])
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;
// ... Perform destroy and unref logic.
}
}Finally, refactor virDomainDriverAutoShutdown to orchestrate these helpers:
void virDomainDriverAutoShutdown(virDomainDriverAutoShutdownConfig *cfg) {
// ... Setup and common logic
// Mark for auto-restore loop
for (size_t i = 0; i < numDomains; i++) {
// ... Auto-restore logic
}
if (cfg->trySave != VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE)
perform_managed_save(domains, transient, numDomains, cfg);
if (cfg->tryShutdown != VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE)
perform_shutdown(domains, transient, numDomains, cfg);
if (cfg->poweroff != VIR_DOMAIN_DRIVER_AUTO_SHUTDOWN_SCOPE_NONE)
perform_poweroff(domains, transient, numDomains, cfg);
// ... Cleanup logic
}This refactoring improves readability and makes it easier to maintain or test each shutdown action individually.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @berrange - I've reviewed your changes - here's some feedback:
Overall Comments:
- The introduction of
virNetDaemonQuitPhaseand the refactoring of the shutdown process invirNetDaemonRunlooks good. - Consider adding a comment to explain why the
stopCbis only called ifquit != VIR_NET_DAEMON_QUIT_NONEinvirNetDaemonStop.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@CodiumAI-Agent /review |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces new features and enhancements to libvirt's domain and daemon management. It adds a "one-time autostart" capability for domains, allowing them to be configured to autostart only on the next boot, with corresponding API, driver, and CLI support. The shutdown process for the daemon is refactored into a multi-phase state machine, enabling more granular lifecycle management and explicit stop, prepare, and wait callbacks. New configuration options are provided for automatic VM shutdown behavior during host or session termination, including managed save, graceful shutdown, and forced poweroff, with tunable scopes and timeouts. The virt-admin and virsh tools are extended with new commands and options to utilize these features. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant virsh/virt-admin
participant libvirt API
participant Hypervisor Driver
participant Daemon
User->>virsh/virt-admin: Run "autostart --once" or "daemon-shutdown --preserve"
virsh/virt-admin->>libvirt API: virDomainSetAutostartOnce / virAdmConnectDaemonShutdown
libvirt API->>Hypervisor Driver: Set/Get autostartOnce / Initiate shutdown
Hypervisor Driver->>Daemon: Update domain config / Begin shutdown sequence
Daemon->>Hypervisor Driver: (On shutdown) stop, prepare, wait callbacks
Daemon->>User: Command result (success/failure)
sequenceDiagram
participant Systemd/OS
participant Daemon
participant Hypervisor Driver
Systemd/OS->>Daemon: Host shutdown or session end signal
Daemon->>Hypervisor Driver: stopCb (preserve state)
Daemon->>Hypervisor Driver: prepareCb (prepare for shutdown)
Daemon->>Hypervisor Driver: waitCb (finalize shutdown)
Daemon->>Systemd/OS: Notify completion
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (35)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
his series starts the work needed to obsolete the libvirt-guests.sh
script which has grown a surprisingly large amount of functionality.
Currently the virt daemons will acquire inhibitors to delay OS shutdown
when VMs are running. The libvirt-guests.service unit can be used to
call libvirt-guests.sh to shutdown running VMs on system shutdown.
This split is a bad architecture because libvirt-guests.service will
only run once the system has decided to initiate the shutdown sequence.
When the user requests as shutdown while inhibitors are present, logind
will emit a "PrepareForShutdown" signal over dbus. Applications are
supposed to respond to this by preserving state & releasing their
inhibitors, which in turns allows shutdown to be initiated.
The remote daemon already has support for listening for the
"PrepareForShutdown" signal, but only does this for session instances,
not system instances.
This series essentially takes that logic and expands it to run in the
system instances too, thus conceptually making libvirt-guests.service
obsolete.
It is slightly more complicated than that though for many reasons...
Saving running VMs can take a very long time. The inhibitor delay
can be as low as 5 seconds, and when killing a service, systemd may
not wait very long for it to terminate. libvirt-guests.service deals
with this by setting TimeoutStopSecs=0 to make systemd wait forever.
This is undesirable to set in libvirtd.service though, as we would
like systemd to kill the daemon aggressively if it hangs. The series
thus uses the notification protocol to request systemd give it more
time to shutdown, as long as we're in the phase of saving running
VMs. A bug in this code will still result in systemd waiting forever,
which is no different from libvirt-guests.service, but a bug in any
other part of the libvirt daemon shutdown code will result in systemd
killing us.
The existing logic for saving VMs in the session daemons had many
feature gaps compared to libvirt-guests.sh. Thus there is code to
add support
graceful shutdown and force poweroff, which is more flexible
than ON_SHUTDOWN=nnn, as we can try the whole sequence of
options
To aid in testing this logic, virt-admin gains a new command
'virt-admin daemon-shutdown --preserve'
All this new functionality is wired up into the QEMU driver, and is
made easily accessible to other hypervisor drivers, so would easily
be extendable to Xen, CH, LXC drivers, but this is not done in this
series. IOW, libvirt-guests.service is not yet fully obsolete.
The new functionality is also not enabled by default for the system
daemon, it requires explicit admin changes to /etc/libvirt/qemu.conf
to enable it. This is because it would clash with execution of the
libvirt-guests.service if both were enabled.
It is highly desirable that we enable this by default though, so we
need to figure out a upgrade story wrt libvirt-guests.service.
The only libvirt-guests.sh features not implemented are:
PARALLEL_SHUTDOWN=nn.
When doing a graceful shutdown we initiate it on every single VM
at once, and then monitor progress of all of them in parallel.
SYNC_TIME=nn
When make not attempt to sync guest time when restoring from
managed save. This ought to be fixed
Summary by Sourcery
Enhance libvirt's daemon shutdown and VM autostart capabilities by implementing a more flexible and robust mechanism for handling system shutdown and VM lifecycle management
New Features:
Enhancements:
Chores:
Summary by CodeRabbit
New Features
daemon-shutdowncommand in virt-admin, with an optional--preserveflag to save daemon state before shutdown.Bug Fixes
Documentation
Refactor