[pve-devel] [PATCH storage] Use a common find_free_diskname in all plugins

Fabian Grünbichler f.gruenbichler at proxmox.com
Mon Dec 9 14:12:07 CET 2019


seems like a good idea! not tested yes, but some comments/pitfalls inline..

On December 9, 2019 1:35 pm, Fabian Ebner wrote:
> The local versions of find_free_diskname retrieved the relevant disk list using
> plugin-specific code and called get_next_vm_diskname. We can use list_images
> instead to allow for a common interface and avoid having those similar methods.
> 
> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
> ---
> 
> I did not test for Glusterfs, non-thin LVM and ZFS over iSCSI. It seems to work
> for the other plugins.
> 
>  PVE/Storage/GlusterfsPlugin.pm | 15 ++-------------
>  PVE/Storage/LVMPlugin.pm       | 10 +---------
>  PVE/Storage/LvmThinPlugin.pm   |  4 ++--
>  PVE/Storage/Plugin.pm          | 21 ++++++++-------------
>  PVE/Storage/RBDPlugin.pm       | 27 ++-------------------------
>  PVE/Storage/ZFSPlugin.pm       |  2 +-
>  PVE/Storage/ZFSPoolPlugin.pm   | 16 +++-------------
>  7 files changed, 19 insertions(+), 76 deletions(-)
> 
> diff --git a/PVE/Storage/GlusterfsPlugin.pm b/PVE/Storage/GlusterfsPlugin.pm
> index b3e5553..2cf2da9 100644
> --- a/PVE/Storage/GlusterfsPlugin.pm
> +++ b/PVE/Storage/GlusterfsPlugin.pm
> @@ -160,17 +160,6 @@ sub parse_name_dir {
>      die "unable to parse volume filename '$name'\n";
>  }
>  
> -my $find_free_diskname = sub {
> -    my ($imgdir, $vmid, $fmt, $scfg) = @_;
> -
> -    my $disk_list = [];
> -
> -    my $dh = IO::Dir->new ($imgdir);
> -    @$disk_list = $dh->read() if defined($dh);
> -
> -    return PVE::Storage::Plugin::get_next_vm_diskname($disk_list, $imgdir, $vmid, $fmt, $scfg, 1);
> -};
> -
>  sub path {
>      my ($class, $scfg, $volname, $storeid, $snapname) = @_;
>  
> @@ -225,7 +214,7 @@ sub clone_image {
>  
>      mkpath $imagedir;
>  
> -    my $name = $find_free_diskname->($imagedir, $vmid, "qcow2", $scfg);
> +    my $name = $class->find_free_diskname($imagedir, $scfg, $vmid, "qcow2", 1);
>  
>      warn "clone $volname: $vtype, $name, $vmid to $name (base=../$basevmid/$basename)\n";
>  
> @@ -253,7 +242,7 @@ sub alloc_image {
>  
>      mkpath $imagedir;
>  
> -    $name = $find_free_diskname->($imagedir, $vmid, $fmt, $scfg) if !$name;
> +    $name = $class->find_free_diskname($imagedir, $scfg, $vmid, $fmt, 1) if !$name;
>  
>      my (undef, $tmpfmt) = parse_name_dir($name);
>  
> diff --git a/PVE/Storage/LVMPlugin.pm b/PVE/Storage/LVMPlugin.pm
> index c652e2a..13e6409 100644
> --- a/PVE/Storage/LVMPlugin.pm
> +++ b/PVE/Storage/LVMPlugin.pm
> @@ -303,14 +303,6 @@ sub clone_image {
>      die "can't clone images in lvm storage\n";
>  }
>  
> -sub lvm_find_free_diskname {
> -    my ($lvs, $vg, $storeid, $vmid, $scfg) = @_;
> -
> -    my $disk_list = [ keys %{$lvs->{$vg}} ];
> -
> -    return PVE::Storage::Plugin::get_next_vm_diskname($disk_list, $storeid, $vmid, undef, $scfg);
> -}
> -
>  sub lvcreate {
>      my ($vg, $name, $size, $tags) = @_;
>  
> @@ -345,7 +337,7 @@ sub alloc_image {
>  
>      die "not enough free space ($free < $size)\n" if $free < $size;
>  
> -    $name = lvm_find_free_diskname(lvm_list_volumes($vg), $vg, $storeid, $vmid, $scfg)
> +    $name = $class->find_free_diskname($storeid, $scfg, $vmid)

this now potentially skips untagged LVs, and thus return a volid "slot" 
that is actually used..

>  	if !$name;
>  
>      lvcreate($vg, $name, $size, ["pve-vm-$vmid"]);
> diff --git a/PVE/Storage/LvmThinPlugin.pm b/PVE/Storage/LvmThinPlugin.pm
> index aafc202..b00758d 100644
> --- a/PVE/Storage/LvmThinPlugin.pm
> +++ b/PVE/Storage/LvmThinPlugin.pm
> @@ -97,7 +97,7 @@ sub alloc_image {
>  
>      my $lvs = PVE::Storage::LVMPlugin::lvm_list_volumes($vg);
>  
> -    $name = PVE::Storage::LVMPlugin::lvm_find_free_diskname($lvs, $vg, $storeid, $vmid, $scfg)
> +    $name = $class->find_free_diskname($storeid, $scfg, $vmid)

same applies here, since it is derived.

>  	if !$name;
>  
>      my $cmd = ['/sbin/lvcreate', '-aly', '-V', "${size}k", '--name', $name,
> @@ -270,7 +270,7 @@ sub clone_image {
>  
>      my $lvs = PVE::Storage::LVMPlugin::lvm_list_volumes($vg);
>  
> -    my $name =  PVE::Storage::LVMPlugin::lvm_find_free_diskname($lvs, $vg, $storeid, $vmid, $scfg);
> +    my $name =  $class->find_free_diskname($storeid, $scfg, $vmid);

same

>  
>      my $cmd = ['/sbin/lvcreate', '-n', $name, '-prw', '-kn', '-s', $lv];
>      run_command($cmd, errmsg => "clone image '$lv' error");
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index d0f1df6..54aae41 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -559,7 +559,7 @@ sub get_next_vm_diskname {
>  
>      my $disk_ids = {};
>      foreach my $disk (@$disk_list) {
> -	my $disknum = $get_vm_disk_number->($disk, $scfg, $vmid, $suffix);
> +	my $disknum = $get_vm_disk_number->($disk->{volid}, $scfg, $vmid, $suffix);

this changes the interface of this method? previously, $disk_list was a 
list of 'disk names', whatever that means. now it's a list of 'disk 
objects', with the assumption that a 'volid' member is defined.

maybe 'find_free_diskname' could do the mapping for us, and we could 
drop this hunk? also see comment regarding RBDPlugin below..

>  	$disk_ids->{$disknum} = 1 if defined($disknum);
>      }
>  
> @@ -572,18 +572,13 @@ sub get_next_vm_diskname {
>      die "unable to allocate an image name for VM $vmid in storage '$storeid'\n"
>  }
>  
> -my $find_free_diskname = sub {
> -    my ($imgdir, $vmid, $fmt, $scfg) = @_;
> +sub find_free_diskname {
> +    my ($class, $storeid, $scfg, $vmid, $fmt, $add_fmt_suffix) = @_;
>  
> -    my $disk_list = [];
> +    my $disk_list = $class->list_images($storeid, $scfg, $vmid);
>  
> -    if (defined(my $dh = IO::Dir->new($imgdir))) {
> -	@$disk_list = $dh->read();
> -	$dh->close();
> -    }
> -
> -    return  get_next_vm_diskname($disk_list, $imgdir, $vmid, $fmt, $scfg, 1);
> -};
> +    return get_next_vm_diskname($disk_list, $storeid, $vmid, $fmt, $scfg, $add_fmt_suffix);
> +}
>  
>  sub clone_image {
>      my ($class, $scfg, $storeid, $volname, $vmid, $snap) = @_;
> @@ -607,7 +602,7 @@ sub clone_image {
>  
>      mkpath $imagedir;
>  
> -    my $name = $find_free_diskname->($imagedir, $vmid, "qcow2", $scfg);
> +    my $name = $class->find_free_diskname($imagedir, $scfg, $vmid, "qcow2", 1);
>  
>      warn "clone $volname: $vtype, $name, $vmid to $name (base=../$basevmid/$basename)\n";
>  
> @@ -639,7 +634,7 @@ sub alloc_image {
>  
>      mkpath $imagedir;
>  
> -    $name = $find_free_diskname->($imagedir, $vmid, $fmt, $scfg) if !$name;
> +    $name = $class->find_free_diskname($imagedir, $scfg, $vmid, $fmt, 1) if !$name;
>  
>      my (undef, $tmpfmt) = parse_name_dir($name);
>  
> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
> index 538d6bb..c552785 100644
> --- a/PVE/Storage/RBDPlugin.pm
> +++ b/PVE/Storage/RBDPlugin.pm
> @@ -362,29 +362,6 @@ sub path {
>      return ($path, $vmid, $vtype);
>  }
>  
> -my $find_free_diskname = sub {
> -    my ($storeid, $scfg, $vmid) = @_;
> -
> -    my $cmd = &$rbd_cmd($scfg, $storeid, 'ls');

this command and rbd_ls are not the same at all. 'rbd ls' is a single 
operation to list available images, it just returns their names. rbd_ls 
uses 'rbd ls -l', which does an 'rbd ls', and then proceeds to retrieve 
image information for each of the listed images, and can thus take way 
longer than our default 30s timeout!

since we can't reconstruct a volid from 'rbd ls' output (it's missing 
the base/linked clone information), we'd at least need to override 
find_free_diskname in this plugin.. but that would only be sanely 
possible if Plugin::get_next_vm_diskname keeps the old 
interface/$disk_list

> -    my $disk_list = [];
> -
> -    my $parser = sub {
> -	my $line = shift;
> -	if ($line =~ m/^(.*)$/) { # untaint
> -	    push @$disk_list, $1;
> -	}
> -    };
> -
> -    eval {
> -	run_rbd_command($cmd, errmsg => "rbd error", errfunc => sub {}, outfunc => $parser);
> -    };
> -    my $err = $@;
> -
> -    die $err if $err && $err !~ m/doesn't contain rbd images/;
> -
> -    return PVE::Storage::Plugin::get_next_vm_diskname($disk_list, $storeid, $vmid, undef, $scfg);
> -};
> -
>  sub create_base {
>      my ($class, $storeid, $scfg, $volname) = @_;
>  
> @@ -438,7 +415,7 @@ sub clone_image {
>      die "$volname is not a base image and snapname is not provided\n" 
>  	if !$isBase && !length($snapname);
>  
> -    my $name = $find_free_diskname->($storeid, $scfg, $vmid);
> +    my $name = $class->find_free_diskname($storeid, $scfg, $vmid);
>  
>      warn "clone $volname: $basename snapname $snap to $name\n";
>  
> @@ -469,7 +446,7 @@ sub alloc_image {
>      die "illegal name '$name' - should be 'vm-$vmid-*'\n"
>  	if  $name && $name !~ m/^vm-$vmid-/;
>  
> -    $name = $find_free_diskname->($storeid, $scfg, $vmid) if !$name;
> +    $name = $class->find_free_diskname($storeid, $scfg, $vmid) if !$name;
>  
>      my $cmd = &$rbd_cmd($scfg, $storeid, 'create', '--image-format' , 2, '--size', int(($size+1023)/1024), $name);
>      run_rbd_command($cmd, errmsg => "rbd create $name' error");
> diff --git a/PVE/Storage/ZFSPlugin.pm b/PVE/Storage/ZFSPlugin.pm
> index 8c6709c..383f0a0 100644
> --- a/PVE/Storage/ZFSPlugin.pm
> +++ b/PVE/Storage/ZFSPlugin.pm
> @@ -294,7 +294,7 @@ sub alloc_image {
>  
>      my $volname = $name;
>  
> -    $volname = $class->zfs_find_free_diskname($storeid, $scfg, $vmid, $fmt) if !$volname;
> +    $volname = $class->find_free_diskname($storeid, $scfg, $vmid, $fmt) if !$volname;
>      
>      $class->zfs_create_zvol($scfg, $volname, $size);
>   
> diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
> index 456fb40..12dc2be 100644
> --- a/PVE/Storage/ZFSPoolPlugin.pm
> +++ b/PVE/Storage/ZFSPoolPlugin.pm
> @@ -240,7 +240,7 @@ sub alloc_image {
>  
>  	die "illegal name '$volname' - should be 'vm-$vmid-*'\n"
>  	    if $volname && $volname !~ m/^vm-$vmid-/;
> -	$volname = $class->zfs_find_free_diskname($storeid, $scfg, $vmid, $fmt)
> +	$volname = $class->find_free_diskname($storeid, $scfg, $vmid, $fmt)
>  	    if !$volname;
>  
>  	$class->zfs_create_zvol($scfg, $volname, $size);
> @@ -250,7 +250,7 @@ sub alloc_image {
>  
>  	die "illegal name '$volname' - should be 'subvol-$vmid-*'\n"
>  	    if $volname && $volname !~ m/^subvol-$vmid-/;
> -	$volname = $class->zfs_find_free_diskname($storeid, $scfg, $vmid, $fmt)
> +	$volname = $class->find_free_diskname($storeid, $scfg, $vmid, $fmt)
>  	    if !$volname;
>  
>  	die "illegal name '$volname' - should be 'subvol-$vmid-*'\n"
> @@ -423,16 +423,6 @@ sub zfs_list_zvol {
>      return $list;
>  }
>  
> -sub zfs_find_free_diskname {
> -    my ($class, $storeid, $scfg, $vmid, $format) = @_;
> -
> -    my $volumes = $class->zfs_list_zvol($scfg);
> -    my $dat = $volumes->{$scfg->{pool}};
> -
> -    my $disk_list = [ keys %$dat ];
> -    return PVE::Storage::Plugin::get_next_vm_diskname($disk_list, $storeid, $vmid, $format, $scfg);
> -}
> -
>  sub zfs_get_latest_snapshot {
>      my ($class, $scfg, $volname) = @_;
>  
> @@ -612,7 +602,7 @@ sub clone_image {
>  
>      die "clone_image only works on base images\n" if !$isBase;
>  
> -    my $name = $class->zfs_find_free_diskname($storeid, $scfg, $vmid, $format);
> +    my $name = $class->find_free_diskname($storeid, $scfg, $vmid, $format);
>  
>      if ($format eq 'subvol') {
>  	my $size = $class->zfs_request($scfg, undef, 'list', '-H', '-o', 'refquota', "$scfg->{pool}/$basename");
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 



More information about the pve-devel mailing list