[pve-devel] [PATCH pve-container] Make volume create and mount code independent of storage plugin types

[Fabian Grünbichler]

general remark, rest of comments inline:

the indentation is all messed up. I know our codebase is not very clean
in this regard anyway, but for new / touched code we try to keep / make
it clean. our indentation for perl code is a mix of tabs and spaces,
with tabs being 8 spaces long.

the first indentation level is indented with 4 spaces, the second with
1 tab, the third with 1 tab and 4 spaces, the fourth with 2 tabs, and
so on. if you use vim, I recommend enabling listmode to easily
differentiate between the two.

On Sun, Sep 11, 2016 at 09:07:36AM +0300, Dmitry Petuhov wrote:
> Instead, rely on plugin-defined supported formats to decide if it
> supports subvols.
> 
> Still workaround 'rbd' plugin, that cannot be used for LXC without
> krbd. Maybe better way is to pass $scfg into plugin's plugindata()
> in storage code? So plugin could show instance's capabilities
> instead of whole plugin's capabilities, that may or may not be available
> in instance.

the second paragraph does not belong into the commit message ;)
either include it as a comment below the --- , or in the cover letter,
or as a separate mail altogether.

> 
> Signed-off-by: Dmitry Petuhov <mityapetuhov at gmail.com>
> ---
>  src/PVE/LXC.pm | 45 +++++++++++++++------------------------------
>  1 file changed, 15 insertions(+), 30 deletions(-)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 35ce796..70d13e4 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -1238,12 +1238,8 @@ sub mountpoint_mount {
>  	    if ($scfg->{path}) {
>  		$mounted_dev = run_with_loopdev($domount, $path);
>  		$use_loopdev = 1;
> -	    } elsif ($scfg->{type} eq 'drbd' || $scfg->{type} eq 'lvm' ||
> -		     $scfg->{type} eq 'rbd' || $scfg->{type} eq 'lvmthin') {
> -		$mounted_dev = $path;
> -		&$domount($path);
>  	    } else {
> -		die "unsupported storage type '$scfg->{type}'\n";
> +		&$domount($path);
>  	    }
>  	    return wantarray ? ($path, $use_loopdev, $mounted_dev) : $path;
>  	} else {

you drop the $mounted_dev assignment in the else branch. after a quick
search of the calling code, this will probably at least break quota
support for such volumes?

> @@ -1330,37 +1326,26 @@ sub create_disks {
>  
>  		my $size_kb = int(${size_gb}*1024) * 1024;
>  
> -		my $scfg = PVE::Storage::storage_config($storecfg, $storage);
>  		# fixme: use better naming ct-$vmid-disk-X.raw?
>  
> -		if ($scfg->{type} eq 'dir' || $scfg->{type} eq 'nfs') {
> -		    if ($size_kb > 0) {
> -			$volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw',
> -							   undef, $size_kb);
> -			format_disk($storecfg, $volid, $rootuid, $rootgid);
> -		    } else {
> +                if ($size_kb > 0) {
> +		    my $scfg = PVE::Storage::storage_config($storecfg, $storage);
> +                    die "krbd option must be enabled on storage type 'rbd'\n" if ($scfg->{type} eq 'rbd') && !$scfg->{krbd};
> +
> +		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw',
> +						       undef, $size_kb);
> +                    format_disk($storecfg, $volid, $rootuid, $rootgid);
> +                } else {


> +                    my ($defFormat, $validFormats) = PVE::Storage::storage_default_format($storecfg, $storage);

this seems to be copied verbatim from QemuServer.pm, but the camelCase
is still wrong (here and there). def_format and valid_formats please ;)

> +	            if (grep { $_ eq 'subvol' } @$validFormats) {
>  			$volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'subvol',
>  							   undef, 0);
>  			push @$chown_vollist, $volid;
> -		    }
> -		} elsif ($scfg->{type} eq 'zfspool') {
> +                    } else {
> +                        die "Selected storage does not support subvols. Please, specify image size or select another storage";
> +                    }
> +                }
>  
> -		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'subvol',
> -					               undef, $size_kb);
> -		    push @$chown_vollist, $volid;
> -		} elsif ($scfg->{type} eq 'drbd' || $scfg->{type} eq 'lvm' || $scfg->{type} eq 'lvmthin') {
> -
> -		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', undef, $size_kb);
> -		    format_disk($storecfg, $volid, $rootuid, $rootgid);
> -
> -		} elsif ($scfg->{type} eq 'rbd') {
> -
> -		    die "krbd option must be enabled on storage type '$scfg->{type}'\n" if !$scfg->{krbd};
> -		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', undef, $size_kb);
> -		    format_disk($storecfg, $volid, $rootuid, $rootgid);
> -		} else {
> -		    die "unable to create containers on storage type '$scfg->{type}'\n";
> -		}
>  		push @$vollist, $volid;
>  		$mountpoint->{volume} = $volid;
>  		$mountpoint->{size} = $size_kb * 1024;

subvols are not only for size == 0 , e.g. ZFS supports subvols with and
without size (limit). ZFS is a bit tricky unfortunately, as for
containers we always want subvols (regular ZFS filesystems), and for
VMs we always want raw volumes ("zvols" in ZFS speak). so changing the
default format does not really work, and we still need a special case
for ZFS (and future storages with the same problem) here?