Re: [libvirt] [PATCH] allow memballoon type of none to desactivate it
On Tue, Aug 10, 2010 at 12:09:47AM +0200, Daniel Veillard wrote:
Mail disapeared when trying to send it... sending again ...
On Mon, Aug 09, 2010 at 07:38:32PM +0100, Daniel P. Berrange wrote:
On Mon, Aug 09, 2010 at 08:34:23PM +0200, Daniel Veillard wrote:
Grumpf ... that mean at the internal stucture level we need to add an extra field, that is detected at a completely different time in parsing too ... more complex in general, but I can understand the purity POV.
I don't see this as a real problem. It is no different from the way that we automatically add <controller> devices at the end of parsing, if we saw any <disk> or <channel> devices previously.
Huh ??? You can have a controller without a disk and you don't automagically add a controller even if nothing was suggested by the user, it is widely different. First you need to keep a tristate for def->balloonable, was that attribute defined, or not, then add the error case if it's not "yes" or "no", then you need to care for all the different cases of the tristate when you actually parse the devices section, how do you handle <memballon> definition when balloonable="no" was found, discard, error ?
We'd raise an error becasue the requested config doesn't make sense. It doesn't actually need to be a tri-state - ballooning is either enabled or not. If a user hasn't specified which in the XML, then we'd pick the hypervisor default for that.
I find this different because you provide the information in 2 places at different levels and you ned to cope with conflict ... while still maintaining the current behaviour of autoadding. This then breaks nearly all the qemu XML tests due to the extra balloonable="yes" added automatically to the output since the qemu driver makes that a default.
Yep, that's an unavoidable side-effect of adding more default data to our XML output, but we've hit this many times in the past. eg adding disk controllers, required us to change every single test datafile.
And now you have to explain in documentation all the various cases instead of a simple one liner about using "none".
When data is defined at 2 different places by design, well you designed a mess. And in that case that's cerainly true. And after looking at the situation in detail, no I don't by the "purity" point of view, this actually make things *harder* both for user and from a implementation point of view.
I firmly think that in that case the proper way to do this is to keep the information in one place and that place is the <memballoon> device, type='none' may look ugly to you, but at least the behaviour will be predictable, users will know where to look and that can be explained to the in a single line/sentence.
Neither option is keeping all the information in one place. We already have a four-way split of memory information with <memory> vs <currentMemory>, <memoryBacking> and now <memballoon>. The first three are defining attributes of the VM itself, while the latter is defining information about the device - specifically we added it to provide info about PCI device IDs & the device alias. Whether a guest domain supports ballooning or not is an attribute of the domain itself. If there is no balloon, then there is no device to provide information on, so none of the data that appears under <memballoon> even exists, nor can you pass this device element to the hotplug APIs, because there's no actal device behind it. This makes the <memballoon> element non-sensical to me.
Current patch provided, I strongly suggest to not continue down this line and apply the previous patch instead.
To be fair, I don't actually like either patch, I'm just going for what I think is the least worst option long term. If we could go back in time, I'd never have enabled the balloon device by default in the first place and required an explicit <memballoon>. I'd actually prefer to just ignore this problem completely, and declare that all KVM guest must always have a balloon and not provide any way to disable it. The issue of limited PCI slots is really a QEMU flaw - it should be providing a PCI bridge support so you can have a much less limited number of PCI slots. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, Aug 09, 2010 at 11:47:53PM +0100, Daniel P. Berrange wrote:
On Tue, Aug 10, 2010 at 12:09:47AM +0200, Daniel Veillard wrote:
Mail disapeared when trying to send it... sending again ...
On Mon, Aug 09, 2010 at 07:38:32PM +0100, Daniel P. Berrange wrote:
On Mon, Aug 09, 2010 at 08:34:23PM +0200, Daniel Veillard wrote:
Grumpf ... that mean at the internal stucture level we need to add an extra field, that is detected at a completely different time in parsing too ... more complex in general, but I can understand the purity POV.
I don't see this as a real problem. It is no different from the way that we automatically add <controller> devices at the end of parsing, if we saw any <disk> or <channel> devices previously.
Huh ??? You can have a controller without a disk and you don't automagically add a controller even if nothing was suggested by the user, it is widely different. First you need to keep a tristate for def->balloonable, was that attribute defined, or not, then add the error case if it's not "yes" or "no", then you need to care for all the different cases of the tristate when you actually parse the devices section, how do you handle <memballon> definition when balloonable="no" was found, discard, error ?
We'd raise an error becasue the requested config doesn't make sense.
okay, but that's 2 error cases we introduced by allowing the setting in 2 places. Actually if we go there having memory != currentMemory and balloonable='no' should also raise an error... it doesn't make sense.
It doesn't actually need to be a tri-state - ballooning is either enabled or not. If a user hasn't specified which in the XML, then we'd pick the hypervisor default for that.
Hum, I don't see adding balloonable="no" systematically to all config files when we actually just don't know or just can't tell. If we take the ESX example maybe we can, maybe we can't and very likely we simply don't have support and adding this when we have no control (assuming we can't or it's not implemented) sounds bad. Actually if we were to pick a default while the feature is not implemented, we may just end up with picking something which just doesn't make sense long term.
I find this different because you provide the information in 2 places at different levels and you ned to cope with conflict ... while still maintaining the current behaviour of autoadding. This then breaks nearly all the qemu XML tests due to the extra balloonable="yes" added automatically to the output since the qemu driver makes that a default.
Yep, that's an unavoidable side-effect of adding more default data to our XML output, but we've hit this many times in the past. eg adding disk controllers, required us to change every single test datafile.
As I said "Grumpf" because clearly that's not needed if you just do it by defining or not the membaloon device.
And now you have to explain in documentation all the various cases instead of a simple one liner about using "none".
When data is defined at 2 different places by design, well you designed a mess. And in that case that's cerainly true. And after looking at the situation in detail, no I don't by the "purity" point of view, this actually make things *harder* both for user and from a implementation point of view.
I firmly think that in that case the proper way to do this is to keep the information in one place and that place is the <memballoon> device, type='none' may look ugly to you, but at least the behaviour will be predictable, users will know where to look and that can be explained to the in a single line/sentence.
Neither option is keeping all the information in one place. We already have a four-way split of memory information with <memory> vs <currentMemory>, <memoryBacking> and now <memballoon>. The first three are defining attributes of the VM itself, while the latter is defining information about the device - specifically we added it to provide info about PCI device IDs & the device alias. Whether a guest domain supports ballooning or not is an attribute of the domain itself.
I disagree. It is an attribute of the domain, proper functionning requires support at the guest OS level so it's clearly a domain property. If there is an OS without balooning support
If there is no balloon, then there is no device to provide information on, so none of the data that appears under <memballoon> even exists, nor can you pass this device element to the hotplug APIs, because there's no actal device behind it. This makes the <memballoon> element non-sensical to me.
Current patch provided, I strongly suggest to not continue down this line and apply the previous patch instead.
To be fair, I don't actually like either patch, I'm just going for what I think is the least worst option long term. If we could go back in time, I'd never have enabled the balloon device by default in the first place and required an explicit <memballoon>.
explicit sounds wrong to me too because it's really related to the guest.
I'd actually prefer to just ignore this problem completely, and declare that all KVM guest must always have a balloon and not provide any way to disable it.
Sounds wrong to me. We are putting a policy in libvirt and I dislike that. The hypervisor has the option of enabling ot or not. We shouldn't forbid using either option even if we decided to put a default.
The issue of limited PCI slots is really a QEMU flaw - it should be providing a PCI bridge support so you can have a much less limited number of PCI slots.
I don't care about the reason which may drive people to switching it or not. I'm not trying to change QEmu/KVM or whatever hypervisor we are trying to drive, I want to expose the capabilities to libvirt, and since balooning is not forced with that hypervisor I don't see why we should impose that policy. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (2)
-
Daniel P. Berrange -
Daniel Veillard