This is consistent with what is already done for all other
daemons.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 src/locking/virtlockd.service.in | 2 +-
 src/logging/virtlogd.service.in  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/locking/virtlockd.service.in b/src/locking/virtlockd.service.in
index e7f8057c06..dc43f771cd 100644
--- a/src/locking/virtlockd.service.in
+++ b/src/locking/virtlockd.service.in
@@ -8,7 +8,7 @@ Documentation=https://libvirt.org
 
 [Service]
 EnvironmentFile=-@sysconfdir@/sysconfig/virtlockd
-ExecStart=@sbindir@/virtlockd $VIRTLOCKD_ARGS
+ExecStart=@sbindir@/virtlockd --timeout 120 $VIRTLOCKD_ARGS
 ExecReload=/bin/kill -USR1 $MAINPID
 # Loosing the locks is a really bad thing that will
 # cause the machine to be fenced (rebooted), so make
diff --git a/src/logging/virtlogd.service.in b/src/logging/virtlogd.service.in
index daff48e67d..7ad9545581 100644
--- a/src/logging/virtlogd.service.in
+++ b/src/logging/virtlogd.service.in
@@ -8,7 +8,7 @@ Documentation=https://libvirt.org
 
 [Service]
 EnvironmentFile=-@sysconfdir@/sysconfig/virtlogd
-ExecStart=@sbindir@/virtlogd $VIRTLOGD_ARGS
+ExecStart=@sbindir@/virtlogd --timeout 120 $VIRTLOGD_ARGS
 ExecReload=/bin/kill -USR1 $MAINPID
 # Loosing the logs is a really bad thing that will
 # cause the machine to be fenced (rebooted), so make
-- 
2.25.1

On a Wednesday in 2020, Andrea Bolognani wrote:
>This is consistent with what is already done for all other
>daemons.
>
>Signed-off-by: Andrea Bolognani <abologna@redhat.com>
>---
> src/locking/virtlockd.service.in | 2 +-
> src/logging/virtlogd.service.in  | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano

On Wed, Apr 01, 2020 at 08:53:41PM +0200, Andrea Bolognani wrote:
> This is consistent with what is already done for all other
> daemons.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  src/locking/virtlockd.service.in | 2 +-
>  src/logging/virtlogd.service.in  | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/locking/virtlockd.service.in b/src/locking/virtlockd.service.in
> index e7f8057c06..dc43f771cd 100644
> --- a/src/locking/virtlockd.service.in
> +++ b/src/locking/virtlockd.service.in
> @@ -8,7 +8,7 @@ Documentation=https://libvirt.org
>  
>  [Service]
>  EnvironmentFile=-@sysconfdir@/sysconfig/virtlockd
> -ExecStart=@sbindir@/virtlockd $VIRTLOCKD_ARGS
> +ExecStart=@sbindir@/virtlockd --timeout 120 $VIRTLOCKD_ARGS
>  ExecReload=/bin/kill -USR1 $MAINPID
>  # Loosing the locks is a really bad thing that will
>  # cause the machine to be fenced (rebooted), so make

I think this is safe, because IIRC we intentionally leak the RPC
connection FD to QEMU and thus will keep it open & inhibiti shutdown.

> diff --git a/src/logging/virtlogd.service.in b/src/logging/virtlogd.service.in
> index daff48e67d..7ad9545581 100644
> --- a/src/logging/virtlogd.service.in
> +++ b/src/logging/virtlogd.service.in
> @@ -8,7 +8,7 @@ Documentation=https://libvirt.org
>  
>  [Service]
>  EnvironmentFile=-@sysconfdir@/sysconfig/virtlogd
> -ExecStart=@sbindir@/virtlogd $VIRTLOGD_ARGS
> +ExecStart=@sbindir@/virtlogd --timeout 120 $VIRTLOGD_ARGS
>  ExecReload=/bin/kill -USR1 $MAINPID
>  # Loosing the logs is a really bad thing that will
>  # cause the machine to be fenced (rebooted), so make

I'm fairly sure this is not safe on its own.

virLogDaemonInhibitor only inhibits timer shutdown for the unprivileged
daemon. This setting a timeout will cause the virtlogd to shutdown even
when log files are open. I can't remember why I special cased this in
the code now, but fairly sure we'll need to fix that first.


Can you test to ensure that they don't prematurely shut down when logs
or locks are held.

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 Thu, 2020-04-02 at 13:10 +0100, Daniel P. Berrangé wrote:
> On Wed, Apr 01, 2020 at 08:53:41PM +0200, Andrea Bolognani wrote:
> >  [Service]
> >  EnvironmentFile=-@sysconfdir@/sysconfig/virtlogd
> > -ExecStart=@sbindir@/virtlogd $VIRTLOGD_ARGS
> > +ExecStart=@sbindir@/virtlogd --timeout 120 $VIRTLOGD_ARGS
> >  ExecReload=/bin/kill -USR1 $MAINPID
> >  # Loosing the logs is a really bad thing that will
> >  # cause the machine to be fenced (rebooted), so make
> 
> I'm fairly sure this is not safe on its own.
> 
> virLogDaemonInhibitor only inhibits timer shutdown for the unprivileged
> daemon. This setting a timeout will cause the virtlogd to shutdown even
> when log files are open. I can't remember why I special cased this in
> the code now, but fairly sure we'll need to fix that first.

