From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 95CAC1FF16F for ; 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 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Cc: pve-devel@lists.proxmox.com Content-Type: multipart/mixed; boundary="===============7877951659468287180==" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" --===============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 =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==--