[PATCH] qemu: save status xml after generating taint message

Kristina Hanicova posted 1 patch 2 years, 8 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/285bd6dbacdc0833c49f75bd6be40668bb9815c8.1626869027.git.khanicov@redhat.com
There is a newer version of this series
src/qemu/qemu_domain.c | 11 +++++++++++
src/qemu/qemu_domain.h |  5 +++++
src/qemu/qemu_driver.c |  4 ++--
3 files changed, 18 insertions(+), 2 deletions(-)
[PATCH] qemu: save status xml after generating taint message
Posted by Kristina Hanicova 2 years, 8 months ago
We didn't always save status xml after generating new taint
message, which resulted in it being deleted in case of a libvirtd
restart.  Some taint messages were preserved thanks to saving
status xml separately at the end of the calling functions (which
makes sense, because qemuDomainObjTaint was usually called there
multiple times).  But for special cases (e.g. When only few taint
messages are generated) we need a separate function for
generating them and saving status xml explicitly.

Resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=1965589

Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
---
 src/qemu/qemu_domain.c | 11 +++++++++++
 src/qemu/qemu_domain.h |  5 +++++
 src/qemu/qemu_driver.c |  4 ++--
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ac1d8ef151..4b9e717c16 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6468,6 +6468,17 @@ void qemuDomainObjTaint(virQEMUDriver *driver,
     qemuDomainObjTaintMsg(driver, obj, taint, logCtxt, NULL);
 }
 
