[pve-devel] [PATCH qemu-server] api: vmlist: add plugged cpu count to vmlist table

[pve-devel] [PATCH qemu-server] api: vmlist: add plugged cpu count to vmlist table

[Daniel Kral]

Includes the maximum amount of cpu cores in the vmlist as it
semantically fits in with the other properties. This allows for a more
comfortable view of a node's VM configured CPU core counts in general
and by users of "pvereport" without calculating each core count manually
with the respective VM's configuration files.

Suggested-by: Hannes Dürr <h.duerr@proxmox.com>
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 PVE/CLI/qm.pm | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index 4214a7ca..389a97d6 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -1124,12 +1124,13 @@ our $cmddef = {
 	my $vmlist = shift;
 	exit 0 if (!scalar(@$vmlist));
 
-	printf "%10s %-20s %-10s %-10s %12s %-10s\n",
-	qw(VMID NAME STATUS MEM(MB) BOOTDISK(GB) PID);
+	printf "%10s %-20s %-10s %-5s %10s %12s %-10s\n",
+	qw(VMID NAME STATUS CORES MEM(MB) BOOTDISK(GB) PID);
 
 	foreach my $rec (sort { $a->{vmid} <=> $b->{vmid} } @$vmlist) {
-	    printf "%10s %-20s %-10s %-10s %12.2f %-10s\n", $rec->{vmid}, $rec->{name},
+	    printf "%10s %-20s %-10s %-5s %10s %12.2f %-10s\n", $rec->{vmid}, $rec->{name},
 	        $rec->{qmpstatus} || $rec->{status},
+		$rec->{cpus} || 0,
 	        ($rec->{maxmem} || 0)/(1024*1024),
 	        ($rec->{maxdisk} || 0)/(1024*1024*1024),
 	        $rec->{pid} || 0;
-- 
2.39.5



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

Re: [pve-devel] [PATCH qemu-server] api: vmlist: add plugged cpu count to vmlist table

[Daniel Herzig]

I just tested this patch and think it gives very valuable information.

One thing I noticed, more by chance than intentional -- if the host has
a lower cpu-count than the count configured for the VM, the stats will
not be updated. E.g. my pve-host only has 2 cores available and if I
configure 4 cores for the VM, `qm list` will still show 2 cores,
although the VM is actually configured in an unbootable state.

Do you think it's possible to render something like '$NUMBER:
too_many_for_this_host' (or something similar) in this edge case?


Daniel Kral <d.kral@proxmox.com> writes:

> Includes the maximum amount of cpu cores in the vmlist as it
> semantically fits in with the other properties. This allows for a more
> comfortable view of a node's VM configured CPU core counts in general
> and by users of "pvereport" without calculating each core count manually
> with the respective VM's configuration files.
>
> Suggested-by: Hannes Dürr <h.duerr@proxmox.com>
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
>  PVE/CLI/qm.pm | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
> index 4214a7ca..389a97d6 100755
> --- a/PVE/CLI/qm.pm
> +++ b/PVE/CLI/qm.pm
> @@ -1124,12 +1124,13 @@ our $cmddef = {
>  	my $vmlist = shift;
>  	exit 0 if (!scalar(@$vmlist));
>  
> -	printf "%10s %-20s %-10s %-10s %12s %-10s\n",
> -	qw(VMID NAME STATUS MEM(MB) BOOTDISK(GB) PID);
> +	printf "%10s %-20s %-10s %-5s %10s %12s %-10s\n",
> +	qw(VMID NAME STATUS CORES MEM(MB) BOOTDISK(GB) PID);
>  
>  	foreach my $rec (sort { $a->{vmid} <=> $b->{vmid} } @$vmlist) {
> -	    printf "%10s %-20s %-10s %-10s %12.2f %-10s\n", $rec->{vmid}, $rec->{name},
> +	    printf "%10s %-20s %-10s %-5s %10s %12.2f %-10s\n", $rec->{vmid}, $rec->{name},
>  	        $rec->{qmpstatus} || $rec->{status},
> +		$rec->{cpus} || 0,
>  	        ($rec->{maxmem} || 0)/(1024*1024),
>  	        ($rec->{maxdisk} || 0)/(1024*1024*1024),
>  	        $rec->{pid} || 0;


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

Re: [pve-devel] [PATCH qemu-server] api: vmlist: add plugged cpu count to vmlist table

[Daniel Kral]

On 1/13/25 10:45, Daniel Herzig wrote:
> I just tested this patch and think it gives very valuable information.

Thanks for taking a look at this and sorry for taking some time to get 
back to this.

> 
> One thing I noticed, more by chance than intentional -- if the host has
> a lower cpu-count than the count configured for the VM, the stats will
> not be updated. E.g. my pve-host only has 2 cores available and if I
> configure 4 cores for the VM, `qm list` will still show 2 cores,
> although the VM is actually configured in an unbootable state.
> 
> Do you think it's possible to render something like '$NUMBER:
> too_many_for_this_host' (or something similar) in this edge case?

Thanks for pointing this out!

That's interesting, in `vmstatus` we're clamping the `$cpus` property to 
the maximum available cpu core count if `$sockets * $cores` above that. 
But if `$vcpus` is set to something greater than that, it won't be 
clamped to the maximum amount.

I've discussed this off-list with @Hannes Dürr: We've argued that this 
would overbloat the `qm list` as `vmstatus` returns the clamped `$cpus` 
count and we'd need to duplicate the unclamped calculation, especially 
as you also pointed out that the VM is in an unbootable state.

What do you think about fixing the problem from the other side: I could 
do a follow up where it's not possible to actively assign a VM more 
resources than currently available, since it would not boot anyway? I 
think there's a negligible amount of users who'd want to configure more 
resources and only later upgrade to the required primary memory or a 
heavier CPU (wrt to their core count).




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

Re: [pve-devel] [PATCH qemu-server] api: vmlist: add plugged cpu count to vmlist table

[Thomas Lamprecht]

Am 17.01.25 um 16:10 schrieb Daniel Kral:
> What do you think about fixing the problem from the other side: I could 
> do a follow up where it's not possible to actively assign a VM more 
> resources than currently available, since it would not boot anyway? I 
> think there's a negligible amount of users who'd want to configure more 
> resources and only later upgrade to the required primary memory or a 
> heavier CPU (wrt to their core count).

Such a VM might not boot on it's current node, but it could boot on
another node with more CPU cores. Often not a big issue, but we avoid
such restrictions as they often create more friction without providing
that much help – a UI only hint in case of setting more cores than the
current node supports might be better here.

Maybe best to just ignore the glitch in the output for now, or indeed
duplicate the calculation, or look into why vmstatus does it that way
and see if it could be unclamped there (or a new property is warranted).

btw. you use the "api" prefix for this patch, but it should be "cli",
as this is not changing the (REST) API in any way.


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

Re: [pve-devel] [PATCH qemu-server] api: vmlist: add plugged cpu count to vmlist table

[Daniel Kral]

On 1/17/25 16:27, Thomas Lamprecht wrote:
> Am 17.01.25 um 16:10 schrieb Daniel Kral:
>> What do you think about fixing the problem from the other side: I could
>> do a follow up where it's not possible to actively assign a VM more
>> resources than currently available, since it would not boot anyway? I
>> think there's a negligible amount of users who'd want to configure more
>> resources and only later upgrade to the required primary memory or a
>> heavier CPU (wrt to their core count).
> 
> Such a VM might not boot on it's current node, but it could boot on
> another node with more CPU cores. Often not a big issue, but we avoid
> such restrictions as they often create more friction without providing
> that much help – a UI only hint in case of setting more cores than the
> current node supports might be better here.

Thanks for the insight, that makes sense and I haven't thought about it 
in the cluster context, I'll keep that in mind for future changes!

> 
> Maybe best to just ignore the glitch in the output for now, or indeed
> duplicate the calculation, or look into why vmstatus does it that way
> and see if it could be unclamped there (or a new property is warranted).
> 
> btw. you use the "api" prefix for this patch, but it should be "cli",
> as this is not changing the (REST) API in any way.

Thanks for pointing that out, I'll change that for a v2! I'll also take 
a second closer look why it has been clamped there and if it can be 
unclamped in a separate patch for v2.


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