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

[Stoiko Ivanov]

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%)

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


> 
> - 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>

> 
> [1] https://bugzilla.proxmox.com/show_bug.cgi?id=2413
> 
> manager:
> 
> Friedrich Weber (3):
>   node: options: add config option for ballooning target
>   fix #2413: pvestatd: read ballooning RAM usage target from node config
>   ui: node options: allow editing the ballooning RAM usage target
> 
>  PVE/NodeConfig.pm                    |  8 ++++++++
>  PVE/Service/pvestatd.pm              | 10 +++++++---
>  www/manager6/node/NodeOptionsView.js | 18 ++++++++++++++++++
>  3 files changed, 33 insertions(+), 3 deletions(-)
> 
> 
> docs:
> 
> Friedrich Weber (1):
>   pvenode: document ballooning-target node option
> 
>  pvenode.adoc | 13 +++++++++++++
>  qm.adoc      | 13 ++++++++-----
>  2 files changed, 21 insertions(+), 5 deletions(-)
> 
> 
> Summary over all repositories:
>   5 files changed, 54 insertions(+), 8 deletions(-)
>