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 ? :-)

On Mon, Mar 29, 2010 at 04:33:32PM -0600, Eric Blake wrote: > On 03/11/2010 08:46 AM, Daniel Veillard wrote: >> On Wed, Mar 10, 2010 at 03:33:55PM -0500, Chris Lalancette wrote: > > [revisiting something still flagged in my inbox] > >>> 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 ? :-) > > I didn't see any other opinions. Confining the fix to just virsh makes > sense to me, to not even attempt virDomainSetMemory unless we know > virDomainSetMaxMemory worked. But what happens if SetMaxMemory > successfully chops to a smaller size, but then SetMemory fails to follow > suit? The logic may be more complicated than just swapping the two actions. Well the thing is that if the hypervisor accepted to reduce the maximum memory to a given value, it should not fail to accept that value as the current memory usage target, that would be inconsistent and in this case we can report the inconsistency in good faith I think,