[pve-devel] [PATCH storage 2/2] refactor finding next diskname for all plugins

Thomas Lamprecht t.lamprecht at proxmox.com
Fri Aug 24 17:01:32 CEST 2018


On 8/22/18 3:42 PM, Stoiko Ivanov wrote:
> Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
> ---
>  PVE/Storage/DRBDPlugin.pm      | 15 ++-------------
>  PVE/Storage/GlusterfsPlugin.pm | 27 +++++++++------------------
>  PVE/Storage/LVMPlugin.pm       | 20 ++++----------------
>  PVE/Storage/LvmThinPlugin.pm   |  4 ++--
>  PVE/Storage/Plugin.pm          | 21 ++++++---------------
>  PVE/Storage/RBDPlugin.pm       | 15 +++------------
>  PVE/Storage/SheepdogPlugin.pm  | 17 ++---------------
>  PVE/Storage/ZFSPoolPlugin.pm   | 16 ++--------------
>  8 files changed, 30 insertions(+), 105 deletions(-)
> 
> diff --git a/PVE/Storage/DRBDPlugin.pm b/PVE/Storage/DRBDPlugin.pm
> index e0c8260..69be9fd 100644
> --- a/PVE/Storage/DRBDPlugin.pm
> +++ b/PVE/Storage/DRBDPlugin.pm
> @@ -177,21 +177,10 @@ sub alloc_image {
>  
>      my $hdl = connect_drbdmanage_service();
>      my $volumes = drbd_list_volumes($hdl);
> +    my $disk_list = [ keys(%$volumes) ];
>  
>      die "volume '$name' already exists\n" if defined($name) && $volumes->{$name};
> -    
> -    if (!defined($name)) {	
> -	for (my $i = 1; $i < 100; $i++) {
> -	    my $tn = "vm-$vmid-disk-$i";
> -	    if (!defined ($volumes->{$tn})) {
> -		$name = $tn;
> -		last;
> -	    }
> -	}
> -    }
> -
> -    die "unable to allocate an image name for VM $vmid in storage '$storeid'\n"
> -	if !defined($name);
> +    $name //= PVE::Storage::Plugin::get_next_vm_diskname($disk_list, $storeid, $vmid, undef, $scfg);
>  
>      my ($rc, $res) = $hdl->create_resource($name, {});
>      check_drbd_res($rc);
> diff --git a/PVE/Storage/GlusterfsPlugin.pm b/PVE/Storage/GlusterfsPlugin.pm
> index f3aa030..284868b 100644
> --- a/PVE/Storage/GlusterfsPlugin.pm
> +++ b/PVE/Storage/GlusterfsPlugin.pm
> @@ -162,23 +162,14 @@ sub parse_name_dir {
>  }
>  
>  my $find_free_diskname = sub {
> -    my ($imgdir, $vmid, $fmt) = @_;
> -
> -    my $disk_ids = {};
> -    PVE::Tools::dir_glob_foreach($imgdir,
> -                                 qr!(vm|base)-$vmid-disk-(\d+)\..*!,
> -                                 sub {
> -                                     my ($fn, $type, $disk) = @_;
> -                                     $disk_ids->{$disk} = 1;
> -                                 });
> -
> -    for (my $i = 1; $i < 100; $i++) {
> -        if (!$disk_ids->{$i}) {
> -            return "vm-$vmid-disk-$i.$fmt";
> -        }
> -    }
> +    my ($imgdir, $vmid, $fmt, $scfg) = @_;
> +
> +    my $disk_list = [];
> +
> +    my $dh = IO::Dir->new ($imgdir);
> +    @$disk_list = $dh->read() if defined($dh);
>  
> -    die "unable to allocate a new image name for VM $vmid in '$imgdir'\n";
> +    return PVE::Storage::Plugin::get_next_vm_diskname($disk_list, $imgdir, $vmid, $fmt, $scfg, 1);
>  };
>  
>  sub path {
> @@ -235,7 +226,7 @@ sub clone_image {
>  
>      mkpath $imagedir;
>  
> -    my $name = &$find_free_diskname($imagedir, $vmid, "qcow2");
> +    my $name = &$find_free_diskname($imagedir, $vmid, "qcow2", $scfg);

nit and not too important:
maybe transform this and all other ones to $code->() syntax if it gets
touched anyway?

>  
>      warn "clone $volname: $vtype, $name, $vmid to $name (base=../$basevmid/$basename)\n";
>  
> @@ -263,7 +254,7 @@ sub alloc_image {
>  
>      mkpath $imagedir;
>  
> -    $name = &$find_free_diskname($imagedir, $vmid, $fmt) if !$name;
> +    $name = &$find_free_diskname($imagedir, $vmid, $fmt, $scfg) if !$name;
>  
>      my (undef, $tmpfmt) = parse_name_dir($name);
>  
> diff --git a/PVE/Storage/LVMPlugin.pm b/PVE/Storage/LVMPlugin.pm
> index 19bac55..a638f0e 100644
> --- a/PVE/Storage/LVMPlugin.pm
> +++ b/PVE/Storage/LVMPlugin.pm
> @@ -289,25 +289,13 @@ sub clone_image {
>  }
>  
>  sub lvm_find_free_diskname {
> -    my ($lvs, $vg, $storeid, $vmid) = @_;
> +    my ($lvs, $vg, $storeid, $vmid, $scfg) = @_;
>  
>      my $name;
           ^^^^
unused variable (it was already previously)

>  
> -    my $disk_ids = {};
> -    my @vols = keys(%{$lvs->{$vg}});
> -
> -    foreach my $vol (@vols) {
> -	if ($vol =~ m/(vm|base)-\Q$vmid\E-disk-(\d+)/){
> -	    $disk_ids->{$2} = 1;
> -	}
> -    }
> -
> -    for (my $i = 1; $i < 100; $i++) {
> -	return "vm-$vmid-disk-$i" if !$disk_ids->{$i};
> -    }
> -
> -    die "unable to allocate an image name for ID $vmid in storage '$storeid'\n";
> +    my $disk_list = [ keys(%{$lvs->{$vg}}) ];

maybe remove the parenthesis from keys to make it look less like a
special chars ABC soup :)

>  
> +    return PVE::Storage::Plugin::get_next_vm_diskname($disk_list, $storeid, $vmid, undef, $scfg);
>  }
>  
>  sub alloc_image {
> @@ -328,7 +316,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)
> +    $name = lvm_find_free_diskname(lvm_list_volumes($vg), $vg, $storeid, $vmid, $scfg)
>  	if !$name;
>  
>      my $cmd = ['/sbin/lvcreate', '-aly', '--addtag', "pve-vm-$vmid", '--size', "${size}k", '--name', $name, $vg];
> diff --git a/PVE/Storage/LvmThinPlugin.pm b/PVE/Storage/LvmThinPlugin.pm
> index d08eb77..122fb37 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)
> +    $name = PVE::Storage::LVMPlugin::lvm_find_free_diskname($lvs, $vg, $storeid, $vmid, $scfg)
>  	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);
> +    my $name =  PVE::Storage::LVMPlugin::lvm_find_free_diskname($lvs, $vg, $storeid, $vmid, $scfg);
>  
>      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 e9fe4ec..7ff0879 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -555,23 +555,14 @@ sub get_next_vm_diskname {
>  };
>  
>  my $find_free_diskname = sub {
> -    my ($imgdir, $vmid, $fmt) = @_;
> +    my ($imgdir, $vmid, $fmt, $scfg) = @_;
>  
> -    my $disk_ids = {};
> -    PVE::Tools::dir_glob_foreach($imgdir,
> -				 qr!(vm|base)-$vmid-disk-(\d+)\..*!,
> -				 sub {
> -				     my ($fn, $type, $disk) = @_;
> -				     $disk_ids->{$disk} = 1;
> -				 });
> +    my $disk_list = [];
>  
> -    for (my $i = 1; $i < 100; $i++) {
> -	if (!$disk_ids->{$i}) {
> -	    return "vm-$vmid-disk-$i.$fmt";
> -	}
> -    }
> +    my $dh = IO::Dir->new ($imgdir);
> +    @$disk_list = $dh->read() if defined($dh);
>  
> -    die "unable to allocate a new image name for VM $vmid in '$imgdir'\n";
> +    return  get_next_vm_diskname($disk_list, $imgdir, $vmid, $fmt, $scfg, 1);
>  };
>  
>  sub clone_image {
> @@ -628,7 +619,7 @@ sub alloc_image {
>  
>      mkpath $imagedir;
>  
> -    $name = &$find_free_diskname($imagedir, $vmid, $fmt) if !$name;
> +    $name = &$find_free_diskname($imagedir, $vmid, $fmt, $scfg) if !$name;
>  
>      my (undef, $tmpfmt) = parse_name_dir($name);
>  
> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
> index be88ad7..74b94d1 100644
> --- a/PVE/Storage/RBDPlugin.pm
> +++ b/PVE/Storage/RBDPlugin.pm
> @@ -330,14 +330,11 @@ my $find_free_diskname = sub {
>      my ($storeid, $scfg, $vmid) = @_;
>  
>      my $cmd = &$rbd_cmd($scfg, $storeid, 'ls');
> -    my $disk_ids = {};
> +    my $disk_list = [];
>  
>      my $parser = sub {
>  	my $line = shift;
> -
> -	if ($line =~  m/^(vm|base)-\Q$vmid\E+-disk-(\d+)$/) {
> -	    $disk_ids->{$2} = 1;
> -	}
> +	push @$disk_list, $line;
>      };
>  
>      eval {
> @@ -347,14 +344,8 @@ my $find_free_diskname = sub {
>  
>      die $err if $err && $err !~ m/doesn't contain rbd images/;
>  
> -    #fix: can we search in $rbd hash key with a regex to find (vm|base) ?
> -    for (my $i = 1; $i < 100; $i++) {
> -        if (!$disk_ids->{$i}) {
> -            return "vm-$vmid-disk-$i";
> -        }
> -    }
> +    return PVE::Storage::Plugin::get_next_vm_diskname($disk_list, $storeid, $vmid, undef, $scfg);
>  

also remove this empty line?
> -    die "unable to allocate an image name for VM $vmid in storage '$storeid'\n";
>  };
>  
>  sub create_base {
> diff --git a/PVE/Storage/SheepdogPlugin.pm b/PVE/Storage/SheepdogPlugin.pm
> index f10ef31..65047c6 100644
> --- a/PVE/Storage/SheepdogPlugin.pm
> +++ b/PVE/Storage/SheepdogPlugin.pm
> @@ -151,22 +151,9 @@ my $find_free_diskname = sub {
>  
>      my $sheepdog = sheepdog_ls($scfg, $storeid);
>      my $dat = $sheepdog->{$storeid};
> -    my $disk_ids = {};
> +    my $disk_list = [ keys(%$dat) ];
>  
> -    foreach my $image (keys %$dat) {
> -	my $volname = $dat->{$image}->{name};
> -	if ($volname =~ m/(vm|base)-$vmid-disk-(\d+)/){
> -	    $disk_ids->{$2} = 1;
> -	}
> -    }
> -
> -    for (my $i = 1; $i < 100; $i++) {
> -	if (!$disk_ids->{$i}) {
> -	    return "vm-$vmid-disk-$i";
> -	}
> -    }
> -
> -    die "unable to allocate an image name for VM $vmid in storage '$storeid'\n";
> +    return PVE::Storage::Plugin::get_next_vm_diskname($disk_list, $storeid, $vmid, undef, $scfg);
>  };
>  
>  sub create_base {
> diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
> index 32e53aa..4455304 100644
> --- a/PVE/Storage/ZFSPoolPlugin.pm
> +++ b/PVE/Storage/ZFSPoolPlugin.pm
> @@ -394,20 +394,8 @@ sub zfs_find_free_diskname {
>      my $disk_ids = {};
           ^^^^^^^^
now unused...

>      my $dat = $volumes->{$scfg->{pool}};
>  
> -    foreach my $image (keys %$dat) {
> -        my $volname = $dat->{$image}->{name};
> -        if ($volname =~ m/(vm|base|subvol|basevol)-$vmid-disk-(\d+)/){
> -            $disk_ids->{$2} = 1;
> -        }
> -    }
> -
> -    for (my $i = 1; $i < 100; $i++) {
> -        if (!$disk_ids->{$i}) {
> -            return $format eq 'subvol' ? "subvol-$vmid-disk-$i" : "vm-$vmid-disk-$i";
> -        }
> -    }
> -
> -    die "unable to allocate an image name for VM $vmid in storage '$storeid'\n";
> +    my $disk_list = [ keys(%$dat) ];

nit: we normally do not use parenthesis with the 'keys' or 'values' keyword (e.g. see
above deleted foreach line), maybe I'm just used to but those make readability for me
even harder..^^

> +    return PVE::Storage::Plugin::get_next_vm_diskname($disk_list, $storeid, $vmid, $format, $scfg);
>  }
>  
>  sub zfs_get_latest_snapshot {
> 





More information about the pve-devel mailing list