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@xxxxxxxxxx>
> > ---
> > 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.
> > @@ -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.
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 :|
