[pve-devel] [PATCH-SERIES v3 container/qemu-server] fix #3421: allow custom storage plugins to support rootfs

[pve-devel] [PATCH-SERIES v3 container/qemu-server] fix #3421: allow custom storage plugins to support rootfs

[Lorenz Stechauner]

changes to v2:
* typo s/supoort/support/
* more detailed error messages
* implemented check also for vms

pve-container:

Lorenz Stechauner (1):
  fix #3421: allow custom storage plugins to support rootfs

 src/PVE/LXC.pm | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)


qemu-server:

Lorenz Stechauner (1):
  vm_start: check if storages of volumes support content images

 PVE/QemuServer.pm | 7 +++++++
 1 file changed, 7 insertions(+)

-- 
2.20.1

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

[Lorenz Stechauner]

it is now necessary for storages to support the 'rootdir' content in
order to start containers on them. all native storage plugins
already report the rootdir content correctly.

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 src/PVE/LXC.pm | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 7e6f378..556f7fa 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1682,15 +1682,16 @@ sub __mountpoint_mount {
 		}
 	    };
 	    my $use_loopdev = 0;
-	    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') {
-		$mounted_dev = $path;
-		&$domount($path);
+	    if ($scfg->{content}->{rootdir}) {
+		if ($scfg->{path}) {
+		    $mounted_dev = run_with_loopdev($domount, $path, $readonly);
+		    $use_loopdev = 1;
+		} else {
+		    $mounted_dev = $path;
+		    &$domount($path);
+		}
 	    } else {
-		die "unsupported storage type '$scfg->{type}'\n";
+		die "storage '$storage' does not support containers\n";
 	    }
 	    return wantarray ? ($path, $use_loopdev, $mounted_dev) : $path;
 	} else {
@@ -1871,7 +1872,7 @@ sub alloc_disk {
 
     eval {
 	my $do_format = 0;
-	if ($scfg->{type} eq 'dir' || $scfg->{type} eq 'nfs' || $scfg->{type} eq 'cifs' ) {
+	if ($scfg->{content}->{rootdir} && $scfg->{path}) {
 	    if ($size_kb > 0) {
 		$volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw',
 						   undef, $size_kb);
@@ -1882,21 +1883,14 @@ sub alloc_disk {
 		$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') {
-
+	} elsif ($scfg->{content}->{rootdir}) {
 	    $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', undef, $size_kb);
 	    $do_format = 1;
 	} else {
-	    die "unable to create containers on storage type '$scfg->{type}'\n";
+	    die "storage '$storage' does not support containers\n";
 	}
 	format_disk($storecfg, $volid, $rootuid, $rootgid) if $do_format;
     };
-- 
2.20.1

[pve-devel] [PATCH v3 qemu-server 1/1] vm_start: check if storages of volumes support content images

[Lorenz Stechauner]

it is now necessary for storages to support the 'images' content in
order to start vms on them. all native storage plugins already
report the images content correctly.

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 PVE/QemuServer.pm | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 0bfbca4..3b656a1 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5124,6 +5124,13 @@ sub vm_start_nolock {
     my ($cmd, $vollist, $spice_port) = config_to_command($storecfg, $vmid,
 	$conf, $defaults, $forcemachine, $forcecpu, $params->{'pbs-backing'});
 
+    for my $vol (@$vollist) {
+	if (my ($storeid, $volname) = PVE::Storage::parse_volume_id($vol, 1)) {
+	    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
+	    die "storage '$storeid' does not support vm disks\n" if !$scfg->{content}->{images};
+	}
+    }
+
     my $migration_ip;
     my $get_migration_ip = sub {
 	my ($nodename) = @_;
-- 
2.20.1

Re: [pve-devel] [PATCH-SERIES v3 container/qemu-server] fix #3421: allow custom storage plugins to support rootfs

[Fabian Ebner]

There's an edge case with 'restart' migration for containers that breaks 
because of the new content type on startup checks:
If there is an already running container with a volume on storage A, and 
now storage A is reconfigured to not support 'rootdir' anymore, then 
migration itself does work, but there'll be an error on startup on the 
remote node. It would be nicer if the error would appear at the start of 
the migration already.

For VMs (with 'online' migration) the situation is not as bad, because 
the remote start happens earlier, so the VM will still be running on the 
original node after the error.

And one can offline migrate such unstartable guests around ;)

IMHO, if we add the checks for content type on startup, it's all the 
more reason to have content type checks for migration as well. For VM 
migration with the targetstorage option, there already are such checks.


It's a tangential problem of course, your patches look fine to me:

Reviewed-by: Fabian Ebner <f.ebner@proxmox.com>

Am 27.05.21 um 14:23 schrieb Lorenz Stechauner:
> changes to v2:
> * typo s/supoort/support/
> * more detailed error messages
> * implemented check also for vms
> 
> pve-container:
> 
> Lorenz Stechauner (1):
>    fix #3421: allow custom storage plugins to support rootfs
> 
>   src/PVE/LXC.pm | 30 ++++++++++++------------------
>   1 file changed, 12 insertions(+), 18 deletions(-)
> 
> 
> qemu-server:
> 
> Lorenz Stechauner (1):
>    vm_start: check if storages of volumes support content images
> 
>   PVE/QemuServer.pm | 7 +++++++
>   1 file changed, 7 insertions(+)
> 

Re: [pve-devel] [PATCH-SERIES v3 container/qemu-server] fix #3421: allow custom storage plugins to support rootfs

[Fabian Ebner]

Since I just ran into it: It also breaks (at least container) backups 
when there is a volume on a misconfigured storage.

Am 02.06.21 um 09:29 schrieb Fabian Ebner:
> There's an edge case with 'restart' migration for containers that breaks 
> because of the new content type on startup checks:
> If there is an already running container with a volume on storage A, and 
> now storage A is reconfigured to not support 'rootdir' anymore, then 
> migration itself does work, but there'll be an error on startup on the 
> remote node. It would be nicer if the error would appear at the start of 
> the migration already.
> 
> For VMs (with 'online' migration) the situation is not as bad, because 
> the remote start happens earlier, so the VM will still be running on the 
> original node after the error.
> 
> And one can offline migrate such unstartable guests around ;)
> 
> IMHO, if we add the checks for content type on startup, it's all the 
> more reason to have content type checks for migration as well. For VM 
> migration with the targetstorage option, there already are such checks.
> 
> 
> It's a tangential problem of course, your patches look fine to me:
> 
> Reviewed-by: Fabian Ebner <f.ebner@proxmox.com>
> 
> Am 27.05.21 um 14:23 schrieb Lorenz Stechauner:
>> changes to v2:
>> * typo s/supoort/support/
>> * more detailed error messages
>> * implemented check also for vms
>>
>> pve-container:
>>
>> Lorenz Stechauner (1):
>>    fix #3421: allow custom storage plugins to support rootfs
>>
>>   src/PVE/LXC.pm | 30 ++++++++++++------------------
>>   1 file changed, 12 insertions(+), 18 deletions(-)
>>
>>
>> qemu-server:
>>
>> Lorenz Stechauner (1):
>>    vm_start: check if storages of volumes support content images
>>
>>   PVE/QemuServer.pm | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

[pve-devel] applied: [PATCH v3 container 1/1] fix #3421: allow custom storage plugins to support rootfs

[Thomas Lamprecht]

On 27.05.21 14:23, Lorenz Stechauner wrote:
> it is now necessary for storages to support the 'rootdir' content in
> order to start containers on them. all native storage plugins
> already report the rootdir content correctly.
> 
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>  src/PVE/LXC.pm | 30 ++++++++++++------------------
>  1 file changed, 12 insertions(+), 18 deletions(-)
> 
>

applied, with Fabi's R-b tag, thanks!

Edited the error message a bit, to convey that the "rootdir" content-type may
just not be configured

Re: [pve-devel] [PATCH v3 qemu-server 1/1] vm_start: check if storages of volumes support content images

[Thomas Lamprecht]

On 27.05.21 14:23, Lorenz Stechauner wrote:
> it is now necessary for storages to support the 'images' content in
> order to start vms on them. all native storage plugins already
> report the images content correctly.
> 
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>  PVE/QemuServer.pm | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 0bfbca4..3b656a1 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -5124,6 +5124,13 @@ sub vm_start_nolock {
>      my ($cmd, $vollist, $spice_port) = config_to_command($storecfg, $vmid,
>  	$conf, $defaults, $forcemachine, $forcecpu, $params->{'pbs-backing'});
>  
> +    for my $vol (@$vollist) {
> +	if (my ($storeid, $volname) = PVE::Storage::parse_volume_id($vol, 1)) {
> +	    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
> +	    die "storage '$storeid' does not support vm disks\n" if !$scfg->{content}->{images};
> +	}
> +    }

we'd already loop overall volumes in cfg2cmd, so why not check there (with a small
helper method to avoid adding to much bloat there)?

Also, are you aware that efidisk and VM state images are included by this check 
ensured that it's consistent regarding the handling of those special "files" elsewhere?