[pve-devel] [PATCH-SERIES v2 manager] stricter storage rules for migration

[pve-devel] [PATCH-SERIES v2 manager] stricter storage rules for migration

[Fabian Ebner]

Changes from v1:
    * dropped already applied patches
    * rebased
    * adapted/improved messages

Fabian Ebner (2):
  pve6to7: add check for guest images on misconfigured storages
  pve6to7: check for misconfigured content type 'none'

 PVE/CLI/pve6to7.pm | 50 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

-- 
2.20.1

[pve-devel] [PATCH v2 manager 1/2] pve6to7: add check for guest images on misconfigured storages

[Fabian Ebner]

migration and (container) startup will no longer work when the storage's content
type is not correct, and unreferenced volumes on such storages will not be
scanned for anymore.

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

Changes from v1:
    * generalized warning and commit message, since (container) startup also has
      a check now.

 PVE/CLI/pve6to7.pm | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/PVE/CLI/pve6to7.pm b/PVE/CLI/pve6to7.pm
index 80309ac7..60464861 100644
--- a/PVE/CLI/pve6to7.pm
+++ b/PVE/CLI/pve6to7.pm
@@ -702,6 +702,47 @@ sub check_description_lengths {
     }
 }
 
+sub check_storage_content {
+    log_info("Scanning for guest images on storages without images/rootdir content type..");
+
+    my $found;
+
+    my $storage_cfg = PVE::Storage::config();
+
+    for my $storeid (keys $storage_cfg->{ids}->%*) {
+	my $scfg = $storage_cfg->{ids}->{$storeid};
+
+	next if !PVE::Storage::storage_check_enabled($storage_cfg, $storeid, undef, 1);
+
+	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.
+	# It's enough to *not* skip 'dir', because it is the only other storage that supports 'none'
+	# and 'images' or 'rootdir', hence being potentially misconfigured.
+	next if $scfg->{type} ne 'dir' && $scfg->{content}->{none};
+
+	my $res = PVE::Storage::vdisk_list($storage_cfg, $storeid);
+	my $disk_list = $res->{$storeid};
+
+	my @volumes = map { $_->{volid} } $disk_list->@*;
+
+	if (scalar(@volumes) > 0) {
+	    $found = 1;
+	    log_warn("storage '$storeid' - neither content type 'images' nor 'rootdir' " .
+		"configured, but found guest volume(s) " . join(',', @volumes));
+	}
+    }
+
+    if ($found) {
+	log_warn("PVE 7.0 enforces stricter content type checks. Guests referencing the above " .
+	    "volumes will not work until the storage configuration is fixed.");
+    } else {
+	log_pass("none found");
+    }
+}
+
 sub check_misc {
     print_header("MISCELLANEOUS CHECKS");
     my $ssh_config = eval { PVE::Tools::file_get_contents('/root/.ssh/config') };
@@ -796,6 +837,7 @@ sub check_misc {
     check_cifs_credential_location();
     check_custom_pool_roles();
     check_description_lengths();
+    check_storage_content();
 }
 
 __PACKAGE__->register_method ({
-- 
2.20.1

[pve-devel] [PATCH v2 manager 2/2] pve6to7: check for misconfigured content type 'none'

[Fabian Ebner]

which will be a hard error (i.e. section will be skipped when parsing) in PVE
7.0

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

Changes from v1:
    * Also change message for passing the check to match with changed initial
      log line

 PVE/CLI/pve6to7.pm | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/PVE/CLI/pve6to7.pm b/PVE/CLI/pve6to7.pm
index 60464861..7777aa6c 100644
--- a/PVE/CLI/pve6to7.pm
+++ b/PVE/CLI/pve6to7.pm
@@ -18,6 +18,7 @@ use PVE::JSONSchema;
 use PVE::NodeConfig;
 use PVE::RPCEnvironment;
 use PVE::Storage;
+use PVE::Storage::Plugin;
 use PVE::Tools qw(run_command split_list);
 use PVE::QemuConfig;
 use PVE::QemuServer;
@@ -703,7 +704,7 @@ sub check_description_lengths {
 }
 
 sub check_storage_content {
-    log_info("Scanning for guest images on storages without images/rootdir content type..");
+    log_info("Checking storage content type configuration..");
 
     my $found;
 
@@ -714,6 +715,13 @@ sub check_storage_content {
 
 	next if !PVE::Storage::storage_check_enabled($storage_cfg, $storeid, undef, 1);
 
+	my $valid_content = PVE::Storage::Plugin::valid_content_types($scfg->{type});
+
+	if (scalar(keys $scfg->{content}->%*) == 0 && !$valid_content->{none}) {
+	    log_fail("storage '$storeid' does not support configured content type 'none'");
+	    delete $scfg->{content}->{none}; # scan for guest images below
+	}
+
 	next if $scfg->{content}->{images};
 	next if $scfg->{content}->{rootdir};
 
@@ -739,7 +747,7 @@ sub check_storage_content {
 	log_warn("PVE 7.0 enforces stricter content type checks. Guests referencing the above " .
 	    "volumes will not work until the storage configuration is fixed.");
     } else {
-	log_pass("none found");
+	log_pass("no problems found");
     }
 }
 
-- 
2.20.1

[pve-devel] applied-series: [PATCH-SERIES v2 manager] stricter storage rules for migration

[Thomas Lamprecht]

On 21.06.21 16:31, Fabian Ebner wrote:
> Changes from v1:
>     * dropped already applied patches
>     * rebased
>     * adapted/improved messages
> 
> Fabian Ebner (2):
>   pve6to7: add check for guest images on misconfigured storages
>   pve6to7: check for misconfigured content type 'none'
> 
>  PVE/CLI/pve6to7.pm | 50 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 


applied, thanks! I adapted the output a bit to list them newline-separated, was a bit
clearer here, in my case.

And maybe we want to further filter those by the ones actually found in VM configs?

As else it may be a false-positive, e.g., one may have added the same storage twice,
which can be OK as long as both entries do not share any content-type, as then there
should be no locking issue or other issues stemming from our base assumption that one
storage is there only once (a use case could be priv. separation).

It at least feels a bit odd to me to get warnings for disks which's VMID is neither a
guest nor a configured anywhere in the cluster.

Re: [pve-devel] applied-series: [PATCH-SERIES v2 manager] stricter storage rules for migration

[Fabian Ebner]

Am 21.06.21 um 17:18 schrieb Thomas Lamprecht:
> On 21.06.21 16:31, Fabian Ebner wrote:
>> Changes from v1:
>>      * dropped already applied patches
>>      * rebased
>>      * adapted/improved messages
>>
>> Fabian Ebner (2):
>>    pve6to7: add check for guest images on misconfigured storages
>>    pve6to7: check for misconfigured content type 'none'
>>
>>   PVE/CLI/pve6to7.pm | 50 ++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 50 insertions(+)
>>
> 
> 
> applied, thanks! I adapted the output a bit to list them newline-separated, was a bit
> clearer here, in my case.
> 
> And maybe we want to further filter those by the ones actually found in VM configs?
> 
> As else it may be a false-positive, e.g., one may have added the same storage twice,
> which can be OK as long as both entries do not share any content-type, as then there
> should be no locking issue or other issues stemming from our base assumption that one
> storage is there only once (a use case could be priv. separation).
> 
> It at least feels a bit odd to me to get warnings for disks which's VMID is neither a
> guest nor a configured anywhere in the cluster.
> 

Makes sense, I'll send a follow-up.