If we're not convinced this is safe, then we better revert
02b6005063d6 before 6.2.0 is tagged.

> Can you test to ensure that they don't prematurely shut down when logs
> or locks are held.

I have been running some variation of master (including the commit
mentioned above) for a while now and I haven't encountered any issues
with it. What exactly should I be looking for?

-- 
Andrea Bolognani / Red Hat / Virtualization

On Thu, Apr 02, 2020 at 02:20:27PM +0200, Andrea Bolognani wrote:
> On Thu, 2020-04-02 at 13:10 +0100, Daniel P. Berrangé wrote:
> > On Wed, Apr 01, 2020 at 08:53:41PM +0200, Andrea Bolognani wrote:
> > >  [Service]
> > >  EnvironmentFile=-@sysconfdir@/sysconfig/virtlogd
> > > -ExecStart=@sbindir@/virtlogd $VIRTLOGD_ARGS
> > > +ExecStart=@sbindir@/virtlogd --timeout 120 $VIRTLOGD_ARGS
> > >  ExecReload=/bin/kill -USR1 $MAINPID
> > >  # Loosing the logs is a really bad thing that will
> > >  # cause the machine to be fenced (rebooted), so make
> > 
> > I'm fairly sure this is not safe on its own.
> > 
> > virLogDaemonInhibitor only inhibits timer shutdown for the unprivileged
> > daemon. This setting a timeout will cause the virtlogd to shutdown even
> > when log files are open. I can't remember why I special cased this in
> > the code now, but fairly sure we'll need to fix that first.
> 
> If we're not convinced this is safe, then we better revert
> 02b6005063d6 before 6.2.0 is tagged.
> 
> > Can you test to ensure that they don't prematurely shut down when logs
> > or locks are held.
> 
> I have been running some variation of master (including the commit
> mentioned above) for a while now and I haven't encountered any issues
> with it. What exactly should I be looking for?

Just run "virtlogd --timeout 20" and then start a QEMU guest. I see that
virtlogd shuts down while the guest is still running, thus breaking the
logfile writing.  NB run the system instance, not session instance.

A similar test for virtlockd - it mustn't shutdown while any QEMU guest
with disks, is running. As mentioned above, I think it is probably fine
already, but worth checking

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 Thu, 2020-04-02 at 13:36 +0100, Daniel P. Berrangé wrote:
> On Thu, Apr 02, 2020 at 02:20:27PM +0200, Andrea Bolognani wrote:
> > On Thu, 2020-04-02 at 13:10 +0100, Daniel P. Berrangé wrote:
> > > virLogDaemonInhibitor only inhibits timer shutdown for the unprivileged
> > > daemon. This setting a timeout will cause the virtlogd to shutdown even
> > > when log files are open. I can't remember why I special cased this in
> > > the code now, but fairly sure we'll need to fix that first.
> > 
> > If we're not convinced this is safe, then we better revert
> > 02b6005063d6 before 6.2.0 is tagged.
> > 
> > > Can you test to ensure that they don't prematurely shut down when logs
> > > or locks are held.
> > 
> > I have been running some variation of master (including the commit
> > mentioned above) for a while now and I haven't encountered any issues
> > with it. What exactly should I be looking for?
> 
> Just run "virtlogd --timeout 20" and then start a QEMU guest. I see that
> virtlogd shuts down while the guest is still running, thus breaking the
> logfile writing.  NB run the system instance, not session instance.

I started a VM, waited a bit, and sure enough virtlogd quit on its
own. However, after I ssh'd into the VM and executed poweroff,
virtlogd was socket-activated (as expected) and the log file, which
I was tailing from another terminal, was updated to report the fact
that the VM was shutting down. So, at least from this very simple
test, it would seem that there is no ill effect resulting from
letting virtlogd shut itself down after a timeout.

However, given the concern you've raised, I would personally err on
the side of caution and merge patch 3/8 from this series right away,
so that we are sure 6.2.0 is released in a known-good state. Any
objection to that?

> A similar test for virtlockd - it mustn't shutdown while any QEMU guest
> with disks, is running. As mentioned above, I think it is probably fine
> already, but worth checking

I've never used virtlockd, so I'm not even sure how to make libvirt
take advantage of it. Either way, as of current master the timeout is
not set for virtlockd, so we can take our time figuring this one out.

-- 
Andrea Bolognani / Red Hat / Virtualization

