From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <f.gruenbichler@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by lists.proxmox.com (Postfix) with ESMTPS id 430CB96187
for <pve-devel@lists.proxmox.com>; Mon, 15 Apr 2024 11:38:22 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 269F26939
for <pve-devel@lists.proxmox.com>; Mon, 15 Apr 2024 11:38:22 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
[94.136.29.106])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by firstgate.proxmox.com (Proxmox) with ESMTPS
for <pve-devel@lists.proxmox.com>; Mon, 15 Apr 2024 11:38:21 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
by proxmox-new.maurer-it.com (Proxmox) with ESMTP id C774B44A8A
for <pve-devel@lists.proxmox.com>; Mon, 15 Apr 2024 11:38:20 +0200 (CEST)
Date: Mon, 15 Apr 2024 11:38:16 +0200
From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= <f.gruenbichler@proxmox.com>
To: Wolfgang Bumiller <w.bumiller@proxmox.com>
Cc: pve-devel@lists.proxmox.com
References: <20240410131316.1208679-1-f.gruenbichler@proxmox.com>
<20240410131316.1208679-10-f.gruenbichler@proxmox.com>
<cofr4ipem2za7ojhxika5fec5gdjrys6qjudouxu6d7cv2ekax@am2mfhvmwqrh>
In-Reply-To: <cofr4ipem2za7ojhxika5fec5gdjrys6qjudouxu6d7cv2ekax@am2mfhvmwqrh>
MIME-Version: 1.0
User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid)
Message-Id: <1713173754.uwkriy5wgf.astroid@yuna.none>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.056 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 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>
X-List-Received-Date: Mon, 15 Apr 2024 09:38:22 -0000
On April 11, 2024 11:17 am, Wolfgang Bumiller wrote:
> On Wed, Apr 10, 2024 at 03:13:06PM +0200, Fabian Gr=C3=BCnbichler wrote:
>> one for combining the per-node broadcasted values, one for checking a po=
ol's
>> limit, and one specific helper for checking guest-related actions such a=
s
>> starting a VM.
>>=20
>> Signed-off-by: Fabian Gr=C3=BCnbichler <f.gruenbichler@proxmox.com>
>> ---
>> src/PVE/GuestHelpers.pm | 190 ++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 190 insertions(+)
>>=20
>> diff --git a/src/PVE/GuestHelpers.pm b/src/PVE/GuestHelpers.pm
>> index 961a7b8..36b44bb 100644
>> --- a/src/PVE/GuestHelpers.pm
>> +++ b/src/PVE/GuestHelpers.pm
>> @@ -416,4 +416,194 @@ sub check_vnet_access {
>> if !($tag || $trunks);
>> }
>> =20
>> +# combines the broadcasted pool usage information to get per-pool stats
>> +#
>> +# $pools parsed pool info from user.cfg
>> +# $usage broadcasted KV hash
>> +# $pool filter for specific pool
>> +# $skip skip a certain guest to ignore its current usage
>> +#
>> +# returns usage hash:
>> +# pool -> cpu/mem/.. -> run/config -> $usage
>> +sub get_pool_usage {
>> + my ($pools, $usage, $pool, $skip) =3D @_;
>> +
>> + my $res =3D {};
>> + my $included_guests =3D {};
>> + for my $node (keys $usage->%*) {
>> + my $node_usage =3D JSON::decode_json($usage->{$node} // '');
>> +
>> + # long IDs first, so we can add children to their parents right away
>> + for my $poolid (sort {$b cmp $a} keys $pools->%*) {
>> + next if defined($pool) && !($pool eq $poolid || $poolid =3D~ m!^$p=
ool/! || $pool =3D~ m!^$poolid/!);
>> +
>> + my $d =3D $res->{$poolid} //=3D {
>> + cpu =3D> {
>> + run =3D> 0,
>> + config =3D> 0,
>> + },
>> + mem =3D> {
>> + run =3D> 0,
>> + config =3D> 0,
>> + },
>> + };
>> +
>> + my $pool_usage =3D $node_usage->{data}->{$poolid} // {};
>> + for my $vmid (keys $pool_usage->%*) {
>> + # only include once in case of migration between broadcast
>> + next if $included_guests->{$vmid};
>> + next if $skip && $skip->{$vmid};
>> + $included_guests->{$vmid} =3D 1;
>> +
>> + my $vm_data =3D $pool_usage->{$vmid};
>> + for my $key (keys $vm_data->%*) {
>> + next if $key eq 'running';
>> + $d->{$key}->{run} +=3D $vm_data->{$key}->{run} if $vm_data->{runn=
ing};
>> + $d->{$key}->{config} +=3D $vm_data->{$key}->{config};
>=20
> So here we autovivify more keys inside $d, so simply receiving them in
> the $pool_usage will add them
well, yeah.. if they are set in pool_usage, then they represent a usage
attribute and should be summed up? ;)
> ...
>=20
>> + }
>> + }
>> +
>> + if (my $parent =3D $pools->{$poolid}->{parent}) {
>> + $res->{$parent} //=3D {
>> + cpu =3D> {
>> + run=3D> 0,
>> + config =3D> 0,
>> + },
>> + mem =3D> {
>> + run =3D> 0,
>> + config =3D> 0,
>> + },
>> + };
>=20
> ...
> so would it make sense to just iterate over `keys %$d` here instead
> of having `cpu` and `mem` hardcoded?
yes
> And/or maybe access-control should expose a list derived from
> $pool_limits_desc directly (or helper sub) to deal with the
> `-run`/`-config` suffixes
that might make sense as well, yes.
> ...
>=20
>> + $res->{$parent}->{cpu}->{run} +=3D $d->{cpu}->{run};
>> + $res->{$parent}->{mem}->{run} +=3D $d->{mem}->{run};
>> + $res->{$parent}->{cpu}->{config} +=3D $d->{cpu}->{config};
>> + $res->{$parent}->{mem}->{config} +=3D $d->{mem}->{config};
>> + }
>> + }
>> + }
>> +
>> + return $res;
>> +}
>> +
>> +# checks whether a pool is (or would be) over its resource limits
>> +#
>> +# $changes is for checking limits for config/state changes like VM star=
ts, if
>> +# set, only the limits with changes are checked (see check_guest_pool_l=
imit)
>> +#
>> +# return value indicates whether any limit was overstepped or not (if $=
noerr is set)
>> +sub check_pool_limits {
>> + my ($usage, $limits, $noerr, $changes) =3D @_;
>> +
>> + my $over =3D {};
>> + my $only_changed =3D defined($changes);
>> +
>> + my $check_limit =3D sub {
>> + my ($key, $running, $limit, $change) =3D @_;
>> +
>> + return if $only_changed && $change =3D=3D 0;
>> +
>> + my $kind =3D $running ? 'run' : 'config';
>> +
>> + my $value =3D $usage->{$key}->{$kind};
>> + $value =3D int($value);
>> + $value +=3D $change;
>> + $value =3D $value / (1024*1024) if $key eq 'mem';
>> + if ($limit < $value) {
>> + $over->{$key}->{$kind}->{change} =3D $change if $change;
>> + $over->{$key}->{$kind}->{over} =3D 1;
>> + }
>> + };
>> +
>> + my $get_change =3D sub {
>> + my ($key, $running) =3D @_;
>> +
>> + return 0 if !defined($changes);
>> +
>> + my $check_running =3D defined($changes->{running}) && $changes->{runni=
ng} ? 1 : 0;
>> +
>> + if ($running =3D=3D $check_running) {
>> + return $changes->{$key} // 0;
>> + } else {
>> + return 0;
>> + }
>> + };
>> +
>> + if (my $limit =3D $limits->{"mem-run"}) {
>> + my $change =3D $get_change->('mem', 1);
>> + $check_limit->('mem', 1, $limit, $change);
>> + }
>> +
>> + if (my $limit =3D $limits->{"mem-config"}) {
>> + my $change =3D $get_change->('mem', 0);
>> + $check_limit->('mem', 0, $limit, $change);
>> + }
>=20
> ...
> Similarly this could then iterate over the list.
>=20
>> +
>> + if (my $limit =3D $limits->{"cpu-run"}) {
>> + my $change =3D $get_change->('cpu', 1);
>> + $check_limit->('cpu', 1, $limit, $change);
>> + }
>> +
>> + if (my $limit =3D $limits->{"cpu-config"}) {
>> + my $change =3D $get_change->('cpu', 0);
>> + $check_limit->('cpu', 0, $limit, $change);
>> + }
>> +
>> + if (!$noerr) {
>> + my $msg =3D '';
>> + for my $key (keys $over->%*) {
>> + for my $kind (keys $over->{$key}->%*) {
>> + my $value =3D $usage->{$key}->{$kind};
>> + $value =3D $value / (1024*1024) if $key eq 'mem';
>> + my $change =3D $over->{$key}->{$kind}->{change};
>> + if ($change) {
>> + $change =3D $change / (1024*1024) if $key eq 'mem';
>> + $value =3D "$value + $change" if $change;
>> + }
>> + my $limit =3D $limits->{"$key-$kind"};
>> + $msg .=3D "($kind) $key: $value over $limit, ";
>> + }
>> + }
>> + if ($msg) {
>> + $msg =3D~ s/, $//;
>> + die "pool limits exhausted: $msg\n";
>> + }
>> + }
>> +
>> + return $over->%* ? 1 : 0;
>> +}
>> +
>> +# checks whether the given changes for a certain guest would overstep a=
pool limit
>> +#
>> +# $changes is an optional hash containing
>> +# - absolute: flag whether changes are relative or absolute
>> +# - running: flag whether the config or running limits should be checke=
d
>> +# - cpu: change value for cpu limit
>> +# - mem: change value for mem limit
>> +# all elements are optional
>> +#
>> +# if no $changes is provided, the limits are checked against the curren=
t usage
>> +#
>> +# $poolid allows overriding the guest's pool membership, for example in=
case it
>> +# is not yet properly set when creating the guest
>> +sub check_guest_pool_limit {
>> + my ($vmid, $changes, $poolid) =3D @_;
>> +
>> + my $user_cfg =3D PVE::Cluster::cfs_read_file("user.cfg");
>> +
>> + $poolid =3D $user_cfg->{vms}->{$vmid} if !defined($poolid);
>> + if ($poolid) {
>> + my $pool =3D $user_cfg->{pools}->{$poolid};
>> +
>> + my $limits =3D $pool->{limits};
>> + return if !$limits;
>> +
>> + my $skip =3D {};
>> + $skip->{$vmid} =3D 1 if $changes && $changes->{absolute};
>> + my $usage =3D PVE::Cluster::get_node_kv('pool-usage');
>> +
>> + $usage =3D PVE::GuestHelpers::get_pool_usage($user_cfg->{pools}, $usag=
e, $poolid, $skip);
>=20
> ^ This is already inside PVE::GuestHelpers ;-)
ack :)
>=20
>> + check_pool_limits($usage->{$poolid}, $limits, 0, $changes);
>> + }
>> +}
>> +
>> 1;
>> --=20
>> 2.39.2
>=20