Re: [libvirt] virsh setmaxmem

On Wed, Mar 10, 2010 at 03:33:55PM -0500, Chris Lalancette wrote:

> All, > My recent patch to remove qemudDomainSetMaxMem() revealed some surprising > (to me) behavior of "virsh setmaxmem". In particular, if you run setmaxmem, and > the hypervisor doesn't support it, this fact will be reported to you. However, > if the amount you were setting for maxmem happens to be lower than "currentMemory", > setmaxmem will silently balloon down the domain for you. This is a surprising > result exactly because virsh reports error, but did something anyway. What I > would expect is that if a hypervisor doesn't support virDomainSetMaxMem(), nothing > at all is done. > If we agree that this is behavior that needs to be fixed, I would suggest > that we move the code to do the auto-ballooning into each of the individual > drivers that *do* support virDomainSetMaxMem(). Currently that includes the > test driver, the LXC driver, and the Xen driver. This way, we maintain the > current behavior for the hypervisors that support this call, and make sure > not to do anything at all for the hypervisors that do not. > Opinions? Hum ... looking at cmdSetmaxmem() in tools/virsh.c it does ------------------------------------------------- if (kilobytes < info.memory) { if (virDomainSetMemory(dom, kilobytes) != 0) { virDomainFree(dom); vshError(ctl, "%s", _("Unable to shrink current MemorySize")); return FALSE; } } if (virDomainSetMaxMemory(dom, kilobytes) != 0) { vshError(ctl, "%s", _("Unable to change MaxMemorySize")); ret = FALSE; } ------------------------------------------------- maybe the two need to be reversed, i.e. after checking that virDomainSetMaxMemory actually is supported then we try to shrink the domain memory. It's a bit of a strange thing because it's somehow a chicken and egg problem, but you can't tell from virsh if virDomainSetMaxMemory() call is supported a priori. But I'm not too fond of changing the behaviour of existing APIs due to this. It's not that virDomainSetMaxMem() itself did the shrinking, it's virsh, and IMHO only virsh need to be fixed. More opinions ? :-)