On Thu, Apr 02, 2020 at 14:51:13 +0200, Andrea Bolognani wrote:
> On Thu, 2020-04-02 at 13:36 +0100, Daniel P. Berrangé wrote:
> > On Thu, Apr 02, 2020 at 02:20:27PM +0200, Andrea Bolognani wrote:
> > > On Thu, 2020-04-02 at 13:10 +0100, Daniel P. Berrangé wrote:
> > > > virLogDaemonInhibitor only inhibits timer shutdown for the unprivileged
> > > > daemon. This setting a timeout will cause the virtlogd to shutdown even
> > > > when log files are open. I can't remember why I special cased this in
> > > > the code now, but fairly sure we'll need to fix that first.
> > > 
> > > If we're not convinced this is safe, then we better revert
> > > 02b6005063d6 before 6.2.0 is tagged.
> > > 
> > > > Can you test to ensure that they don't prematurely shut down when logs
> > > > or locks are held.
> > > 
> > > I have been running some variation of master (including the commit
> > > mentioned above) for a while now and I haven't encountered any issues
> > > with it. What exactly should I be looking for?
> > 
> > Just run "virtlogd --timeout 20" and then start a QEMU guest. I see that
> > virtlogd shuts down while the guest is still running, thus breaking the
> > logfile writing.  NB run the system instance, not session instance.
> 
> I started a VM, waited a bit, and sure enough virtlogd quit on its
> own. However, after I ssh'd into the VM and executed poweroff,
> virtlogd was socket-activated (as expected) and the log file, which
> I was tailing from another terminal, was updated to report the fact
> that the VM was shutting down. So, at least from this very simple
> test, it would seem that there is no ill effect resulting from
> letting virtlogd shut itself down after a timeout.

The log entry about VM shutdown is added by libvirt and not by qemu. In
fact there's no way for qemu to "socket activate" anything since we are
logging the stderr/out of qemu via vitlogd via a pipe. If vitlogd exits
while a VM is running you lose stdout/stderr logging of the qemu process
and that is _very_ bad.

On Thu, Apr 02, 2020 at 02:51:13PM +0200, Andrea Bolognani wrote:
> On Thu, 2020-04-02 at 13:36 +0100, Daniel P. Berrangé wrote:
> > On Thu, Apr 02, 2020 at 02:20:27PM +0200, Andrea Bolognani wrote:
> > > On Thu, 2020-04-02 at 13:10 +0100, Daniel P. Berrangé wrote:
> > > > virLogDaemonInhibitor only inhibits timer shutdown for the unprivileged
> > > > daemon. This setting a timeout will cause the virtlogd to shutdown even
> > > > when log files are open. I can't remember why I special cased this in
> > > > the code now, but fairly sure we'll need to fix that first.
> > > 
> > > If we're not convinced this is safe, then we better revert
> > > 02b6005063d6 before 6.2.0 is tagged.
> > > 
> > > > Can you test to ensure that they don't prematurely shut down when logs
> > > > or locks are held.
> > > 
> > > I have been running some variation of master (including the commit
> > > mentioned above) for a while now and I haven't encountered any issues
> > > with it. What exactly should I be looking for?
> > 
> > Just run "virtlogd --timeout 20" and then start a QEMU guest. I see that
> > virtlogd shuts down while the guest is still running, thus breaking the
> > logfile writing.  NB run the system instance, not session instance.
> 
> I started a VM, waited a bit, and sure enough virtlogd quit on its
> own. However, after I ssh'd into the VM and executed poweroff,
> virtlogd was socket-activated (as expected) and the log file, which
> I was tailing from another terminal, was updated to report the fact
> that the VM was shutting down. So, at least from this very simple
> test, it would seem that there is no ill effect resulting from
> letting virtlogd shut itself down after a timeout.

Anything that QEMU would have written to the logfile is lost
though.

> However, given the concern you've raised, I would personally err on
> the side of caution and merge patch 3/8 from this series right away,
> so that we are sure 6.2.0 is released in a known-good state. Any
> objection to that?

Yes, we need to revert that change asap.

> > A similar test for virtlockd - it mustn't shutdown while any QEMU guest
> > with disks, is running. As mentioned above, I think it is probably fine
> > already, but worth checking
> 
> I've never used virtlockd, so I'm not even sure how to make libvirt
> take advantage of it. Either way, as of current master the timeout is
> not set for virtlockd, so we can take our time figuring this one out.

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 Thu, 2020-04-02 at 14:01 +0100, Daniel P. Berrangé wrote:
> On Thu, Apr 02, 2020 at 02:51:13PM +0200, Andrea Bolognani wrote:
> > I started a VM, waited a bit, and sure enough virtlogd quit on its
> > own. However, after I ssh'd into the VM and executed poweroff,
> > virtlogd was socket-activated (as expected) and the log file, which
> > I was tailing from another terminal, was updated to report the fact
> > that the VM was shutting down. So, at least from this very simple
> > test, it would seem that there is no ill effect resulting from
> > letting virtlogd shut itself down after a timeout.
> 
> Anything that QEMU would have written to the logfile is lost
> though.
> 
> > However, given the concern you've raised, I would personally err on
> > the side of caution and merge patch 3/8 from this series right away,
> > so that we are sure 6.2.0 is released in a known-good state. Any
> > objection to that?
> 
> Yes, we need to revert that change asap.

Done.

-- 
Andrea Bolognani / Red Hat / Virtualization