Re: [pve-devel] [PATCH container] fix #3421: allow custom storage plugins to support rootfs

[Fabian Ebner <f.ebner@proxmox.com>]

Had a look at this patch. The idea is fine, but there are a few minor 
problems that could be avoided.

Am 11.05.21 um 13:02 schrieb Lorenz Stechauner:
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>   src/PVE/LXC.pm | 28 +++++++---------------------
>   1 file changed, 7 insertions(+), 21 deletions(-)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 7e6f378..9c1227c 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -1685,8 +1685,7 @@ sub __mountpoint_mount {
>   	    if ($scfg->{path}) {
>   		$mounted_dev = run_with_loopdev($domount, $path, $readonly);
>   		$use_loopdev = 1;
> -	    } elsif ($scfg->{type} eq 'drbd' || $scfg->{type} eq 'lvm' ||
> -		     $scfg->{type} eq 'rbd' || $scfg->{type} eq 'lvmthin') {
> +	    } elsif ($scfg->{content}->{rootdir}) {
>   		$mounted_dev = $path;
>   		&$domount($path);
>   	    } else {

Nit: The error message in the else branch does not match up with what is 
checked for anymore.

This also breaks starting containers where the content type for one of 
the mount point volumes is incorrectly set, which did work previously. 
Maybe just always take the other branch for non-filesystem-based 
storages (i.e. those without $scfg->{path})?

> @@ -1871,30 +1870,17 @@ sub alloc_disk {
>   
>       eval {
>   	my $do_format = 0;
> -	if ($scfg->{type} eq 'dir' || $scfg->{type} eq 'nfs' || $scfg->{type} eq 'cifs' ) {
> +	if ($scfg->{type} eq 'zfspool') {
> +	    $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'subvol', undef, $size_kb);

Style nit: please avoid line length > 100.

> +	    $needs_chown = 1;
> +	} elsif ($scfg->{content}->{rootdir}) {
>   	    if ($size_kb > 0) {
> -		$volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw',
> -						   undef, $size_kb);
> +		$volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', undef, $size_kb);

Style nit: please avoid line length > 100.

>   		$do_format = 1;
>   	    } else {
> -		$volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'subvol',
> -						   undef, 0);
> +		$volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'subvol', undef, 0);
>   		$needs_chown = 1;
>   	    }
> -	} elsif ($scfg->{type} eq 'zfspool') {
> -
> -	    $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'subvol',
> -					       undef, $size_kb);
> -	    $needs_chown = 1;
> -	} 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);
> -	    $do_format = 1;
> -
> -	} elsif ($scfg->{type} eq 'rbd') {
> -
> -	    $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', undef, $size_kb);
> -	    $do_format = 1;

This changes the requested format and $do_format when size is 0 for rbd, 
drbd, lvm and lvm-thin plugins. Might be worth mentioning in the commit 
message.

It's not really a problem if "previously fails" iff "now fails", but for 
rbd it's not the case. After applying the patch it's possible to create 
0-sized rbd disks, which are not formatted. Previously, formatting would 
fail.

Would not be an issue if the rbd storage plugin would actually complain 
about getting the wrong format in the first place, like the other ones do ;)

Still, I feel like it would be better to only handle the size 0 special 
case for file-system based storages, using $scfg->{path} like above.

>   	} else {
>   	    die "unable to create containers on storage type '$scfg->{type}'\n";
>   	}
> 

Same situation as for the else branch above, although this one probably 
cannot be triggered, as there's an earlier check for content type support.