[libvirt] [PATCH] allow memballoon type of none to desactivate it
The balloon device is automatically added to qemu guests if supported, but it may be useful to desactivate it. The simplest to not change the existing behaviour is to allow <memballoon type="none"/> as an extra option to desactivate it (it is automatically added if the memballoon construct is missing for the domain). The following simple patch just adds the extra option and does not change the default behaviour but avoid creating a balloon device if type="none" is used. Daniel P.S.: there is also a XEN type uspposedly for xen but I was unable to find any baloon code in our current xen driver so no change there -- 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/
On Mon, Aug 09, 2010 at 06:38:27PM +0200, Daniel Veillard wrote:
The balloon device is automatically added to qemu guests if supported, but it may be useful to desactivate it. The simplest to not change the existing behaviour is to allow <memballoon type="none"/> as an extra option to desactivate it (it is automatically added if the memballoon construct is missing for the domain). The following simple patch just adds the extra option and does not change the default behaviour but avoid creating a balloon device if type="none" is used.
I really don't like the idea of 'type=none' devices in general. I don't think we should have an element insides <devices> that doesn't actually represent a device. If we want to disable the balloon, then I think we should aim for an element or attribute elsewhere to toggle it. eg, perhaps the earlier <memory> element can indicate whether it supports ballooning. eg <memory ballonable='yes|no'>2423423432</memory> Thus if ballooning is not enabled, the <memballoon> device would never need to appear within <devices>
P.S.: there is also a XEN type uspposedly for xen but I was unable to find any baloon code in our current xen driver so no change there
Xen always has a balloon device, so we should make Xen driver always add the <memballoon> device. 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 06:53:34PM +0100, Daniel P. Berrange wrote:
On Mon, Aug 09, 2010 at 06:38:27PM +0200, Daniel Veillard wrote:
The balloon device is automatically added to qemu guests if supported, but it may be useful to desactivate it. The simplest to not change the existing behaviour is to allow <memballoon type="none"/> as an extra option to desactivate it (it is automatically added if the memballoon construct is missing for the domain). The following simple patch just adds the extra option and does not change the default behaviour but avoid creating a balloon device if type="none" is used.
I really don't like the idea of 'type=none' devices in general.
Since we automagically add the devices to describe an internal policy I think we're at fault here.
I don't think we should have an element insides <devices> that doesn't actually represent a device.
If we want to disable the balloon, then I think we should aim for an element or attribute elsewhere to toggle it.
eg, perhaps the earlier <memory> element can indicate whether it supports ballooning. eg
<memory ballonable='yes|no'>2423423432</memory>
Thus if ballooning is not enabled, the <memballoon> device would never need to appear within <devices>
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. 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/
On Mon, Aug 09, 2010 at 08:34:23PM +0200, Daniel Veillard wrote:
On Mon, Aug 09, 2010 at 06:53:34PM +0100, Daniel P. Berrange wrote:
On Mon, Aug 09, 2010 at 06:38:27PM +0200, Daniel Veillard wrote:
The balloon device is automatically added to qemu guests if supported, but it may be useful to desactivate it. The simplest to not change the existing behaviour is to allow <memballoon type="none"/> as an extra option to desactivate it (it is automatically added if the memballoon construct is missing for the domain). The following simple patch just adds the extra option and does not change the default behaviour but avoid creating a balloon device if type="none" is used.
I really don't like the idea of 'type=none' devices in general.
Since we automagically add the devices to describe an internal policy I think we're at fault here.
I don't think we should have an element insides <devices> that doesn't actually represent a device.
If we want to disable the balloon, then I think we should aim for an element or attribute elsewhere to toggle it.
eg, perhaps the earlier <memory> element can indicate whether it supports ballooning. eg
<memory ballonable='yes|no'>2423423432</memory>
Thus if ballooning is not enabled, the <memballoon> device would never need to appear within <devices>
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. 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 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 ? 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. 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. After messing with this for an hour, my patch is now 160 lines, it's very hard to tell from the code what's the behaviour is in each cases and 3 kind of regression tests need to be fixed to have all XMLs changed to add balloonable='yes' to the memory section. 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. Current patch provided, I strongly suggest to not continue down this line and apply the previous patch instead. 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/
On Mon, Aug 09, 2010 at 06:53:34PM +0100, Daniel P. Berrange wrote:
On Mon, Aug 09, 2010 at 06:38:27PM +0200, Daniel Veillard wrote:
The balloon device is automatically added to qemu guests if supported, but it may be useful to desactivate it. The simplest to not change the existing behaviour is to allow <memballoon type="none"/> as an extra option to desactivate it (it is automatically added if the memballoon construct is missing for the domain). The following simple patch just adds the extra option and does not change the default behaviour but avoid creating a balloon device if type="none" is used.
I really don't like the idea of 'type=none' devices in general. I don't think we should have an element insides <devices> that doesn't actually represent a device.
If we want to disable the balloon, then I think we should aim for an element or attribute elsewhere to toggle it.
eg, perhaps the earlier <memory> element can indicate whether it supports ballooning. eg
<memory ballonable='yes|no'>2423423432</memory>
Thus if ballooning is not enabled, the <memballoon> device would never need to appear within <devices>
Okay being stuck and having though about the issue for a couple of days I think <memballoon type="none"/> is the most pragmatic way to avoid forcing the memballoon device on QEmu/KVM users at this point. The issue in general of memory configuration is somehow a mess as you pointed out, there is 4 tunables or constructs appaering to control them, but getting to a cleaner way to manage this may take some time. I'm taking the responsability of adding that extra enum in the list and the single line change in the qemu code (the other change is just a comment), and whatever solution we may end up with I think it will be trivial to detect the enum value and switch it on the fly. From a device management perspective since there can be only 1 <memballoon> device per guest I think handling this issue should be trivial there too. So I commited that vary small patch and will take the burden of making the conversion to whatever construct we may end up picking on the long term for this setting. 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