[libvirt] virsh setmaxmem

[Chris Lalancette]

On 03/30/2010 05:58 AM, Daniel Veillard wrote:
> 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,

Yeah, I agree with this.  If the hypervisor fails SetMemory after SetMaxMemory,
it seems like a hypervisor specific problem and we can report on that.
I'll cook up a patch to just change virsh.

Thanks,
-- 
Chris Lalancette