[pve-devel] [PATCH] qemuserver : vm_status : add extended stats (disks, nics, memory) V2

[Alexandre DERUMIER]

>>Can't we use $d, like: 
>>
>>$d->{nics}->{$dev}->{netout} = $netdev->{$dev}->{receive}; 
>>$d->{nics}->{$dev}->{netin} = $netdev->{$dev}->{transmit}; 

yes sure, I'll change that


>>Can we do above outside the callback (only once)? Or can we avoid that? 
for storagecfg it can be usefull,

for qemu config, we need to read it for each vm in the loop.
So doing outside callback doesn't help. (and we need to pass it in all recursive callbacks)



>>why do you want to skip CDROMS? 
Because this patch series use volumename as key, I don't want to flood stats 
when changing cdrom volume.
Anyway, who's care about cdrom stats ?



>>I never wanted to have such complex code here. 
>>
>>$blockstat->{device} =~ s/drive-//; 
>>$res->{$vmid}->{blockstat}->{$blockstat->{device}} = $blockstat->{stats}; 
>>
>>would be much simpler? 

Oh sorry, I think you wanted that (disk volume name).
I can change this.
(So I don't need parsing qemu && storage cfg, and cdrom from previous comment)



----- Mail original -----
De: "dietmar" <dietmar at proxmox.com>
À: "aderumier" <aderumier at odiso.com>, "pve-devel" <pve-devel at pve.proxmox.com>
Envoyé: Mardi 16 Juin 2015 12:11:21
Objet: Re: [pve-devel] [PATCH] qemuserver : vm_status : add extended stats (disks, nics, memory) V2

comments inline 

> On June 16, 2015 at 11:18 AM Alexandre Derumier <aderumier at odiso.com> wrote: 
> 
> 
> Add extended stats results for each nics,disks and memory on full stats mode 
> only. 
> 
> Signed-off-by: Alexandre Derumier <aderumier at odiso.com> 
> --- 
> PVE/QemuServer.pm | 22 ++++++++++++++++++++++ 
> 1 file changed, 22 insertions(+) 
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm 
> index e89308c..892b051 100644 
> --- a/PVE/QemuServer.pm 
> +++ b/PVE/QemuServer.pm 
> @@ -2347,6 +2347,12 @@ sub vmstatus { 
> 
> $d->{netout} += $netdev->{$dev}->{receive}; 
> $d->{netin} += $netdev->{$dev}->{transmit}; 
> + 
> + if ($full) { 
> + $res->{$vmid}->{nics}->{$dev}->{netout} = $netdev->{$dev}->{receive}; 
> + $res->{$vmid}->{nics}->{$dev}->{netin} = $netdev->{$dev}->{transmit}; 
> + } 
> + 

Can't we use $d, like: 

$d->{nics}->{$dev}->{netout} = $netdev->{$dev}->{receive}; 
$d->{nics}->{$dev}->{netin} = $netdev->{$dev}->{transmit}; 

> } 
> 
> my $ctime = gettimeofday; 
> @@ -2415,6 +2421,7 @@ sub vmstatus { 
> $d->{freemem} = $info->{free_mem}; 
> } 
> 
> + $d->{ballooninfo} = $info; 
> }; 
> 
> my $blockstatscb = sub { 
> @@ -2422,9 +2429,24 @@ sub vmstatus { 
> my $data = $resp->{'return'} || []; 
> my $totalrdbytes = 0; 
> my $totalwrbytes = 0; 
> + 
> + my $cfspath = cfs_config_path($vmid); 
> + my $conf = PVE::Cluster::cfs_read_file($cfspath); 
> + my $storecfg = PVE::Storage::config(); 
> + 

Can we do above outside the callback (only once)? Or can we avoid that? 

> for my $blockstat (@$data) { 
> $totalrdbytes = $totalrdbytes + $blockstat->{stats}->{rd_bytes}; 
> $totalwrbytes = $totalwrbytes + $blockstat->{stats}->{wr_bytes}; 
> + 
> + $blockstat->{device} =~ s/drive-//; 
> + my $drive = parse_drive($blockstat->{device}, 
> $conf->{$blockstat->{device}}); 
> + next if PVE::QemuServer::drive_is_cdrom($drive); 

why do you want to skip CDROMS? 

> + my $volid = $drive->{file}; 
> + my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid); 
> + my ($vtype, $name, $owner) = PVE::Storage::parse_volname($storecfg, 
> $volid); 
> + 
> + $res->{$vmid}->{blockstat}->{$storeid}->{$name} = $blockstat->{stats}; 
> + 

I never wanted to have such complex code here. 

$blockstat->{device} =~ s/drive-//; 
$res->{$vmid}->{blockstat}->{$blockstat->{device}} = $blockstat->{stats}; 

would be much simpler?