[PATCH] qemu: save status xml after generating taint message
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
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.
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 :|
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#L783...
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
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#L783...
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
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#L783... <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
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#L783... <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.
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
participants (5)
-
Daniel P. Berrangé -
Fangge Jin -
Kristina Hanicova -
Michal Prívozník -
Peter Krempa