+void qemuDomainObjTaintAndSave(virQEMUDriver *driver,
+                               virDomainObj *obj,
+                               virDomainTaintFlags taint,
+                               qemuDomainLogContext *logCtxt)
+{
+    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+
+    qemuDomainObjTaintMsg(driver, obj, taint, logCtxt, NULL);
+    ignore_value(virDomainObjSave(obj, driver->xmlopt, cfg->stateDir));
+}
+
 void qemuDomainObjTaintMsg(virQEMUDriver *driver,
                            virDomainObj *obj,
                            virDomainTaintFlags taint,
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index acf6ca5ab6..9bd711cbd4 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -568,6 +568,11 @@ void qemuDomainObjTaint(virQEMUDriver *driver,
                         virDomainTaintFlags taint,
                         qemuDomainLogContext *logCtxt);
 
+void qemuDomainObjTaintAndSave(virQEMUDriver *driver,
+                               virDomainObj *obj,
+                               virDomainTaintFlags taint,
+                               qemuDomainLogContext *logCtxt);
+
 void qemuDomainObjTaintMsg(virQEMUDriver *driver,
                            virDomainObj *obj,
                            virDomainTaintFlags taint,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 521063d438..8652bdc3d8 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14010,7 +14010,7 @@ static int qemuDomainQemuMonitorCommand(virDomainPtr domain, const char *cmd,
 
     priv = vm->privateData;
 
-    qemuDomainObjTaint(driver, vm, VIR_DOMAIN_TAINT_CUSTOM_MONITOR, NULL);
+    qemuDomainObjTaintAndSave(driver, vm, VIR_DOMAIN_TAINT_CUSTOM_MONITOR, NULL);
 
     hmp = !!(flags & VIR_DOMAIN_QEMU_MONITOR_COMMAND_HMP);
 
@@ -16877,7 +16877,7 @@ qemuDomainQemuAgentCommand(virDomainPtr domain,
     if (!qemuDomainAgentAvailable(vm, true))
         goto endjob;
 
-    qemuDomainObjTaint(driver, vm, VIR_DOMAIN_TAINT_CUSTOM_GA_COMMAND, NULL);
+    qemuDomainObjTaintAndSave(driver, vm, VIR_DOMAIN_TAINT_CUSTOM_GA_COMMAND, NULL);
 
     agent = qemuDomainObjEnterAgent(vm);
     ret = qemuAgentArbitraryCommand(agent, cmd, &result, timeout);
-- 
2.31.1

Re: [PATCH] qemu: save status xml after generating taint message
Posted by Peter Krempa 2 years, 8 months ago
On Wed, Jul 21, 2021 at 14:05:05 +0200, Kristina Hanicova wrote:
> We didn't always save status xml after generating new taint
> message, which resulted in it being deleted in case of a libvirtd
> restart.  Some taint messages were preserved thanks to saving
> status xml separately at the end of the calling functions (which
> makes sense, because qemuDomainObjTaint was usually called there
> multiple times).  But for special cases (e.g. When only few taint
> messages are generated) we need a separate function for
> generating them and saving status xml explicitly.

Saving the status XML is a very common operation which we in some cases
repeat a few times when doing an multi-step operation, thus we can
reasonably assume that saving the status XML in all cases when we are
adding a taint on a VM object is okay without the need to special case
operations which don't save the status XML as part of their code.

Re: [PATCH] qemu: save status xml after generating taint message
Posted by Daniel P. Berrangé 2 years, 8 months ago
On Wed, Jul 21, 2021 at 02:29:34PM +0200, Peter Krempa wrote:
> On Wed, Jul 21, 2021 at 14:05:05 +0200, Kristina Hanicova wrote:
> > We didn't always save status xml after generating new taint
> > message, which resulted in it being deleted in case of a libvirtd
> > restart.  Some taint messages were preserved thanks to saving
> > status xml separately at the end of the calling functions (which
> > makes sense, because qemuDomainObjTaint was usually called there
> > multiple times).  But for special cases (e.g. When only few taint
> > messages are generated) we need a separate function for
> > generating them and saving status xml explicitly.
> 
> Saving the status XML is a very common operation which we in some cases
> repeat a few times when doing an multi-step operation, thus we can
> reasonably assume that saving the status XML in all cases when we are
> adding a taint on a VM object is okay without the need to special case
> operations which don't save the status XML as part of their code.

The bug quoted shows a few examples where we fail to save status.

I'm very surprised we don't save status when hotplugging a NIC or a
disk, as the BZ suggests.

Missing status save in QMP monitor command passthrough is less
surprising though since we're not actually changing the VM state
when doing that, so would not have reason to save state except
for the taint message.

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

Re: [PATCH] qemu: save status xml after generating taint message
Posted by Michal Prívozník 2 years, 8 months ago
On 7/21/21 3:02 PM, Daniel P. Berrangé wrote:
> On Wed, Jul 21, 2021 at 02:29:34PM +0200, Peter Krempa wrote:
>> On Wed, Jul 21, 2021 at 14:05:05 +0200, Kristina Hanicova wrote:
>>> We didn't always save status xml after generating new taint
>>> message, which resulted in it being deleted in case of a libvirtd
>>> restart.  Some taint messages were preserved thanks to saving
>>> status xml separately at the end of the calling functions (which
>>> makes sense, because qemuDomainObjTaint was usually called there
>>> multiple times).  But for special cases (e.g. When only few taint
>>> messages are generated) we need a separate function for
>>> generating them and saving status xml explicitly.
>>
>> Saving the status XML is a very common operation which we in some cases
>> repeat a few times when doing an multi-step operation, thus we can
>> reasonably assume that saving the status XML in all cases when we are
>> adding a taint on a VM object is okay without the need to special case
>> operations which don't save the status XML as part of their code.
> 
> The bug quoted shows a few examples where we fail to save status.
> 
> I'm very surprised we don't save status when hotplugging a NIC or a
> disk, as the BZ suggests.

I'm not convinced that the steps there are 100% correct. We do call
virDomainObjSave() after live attach:

https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_driver.c#L7834

> 
> Missing status save in QMP monitor command passthrough is less
> surprising though since we're not actually changing the VM state
> when doing that, so would not have reason to save state except
> for the taint message.

Yep. For a few cases it is hidden in BeginJob() and EndJob() but not for
agent jobs.

Michal

Re: [PATCH] qemu: save status xml after generating taint message
Posted by Fangge Jin 2 years, 8 months ago
On Wed, Jul 21, 2021 at 10:54 PM Michal Prívozník <mprivozn@redhat.com>
wrote:

>
> > The bug quoted shows a few examples where we fail to save status.
> >
> > I'm very surprised we don't save status when hotplugging a NIC or a
> > disk, as the BZ suggests.
>
> I'm not convinced that the steps there are 100% correct. We do call
> virDomainObjSave() after live attach:
>
>
> https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_driver.c#L7834
>
> The two steps for hot-unplug in the BZ actually failed.
(sorry I didn't mention the result in the steps)


> >
> > Missing status save in QMP monitor command passthrough is less
> > surprising though since we're not actually changing the VM state
> > when doing that, so would not have reason to save state except
> > for the taint message.
>
> Yep. For a few cases it is hidden in BeginJob() and EndJob() but not for
> agent jobs.
>
> Michal
>
>
Re: [PATCH] qemu: save status xml after generating taint message
Posted by Michal Prívozník 2 years, 8 months ago
On 7/23/21 5:00 AM, Fangge Jin wrote:
> 
> 
> On Wed, Jul 21, 2021 at 10:54 PM Michal Prívozník <mprivozn@redhat.com
> <mailto:mprivozn@redhat.com>> wrote:
> 
> 
>     > The bug quoted shows a few examples where we fail to save status.
>     >
>     > I'm very surprised we don't save status when hotplugging a NIC or a
>     > disk, as the BZ suggests.
> 
>     I'm not convinced that the steps there are 100% correct. We do call
>     virDomainObjSave() after live attach:
> 
>     https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_driver.c#L7834
>     <https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_driver.c#L7834>
> 
> The two steps for hot-unplug in the BZ actually failed.
> (sorry I didn't mention the result in the steps)
In that case, we have another problem - the domain is marked as
"tainted: use of host cdrom passthrough" even though the hotplug failed
and qemu has never saw host cdrom. And the same goes for "tainted:
network configuration using opaque shell scripts" - the taint happens
before any hotplug is even attempted (and thus possibly even when the
script did not even run).

Michal

Re: [PATCH] qemu: save status xml after generating taint message
Posted by Peter Krempa 2 years, 8 months ago
On Fri, Jul 23, 2021 at 10:27:47 +0200, Michal Prívozník wrote:
> On 7/23/21 5:00 AM, Fangge Jin wrote:
> > 
> > 
> > On Wed, Jul 21, 2021 at 10:54 PM Michal Prívozník <mprivozn@redhat.com
> > <mailto:mprivozn@redhat.com>> wrote:
> > 
> > 
> >     > The bug quoted shows a few examples where we fail to save status.
> >     >
> >     > I'm very surprised we don't save status when hotplugging a NIC or a
> >     > disk, as the BZ suggests.
> > 
> >     I'm not convinced that the steps there are 100% correct. We do call
> >     virDomainObjSave() after live attach:
> > 
> >     https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_driver.c#L7834
> >     <https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_driver.c#L7834>
> > 
> > The two steps for hot-unplug in the BZ actually failed.
> > (sorry I didn't mention the result in the steps)
> In that case, we have another problem - the domain is marked as
> "tainted: use of host cdrom passthrough" even though the hotplug failed
> and qemu has never saw host cdrom. And the same goes for "tainted:
> network configuration using opaque shell scripts" - the taint happens
> before any hotplug is even attempted (and thus possibly even when the
> script did not even run).

IMO we should taint the VM even on a failed attempt. Tainst are not just
because of qemu but also because also libvirt can behave unexpectedly
after such operation and we can't really be sure once we start the
hotplug process.

Re: [PATCH] qemu: save status xml after generating taint message
Posted by Kristina Hanicova 2 years, 8 months ago
On Wed, Jul 21, 2021 at 2:29 PM Peter Krempa <pkrempa@redhat.com> wrote:

> On Wed, Jul 21, 2021 at 14:05:05 +0200, Kristina Hanicova wrote:
> > We didn't always save status xml after generating new taint
> > message, which resulted in it being deleted in case of a libvirtd
> > restart.  Some taint messages were preserved thanks to saving
> > status xml separately at the end of the calling functions (which
> > makes sense, because qemuDomainObjTaint was usually called there
> > multiple times).  But for special cases (e.g. When only few taint
> > messages are generated) we need a separate function for
> > generating them and saving status xml explicitly.
>
> Saving the status XML is a very common operation which we in some cases
> repeat a few times when doing an multi-step operation, thus we can
> reasonably assume that saving the status XML in all cases when we are
> adding a taint on a VM object is okay without the need to special case
> operations which don't save the status XML as part of their code.
>

I didn't want to add saving status xml to the original function mainly
because
of qemuDomainObjCheckTaint, where qemuDomainObjTaint could be potentially
called in multiple for cycles. I thought it would be ineffective, but I will
change the patch if that kind of situation is not likely.

Kristina