* Re: [pve-devel] [PATCH container] fix #3421: allow custom storage plugins to support rootfs
2021-05-11 11:02 [pve-devel] [PATCH container] fix #3421: allow custom storage plugins to support rootfs Lorenz Stechauner
@ 2021-05-12 11:20 ` Fabian Ebner
0 siblings, 0 replies; 2+ messages in thread
From: Fabian Ebner @ 2021-05-12 11:20 UTC (permalink / raw)
To: pve-devel, l.stechauner
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.
^ permalink raw reply [flat|nested] 2+ messages in thread