On Thu, Jan 30, 2025 at 03:10:55PM +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@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.
....what is does with them is practically nothing at all.
They are used to set the 'StatusText' property on a unit, which can b
queried over dbus, or seen with 'systemctl show'. IOW it is practically
useless for telling the user what's happening during a long shutdown
operation.
I was kinda of hoping systemd would display them when it is stuck in
the busy loop waiting for services to quit. Perhaps I'll file an RFE.
> If they don't, we could make the virSystemdNotifyStatus API itself
> issue a VIR_INFO though.
Guess we need to log them directly ourselves after all.
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 :|
