Re: [pve-devel] [PATCH manager 2/4] pvestatd: collect and broadcast pool usage

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On April 11, 2024 11:32 am, Wolfgang Bumiller wrote:
> On Wed, Apr 10, 2024 at 03:13:08PM +0200, Fabian Grünbichler wrote:
>> so that other nodes can query it and both block changes that would violate the
>> limits, and mark pools which are violating it currently accordingly.
>> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>>  PVE/Service/pvestatd.pm | 59 ++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 55 insertions(+), 4 deletions(-)
>> 
>> diff --git a/PVE/Service/pvestatd.pm b/PVE/Service/pvestatd.pm
>> index 2515120c6..d7e4755e9 100755
>> --- a/PVE/Service/pvestatd.pm
>> +++ b/PVE/Service/pvestatd.pm
>> @@ -231,7 +231,7 @@ sub auto_balloning {
>>  }
>>  
>>  sub update_qemu_status {
>> -    my ($status_cfg) = @_;
>> +    my ($status_cfg, $pool_membership, $pool_usage) = @_;
>>  
>>      my $ctime = time();
>>      my $vmstatus = PVE::QemuServer::vmstatus(undef, 1);
>> @@ -242,6 +242,21 @@ sub update_qemu_status {
>>      my $transactions = PVE::ExtMetric::transactions_start($status_cfg);
>>      foreach my $vmid (keys %$vmstatus) {
>>  	my $d = $vmstatus->{$vmid};
>> +
>> +	if (my $pool = $pool_membership->{$vmid}) {
>> +	    $pool_usage->{$pool}->{$vmid} = {
>> +		cpu => {
>> +		    config => ($d->{confcpus} // 0),
>> +		    run => ($d->{runcpus} // 0),
>> +		},
>> +		mem => {
>> +		    config => ($d->{confmem} // 0),
>> +		    run => ($d->{runmem} // 0),
>> +		},
> 
> I feel like it should be possible to build this hash given the `keys` in
> the limit hash... The `cpu-run/config` vs `{cpu}->{run}` vs `runcpu`
> naming feels a bit awkward to me.

not really, unless you want me to introduce a helper that is longer than
the hard-coded variant here?

I already have one for limit key -> usage hash (keys) for places where
we are mostly 'key agnostic' (i.e., PVE::GuestHelpers), but the vmstatus
hash has different keys again (see reply there) and those are only
relevant (in relation to the other limit/usage stuff) for the two subs
here, so if we extend the vmstatus return schema (e.g., with fields for
disk limits), then we'd need to extend the helper here anyway, just like
we'd need to extend the hard-coded variant, so I don't really see a
benefit?

adding a new limit "category" will require changes to:
- pve-access-control (for the user.cfg schema)
- qemu-server/pve-container (for actually providing the usage data via
  vmstatus, and checking the limits in all the places where current
  usage would change)
- pve-manager (for displaying/editing/.. and pvestatd conversion between
  vmstatus and usage, i.e., this part here)