[pve-devel] applied: [PATCH storage v2 1/2] fix #1895: use json for 'rbd ls -l' and 'rbd info'

[Thomas Lamprecht]

On 9/6/18 11:35 AM, Dominik Csapak wrote:
> since ceph changed the plain output format for 12.2.8
> we have to change the code anyway, and when were at it,
> we can change it to the (hopefully) more robust json output
> 
> Co-authored-by: Alwin Antreich <a.antreich at proxmox.com>
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>

applied, thanks!

added a followup on top, cleaning up the unused variables in rbd_volume_info
and some other smaller nits of mine.

> ---
> changes from v1:
> * incorporated the change from alwins patch [1]
> * incorporated thomas feedback:
>  * rename r/l to more meaningful names
>  * check for output of $raw
>  * return hash slice instead of saving the content again
> * refactor parent image name generation
> 
>  PVE/Storage/RBDPlugin.pm | 78 ++++++++++++++++++++++++++----------------------
>  1 file changed, 43 insertions(+), 35 deletions(-)
> 
> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
> index be88ad7..6c0e617 100644
> --- a/PVE/Storage/RBDPlugin.pm
> +++ b/PVE/Storage/RBDPlugin.pm
> @@ -9,6 +9,7 @@ use PVE::Storage::Plugin;
>  use PVE::JSONSchema qw(get_standard_option);
>  use PVE::RADOS;
>  use PVE::Storage::CephTools;
> +use JSON;
>  
>  use base qw(PVE::Storage::Plugin);
>  
> @@ -19,6 +20,12 @@ my $rbd_unittobytes = {
>      "T"  => 1024*1024*1024*1024,
>  };
>  
> +my $get_parent_image_name = sub {
> +    my ($parent) = @_;
> +    return undef if !$parent;
> +    return $parent->{image} . "@" . $parent->{snapshot};
> +};
> +
>  my $add_pool_to_disk = sub {
>      my ($scfg, $disk) = @_;
>  
> @@ -82,7 +89,7 @@ my $krbd_feature_disable = sub {
>      my $krbd_feature_blacklist = ['deep-flatten', 'fast-diff', 'object-map', 'exclusive-lock'];
>      my (undef, undef, undef, undef, $features) = rbd_volume_info($scfg, $storeid, $name);
>  
> -    my $active_features = { map { $_ => 1 } PVE::Tools::split_list($features)};
> +    my $active_features = { map { $_ => 1 } $features };
>      my $incompatible_features = join(',', grep { %$active_features{$_} } @$krbd_feature_blacklist);
>  
>      if ($incompatible_features) {
> @@ -153,25 +160,13 @@ sub run_rbd_command {
>  sub rbd_ls {
>      my ($scfg, $storeid) = @_;
>  
> -    my $cmd = &$rbd_cmd($scfg, $storeid, 'ls', '-l');
> +    my $cmd = &$rbd_cmd($scfg, $storeid, 'ls', '-l', '--format', 'json');
>      my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
>  
> -    my $list = {};
> +    my $raw = '';
>  
>      my $parser = sub {
> -	my $line = shift;
> -
> -	if ($line =~  m/^((vm|base)-(\d+)-\S+)\s+(\d+)(k|M|G|T)\s((\S+)\/((vm|base)-\d+-\S+@\S+))?/) {
> -	    my ($image, $owner, $size, $unit, $parent) = ($1, $3, $4, $5, $8);
> -	    return if $image =~ /@/; #skip snapshots
> -
> -	    $list->{$pool}->{$image} = {
> -		name => $image,
> -		size => $size*$rbd_unittobytes->{$unit},
> -		parent => $parent,
> -		vmid => $owner
> -	    };
> -	}
> +	$raw .= shift;
>      };
>  
>      eval {
> @@ -180,7 +175,26 @@ sub rbd_ls {
>      my $err = $@;
>  
>      die $err if $err && $err !~ m/doesn't contain rbd images/ ;
> -  
> +
> +    my $result = $raw ne '' ? JSON::decode_json($raw) : [];
> +
> +    my $list = {};
> +
> +    foreach my $el (@$result) {
> +	next if defined($el->{snapshot});
> +
> +	my $image = $el->{image};
> +
> +	my ($owner) = $image =~ m/^(?:vm|base)-(\d+)-/;
> +
> +	$list->{$pool}->{$image} = {
> +	    name => $image,
> +	    size => $el->{size},
> +	    parent => $get_parent_image_name->($el->{parent}),
> +	    vmid => $owner
> +	};
> +    }
> +
>      return $list;
>  }
>  
> @@ -189,38 +203,32 @@ sub rbd_volume_info {
>  
>      my $cmd = undef;
>  
> +    my @options = ('info', $volname, '--format', 'json');
>      if($snap){
> -       $cmd = &$rbd_cmd($scfg, $storeid, 'info', $volname, '--snap', $snap);
> -    }else{
> -       $cmd = &$rbd_cmd($scfg, $storeid, 'info', $volname);
> +	push @options, '--snap', $snap;
>      }
>  
> +    $cmd = &$rbd_cmd($scfg, $storeid, @options);
> +
>      my $size = undef;
>      my $parent = undef;
>      my $format = undef;
>      my $protected = undef;
>      my $features = undef;
>  

removed those unused variables

> +    my $raw = '';
>      my $parser = sub {
> -	my $line = shift;
> -
> -	if ($line =~ m/size (\d+) (k|M|G|T)B in (\d+) objects/) {
> -	    $size = $1 * $rbd_unittobytes->{$2} if ($1);
> -	} elsif ($line =~ m/parent:\s(\S+)\/(\S+)/) {
> -	    $parent = $2;
> -	} elsif ($line =~ m/format:\s(\d+)/) {
> -	    $format = $1;
> -	} elsif ($line =~ m/protected:\s(\S+)/) {
> -	    $protected = 1 if $1 eq "True";
> -	} elsif ($line =~ m/features:\s(.+)/) {
> -	    $features = $1;
> -	}
> -
> +	$raw .= shift;
>      };
>  
>      run_rbd_command($cmd, errmsg => "rbd error", errfunc => sub {}, outfunc => $parser);
> +    my $volume = $raw ne '' ? JSON::decode_json($raw) : {};
> +    $volume->{parent} = $get_parent_image_name->($volume->{parent});
> +    if (defined($volume->{protected})) {
> +	$volume->{protected} = $volume->{protected} eq "true" ? 1 : undef;
> +    }
>  
> -    return ($size, $parent, $format, $protected, $features);
> +    return $volume->@{qw(size parent format protected features)};
>  }
>  
>  # Configuration
>