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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list