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

2024-04-15 Thread Fabian Grünbichler
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 
>> ---
>>  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)


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


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

2024-04-11 Thread Wolfgang Bumiller
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 
> ---
>  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.

> + running => $d->{pid} ? 1 : 0,
> + };
> + }
> +
>   my $data;
>   my $status = $d->{qmpstatus} || $d->{status} || 'stopped';
>   my $template = $d->{template} ? $d->{template} : "0";
> @@ -263,6 +278,17 @@ sub update_qemu_status {
>  PVE::ExtMetric::transactions_finish($transactions);
>  }
>  
> +sub update_pool_usage {
> +my ($usage) = @_;
> +
> +my $ctime = time();
> +
> +# TODO? RRD and ExtMetric support here?
> +
> +my $new = { data => $usage, timestamp => $ctime };
> +PVE::Cluster::broadcast_node_kv('pool-usage', encode_json($new));
> +}
> +
>  sub remove_stale_lxc_consoles {
>  
>  my $vmstatus = PVE::LXC::vmstatus();
> @@ -440,7 +466,7 @@ sub rebalance_lxc_containers {
>  }
>  
>  sub update_lxc_status {
> -my ($status_cfg) = @_;
> +my ($status_cfg, $pool_membership, $pool_usage) = @_;
>  
>  my $ctime = time();
>  my $vmstatus = PVE::LXC::vmstatus();
> @@ -449,6 +475,21 @@ sub update_lxc_status {
>  
>  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),
> + },
> + running => $d->{status} eq 'running' ? 1 : 0,
> + };
> + }
> +
>   my $template = $d->{template} ? $d->{template} : "0";
>   my $data;
>   if ($d->{status} eq 'running') { # running
> @@ -540,6 +581,10 @@ sub update_status {
>  syslog('err', $err) if $err;
>  
>  my $status_cfg = PVE::Cluster::cfs_read_file('status.cfg');
> +my $user_cfg = PVE::Cluster::cfs_read_file('user.cfg');
> +my $pools = $user_cfg->{pools};
> +my $pool_membership = $user_cfg->{vms};
> +my $pool_usage = {};
>  
>  eval {
>   update_node_status($status_cfg);
> @@ -548,17 +593,23 @@ sub update_status {
>  syslog('err', "node status update error: $err") if $err;
>  
>  eval {
> - update_qemu_status($status_cfg);
> + update_qemu_status($status_cfg, $pool_membership, $pool_usage);
>  };
>  $err = $@;
>  syslog('err', "qemu status update error: $err") if $err;
>  
>  eval {
> - update_lxc_status($status_cfg);
> + update_lxc_status($status_cfg, $pool_membership, $pool_usage);
>  };
>  $err = $@;
>  syslog('err', "lxc status update error: $err") if $err;
>  
> +eval {
> + update_pool_usage($pool_usage);
> +};
> +$err =$@;
> +syslog('err', "pool usage status update error: $err") if $err;
> +
>  eval {
>   rebalance_lxc_containers();
>  };
> -- 
> 2.39.2


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