[pve-devel] [PATCH manager 1/2] pve6to7: storage content: skip scanning storage if shared

[pve-devel] [PATCH manager 1/2] pve6to7: storage content: skip scanning storage if shared

[Fabian Ebner]

Shared storages are not scanned for migration either, so they cannot
be problematic in this context. This could lead to false positives
where it actually is completely unproblematic:

https://forum.proxmox.com/threads/proxmox-ve-7-0-released.92007/post-401165

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/CLI/pve6to7.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/PVE/CLI/pve6to7.pm b/PVE/CLI/pve6to7.pm
index 69ed6d2e..17da70e8 100644
--- a/PVE/CLI/pve6to7.pm
+++ b/PVE/CLI/pve6to7.pm
@@ -707,6 +707,7 @@ sub check_storage_content {
     for my $storeid (sort keys $storage_cfg->{ids}->%*) {
 	my $scfg = $storage_cfg->{ids}->{$storeid};
 
+	next if $scfg->{shared};
 	next if !PVE::Storage::storage_check_enabled($storage_cfg, $storeid, undef, 1);
 
 	my $valid_content = PVE::Storage::Plugin::valid_content_types($scfg->{type});
-- 
2.20.1

[pve-devel] [PATCH manager 2/2] pve6to7: storage content: ignore misconfigured unreferenced volumes

[Fabian Ebner]

If the same local storage is configured twice with content type
separation, migration in PVE 6 would lead to the volumes being
duplicated. As that would happen for every migration, such an issue
would likely be noticed already, and in PVE 7 such configuration is
not problematic for migration anymore. Also, misconfigured
unreferenced volumes are not an issue with respect to the upgrade
itself, just drop the check.

It's not necessary to scan storages with either 'images' or 'rootdir'
anymore, as only the log_info() remains.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/CLI/pve6to7.pm | 43 ++++++-------------------------------------
 1 file changed, 6 insertions(+), 37 deletions(-)

diff --git a/PVE/CLI/pve6to7.pm b/PVE/CLI/pve6to7.pm
index 17da70e8..7d7b09d2 100644
--- a/PVE/CLI/pve6to7.pm
+++ b/PVE/CLI/pve6to7.pm
@@ -695,15 +695,11 @@ sub check_description_lengths {
 sub check_storage_content {
     log_info("Checking storage content type configuration..");
 
-    my $found_referenced;
-    my $found_unreferenced;
+    my $found;
     my $pass = 1;
 
     my $storage_cfg = PVE::Storage::config();
 
-    my $potentially_affected = {};
-    my $referenced_volids = {};
-
     for my $storeid (sort keys $storage_cfg->{ids}->%*) {
 	my $scfg = $storage_cfg->{ids}->{$storeid};
 
@@ -718,7 +714,8 @@ sub check_storage_content {
 	    delete $scfg->{content}->{none}; # scan for guest images below
 	}
 
-	next if $scfg->{content}->{images} && $scfg->{content}->{rootdir};
+	next if $scfg->{content}->{images};
+	next if $scfg->{content}->{rootdir};
 
 	# Skip 'iscsi(direct)' (and foreign plugins with potentially similiar behavior) with 'none',
 	# because that means "use LUNs directly" and vdisk_list() in PVE 6.x still lists those.
@@ -739,12 +736,8 @@ sub check_storage_content {
 	}
 	my @volids = map { $_->{volid} } $res->{$storeid}->@*;
 
-	for my $volid (@volids) {
-	    $potentially_affected->{$volid} = 1;
-	}
-
 	my $number = scalar(@volids);
-	if ($number > 0 && !$scfg->{content}->{images} && !$scfg->{content}->{rootdir}) {
+	if ($number > 0) {
 	    log_info("storage '$storeid' - neither content type 'images' nor 'rootdir' configured"
 		.", but found $number guest volume(s)");
 	}
@@ -753,8 +746,6 @@ sub check_storage_content {
     my $check_volid = sub {
 	my ($volid, $vmid, $vmtype, $reference) = @_;
 
-	$referenced_volids->{$volid} = 1 if $reference ne 'unreferenced';
-
 	my $guesttext = $vmtype eq 'qemu' ? 'VM' : 'CT';
 	my $prefix = "$guesttext $vmid - volume '$volid' ($reference)";
 
@@ -777,19 +768,14 @@ sub check_storage_content {
 	}
 
 	if (!$scfg->{content}->{$vtype}) {
-	    $found_referenced = 1 if $reference ne 'unreferenced';
-	    $found_unreferenced = 1 if $reference eq 'unreferenced';
+	    $found = 1;
 	    $pass = 0;
 	    log_warn("$prefix - storage does not have content type '$vtype' configured.");
 	}
     };
 
-    my $guests = {};
-
     my $cts = PVE::LXC::config_list();
     for my $vmid (sort { $a <=> $b } keys %$cts) {
-	$guests->{$vmid} = 'lxc';
-
 	my $conf = PVE::LXC::Config->load_config($vmid);
 
 	my $volhash = {};
@@ -817,8 +803,6 @@ sub check_storage_content {
 
     my $vms = PVE::QemuServer::config_list();
     for my $vmid (sort { $a <=> $b } keys %$vms) {
-	$guests->{$vmid} = 'qemu';
-
 	my $conf = PVE::QemuConfig->load_config($vmid);
 
 	my $volhash = {};
@@ -849,26 +833,11 @@ sub check_storage_content {
 	}
     }
 
-    if ($found_referenced) {
+    if ($found) {
 	log_warn("Proxmox VE 7.0 enforces stricter content type checks. The guests above " .
 	    "might not work until the storage configuration is fixed.");
     }
 
-    for my $volid (sort keys $potentially_affected->%*) {
-	next if $referenced_volids->{$volid}; # already checked
-
-	my (undef, undef, $vmid) = PVE::Storage::parse_volname($storage_cfg, $volid);
-	my $vmtype = $guests->{$vmid};
-	next if !$vmtype;
-
-	$check_volid->($volid, $vmid, $vmtype, 'unreferenced');
-    }
-
-    if ($found_unreferenced) {
-	log_warn("When migrating, Proxmox VE 7.0 only scans storages with the appropriate " .
-	    "content types for unreferenced guest volumes.");
-    }
-
     if ($pass) {
 	log_pass("no problems found");
     }
-- 
2.20.1

Re: [pve-devel] [PATCH manager 2/2] pve6to7: storage content: ignore misconfigured unreferenced volumes

[Fabian Grünbichler]

On July 7, 2021 12:22 pm, Fabian Ebner wrote:
> If the same local storage is configured twice with content type
> separation, migration in PVE 6 would lead to the volumes being
> duplicated. As that would happen for every migration, such an issue
> would likely be noticed already, and in PVE 7 such configuration is
> not problematic for migration anymore. Also, misconfigured
> unreferenced volumes are not an issue with respect to the upgrade
> itself, just drop the check.

but those checks also catch storages that are misconfigured for which no 
such inverse storage with opposite content type and otherwise identical 
settings exists? we can't just drop them altogether?

we COULD skip them conditionally for storage pairs (same type, same 
'path'/pool/pool+mons/.., one with images on with rootfs), but such 
setups are still wrong and not properly separated IMHO. and stuff like 
dir-storage on-top of other dir-like storage with volumes stored in the 
same path are not really "detectable" in a reliable and cheap fashion 
and should really be fixed by the user (e.g., by moving the dir storage 
into a non-confusable sub-dir of the backing storage).

> 
> It's not necessary to scan storages with either 'images' or 'rootdir'
> anymore, as only the log_info() remains.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/CLI/pve6to7.pm | 43 ++++++-------------------------------------
>  1 file changed, 6 insertions(+), 37 deletions(-)
> 
> diff --git a/PVE/CLI/pve6to7.pm b/PVE/CLI/pve6to7.pm
> index 17da70e8..7d7b09d2 100644
> --- a/PVE/CLI/pve6to7.pm
> +++ b/PVE/CLI/pve6to7.pm
> @@ -695,15 +695,11 @@ sub check_description_lengths {
>  sub check_storage_content {
>      log_info("Checking storage content type configuration..");
>  
> -    my $found_referenced;
> -    my $found_unreferenced;
> +    my $found;
>      my $pass = 1;
>  
>      my $storage_cfg = PVE::Storage::config();
>  
> -    my $potentially_affected = {};
> -    my $referenced_volids = {};
> -
>      for my $storeid (sort keys $storage_cfg->{ids}->%*) {
>  	my $scfg = $storage_cfg->{ids}->{$storeid};
>  
> @@ -718,7 +714,8 @@ sub check_storage_content {
>  	    delete $scfg->{content}->{none}; # scan for guest images below
>  	}
>  
> -	next if $scfg->{content}->{images} && $scfg->{content}->{rootdir};
> +	next if $scfg->{content}->{images};
> +	next if $scfg->{content}->{rootdir};
>  
>  	# Skip 'iscsi(direct)' (and foreign plugins with potentially similiar behavior) with 'none',
>  	# because that means "use LUNs directly" and vdisk_list() in PVE 6.x still lists those.
> @@ -739,12 +736,8 @@ sub check_storage_content {
>  	}
>  	my @volids = map { $_->{volid} } $res->{$storeid}->@*;
>  
> -	for my $volid (@volids) {
> -	    $potentially_affected->{$volid} = 1;
> -	}
> -
>  	my $number = scalar(@volids);
> -	if ($number > 0 && !$scfg->{content}->{images} && !$scfg->{content}->{rootdir}) {
> +	if ($number > 0) {
>  	    log_info("storage '$storeid' - neither content type 'images' nor 'rootdir' configured"
>  		.", but found $number guest volume(s)");
>  	}
> @@ -753,8 +746,6 @@ sub check_storage_content {
>      my $check_volid = sub {
>  	my ($volid, $vmid, $vmtype, $reference) = @_;
>  
> -	$referenced_volids->{$volid} = 1 if $reference ne 'unreferenced';
> -
>  	my $guesttext = $vmtype eq 'qemu' ? 'VM' : 'CT';
>  	my $prefix = "$guesttext $vmid - volume '$volid' ($reference)";
>  
> @@ -777,19 +768,14 @@ sub check_storage_content {
>  	}
>  
>  	if (!$scfg->{content}->{$vtype}) {
> -	    $found_referenced = 1 if $reference ne 'unreferenced';
> -	    $found_unreferenced = 1 if $reference eq 'unreferenced';
> +	    $found = 1;
>  	    $pass = 0;
>  	    log_warn("$prefix - storage does not have content type '$vtype' configured.");
>  	}
>      };
>  
> -    my $guests = {};
> -
>      my $cts = PVE::LXC::config_list();
>      for my $vmid (sort { $a <=> $b } keys %$cts) {
> -	$guests->{$vmid} = 'lxc';
> -
>  	my $conf = PVE::LXC::Config->load_config($vmid);
>  
>  	my $volhash = {};
> @@ -817,8 +803,6 @@ sub check_storage_content {
>  
>      my $vms = PVE::QemuServer::config_list();
>      for my $vmid (sort { $a <=> $b } keys %$vms) {
> -	$guests->{$vmid} = 'qemu';
> -
>  	my $conf = PVE::QemuConfig->load_config($vmid);
>  
>  	my $volhash = {};
> @@ -849,26 +833,11 @@ sub check_storage_content {
>  	}
>      }
>  
> -    if ($found_referenced) {
> +    if ($found) {
>  	log_warn("Proxmox VE 7.0 enforces stricter content type checks. The guests above " .
>  	    "might not work until the storage configuration is fixed.");
>      }
>  
> -    for my $volid (sort keys $potentially_affected->%*) {
> -	next if $referenced_volids->{$volid}; # already checked
> -
> -	my (undef, undef, $vmid) = PVE::Storage::parse_volname($storage_cfg, $volid);
> -	my $vmtype = $guests->{$vmid};
> -	next if !$vmtype;
> -
> -	$check_volid->($volid, $vmid, $vmtype, 'unreferenced');
> -    }
> -
> -    if ($found_unreferenced) {
> -	log_warn("When migrating, Proxmox VE 7.0 only scans storages with the appropriate " .
> -	    "content types for unreferenced guest volumes.");
> -    }
> -
>      if ($pass) {
>  	log_pass("no problems found");
>      }
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH manager 2/2] pve6to7: storage content: ignore misconfigured unreferenced volumes

[Thomas Lamprecht]

On 08.07.21 09:29, Fabian Grünbichler wrote:
> On July 7, 2021 12:22 pm, Fabian Ebner wrote:
>> If the same local storage is configured twice with content type
>> separation, migration in PVE 6 would lead to the volumes being
>> duplicated. As that would happen for every migration, such an issue
>> would likely be noticed already, and in PVE 7 such configuration is
>> not problematic for migration anymore. Also, misconfigured
>> unreferenced volumes are not an issue with respect to the upgrade
>> itself, just drop the check.
> 
> but those checks also catch storages that are misconfigured for which no 
> such inverse storage with opposite content type and otherwise identical 
> settings exists? we can't just drop them altogether?
> 
> we COULD skip them conditionally for storage pairs (same type, same 
> 'path'/pool/pool+mons/.., one with images on with rootfs), but such 
> setups are still wrong and not properly separated IMHO. and stuff like 
> dir-storage on-top of other dir-like storage with volumes stored in the 
> same path are not really "detectable" in a reliable and cheap fashion 
> and should really be fixed by the user (e.g., by moving the dir storage 
> into a non-confusable sub-dir of the backing storage).

as long as the content types are non-overlapping it is separated though,
a user in the forum had a GlusterFS entry used by QEMU directly and that
GlusterFS mounted on the system for containers, both added as storage but
strictly separating content types.

I.e., an OK setup where changing anything could actually lead to breakage,
but the warning as it was did not really reflect that. If that can be
improved then OK, but as is it was just not worth for what I perceive as
rather small in-practice risk of that change.

Re: [pve-devel] [PATCH manager 1/2] pve6to7: storage content: skip scanning storage if shared

[Fabian Grünbichler]

On July 7, 2021 12:22 pm, Fabian Ebner wrote:
> Shared storages are not scanned for migration either, so they cannot
> be problematic in this context. This could lead to false positives
> where it actually is completely unproblematic:
> 
> https://forum.proxmox.com/threads/proxmox-ve-7-0-released.92007/post-401165

but the behaviour changes for shared storages as well? previously, 
vdisk_list would list regardless of content type settings, now it 
properly filters, so volumes on shared storage "disappear"..

> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/CLI/pve6to7.pm | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/PVE/CLI/pve6to7.pm b/PVE/CLI/pve6to7.pm
> index 69ed6d2e..17da70e8 100644
> --- a/PVE/CLI/pve6to7.pm
> +++ b/PVE/CLI/pve6to7.pm
> @@ -707,6 +707,7 @@ sub check_storage_content {
>      for my $storeid (sort keys $storage_cfg->{ids}->%*) {
>  	my $scfg = $storage_cfg->{ids}->{$storeid};
>  
> +	next if $scfg->{shared};
>  	next if !PVE::Storage::storage_check_enabled($storage_cfg, $storeid, undef, 1);
>  
>  	my $valid_content = PVE::Storage::Plugin::valid_content_types($scfg->{type});
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

[pve-devel] applied: [PATCH manager 1/2] pve6to7: storage content: skip scanning storage if shared

[Thomas Lamprecht]

On 07.07.21 12:22, Fabian Ebner wrote:
> Shared storages are not scanned for migration either, so they cannot
> be problematic in this context. This could lead to false positives
> where it actually is completely unproblematic:
> 
> https://forum.proxmox.com/threads/proxmox-ve-7-0-released.92007/post-401165
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/CLI/pve6to7.pm | 1 +
>  1 file changed, 1 insertion(+)
> 
>

forgot to reply yesterday, but already applied both patches as IMO the confusion
causing element was way higher than any actual real risk for those unreferenced
volumes. If it seems it would have helped actually then I'd vouch for doing it
on "--full" only and to explicitly note when the warning is not an issue.