Re: [libvirt] [PATCH v2 06/15] qemu: Defer capability check to command line generation time
On Mon, 2018-02-19 at 14:30 +0100, Peter Krempa wrote: > On Mon, Feb 19, 2018 at 13:21:35 +, Daniel Berrange wrote: > > On Mon, Feb 19, 2018 at 02:13:14PM +0100, Peter Krempa wrote: > > > The only situation when we should not fail if QEMU is not installed and > > > you restart libvirtd. Making defined domains disappear is bad. I think we can all agree on that :) > > A long time back we did have a patch series somewhere that tried to > > keep domain's loaded, but mark them as "broken" if the QEMU we needed > > was missing. Can't remember why we never went further with it now... > > Well in the meanwhile I've made qemuCaps in the PostParse callbacks > optional and it's re-run in such case on startup of the VM. The > validation callbacks are not run when parsing status/config XMLs so it > should mostly work right now properly. (at least it fixed my case when > my binaries failed to exec due to broken deps) Okay, I'll leave capabilities checks in the validate callback. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 06/15] qemu: Defer capability check to command line generation time
On Mon, Feb 19, 2018 at 13:21:35 +, Daniel Berrange wrote: > On Mon, Feb 19, 2018 at 02:13:14PM +0100, Peter Krempa wrote: > > On Mon, Feb 19, 2018 at 13:04:08 +, Daniel Berrange wrote: > > > On Mon, Feb 19, 2018 at 10:29:18AM +0100, Andrea Bolognani wrote: > > > > On Mon, 2018-02-19 at 07:24 +0100, Peter Krempa wrote: > > > > > On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote: > > > > > > Validate time is a bit too early to check whether the required > > > > > > capabilities are available, since the QEMU binary might have > > > > > > been updated or replaced by the time we are asked to run the > > > > > > guest. > > > > > > > > > > So are you having problem with the fact that the definition will be > > > > > rejected right away and not just when you try to start it? > > > > > > > > > > Validate is re-run when starting the VM so a downgrade is handled > > > > > properly. > > > > > > > > Right, but isn't checking for QEMU capabilities at validate time > > > > unreasonably strict? A guest which uses eg. an invalid combination > > > > of machine type and architecture will never become valid at a later > > > > point, but a guest should not be considered invalid just because > > > > the QEMU binary you happened to have installed at the time you > > > > defined it lacked some features - the guest itself is perfectly > > > > valid, it just can't be run :) > > > > > > Given that we increasingly fill in alot of information in the XML at > > > define > > > time, we already have a general expectation that the QEMU binary will be > > > present at define time. I think this is not unreasonable - we suggest apps > > > call virConnectGetCapabilities and/or virDomainGetCapabilities to > > > understand > > > what is installed/available before creating an XML document to define. > > > Those > > > APIs of course require binaries to be installed too. So I don't think we > > > should really put effort into coping with defining XML for a time when the > > > QEMU binaries aren't installed. Such a scenario is so unlikely to be hit > > > that any code trying to cope with that is going to be largely untested and > > > fragile, so it would be a disservice to pretend it'll be something worth > > > relying on. > > > > The only situation when we should not fail if QEMU is not installed and > > you restart libvirtd. Making defined domains disappear is bad. > > A long time back we did have a patch series somewhere that tried to > keep domain's loaded, but mark them as "broken" if the QEMU we needed > was missing. Can't remember why we never went further with it now... Well in the meanwhile I've made qemuCaps in the PostParse callbacks optional and it's re-run in such case on startup of the VM. The validation callbacks are not run when parsing status/config XMLs so it should mostly work right now properly. (at least it fixed my case when my binaries failed to exec due to broken deps) signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 06/15] qemu: Defer capability check to command line generation time
On Mon, Feb 19, 2018 at 02:13:14PM +0100, Peter Krempa wrote: > On Mon, Feb 19, 2018 at 13:04:08 +, Daniel Berrange wrote: > > On Mon, Feb 19, 2018 at 10:29:18AM +0100, Andrea Bolognani wrote: > > > On Mon, 2018-02-19 at 07:24 +0100, Peter Krempa wrote: > > > > On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote: > > > > > Validate time is a bit too early to check whether the required > > > > > capabilities are available, since the QEMU binary might have > > > > > been updated or replaced by the time we are asked to run the > > > > > guest. > > > > > > > > So are you having problem with the fact that the definition will be > > > > rejected right away and not just when you try to start it? > > > > > > > > Validate is re-run when starting the VM so a downgrade is handled > > > > properly. > > > > > > Right, but isn't checking for QEMU capabilities at validate time > > > unreasonably strict? A guest which uses eg. an invalid combination > > > of machine type and architecture will never become valid at a later > > > point, but a guest should not be considered invalid just because > > > the QEMU binary you happened to have installed at the time you > > > defined it lacked some features - the guest itself is perfectly > > > valid, it just can't be run :) > > > > Given that we increasingly fill in alot of information in the XML at define > > time, we already have a general expectation that the QEMU binary will be > > present at define time. I think this is not unreasonable - we suggest apps > > call virConnectGetCapabilities and/or virDomainGetCapabilities to understand > > what is installed/available before creating an XML document to define. Those > > APIs of course require binaries to be installed too. So I don't think we > > should really put effort into coping with defining XML for a time when the > > QEMU binaries aren't installed. Such a scenario is so unlikely to be hit > > that any code trying to cope with that is going to be largely untested and > > fragile, so it would be a disservice to pretend it'll be something worth > > relying on. > > The only situation when we should not fail if QEMU is not installed and > you restart libvirtd. Making defined domains disappear is bad. A long time back we did have a patch series somewhere that tried to keep domain's loaded, but mark them as "broken" if the QEMU we needed was missing. Can't remember why we never went further with it now... 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] [PATCH v2 06/15] qemu: Defer capability check to command line generation time
On Mon, Feb 19, 2018 at 13:04:08 +, Daniel Berrange wrote: > On Mon, Feb 19, 2018 at 10:29:18AM +0100, Andrea Bolognani wrote: > > On Mon, 2018-02-19 at 07:24 +0100, Peter Krempa wrote: > > > On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote: > > > > Validate time is a bit too early to check whether the required > > > > capabilities are available, since the QEMU binary might have > > > > been updated or replaced by the time we are asked to run the > > > > guest. > > > > > > So are you having problem with the fact that the definition will be > > > rejected right away and not just when you try to start it? > > > > > > Validate is re-run when starting the VM so a downgrade is handled > > > properly. > > > > Right, but isn't checking for QEMU capabilities at validate time > > unreasonably strict? A guest which uses eg. an invalid combination > > of machine type and architecture will never become valid at a later > > point, but a guest should not be considered invalid just because > > the QEMU binary you happened to have installed at the time you > > defined it lacked some features - the guest itself is perfectly > > valid, it just can't be run :) > > Given that we increasingly fill in alot of information in the XML at define > time, we already have a general expectation that the QEMU binary will be > present at define time. I think this is not unreasonable - we suggest apps > call virConnectGetCapabilities and/or virDomainGetCapabilities to understand > what is installed/available before creating an XML document to define. Those > APIs of course require binaries to be installed too. So I don't think we > should really put effort into coping with defining XML for a time when the > QEMU binaries aren't installed. Such a scenario is so unlikely to be hit > that any code trying to cope with that is going to be largely untested and > fragile, so it would be a disservice to pretend it'll be something worth > relying on. The only situation when we should not fail if QEMU is not installed and you restart libvirtd. Making defined domains disappear is bad. The good thing is that at that point all defaults should be already filled in so it should not matter much whether capabilities are present. Other than that, I agree. Checking stuff uprfront is usually better than get to the situation when it fails to start. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 06/15] qemu: Defer capability check to command line generation time
On Mon, Feb 19, 2018 at 10:29:18AM +0100, Andrea Bolognani wrote: > On Mon, 2018-02-19 at 07:24 +0100, Peter Krempa wrote: > > On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote: > > > Validate time is a bit too early to check whether the required > > > capabilities are available, since the QEMU binary might have > > > been updated or replaced by the time we are asked to run the > > > guest. > > > > So are you having problem with the fact that the definition will be > > rejected right away and not just when you try to start it? > > > > Validate is re-run when starting the VM so a downgrade is handled > > properly. > > Right, but isn't checking for QEMU capabilities at validate time > unreasonably strict? A guest which uses eg. an invalid combination > of machine type and architecture will never become valid at a later > point, but a guest should not be considered invalid just because > the QEMU binary you happened to have installed at the time you > defined it lacked some features - the guest itself is perfectly > valid, it just can't be run :) Given that we increasingly fill in alot of information in the XML at define time, we already have a general expectation that the QEMU binary will be present at define time. I think this is not unreasonable - we suggest apps call virConnectGetCapabilities and/or virDomainGetCapabilities to understand what is installed/available before creating an XML document to define. Those APIs of course require binaries to be installed too. So I don't think we should really put effort into coping with defining XML for a time when the QEMU binaries aren't installed. Such a scenario is so unlikely to be hit that any code trying to cope with that is going to be largely untested and fragile, so it would be a disservice to pretend it'll be something worth relying on. 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] [PATCH v2 06/15] qemu: Defer capability check to command line generation time
On 02/19/2018 04:29 AM, Andrea Bolognani wrote: > On Mon, 2018-02-19 at 07:24 +0100, Peter Krempa wrote: >> On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote: >>> Validate time is a bit too early to check whether the required >>> capabilities are available, since the QEMU binary might have >>> been updated or replaced by the time we are asked to run the >>> guest. >> >> So are you having problem with the fact that the definition will be >> rejected right away and not just when you try to start it? >> >> Validate is re-run when starting the VM so a downgrade is handled >> properly. > > Right, but isn't checking for QEMU capabilities at validate time > unreasonably strict? A guest which uses eg. an invalid combination > of machine type and architecture will never become valid at a later > point, but a guest should not be considered invalid just because > the QEMU binary you happened to have installed at the time you > defined it lacked some features - the guest itself is perfectly > valid, it just can't be run :) > IIRC - the decision processing I used for moving the capability checking into Validation for controller options (PCI, SCSI, USB, SATA) was because at least qemuDomainDefValidateMemoryHotplug already had failure paths when capabilities weren't set. Undoing PCI would perhaps mean quite a few more places that need adjustment to get that bald yak. As for the question of "unreasonably strict" at Validation time - isn't a purpose of Validation to ensure certain combinations that were defined would work together properly? It just so happens that using certain combinations of options with a specific version of a binary could be that type of check. Although I do see your point. Personally I'd rather get something "right" when defining it rather than find out later - it's one of those - well why didn't you tell me that before type gripes. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 06/15] qemu: Defer capability check to command line generation time
On Mon, Feb 19, 2018 at 10:29:18 +0100, Andrea Bolognani wrote: > On Mon, 2018-02-19 at 07:24 +0100, Peter Krempa wrote: > > On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote: > > > Validate time is a bit too early to check whether the required > > > capabilities are available, since the QEMU binary might have > > > been updated or replaced by the time we are asked to run the > > > guest. > > > > So are you having problem with the fact that the definition will be > > rejected right away and not just when you try to start it? > > > > Validate is re-run when starting the VM so a downgrade is handled > > properly. > > Right, but isn't checking for QEMU capabilities at validate time > unreasonably strict? Well, the only difference is that you are unable to 'define' such XML. I don't think it's strict since you would not be able to run it anyways. > A guest which uses eg. an invalid combination > of machine type and architecture will never become valid at a later > point, but a guest should not be considered invalid just because > the QEMU binary you happened to have installed at the time you > defined it lacked some features - the guest itself is perfectly > valid, it just can't be run :) I think the term 'guest' here is misleading. The configuration is invalid and thus you can't really create a guest out of it. This is actually a great example, as you actually are not able to define a XML which would fail to be run anyways. The specific reason for the validation callback is to not make the definition vanish in such case. In general, I think that the command line generator is the wrong place for such checks since they are scattered around and you can't really figure out whether you can start such config until you've done a lot of preparation steps. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 06/15] qemu: Defer capability check to command line generation time
On Mon, 2018-02-19 at 07:24 +0100, Peter Krempa wrote: > On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote: > > Validate time is a bit too early to check whether the required > > capabilities are available, since the QEMU binary might have > > been updated or replaced by the time we are asked to run the > > guest. > > So are you having problem with the fact that the definition will be > rejected right away and not just when you try to start it? > > Validate is re-run when starting the VM so a downgrade is handled > properly. Right, but isn't checking for QEMU capabilities at validate time unreasonably strict? A guest which uses eg. an invalid combination of machine type and architecture will never become valid at a later point, but a guest should not be considered invalid just because the QEMU binary you happened to have installed at the time you defined it lacked some features - the guest itself is perfectly valid, it just can't be run :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 06/15] qemu: Defer capability check to command line generation time
On Fri, Feb 16, 2018 at 17:28:03 +0100, Andrea Bolognani wrote: > Validate time is a bit too early to check whether the required > capabilities are available, since the QEMU binary might have > been updated or replaced by the time we are asked to run the > guest. So are you having problem with the fact that the definition will be rejected right away and not just when you try to start it? Validate is re-run when starting the VM so a downgrade is handled properly. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 06/15] qemu: Defer capability check to command line generation time
Validate time is a bit too early to check whether the required
capabilities are available, since the QEMU binary might have
been updated or replaced by the time we are asked to run the
guest.
Move capability checks (back) to qemuBuildControllerDevStr()
and get rid of a lot of redundancy in the process.
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_command.c | 50 +++
src/qemu/qemu_domain.c | 80 ++---
2 files changed, 52 insertions(+), 78 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index a44a1b2d2..a957132bd 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2595,6 +2595,35 @@ qemuBuildUSBControllerDevStr(virDomainControllerDefPtr
def,
return 0;
}
+static int
+virDomainControllerPCIModelNameToQEMUCaps(int modelName)
+{
+switch ((virDomainControllerPCIModelName) modelName) {
+case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE:
+return QEMU_CAPS_DEVICE_PCI_BRIDGE;
+case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE:
+return QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE;
+case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420:
+return QEMU_CAPS_DEVICE_IOH3420;
+case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM:
+return QEMU_CAPS_DEVICE_X3130_UPSTREAM;
+case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM:
+return QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM;
+case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB:
+return QEMU_CAPS_DEVICE_PXB;
+case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE:
+return QEMU_CAPS_DEVICE_PXB_PCIE;
+case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT:
+return QEMU_CAPS_DEVICE_PCIE_ROOT_PORT;
+case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE:
+return QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE;
+case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE:
+case VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_LAST:
+break;
+}
+return -1;
+}
+
/**
* qemuBuildControllerDevStr:
@@ -2727,6 +2756,7 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
case VIR_DOMAIN_CONTROLLER_TYPE_PCI: {
const virDomainPCIControllerOpts *pciopts = &def->opts.pciopts;
const char *modelName =
virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
+int cap =
virDomainControllerPCIModelNameToQEMUCaps(pciopts->modelName);
/* Skip the implicit PHB for pSeries guests */
if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT &&
@@ -2742,6 +2772,18 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
pciopts->modelName);
return -1;
}
+if (cap < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unknown QEMU device for '%s' controller"),
+ modelName);
+return -1;
+}
+if (!virQEMUCapsGet(qemuCaps, cap)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("The '%s' device is not supported by this QEMU
binary"),
+ modelName);
+return -1;
+}
switch ((virDomainControllerModelPCI) def->model) {
case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
@@ -2770,6 +2812,14 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
pciopts->chassis, def->info.alias);
break;
case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+if (pciopts->numaNode != -1 &&
+!virQEMUCapsGet(qemuCaps,
QEMU_CAPS_SPAPR_PCI_HOST_BRIDGE_NUMA_NODE)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("the spapr-pci-host-bridge controller doesn't
"
+ "support numa_node in this QEMU binary"));
+return -1;
+}
+
virBufferAsprintf(&buf, "%s,index=%d,id=%s",
modelName, pciopts->targetIndex,
def->info.alias);
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2bc0259ea..2fbae695a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4244,8 +4244,7 @@ qemuDomainDeviceDefValidateControllerSCSI(const
virDomainControllerDef *controll
static int
qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef
*controller,
- const virDomainDef *def,
- virQEMUCapsPtr qemuCaps)
+ const virDomainDef *def)
{
virDomainControllerModelPCI model = controller->model;
const virDomainPCIControllerOpts *pciopts;
@@ -4322,13 +4321,6 @@ qemuDomainDeviceDefValidateControllerPCI(const
virDomainControllerDef *controlle
return -1;
}
-if (!virQEM
