[pve-devel] [PATCH manager/docs 0/4] fix #2413: make target for ballooning configurable

[Friedrich Weber]

Thanks for the review+test!

On 04/04/2025 14:47, Stoiko Ivanov wrote:
> Thanks for the patch!
> 
> comments inline:
> On Wed, 12 Mar 2025 16:15:02 +0100
> Friedrich Weber <f.weber at proxmox.com> wrote:
> 
>> Automatic memory allocation (ballooning) is implemented in pvestatd, which
>> assigns memory to or reclaims memory from eligible VMs in order to reach a
>> certain target memory usage on the host. The target is currently hardcoded at
>> 80%. Users have reported [1] that this target is unnecessarily low on hosts
>> with large amounts of RAM.
>>
>> This patch series makes the ballooning target configurable. For this, it adds a
>> new node config option `ballooning-target`. pvestatd then reads the target from
>> that option.
>>
>> Some potential discussion points:
>>
>> - Is it OK to have this as a node option? I imagine some users may want to set
>>   different targets on different nodes. But other users may want to set one
>>   target for the whole cluster. Should we have a `ballooning-target` datacenter
>>   option that can be overridden by a `ballooning-target` node option? This might
>>   be overkill for such a simple option, though.
> my initial thought would put this on the Datacenter-level (expecting most
> nodes to be similarly specced - and thus have similar settings).
> OTOH your approach gives the user more flexibility (if at the expense of
> having to set that (via config-management or manually) for all nodes).
> 
> I think we could keep it on the node-level, and if there is demand for
> this follow it up as you suggested:
> node-setting > datacenter-setting > default(80%)

Yeah, that sounds sensible.

> 
>>
>> - Instead of a `ballooning-target` option, should we go for some general
>>   `ballooning-settings` with a property string like `target=80`, in case we want
>>   to make other values (such as `$maxchange`, which is currently hardcoded at 100
>>   MiB) configurable in the future too?
> we could cross that bridge when we get to it/when someone requests this
> change (via versioned postinst renaming)? - at least I don't see the gain
> - especially when it's in node.cfg.
> and even if we have a datacenter.cfg-wide setting in a mixed-version
> cluster while upgrading the config-parser would complain about an unknown
> entry in a property-string as much as about an unknown setting IIRC?

True, though if we might want a general ballooning-settings option in
the future (like `ballooning-settings: target=80,step=100`), going for
`ballooning-settings` already now would spare us from the hassle of
renaming/deprecating ballooning-target -> ballooning-settings on
upgrade. I don't think it would help in the mixed-version scenario,
that's true.

>>
>> - In a mixed-version cluster where node 1 has this patch series and node 2
>>   doesn't yet, any attempt to change node 2's ballooning target from node 1's GUI
>>   will error out with `ballooning-target: property is not defined in schema`.
>>   This is not very nice, but not terrible either, as the error message is
>>   relatively descriptive. Still, is there an easy way to avoid this (checking
>>   client-side whether the option exists?)
> I think the error-message is acceptable here (and nodes in a cluster
> should run the same versions sooner rather than later anyways).
> 
> gave it a quick spin (by installing on my host, setting the target to 10%,
> adding a `balloon` setting for a VM of mine and running `pvestatd start
> --debug 1`, watching it inflate the balloon and the setting the target to
> 99%, watching it deflate it again).
> 
> For me this can be applied as-is:
> 
> Tested-by: Stoiko Ivanov <s.ivanov at proxmox.com>
> Reviewed-by: Stoiko Ivanov <s.ivanov at proxmox.com>
> 

Thanks!