[libvirt] [PATCHv2] libvirt: add daemon itself as shutdown reason

John Ferlan posted 1 patch 5 years, 5 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20181113203659.32624-1-jferlan@redhat.com
include/libvirt/libvirt-domain.h | 2 ++
src/conf/domain_conf.c           | 3 ++-
src/qemu/qemu_process.c          | 6 +++++-
tools/virsh-domain-monitor.c     | 3 ++-
4 files changed, 11 insertions(+), 3 deletions(-)
[libvirt] [PATCHv2] libvirt: add daemon itself as shutdown reason
Posted by John Ferlan 5 years, 5 months ago
From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>

This patch introduces a new shutdown reason "daemon" in order
to indicate that the daemon needed to force shutdown the domain
as the best course of action to take at the moment.

This action would occur during reconnection when processing
encounters an error once the monitor reconnection is successful.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
---

 Since I hadn't seen a yay or nay on my last response related to
 changes - figured I'd post things as they are now just to make it
 easier to determine whether the changes made were acceptible
 This is essentially your original patch merged with my other
 changes and the removal of the qemu_process.c change.

 include/libvirt/libvirt-domain.h | 2 ++
 src/conf/domain_conf.c           | 3 ++-
 src/qemu/qemu_process.c          | 6 +++++-
 tools/virsh-domain-monitor.c     | 3 ++-
 4 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index fdd2d6b8ea..71debd92b8 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -145,6 +145,8 @@ typedef enum {
     VIR_DOMAIN_SHUTOFF_FAILED = 6,      /* domain failed to start */
     VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT = 7, /* restored from a snapshot which was
                                            * taken while domain was shutoff */
+    VIR_DOMAIN_SHUTOFF_DAEMON = 8,      /* daemon decides to kill domain
+                                           during reconnection processing */
 # ifdef VIR_ENUM_SENTINELS
     VIR_DOMAIN_SHUTOFF_LAST
 # endif
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e8e0adc819..b45f25fabb 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -779,7 +779,8 @@ VIR_ENUM_IMPL(virDomainShutoffReason, VIR_DOMAIN_SHUTOFF_LAST,
               "migrated",
               "saved",
               "failed",
-              "from snapshot")
+              "from snapshot",
+              "daemon")
 
 VIR_ENUM_IMPL(virDomainCrashedReason, VIR_DOMAIN_CRASHED_LAST,
               "unknown",
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1850923914..59a94bb74a 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7978,10 +7978,14 @@ qemuProcessReconnect(void *opaque)
          *
          * If we cannot get to the monitor when the QEMU command
          * line used -no-shutdown, then we can safely say that the
-         * domain crashed; otherwise, we don't really know. */
+         * domain crashed; otherwise, if the monitor was started,
+         * then we can blame ourselves, else we failed before the
+         * monitor started so we don't really know. */
         if (!priv->mon && tryMonReconn &&
             qemuDomainIsUsingNoShutdown(priv))
             state = VIR_DOMAIN_SHUTOFF_CRASHED;
+        else if (priv->mon)
+            state = VIR_DOMAIN_SHUTOFF_DAEMON;
         else
             state = VIR_DOMAIN_SHUTOFF_UNKNOWN;
 
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index 3a2636377d..f0ad558f87 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -212,7 +212,8 @@ VIR_ENUM_IMPL(virshDomainShutoffReason,
               N_("migrated"),
               N_("saved"),
               N_("failed"),
-              N_("from snapshot"))
+              N_("from snapshot"),
+              N_("daemon"))
 
 VIR_ENUM_DECL(virshDomainCrashedReason)
 VIR_ENUM_IMPL(virshDomainCrashedReason,
-- 
2.17.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] libvirt: add daemon itself as shutdown reason
Posted by Andrea Bolognani 5 years, 5 months ago
On Tue, 2018-11-13 at 15:36 -0500, John Ferlan wrote:
> From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> 
> This patch introduces a new shutdown reason "daemon" in order
> to indicate that the daemon needed to force shutdown the domain
> as the best course of action to take at the moment.
> 
> This action would occur during reconnection when processing
> encounters an error once the monitor reconnection is successful.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> Reviewed-by: John Ferlan <jferlan@redhat.com>

This made language bindings sad:

  https://ci.centos.org/view/libvirt/job/libvirt-go-check/106/
  https://ci.centos.org/view/libvirt/job/libvirt-perl-check/107/

Interestingly, Python doesn't seem to be bothered at all.

Anyway, it would be great if someone would add support for the new
enum value to Go and Perl bindings and make CI happy again :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] libvirt: add daemon itself as shutdown reason
Posted by Daniel P. Berrangé 5 years, 5 months ago
On Wed, Nov 14, 2018 at 05:19:35PM +0100, Andrea Bolognani wrote:
> On Tue, 2018-11-13 at 15:36 -0500, John Ferlan wrote:
> > From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> > 
> > This patch introduces a new shutdown reason "daemon" in order
> > to indicate that the daemon needed to force shutdown the domain
> > as the best course of action to take at the moment.
> > 
> > This action would occur during reconnection when processing
> > encounters an error once the monitor reconnection is successful.
> > 
> > Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> > Reviewed-by: John Ferlan <jferlan@redhat.com>
> 
> This made language bindings sad:
> 
>   https://ci.centos.org/view/libvirt/job/libvirt-go-check/106/
>   https://ci.centos.org/view/libvirt/job/libvirt-perl-check/107/

This is normal - the check jobs are explicitly intended to fail
whenever public API additions are made to remind us (me) to add
support there.

> Interestingly, Python doesn't seem to be bothered at all.

python auto-generates some of its code.

> Anyway, it would be great if someone would add support for the new
> enum value to Go and Perl bindings and make CI happy again :)

I'm doing it..

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] [PATCHv2] libvirt: add daemon itself as shutdown reason
Posted by Erik Skultety 5 years, 5 months ago
On Tue, Nov 13, 2018 at 03:36:59PM -0500, John Ferlan wrote:
> From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>
> This patch introduces a new shutdown reason "daemon" in order
> to indicate that the daemon needed to force shutdown the domain
> as the best course of action to take at the moment.
>
> This action would occur during reconnection when processing
> encounters an error once the monitor reconnection is successful.
>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> Reviewed-by: John Ferlan <jferlan@redhat.com>
> ---

FWIW:
Reviewed-by: Erik Skultety <eskultet@redhat.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] libvirt: add daemon itself as shutdown reason
Posted by Nikolay Shirokovskiy 5 years, 5 months ago

On 13.11.2018 23:36, John Ferlan wrote:
> From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> 
> This patch introduces a new shutdown reason "daemon" in order
> to indicate that the daemon needed to force shutdown the domain
> as the best course of action to take at the moment.
> 
> This action would occur during reconnection when processing
> encounters an error once the monitor reconnection is successful.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> Reviewed-by: John Ferlan <jferlan@redhat.com>
> ---
> 
>  Since I hadn't seen a yay or nay on my last response related to
>  changes - figured I'd post things as they are now just to make it
>  easier to determine whether the changes made were acceptible
>  This is essentially your original patch merged with my other
>  changes and the removal of the qemu_process.c change.
> 
>  include/libvirt/libvirt-domain.h | 2 ++
>  src/conf/domain_conf.c           | 3 ++-
>  src/qemu/qemu_process.c          | 6 +++++-
>  tools/virsh-domain-monitor.c     | 3 ++-
>  4 files changed, 11 insertions(+), 3 deletions(-)
> 

I thought you are going to push the patch after last letter)

ACK

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