1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] tools: skip libvirt-guests fast if libvirtd is not active

Christian Ehrhardt posted 1 patch 4 years, 4 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20191203085615.4016-1-christian.ehrhardt@canonical.com
tools/libvirt-guests.sh.in | 8 ++++++++
1 file changed, 8 insertions(+)
[libvirt] [PATCH] tools: skip libvirt-guests fast if libvirtd is not active
Posted by Christian Ehrhardt 4 years, 4 months ago 
The most common operation of libvirt-guests is to manage the local
libvirtd. But users might have disabled that and while we are
After=libvirtd for ordering we are not Requiring it..
OTOH adding that or any harder dependency might affect our ordering.

But if people have disabled libvirt they will do a full retry loop
until timeout. Lets check if the local service is active at all and skip
fast if it is not.

Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1854653

Reported-by: Doug Smythies <dsmythies@telus.net>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 tools/libvirt-guests.sh.in | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in
index 4bc6e866f0..5a9930ee2f 100644
--- a/tools/libvirt-guests.sh.in
+++ b/tools/libvirt-guests.sh.in
@@ -90,6 +90,14 @@ test_connect()
 {
     uri=$1
 
+    if [ "x$uri" = xdefault ]; then
+        # Default config is most common and for the local libvirtd
+        # Check if it is active before wasting time in connect loop
+        if ! systemctl -q is-active libvirtd; then
+            return 1
+        fi
+    fi
+
     i=${CONNECT_RETRIES}
     while [ $i -gt 0 ]; do
         run_virsh "$uri" connect 2>/dev/null
-- 
2.24.0


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tools: skip libvirt-guests fast if libvirtd is not active
Posted by Michal Privoznik 4 years, 4 months ago 
On 12/3/19 9:56 AM, Christian Ehrhardt wrote:
> The most common operation of libvirt-guests is to manage the local
> libvirtd. But users might have disabled that and while we are
> After=libvirtd for ordering we are not Requiring it..
> OTOH adding that or any harder dependency might affect our ordering.
> 
> But if people have disabled libvirt they will do a full retry loop
> until timeout. Lets check if the local service is active at all and skip
> fast if it is not.
> 
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1854653
> 
> Reported-by: Doug Smythies <dsmythies@telus.net>
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
>   tools/libvirt-guests.sh.in | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in
> index 4bc6e866f0..5a9930ee2f 100644
> --- a/tools/libvirt-guests.sh.in
> +++ b/tools/libvirt-guests.sh.in
> @@ -90,6 +90,14 @@ test_connect()
>   {
>       uri=$1
>   
> +    if [ "x$uri" = xdefault ]; then
> +        # Default config is most common and for the local libvirtd
> +        # Check if it is active before wasting time in connect loop
> +        if ! systemctl -q is-active libvirtd; then

Systemd is still not the only init out there:

moe ~ # systemctl -q is-active libvirtd
-su: systemctl: command not found

However, this will make libvirt-guests unusable when using split daemons 
(which are not enabled yet). So I guess we need to check whether either 
libvirtd or any of the hypervisor daemons is running (we can't assume 
the default URI is qemu:///system). Also, it would be nice to print a 
message why we tested connection unusable. Something like:

   eval_gettext "Libvirtd is not running. Skipping."
   return 1

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tools: skip libvirt-guests fast if libvirtd is not active
Posted by Christian Ehrhardt 4 years, 4 months ago 
On Thu, Dec 12, 2019 at 7:04 PM Michal Privoznik <mprivozn@redhat.com>
wrote:

> On 12/3/19 9:56 AM, Christian Ehrhardt wrote:
> > The most common operation of libvirt-guests is to manage the local
> > libvirtd. But users might have disabled that and while we are
> > After=libvirtd for ordering we are not Requiring it..
> > OTOH adding that or any harder dependency might affect our ordering.
> >
> > But if people have disabled libvirt they will do a full retry loop
> > until timeout. Lets check if the local service is active at all and skip
> > fast if it is not.
> >
> > Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1854653
> >
> > Reported-by: Doug Smythies <dsmythies@telus.net>
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > ---
> >   tools/libvirt-guests.sh.in | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in
> > index 4bc6e866f0..5a9930ee2f 100644
> > --- a/tools/libvirt-guests.sh.in
> > +++ b/tools/libvirt-guests.sh.in
> > @@ -90,6 +90,14 @@ test_connect()
> >   {
> >       uri=$1
> >
> > +    if [ "x$uri" = xdefault ]; then
> > +        # Default config is most common and for the local libvirtd
> > +        # Check if it is active before wasting time in connect loop
> > +        if ! systemctl -q is-active libvirtd; then
>
> Systemd is still not the only init out there:
>
> moe ~ # systemctl -q is-active libvirtd
> -su: systemctl: command not found
>
> However, this will make libvirt-guests unusable when using split daemons
> (which are not enabled yet). So I guess we need to check whether either
> libvirtd or any of the hypervisor daemons is running (we can't assume
> the default URI is qemu:///system).


Just for completeness - we can assume that because that is what the
["x$uri" = xdefault] check is for.
For any non default cases it would have not tried to fast path it.

But I like Daniels suggestion of maybe just dropping the retry logic on
this one.
Therefore consider this patch obsolete and I'll send a very different v2
somewhen later.

Also, it would be nice to print a
> message why we tested connection unusable. Something like:
>
>    eval_gettext "Libvirtd is not running. Skipping."
>    return 1
>
> Michal
>
>

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tools: skip libvirt-guests fast if libvirtd is not active
Posted by Daniel P. Berrangé 4 years, 4 months ago 
On Tue, Dec 03, 2019 at 09:56:15AM +0100, Christian Ehrhardt wrote:
> The most common operation of libvirt-guests is to manage the local
> libvirtd. But users might have disabled that and while we are
> After=libvirtd for ordering we are not Requiring it..
> OTOH adding that or any harder dependency might affect our ordering.
> 
> But if people have disabled libvirt they will do a full retry loop
> until timeout. Lets check if the local service is active at all and skip
> fast if it is not.

I don't really get why we have that retry logic at all.
If libvirtd is running, or systemd autostart is present then
virsh connect should work immediately.  If it autostarts and
no guests are present, then ther's no work todo so it'll be
fast enough.

If it fails something either it is stopped and autostart is
disabled, or it is completely broken.

Either way,  a retry loop looks pointless to me.

IOW, instead of adding a check on libvirtd, just
kill the retry loop.


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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tools: skip libvirt-guests fast if libvirtd is not active
Posted by Christian Ehrhardt 4 years, 4 months ago 
On Fri, Dec 13, 2019 at 11:23 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Tue, Dec 03, 2019 at 09:56:15AM +0100, Christian Ehrhardt wrote:
> > The most common operation of libvirt-guests is to manage the local
> > libvirtd. But users might have disabled that and while we are
> > After=libvirtd for ordering we are not Requiring it..
> > OTOH adding that or any harder dependency might affect our ordering.
> >
> > But if people have disabled libvirt they will do a full retry loop
> > until timeout. Lets check if the local service is active at all and skip
> > fast if it is not.
>
> I don't really get why we have that retry logic at all.
> If libvirtd is running, or systemd autostart is present then
> virsh connect should work immediately.  If it autostarts and
> no guests are present, then ther's no work todo so it'll be
> fast enough.
>
> If it fails something either it is stopped and autostart is
> disabled, or it is completely broken.
>
> Either way,  a retry loop looks pointless to me.
>

Yeah I agree and thank you for that POV.
Maybe the loop sits there since more unreliable times and just is outdated
now.


> IOW, instead of adding a check on libvirtd, just
> kill the retry loop.
>
>
> 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 :|
>
>

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 0/1] speed up libvirt-guests
Posted by Christian Ehrhardt 4 years, 4 months ago 
After discussing a fast-skip for local connections we got to the idea
that the loop is droppable. So let us give that approach a review on the
list.

Updates in v2:
- stop fastpathing if the systemd service is down
- instead drop the rety loop entirely

Christian Ehrhardt (1):
  tools: do not loop in libvirt-guests test_connect

 tools/libvirt-guests.sh.in | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

-- 
2.24.1


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/1] tools: do not loop in libvirt-guests test_connect
Posted by Christian Ehrhardt 4 years, 4 months ago 
These days libvirt is pretty reliable and even remote connections
(not the default for libvirt-guests anyway) either work or fail but are
uncommon to be flaky.

On the other hand users might have disabled the service and while we are
After=libvirtd for ordering we are not Requiring it. Adding that or any
harder dependency might break our ordering. But if people have disabled
libvirt they will do a full retry loop until timeout.

Lets drop the loop to be much faster if a remote is not reachable.

Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1854653

Reported-by: Doug Smythies <dsmythies@telus.net>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 tools/libvirt-guests.sh.in | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in
index 4bc6e866f0..a881f6266e 100644
--- a/tools/libvirt-guests.sh.in
+++ b/tools/libvirt-guests.sh.in
@@ -37,8 +37,6 @@ SHUTDOWN_TIMEOUT=300
 PARALLEL_SHUTDOWN=0
 START_DELAY=0
 BYPASS_CACHE=0
-CONNECT_RETRIES=10
-RETRIES_SLEEP=1
 SYNC_TIME=0
 
 test -f "$sysconfdir"/sysconfig/libvirt-guests &&
@@ -90,19 +88,12 @@ test_connect()
 {
     uri=$1
 
-    i=${CONNECT_RETRIES}
-    while [ $i -gt 0 ]; do
-        run_virsh "$uri" connect 2>/dev/null
-        if [ $? -eq 0 ]; then
-            return 0;
-        fi
-        sleep ${RETRIES_SLEEP}
-        eval_gettext "Unable to connect to libvirt currently. Retrying .. \$i"
-        i=$(($i-1))
-    done
-    eval_gettext "Can't connect to \$uri. Skipping."
-    echo
-    return 1
+    if run_virsh "$uri" connect 2>/dev/null; then
+        return 0;
+    else
+        eval_gettext "Can't connect to \$uri. Skipping."
+        return 1
+    fi
 }
 
 # list_guests URI PERSISTENT
-- 
2.24.1


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/1] tools: do not loop in libvirt-guests test_connect
Posted by Daniel P. Berrangé 4 years, 4 months ago 
On Mon, Dec 16, 2019 at 08:20:59AM +0100, Christian Ehrhardt wrote:
> These days libvirt is pretty reliable and even remote connections
> (not the default for libvirt-guests anyway) either work or fail but are
> uncommon to be flaky.
> 
> On the other hand users might have disabled the service and while we are
> After=libvirtd for ordering we are not Requiring it. Adding that or any
> harder dependency might break our ordering. But if people have disabled
> libvirt they will do a full retry loop until timeout.
> 
> Lets drop the loop to be much faster if a remote is not reachable.
> 
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1854653

I extended this to add:

    This reverts
    
      commit 4e7fc8305a53676ba2362bfaa8ca05c4851b7e12
      Author: Michal Prívozník <mprivozn@redhat.com>
      Date:   Fri Feb 21 12:46:08 2014 +0100
    
        libvirt-guests: Wait for libvirtd to initialize
    
    The race described in that commit no longer exists using systemd as
    we now have socket activation. If not using systemd, then it is also
    safe if using the libvirtd --daemon flag, since the parent process
    won't return to the caller until the child is accepting connections.

and have now pushed this fix.

> 
> Reported-by: Doug Smythies <dsmythies@telus.net>
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
>  tools/libvirt-guests.sh.in | 21 ++++++---------------
>  1 file changed, 6 insertions(+), 15 deletions(-)



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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tools: skip libvirt-guests fast if libvirtd is not active
Posted by Doug Smythies 4 years, 4 months ago 
On 2019.12.03 00:56 Christian Ehrhardt wrote:

> The most common operation of libvirt-guests is to manage the local
> libvirtd. But users might have disabled that and while we are
> After=libvirtd for ordering we are not Requiring it..
> OTOH adding that or any harder dependency might affect our ordering.
>
> But if people have disabled libvirt they will do a full retry loop
> until timeout. Lets check if the local service is active at all and skip
> fast if it is not.
>
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1854653
>
> Reported-by: Doug Smythies <dsmythies@telus.net>
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>

Tested-by: Doug Smythies <dsmythies@telus.net>

Thanks.

> ---
>  tools/libvirt-guests.sh.in | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in
> index 4bc6e866f0..5a9930ee2f 100644
> --- a/tools/libvirt-guests.sh.in
> +++ b/tools/libvirt-guests.sh.in
> @@ -90,6 +90,14 @@ test_connect()
>  {
>      uri=$1
>  
> +    if [ "x$uri" = xdefault ]; then
> +        # Default config is most common and for the local libvirtd
> +        # Check if it is active before wasting time in connect loop
> +        if ! systemctl -q is-active libvirtd; then
> +            return 1
> +        fi
> +    fi
> +
>      i=${CONNECT_RETRIES}
>      while [ $i -gt 0 ]; do
>          run_virsh "$uri" connect 2>/dev/null
> -- 
> 2.24.0



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.