From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
	by lore.proxmox.com (Postfix) with ESMTPS id 046041FF189
	for <inbox@lore.proxmox.com>; Fri,  4 Apr 2025 14:48:04 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
	by firstgate.proxmox.com (Proxmox) with ESMTP id D25561EFE9;
	Fri,  4 Apr 2025 14:47:51 +0200 (CEST)
Date: Fri, 4 Apr 2025 14:47:18 +0200
From: Stoiko Ivanov <s.ivanov@proxmox.com>
To: Friedrich Weber <f.weber@proxmox.com>
Message-ID: <20250404144718.5eb00240@rosa.proxmox.com>
In-Reply-To: <20250312151506.128311-1-f.weber@proxmox.com>
References: <20250312151506.128311-1-f.weber@proxmox.com>
X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-pc-linux-gnu)
MIME-Version: 1.0
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.064 Adjusted score from AWL reputation of From: address
 BAYES_00                 -1.9 Bayes spam probability is 0 to 1%
 DMARC_MISSING             0.1 Missing DMARC policy
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [PATCH manager/docs 0/4] fix #2413: make target for
 ballooning configurable
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>

Thanks for the patch!

comments inline:
On Wed, 12 Mar 2025 16:15:02 +0100
Friedrich Weber <f.weber@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@proxmox.com>
Reviewed-by: Stoiko Ivanov <s.ivanov@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(-)
> 



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel