[pve-devel] [PATCH v2 manager 01/12] Broadcast supported CPU flags

[Thomas Lamprecht]

On 9/30/19 12:58 PM, Stefan Reiter wrote:
> pvestatd will check if the KVM version has changed using
> kvm_user_version (which automatically clears its cache if QEMU/KVM
> updates), and if it has, query supported CPU flags and broadcast them as
> a key-value pair to the cluster.
> 
> Detects value regressions and handles them gracefully (i.e. if
> query_supported_cpu_flags fails, we try to retain the previously
> detected flags).
> 
> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
> ---
> 
> v1 -> v2:
> * broadcast directly in update_supported_cpuflags
> * use kvm_user_version to determine when to re-query CPU flags
> * don't broadcast flags when unchanged or empty
> 
> 
>  PVE/Service/pvestatd.pm | 40 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/Service/pvestatd.pm b/PVE/Service/pvestatd.pm
> index bad1b73d..5ac97c98 100755
> --- a/PVE/Service/pvestatd.pm
> +++ b/PVE/Service/pvestatd.pm
> @@ -78,6 +78,40 @@ sub hup {
>      $restart_request = 1;
>  }
>  
> +my $last_kvm_version = '';
> +
> +sub update_supported_cpuflags {
> +    my $kvm_version = PVE::QemuServer::kvm_user_version();
> +
> +    # only update when QEMU/KVM version has changed, as that is the only reason
> +    # why flags could change without restarting pvestatd
> +    return if $last_kvm_version eq $kvm_version;
> +    $last_kvm_version = $kvm_version;
> +
> +    my $supported_cpuflags;
> +
> +    eval {
> +	$supported_cpuflags = join(" ", @{PVE::QemuServer::query_supported_cpu_flags()});
> +    };

could be written as:

my $supported_cpuflags = eval { join(" ", @{PVE::QemuServer::query_supported_cpu_flags()}) };

> +    warn $@ if $@;
> +
> +    # detect regression
> +    if (!$supported_cpuflags || $supported_cpuflags eq '') {

'' is already falsy in perl so this can be simplified to
if (!$supported_cpuflags) {

> +	my $prev_cpuflags = PVE::Cluster::get_node_kv('cpuflags', $nodename)->{$nodename};

does it really make sense to re-use the old ones?? 
Would it be better to have a sane fallback in query_supported_cpu_flags?

> +	if ($prev_cpuflags && $prev_cpuflags ne '') {

same above

> +	    warn "CPU flag detection failed, using old values\n";
> +	} else {
> +	    warn "CPU flag detection failed and no previous values found\n";
> +	}
> +
> +	# either flags are already in kv store, so no need to re-broadcast,
> +	# or we don't have valid flags, and no point broadcasting empty string
> +	return;

nit: while the same `return undef;` could be a bit more explicit

> +    }
> +
> +    PVE::Cluster::broadcast_node_kv('cpuflags', $supported_cpuflags);
> +}
> +
>  my $generate_rrd_string = sub {
>      my ($data) = @_;
>  
> @@ -97,7 +131,9 @@ sub update_node_status {
>  
>      my $cpuinfo = PVE::ProcFSTools::read_cpuinfo();
>  
> -    my $maxcpu = $cpuinfo->{cpus}; 
> +    my $maxcpu = $cpuinfo->{cpus};
> +
> +    update_supported_cpuflags();
>  
>      my $subinfo = PVE::INotify::read_file('subscription');
>      my $sublevel = $subinfo->{level} || '';
> @@ -110,7 +146,7 @@ sub update_node_status {
>  	$netin += $netdev->{$dev}->{receive};
>  	$netout += $netdev->{$dev}->{transmit};
>      }
> - 
> +
>      my $meminfo = PVE::ProcFSTools::read_meminfo();
>  
>      my $dinfo = df('/', 1);     # output is bytes
>