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 [IPv6:2a01:7e0:0:424::9])
by lore.proxmox.com (Postfix) with ESMTPS id 95CAC1FF16F
for <inbox@lore.proxmox.com>; Thu, 19 Dec 2024 17:05:04 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 4E50EC1C8;
Thu, 19 Dec 2024 17:05:04 +0100 (CET)
From: Daniel Kral <d.kral@proxmox.com>
To: f.gruenbichler@proxmox.com
Date: Thu, 19 Dec 2024 17:04:54 +0100
Message-Id: <20241219160454.185140-1-d.kral@proxmox.com>
X-Mailer: git-send-email 2.39.5
In-Reply-To: <20240416122054.733817-10-f.gruenbichler@proxmox.com>
References: <20240416122054.733817-10-f.gruenbichler@proxmox.com>
MIME-Version: 1.0
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.004 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 v2 guest-common 1/1] helpers: add pool
limit/usage helpers
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: pve-devel@lists.proxmox.com
Content-Type: multipart/mixed; boundary="===============7877951659468287180=="
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
--===============7877951659468287180==
Content-Transfer-Encoding: quoted-printable
On 16/04/2024 14:20, Fabian Gr=C3=BCnbichler wrote:=0D
> one for combining the per-node broadcasted values, one for checking a poo=
l's=0D
> limit, and one specific helper for checking guest-related actions such as=
=0D
> starting a VM.=0D
> =0D
> Signed-off-by: Fabian Gr=C3=BCnbichler <f.gruenbichler@proxmox.com>=0D
> ---=0D
> =0D
> Notes:=0D
> v2:=0D
> - style=0D
> - introduce new helper for mapping limit key to usage hash=0D
> - introduce new helper for default usage hash=0D
> - avoid hard-coding cpu/mem and run/config where sensible=0D
> =0D
> src/PVE/GuestHelpers.pm | 183 ++++++++++++++++++++++++++++++++++++++++=0D
> 1 file changed, 183 insertions(+)=0D
> =0D
> diff --git a/src/PVE/GuestHelpers.pm b/src/PVE/GuestHelpers.pm=0D
> index 961a7b8..e52eaf0 100644=0D
> --- a/src/PVE/GuestHelpers.pm=0D
> +++ b/src/PVE/GuestHelpers.pm=0D
> @@ -416,4 +416,187 @@ sub check_vnet_access {=0D
> if !($tag || $trunks);=0D
> }=0D
> =0D
> +sub pool_limit_to_usage {=0D
> + my ($limit_key) =3D @_;=0D
> +=0D
> + my ($resource, $kind) =3D split(/-/, $limit_key, 2);=0D
> +=0D
> + return ($resource, $kind, $kind eq 'run' ? 1 : 0);=0D
> +}=0D
> +=0D
> +sub pool_default_usage {=0D
> + my $default =3D {};=0D
> +=0D
> + for my $limit (keys $PVE::AccessControl::pool_limits_desc->%*) {=0D
=0D
Perlcritic complains here about the direct access to the=0D
`pool_limits_desc`. I haven't found any other occurence where we=0D
reference a variable across packages directly. I can see that they are=0D
only used here, but would it make sense to move these helpers (which are=0D
unrelated to guests themselves) to the `PVE::AccessControl` package?=0D
=0D
> + my ($resource, $kind) =3D pool_limit_to_usage($limit);=0D
> + $default->{$resource}->{$kind} =3D 0;=0D
> + }=0D
> +=0D
> + return $default;=0D
> +}=0D
> +=0D
> +# combines the broadcasted pool usage information to get per-pool stats=
=0D
> +#=0D
> +# $pools parsed pool info from user.cfg=0D
> +# $usage broadcasted KV hash=0D
> +# $pool filter for specific pool=0D
> +# $skip skip a certain guest to ignore its current usage=0D
> +#=0D
> +# returns usage hash:=0D
> +# pool -> cpu/mem/.. -> run/config -> $usage=0D
> +sub get_pool_usage {=0D
> + my ($pools, $usage, $pool, $skip) =3D @_;=0D
> +=0D
> + my $res =3D {};=0D
> + my $included_guests =3D {};=0D
> + for my $node (keys $usage->%*) {=0D
> + my $node_usage =3D JSON::decode_json($usage->{$node} // '');=0D
=0D
As pointed out in the pve-manager ui patch, `encode_json` and=0D
`decode_json` seem to be a little racy for preserving the order. If=0D
these values stay user visible, we should sort the keys here afterwards,=0D
so users can rely on some preserved order.=0D
=0D
> +=0D
> + # long IDs first, so we can add children to their parents right away=0D
> + for my $poolid (sort {$b cmp $a} keys $pools->%*) {=0D
> + if (=0D
> + defined($pool)=0D
> + && !($pool eq $poolid || $poolid =3D~ m!^$pool/! || $pool =3D~ m!^$poo=
lid/!)=0D
> + ) {=0D
> + next;=0D
> + }=0D
> +=0D
> + my $d =3D $res->{$poolid} //=3D pool_default_usage();=0D
> +=0D
> + my $pool_usage =3D $node_usage->{data}->{$poolid} // {};=0D
> + for my $vmid (keys $pool_usage->%*) {=0D
> + # only include once in case of migration between broadcast=0D
> + next if $included_guests->{$vmid};=0D
> + next if $skip && $skip->{$vmid};=0D
> + $included_guests->{$vmid} =3D 1;=0D
> +=0D
> + my $vm_data =3D $pool_usage->{$vmid};=0D
> + for my $key (keys $vm_data->%*) {=0D
> + next if $key eq 'running';=0D
> + $d->{$key}->{run} +=3D $vm_data->{$key}->{run} if $vm_data->{runni=
ng};=0D
> + $d->{$key}->{config} +=3D $vm_data->{$key}->{config};=0D
> + }=0D
> + }=0D
> +=0D
> + if (my $parent =3D $pools->{$poolid}->{parent}) {=0D
> + $res->{$parent} //=3D pool_default_usage();=0D
> + for my $key (keys $d->%*) {=0D
> + for my $kind (keys $d->{$key}->%*) {=0D
> + $res->{$parent}->{$key}->{$kind} =3D $d->{$key}->{$kind};=0D
> + }=0D
> + }=0D
> + }=0D
> + }=0D
> + }=0D
> +=0D
> + return $res;=0D
> +}=0D
> +=0D
> +# checks whether a pool is (or would be) over its resource limits=0D
> +#=0D
> +# $changes is for checking limits for config/state changes like VM start=
s, if=0D
> +# set, only the limits with changes are checked (see check_guest_pool_li=
mit)=0D
> +#=0D
> +# return value indicates whether any limit was overstepped or not (if $n=
oerr is set)=0D
> +sub check_pool_limits {=0D
> + my ($usage, $limits, $noerr, $changes) =3D @_;=0D
> +=0D
> + my $over =3D {};=0D
> + my $only_changed =3D defined($changes);=0D
> +=0D
> + my $check_limit =3D sub {=0D
> + my ($key, $running, $limit, $change) =3D @_;=0D
> +=0D
> + return if $only_changed && $change =3D=3D 0;=0D
> +=0D
> + my $kind =3D $running ? 'run' : 'config';=0D
> +=0D
> + my $value =3D $usage->{$key}->{$kind};=0D
> + $value =3D int($value);=0D
> + $value +=3D $change;=0D
> + $value =3D $value / (1024*1024) if $key eq 'mem';=0D
> + if ($limit < $value) {=0D
> + $over->{$key}->{$kind}->{change} =3D $change if $change;=0D
> + $over->{$key}->{$kind}->{over} =3D 1;=0D
> + }=0D
> + };=0D
> +=0D
> + my $get_change =3D sub {=0D
> + my ($key, $running) =3D @_;=0D
> +=0D
> + return 0 if !defined($changes);=0D
> +=0D
> + my $check_running =3D defined($changes->{running}) && $changes->{runnin=
g} ? 1 : 0;=0D
> +=0D
> + if ($running =3D=3D $check_running) {=0D
> + return $changes->{$key} // 0;=0D
> + } else {=0D
> + return 0;=0D
> + }=0D
> + };=0D
> +=0D
> + while (my ($key, $limit) =3D each $limits->%*) {=0D
> + my ($resource, $kind, $running) =3D pool_limit_to_usage($key);=0D
> + my $change =3D $get_change->($resource, $running);=0D
> + $check_limit->($resource, $running, $limit, $change);=0D
> + }=0D
> +=0D
> + if (!$noerr) {=0D
> + my $msg =3D '';=0D
> + for my $key (keys $over->%*) {=0D
> + for my $kind (keys $over->{$key}->%*) {=0D
> + my $value =3D $usage->{$key}->{$kind};=0D
> + $value =3D $value / (1024*1024) if $key eq 'mem';=0D
> + my $change =3D $over->{$key}->{$kind}->{change};=0D
> + if ($change) {=0D
> + $change =3D $change / (1024*1024) if $key eq 'mem';=0D
> + $value =3D "$value + $change" if $change;=0D
> + }=0D
> + my $limit =3D $limits->{"$key-$kind"};=0D
> + $msg .=3D "($kind) $key: $value over $limit, ";=0D
> + }=0D
> + }=0D
> + if ($msg) {=0D
> + $msg =3D~ s/, $//;=0D
> + die "pool limits exhausted: $msg\n";=0D
> + }=0D
> + }=0D
> +=0D
> + return $over->%* ? 1 : 0;=0D
> +}=0D
> +=0D
> +# checks whether the given changes for a certain guest would overstep a =
pool limit=0D
> +#=0D
> +# $changes is an optional hash containing=0D
> +# - absolute: flag whether changes are relative or absolute=0D
> +# - running: flag whether the config or running limits should be checked=
=0D
> +# - cpu: change value for cpu limit=0D
> +# - mem: change value for mem limit=0D
=0D
the description could benefit from a unit, i.e. "in bytes".=0D
=0D
FWIW, we could use `PVE::Tools::convert_size` more often to make the=0D
input values more agnostic to prefix (e.g. Mega vs Giga) and base unit=0D
(bit vs byte) to reduce the cognitive load across pve-container and=0D
qemu-server when to use what. We could also use that information above=0D
for the error message to include units (i.e. "$value MiB over $limit").=0D
But that would involve much more effort and I'm unsure it's worth it.=0D
=0D
> +# all elements are optional=0D
> +#=0D
> +# if no $changes is provided, the limits are checked against the current=
usage=0D
> +#=0D
> +# $poolid allows overriding the guest's pool membership, for example in =
case it=0D
> +# is not yet properly set when creating the guest=0D
> +sub check_guest_pool_limit {=0D
> + my ($vmid, $changes, $poolid) =3D @_;=0D
> +=0D
> + my $user_cfg =3D PVE::Cluster::cfs_read_file("user.cfg");=0D
> +=0D
> + $poolid =3D $user_cfg->{vms}->{$vmid} if !defined($poolid);=0D
> + if ($poolid) {=0D
> + my $pool =3D $user_cfg->{pools}->{$poolid};=0D
> +=0D
> + my $limits =3D $pool->{limits};=0D
> + return if !$limits;=0D
> +=0D
> + my $skip =3D {};=0D
> + $skip->{$vmid} =3D 1 if $changes && $changes->{absolute};=0D
> + my $usage =3D PVE::Cluster::get_node_kv('pool-usage');=0D
> +=0D
> + $usage =3D get_pool_usage($user_cfg->{pools}, $usage, $poolid, $skip);=
=0D
> + check_pool_limits($usage->{$poolid}, $limits, 0, $changes);=0D
> + }=0D
> +}=0D
> +=0D
> 1;=0D
> -- =0D
> 2.39.2=0D
=0D
=0D
=0D
=0D
=0D
--===============7877951659468287180==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
--===============7877951659468287180==--