Re: [libvirt] [PATCH 2/2] qemu: Validate the domain after marking the current domain as transient
On Wed, Mar 08, 2017 at 10:32:32AM +0100, Marc Hartmayer wrote:
On Mon, Feb 27, 2017 at 11:20 AM +0100, "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Mon, Feb 27, 2017 at 10:59:39AM +0100, Marc Hartmayer wrote:
On Thu, Feb 23, 2017 at 05:36 PM +0100, "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Thu, Feb 23, 2017 at 05:22:44PM +0100, Marc Hartmayer wrote:
On Thu, Feb 23, 2017 at 03:33 PM +0100, Michal Privoznik <mprivozn@redhat.com> wrote:
On 02/23/2017 10:44 AM, Marc Hartmayer wrote: > Validate the domain that actually will be started. It's semantically > more clear and also it can detect failures that may have happened in > virDomainObjSetDefTransient(). > > Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> > Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> > Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> > --- > src/qemu/qemu_process.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index a57d136..bd3a8b8 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -4746,9 +4746,6 @@ qemuProcessInit(virQEMUDriverPtr driver, > vm->def->os.machine))) > goto cleanup; > > - if (qemuProcessStartValidate(driver, vm, priv->qemuCaps, caps, flags) < 0) > - goto cleanup; > - > /* Do this upfront, so any part of the startup process can add > * runtime state to vm->def that won't be persisted. This let's us > * report implicit runtime defaults in the XML, like vnc listen/socket > @@ -4757,6 +4754,9 @@ qemuProcessInit(virQEMUDriverPtr driver, > if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm) < 0) > goto cleanup; > > + if (qemuProcessStartValidate(driver, vm, priv->qemuCaps, caps, flags) < 0) > + goto cleanup; > +
This needs to be goto stop for the reasons described in the previous e-mail.
> if (flags & VIR_QEMU_PROCESS_START_PRETEND) { > if (qemuDomainSetPrivatePaths(driver, vm) < 0) > goto cleanup; >
Honestly, I like what we have now better. I mean, SetDefTransient() is very unlikely to fail. It's just doing a copy of domain definition (in a very stupid way, but lets save that for a different discussion). Basically, it will fail on OOM only (which you will not get on a Linux system, unless you really try).
It's semantically more clear (at least for me) and for example it enables us to change some parts of the transient domain before validation (affect the transient domain only, not the persistent).
What are you planning to change in the config before validation ?
I'm implementing a feature which allows to select the boot device at domain start time (something like 'virsh start --with-bootdevice sda domain1'). The main reason why we want this is because the s390 architecture boots only from the first device specified in the boot order.
There's no need to changes in the QEMU driver todo this. You can just query the current XML config, change the boot device in it, and then run virDomainCreateXML to launch the guest with the changed config, or virDomainDefineXML again to make the changed boot device permanent.
First of all, I hope I understand you right :) (you can tell me as soon as you have read the following text)
I've followed your advice and tried to add this functionality without any changes in the QEMU/hypervisor. For this I've created a new function in the remote driver which will call the needed functions (get current XML config for the domain, manipulate the XML config, and then use virDomainCreateXML for starting).
To get the current XML config is straightforward as you've mentioned - just use 'virDomanGetXMLDesc(...)'.
But how can I change the boot device? 1) I could use libxml2 for manipulating the XML string which we will get with calling 'virDomainGetXMLDesc(..)' but this is error-prone and just duplicate work as we have to parse the XML string and the information of it. Also, if there are any additions in the future for boot devices there will be no 'compile time reminder' that you have to edit this function.
Yes, this is the way it is normally done. There is a tradeoff here between wanting to be reminded at compile time but seeing build breakage, and libvirt wanting to be able to extend its config info without breaking apps. The libvirt view is that the latter is more important.
or 2) Parse the XML string into our internal representation (virDomainDef), adjust the boot orders and create a new XML string out of this internal representation (reverse to the way how virDomainDefCopy(...) works). But there is the huge problem:
We need 'virCapsPtr caps' and 'virDomainXMLOptionPtr xmlopt' for virDomainDefFormat(...) and virDomainDefParseString(...) but we're on the remote driver layer and therefore we've no direct access to this. We could add a function to the hypervisor driver interface for getting these but this is just awkward.
The virDomainDef are private data structures just for internal libvirt usage. Even though virsh is in the libvirt source tree, it should be treated as if it were a standalone application and thus only use the public APIs.
So my question to you is: do you have a better idea for this?
Many thanks in advance.
PS. I hope it's okay for you that I'm writing to you only for the moment (as it's somehow off topic). If not I'll send it to the mailing list :)
It isn't off-topic, so I'm copying back in the list. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On Wed, Mar 08, 2017 at 10:41 AM +0100, "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Wed, Mar 08, 2017 at 10:32:32AM +0100, Marc Hartmayer wrote:
On Mon, Feb 27, 2017 at 11:20 AM +0100, "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Mon, Feb 27, 2017 at 10:59:39AM +0100, Marc Hartmayer wrote:
On Thu, Feb 23, 2017 at 05:36 PM +0100, "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Thu, Feb 23, 2017 at 05:22:44PM +0100, Marc Hartmayer wrote:
On Thu, Feb 23, 2017 at 03:33 PM +0100, Michal Privoznik <mprivozn@redhat.com> wrote: > On 02/23/2017 10:44 AM, Marc Hartmayer wrote: >> Validate the domain that actually will be started. It's semantically >> more clear and also it can detect failures that may have happened in >> virDomainObjSetDefTransient(). >> >> Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> >> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> >> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> >> --- >> src/qemu/qemu_process.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c >> index a57d136..bd3a8b8 100644 >> --- a/src/qemu/qemu_process.c >> +++ b/src/qemu/qemu_process.c >> @@ -4746,9 +4746,6 @@ qemuProcessInit(virQEMUDriverPtr driver, >> vm->def->os.machine))) >> goto cleanup; >> >> - if (qemuProcessStartValidate(driver, vm, priv->qemuCaps, caps, flags) < 0) >> - goto cleanup; >> - >> /* Do this upfront, so any part of the startup process can add >> * runtime state to vm->def that won't be persisted. This let's us >> * report implicit runtime defaults in the XML, like vnc listen/socket >> @@ -4757,6 +4754,9 @@ qemuProcessInit(virQEMUDriverPtr driver, >> if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm) < 0) >> goto cleanup; >> >> + if (qemuProcessStartValidate(driver, vm, priv->qemuCaps, caps, flags) < 0) >> + goto cleanup; >> + > > This needs to be goto stop for the reasons described in the previous e-mail. > >> if (flags & VIR_QEMU_PROCESS_START_PRETEND) { >> if (qemuDomainSetPrivatePaths(driver, vm) < 0) >> goto cleanup; >> > > Honestly, I like what we have now better. I mean, SetDefTransient() is > very unlikely to fail. It's just doing a copy of domain definition (in a > very stupid way, but lets save that for a different discussion). > Basically, it will fail on OOM only (which you will not get on a Linux > system, unless you really try).
It's semantically more clear (at least for me) and for example it enables us to change some parts of the transient domain before validation (affect the transient domain only, not the persistent).
What are you planning to change in the config before validation ?
I'm implementing a feature which allows to select the boot device at domain start time (something like 'virsh start --with-bootdevice sda domain1'). The main reason why we want this is because the s390 architecture boots only from the first device specified in the boot order.
There's no need to changes in the QEMU driver todo this. You can just query the current XML config, change the boot device in it, and then run virDomainCreateXML to launch the guest with the changed config, or virDomainDefineXML again to make the changed boot device permanent.
First of all, I hope I understand you right :) (you can tell me as soon as you have read the following text)
I've followed your advice and tried to add this functionality without any changes in the QEMU/hypervisor. For this I've created a new function in the remote driver which will call the needed functions (get current XML config for the domain, manipulate the XML config, and then use virDomainCreateXML for starting).
To get the current XML config is straightforward as you've mentioned - just use 'virDomanGetXMLDesc(...)'.
But how can I change the boot device? 1) I could use libxml2 for manipulating the XML string which we will get with calling 'virDomainGetXMLDesc(..)' but this is error-prone and just duplicate work as we have to parse the XML string and the information of it. Also, if there are any additions in the future for boot devices there will be no 'compile time reminder' that you have to edit this function.
Yes, this is the way it is normally done. There is a tradeoff here between wanting to be reminded at compile time but seeing build breakage, and libvirt wanting to be able to extend its config info without breaking apps. The libvirt view is that the latter is more important.
or 2) Parse the XML string into our internal representation (virDomainDef), adjust the boot orders and create a new XML string out of this internal representation (reverse to the way how virDomainDefCopy(...) works). But there is the huge problem:
We need 'virCapsPtr caps' and 'virDomainXMLOptionPtr xmlopt' for virDomainDefFormat(...) and virDomainDefParseString(...) but we're on the remote driver layer and therefore we've no direct access to this. We could add a function to the hypervisor driver interface for getting these but this is just awkward.
The virDomainDef are private data structures just for internal libvirt usage. Even though virsh is in the libvirt source tree, it should be treated as if it were a standalone application and thus only use the public APIs.
I am trying to implement this in libvirtd as the s390 architecture has 1. only one boot device – there is no option to add something like multiple boot devices and sort them with a boot order. 2. s390 has no possibility to temporarily change the boot order during booting as it is possible within x86 BIOS. With that in mind the enablement to change the boot device while starting a (transient) domain in libvirtd is a way to enable s390 architecture to do something similar. (snip) -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
participants (2)
-
Daniel P. Berrange -
Marc Hartmayer