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

[Wolfgang Bumiller]

The behavior seems fine at a first look.

On Mon, Sep 19, 2016 at 01:57:17PM +0300, Dmitry Petuhov wrote:
> Instead, rely on plugin-defined supported formats to decide if it
> supports subvols.
> 
> Signed-off-by: Dmitry Petuhov <mityapetuhov at gmail.com>
> ---
> 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.

I doubt there are many storages with such special cases. The general
decision to make here is whether there's a way to get a mountable
path/device here (iow. whether we can "use" the storage without special
tools), which could be an option at some point if we need one.

> 
> Also workaround special-case 'rbdpool' storage, where we allways want to
> use subvols with LXC. Maybe signal that plugin wants such behaviour via
> plugindata() too? Like set default format to subvol instead of raw in
> zfspool?

Adding an option to the plugindata might be a good idea, however not via
the default format. The current "default" format is somewhat misleading
considering at this point we have what looks at first like a preference
depending on usage: VM => zvol, CT => subvol. The same with btrfs, btw.

The distinction as far as I can tell really only comes from the fact
that we cannot use quotas on directory-storage backed "subvolumes",
hence the size=0 special case.
With that in mind, I think that all our cases would be covered if we
could check whether subvolumes can have a size.

The code would then work almost the same as your patch here, but could
then be ordered in a somewhat more obvious way:
  if size=0: use a subvol or error if subvols aren't supported
  else if subvols are supported and support sizes: use a subvol
  else: use the default format
This way the intention is clear: size=0 images make no sense, subvolumes
are preferred, and otherwise the storage-specified default is used.

Maybe plugindata could have a 'subvol' key with a list of properties?
{
	...
	subvol => { size => 1 }
}

Or we make use of the fact that the format is a hash and change the '1'
to a feature list or bitfield or something... but I think a separate
hash like above is the most straightforward way.

> 
>  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..70d3b50 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -1238,12 +1238,9 @@ 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') {
> +	    } else {
>  		$mounted_dev = $path;
>  		&$domount($path);
> -	    } else {
> -		die "unsupported storage type '$scfg->{type}'\n";
>  	    }
>  	    return wantarray ? ($path, $use_loopdev, $mounted_dev) : $path;
>  	} else {
> @@ -1331,43 +1328,31 @@ sub create_disks {
>  		my $size_kb = int(${size_gb}*1024) * 1024;
>  
>  		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};
>  		# 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 && $scfg->{type} ne 'zfspool') {
> +		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw',
> +						       undef, $size_kb);
> +		    format_disk($storecfg, $volid, $rootuid, $rootgid);
> +		} else {
> +		    my (undef, $valid_formats) = PVE::Storage::storage_default_format($storecfg, $storage);
> +		    if (grep { $_ eq 'subvol' } @$valid_formats) {
>  			$volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'subvol',
> -							   undef, 0);
> +							   undef, $size_kb);
>  			push @$chown_vollist, $volid;
> +		    } else {
> +			die "Selected storage does not support subvols. Please, specify image size or select another storage";
>  		    }
> -		} elsif ($scfg->{type} eq 'zfspool') {
> -
> -		    $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;
>  		$conf->{$ms} = PVE::LXC::Config->print_ct_mountpoint($mountpoint, $ms eq 'rootfs');
>  	    } else {
> -                # use specified/existing volid/dir/device
> -                $conf->{$ms} = PVE::LXC::Config->print_ct_mountpoint($mountpoint, $ms eq 'rootfs');
> +		# use specified/existing volid/dir/device
> +		$conf->{$ms} = PVE::LXC::Config->print_ct_mountpoint($mountpoint, $ms eq 'rootfs');
>  	    }
>  	});
>  
> -- 
> 2.1.4
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel