[libvirt] [PATCH] tools: skip libvirt-guests fast if libvirtd is not active
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
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
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
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
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 :|
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
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
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
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 :|
participants (4)
-
Christian Ehrhardt -
Daniel P. Berrangé -
Doug Smythies -
Michal Privoznik