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.we...@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.iva...@proxmox.com> > Reviewed-by: Stoiko Ivanov <s.iva...@proxmox.com> > Thanks! _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
