2:56 a.m.
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(a)telus.net>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)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
11:59 a.m.
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(a)telus.net>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
Tested-by: Doug Smythies <dsmythies(a)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
12:04 p.m.
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(a)telus.net>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)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
1:05 a.m.
On Thu, Dec 12, 2019 at 7:04 PM Michal Privoznik <mprivozn(a)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(a)telus.net>
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)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
4:23 a.m.
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 :|
1:07 a.m.
On Fri, Dec 13, 2019 at 11:23 AM Daniel P. Berrangé <berrange(a)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
1:20 a.m.
New subject: [libvirt] [PATCH v2 0/1] speed up libvirt-guests
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
1:20 a.m.
New subject: [libvirt] [PATCH v2 1/1] tools: do not loop in libvirt-guests test_connect
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(a)telus.net>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)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
4:11 a.m.
New subject: [libvirt] [PATCH v2 1/1] tools: do not loop in libvirt-guests test_connect
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(a)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(a)telus.net>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)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 :|
1803
days inactive
1816
days old
8 comments
4 participants
participants (4)
-
Christian Ehrhardt -
Daniel P. Berrangé -
Doug Smythies -
Michal Privoznik