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

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

[Fabian Ebner]

Enforce that affected storages are enabled and have the correct content type
before doing a migration. And only pick up unreferenced guest volumes from
storages that support the content type.

Can be considered a breaking change, but it'll only affect scenarios where there
is a guest's volume on a misconfigured storage, so it can also be considered a
bug fix. Still, best to do it for a major release.

Another small change regarding misconfigured 'none' content type is thrown in,
patches storage #2 and manager #2, which also should rather be done for a major
release.


Dependency-wise, only patch storage #1 is relevant, as it breaks older migration
code, but should also be limited to scenarios with unused/unreferenced/vmstate
local disks *and* misconfigured storage.


pve-container:

Fabian Ebner (3):
  prefer storage_check_enabled over storage_check_node
  migrate: also test unused volumes
  migrate: enforce that rootdir content type is available


qemu-server:

Fabian Ebner (2):
  prefer storage_check_enabled over storage_check_node
  migrate: enforce that image content type is available

 PVE/API2/Qemu.pm   |  4 ++--
 PVE/QemuMigrate.pm | 31 +++++++++++++++++++------------
 PVE/QemuServer.pm  |  7 +++++--
 3 files changed, 26 insertions(+), 16 deletions(-)


pve-storage:

Fabian Ebner (2):
  vdisk_list: only scan storages with the correct content type(s)
  config: prevent empty content list when content type 'none' is not
    supported

 PVE/Storage.pm        | 8 ++------
 PVE/Storage/Plugin.pm | 4 ++++
 2 files changed, 6 insertions(+), 6 deletions(-)


pve-manager:

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 container 1/3] prefer storage_check_enabled over storage_check_node

[Fabian Ebner]

storage_check_enabled simply checks for the 'disable' option and then calls
storage_check_node.

While not strictly necessary for a second call where only the storage differs,
it is more future-proof: if support for a target storage is added at some point,
it might be easy to miss adapting the call.

For the migration checks, disabled storages are now always caught.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/PVE/API2/LXC.pm    |  4 ++--
 src/PVE/LXC/Migrate.pm | 10 +++++-----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 936debb..5977b2d 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -275,7 +275,7 @@ __PACKAGE__->register_method({
 	my $check_and_activate_storage = sub {
 	    my ($sid) = @_;
 
-	    my $scfg = PVE::Storage::storage_check_node($storage_cfg, $sid, $node);
+	    my $scfg = PVE::Storage::storage_check_enabled($storage_cfg, $sid, $node);
 
 	    raise_param_exc({ storage => "storage '$sid' does not support container directories"})
 		if !$scfg->{content}->{rootdir};
@@ -1378,7 +1378,7 @@ __PACKAGE__->register_method({
 	    PVE::Storage::storage_check_enabled($storecfg, $storage);
 	    if ($target) {
 		# check if storage is available on target node
-		PVE::Storage::storage_check_node($storecfg, $storage, $target);
+		PVE::Storage::storage_check_enabled($storecfg, $storage, $target);
 		# clone only works if target storage is shared
 		my $scfg = PVE::Storage::storage_config($storecfg, $storage);
 		die "can't clone to non-shared storage '$storage'\n" if !$scfg->{shared};
diff --git a/src/PVE/LXC/Migrate.pm b/src/PVE/LXC/Migrate.pm
index b5917e9..95c5799 100644
--- a/src/PVE/LXC/Migrate.pm
+++ b/src/PVE/LXC/Migrate.pm
@@ -63,8 +63,8 @@ sub prepare {
 	die "can't determine assigned storage for mount point '$ms'\n" if !$storage;
 
 	# check if storage is available on both nodes
-	my $scfg = PVE::Storage::storage_check_node($self->{storecfg}, $storage);
-	PVE::Storage::storage_check_node($self->{storecfg}, $storage, $self->{node});
+	my $scfg = PVE::Storage::storage_check_enabled($self->{storecfg}, $storage);
+	PVE::Storage::storage_check_enabled($self->{storecfg}, $storage, $self->{node});
 
 
 	if ($scfg->{shared}) {
@@ -134,8 +134,8 @@ sub phase1 {
 	my ($sid, $volname) = PVE::Storage::parse_volume_id($volid);
 
 	# check if storage is available on both nodes
-	my $scfg = PVE::Storage::storage_check_node($self->{storecfg}, $sid);
-	PVE::Storage::storage_check_node($self->{storecfg}, $sid, $self->{node});
+	my $scfg = PVE::Storage::storage_check_enabled($self->{storecfg}, $sid);
+	PVE::Storage::storage_check_enabled($self->{storecfg}, $sid, $self->{node});
 
 	if ($scfg->{shared}) {
 	    $self->log('info', "volume '$volid' is on shared storage '$sid'")
@@ -192,7 +192,7 @@ sub phase1 {
 	next if @{$dl->{$storeid}} == 0;
 
 	# check if storage is available on target node
-	PVE::Storage::storage_check_node($self->{storecfg}, $storeid, $self->{node});
+	PVE::Storage::storage_check_enabled($self->{storecfg}, $storeid, $self->{node});
 
 	PVE::Storage::foreach_volid($dl, sub {
 	    my ($volid, $sid, $volname) = @_;
-- 
2.30.2

[pve-devel] applied: [PATCH container 1/3] prefer storage_check_enabled over storage_check_node

[Thomas Lamprecht]

On 18.06.21 12:59, Fabian Ebner wrote:
> storage_check_enabled simply checks for the 'disable' option and then calls
> storage_check_node.
> 
> While not strictly necessary for a second call where only the storage differs,
> it is more future-proof: if support for a target storage is added at some point,
> it might be easy to miss adapting the call.
> 
> For the migration checks, disabled storages are now always caught.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/API2/LXC.pm    |  4 ++--
>  src/PVE/LXC/Migrate.pm | 10 +++++-----
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
>

applied, thanks!

[pve-devel] [PATCH container 2/3] migrate: also test unused volumes

[Fabian Ebner]

otherwise an unused volume on a disabled storage is silently left on the old
node, even if referenced.

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

needs the previous patch to work correctly (otherwise disable is still ignored)

 src/PVE/LXC/Migrate.pm | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/src/PVE/LXC/Migrate.pm b/src/PVE/LXC/Migrate.pm
index 95c5799..1e5cb1e 100644
--- a/src/PVE/LXC/Migrate.pm
+++ b/src/PVE/LXC/Migrate.pm
@@ -44,7 +44,7 @@ sub prepare {
     }
     $self->{was_running} = $running;
 
-    PVE::LXC::Config->foreach_volume($conf, sub {
+    PVE::LXC::Config->foreach_volume_full($conf, { include_unused => 1 }, sub {
 	my ($ms, $mountpoint) = @_;
 
 	my $volid = $mountpoint->{volume};
@@ -186,7 +186,7 @@ sub phase1 {
 	next if $scfg->{shared};
 	next if !PVE::Storage::storage_check_enabled($self->{storecfg}, $storeid, undef, 1);
 
-	# get list from PVE::Storage (for unused volumes)
+	# get list from PVE::Storage (for unreferenced volumes)
 	my $dl = PVE::Storage::vdisk_list($self->{storecfg}, $storeid, $vmid);
 
 	next if @{$dl->{$storeid}} == 0;
@@ -208,9 +208,8 @@ sub phase1 {
 	PVE::LXC::Config->foreach_volume($conf->{snapshots}->{$snapname}, $test_mp, $snapname);
     }
 
-    # finally all currently used volumes
-    PVE::LXC::Config->foreach_volume($conf, $test_mp);
-
+    # finally all current volumes
+    PVE::LXC::Config->foreach_volume_full($conf, { include_unused => 1 }, $test_mp);
 
     # additional checks for local storage
     foreach my $volid (keys %$volhash) {
-- 
2.30.2

[pve-devel] applied: [PATCH container 2/3] migrate: also test unused volumes

[Thomas Lamprecht]

On 18.06.21 12:59, Fabian Ebner wrote:
> otherwise an unused volume on a disabled storage is silently left on the old
> node, even if referenced.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> needs the previous patch to work correctly (otherwise disable is still ignored)
> 
>  src/PVE/LXC/Migrate.pm | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
>

applied, thanks!

[pve-devel] [PATCH container 3/3] migrate: enforce that rootdir content type is available

[Fabian Ebner]

and use it for the vdisk_list call too. This avoids scanning (and picking up
volumes from!) storages that are not even configured to hold container images.

Also serves a bit as a preparation to enforce content type on guest startup,
because now migration failure happens early and not only when trying to start
the guest on the remote node.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/PVE/LXC/Migrate.pm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/PVE/LXC/Migrate.pm b/src/PVE/LXC/Migrate.pm
index 1e5cb1e..3cd895d 100644
--- a/src/PVE/LXC/Migrate.pm
+++ b/src/PVE/LXC/Migrate.pm
@@ -66,6 +66,8 @@ sub prepare {
 	my $scfg = PVE::Storage::storage_check_enabled($self->{storecfg}, $storage);
 	PVE::Storage::storage_check_enabled($self->{storecfg}, $storage, $self->{node});
 
+	die "content type 'rootdir' is not available on storage '$storage'\n"
+	    if !$scfg->{content}->{rootdir};
 
 	if ($scfg->{shared}) {
 	    # PVE::Storage::activate_storage checks this for non-shared storages
@@ -187,13 +189,16 @@ sub phase1 {
 	next if !PVE::Storage::storage_check_enabled($self->{storecfg}, $storeid, undef, 1);
 
 	# get list from PVE::Storage (for unreferenced volumes)
-	my $dl = PVE::Storage::vdisk_list($self->{storecfg}, $storeid, $vmid);
+	my $dl = PVE::Storage::vdisk_list($self->{storecfg}, $storeid, $vmid, undef, 'rootdir');
 
 	next if @{$dl->{$storeid}} == 0;
 
 	# check if storage is available on target node
 	PVE::Storage::storage_check_enabled($self->{storecfg}, $storeid, $self->{node});
 
+	die "content type 'rootdir' is not available on storage '$storeid'\n"
+	    if !$scfg->{content}->{rootdir};
+
 	PVE::Storage::foreach_volid($dl, sub {
 	    my ($volid, $sid, $volname) = @_;
 
-- 
2.30.2

[pve-devel] applied: [PATCH container 3/3] migrate: enforce that rootdir content type is available

[Thomas Lamprecht]

On 18.06.21 12:59, Fabian Ebner wrote:
> and use it for the vdisk_list call too. This avoids scanning (and picking up
> volumes from!) storages that are not even configured to hold container images.
> 
> Also serves a bit as a preparation to enforce content type on guest startup,
> because now migration failure happens early and not only when trying to start
> the guest on the remote node.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/LXC/Migrate.pm | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
>

applied, thanks!

[pve-devel] [PATCH qemu-server 1/2] prefer storage_check_enabled over storage_check_node

[Fabian Ebner]

storage_check_enabled simply checks for the 'disable' option and then calls
storage_check_node.

While not strictly necessary for a second call where only the storage differs,
e.g. in case of clone, it is more future-proof: if support for a target storage
is added at some point, it might be easy to miss adapting the call.

For the migration checks, the situation is improved by now always catching
disabled (target) storages.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Qemu.pm   |  4 ++--
 PVE/QemuMigrate.pm | 10 +++++-----
 PVE/QemuServer.pm  |  4 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index bc313f9..eeb07d1 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -3024,7 +3024,7 @@ __PACKAGE__->register_method({
 	    PVE::Storage::storage_check_enabled($storecfg, $storage);
 	    if ($target) {
 		# check if storage is available on target node
-		PVE::Storage::storage_check_node($storecfg, $storage, $target);
+		PVE::Storage::storage_check_enabled($storecfg, $storage, $target);
 		# clone only works if target storage is shared
 		my $scfg = PVE::Storage::storage_config($storecfg, $storage);
 		die "can't clone to non-shared storage '$storage'\n" if !$scfg->{shared};
@@ -3687,7 +3687,7 @@ __PACKAGE__->register_method({
 	if (my $targetstorage = $param->{targetstorage}) {
 	    my $check_storage = sub {
 		my ($target_sid) = @_;
-		PVE::Storage::storage_check_node($storecfg, $target_sid, $target);
+		PVE::Storage::storage_check_enabled($storecfg, $target_sid, $target);
 		$rpcenv->check($authuser, "/storage/$target_sid", ['Datastore.AllocateSpace']);
 		my $scfg = PVE::Storage::storage_config($storecfg, $target_sid);
 		raise_param_exc({ targetstorage => "storage '$target_sid' does not support vm images"})
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 6375a15..2576196 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -340,8 +340,8 @@ sub prepare {
 	# check if storage is available on both nodes
 	my $targetsid = PVE::QemuServer::map_storage($self->{opts}->{storagemap}, $sid);
 
-	my $scfg = PVE::Storage::storage_check_node($self->{storecfg}, $sid);
-	PVE::Storage::storage_check_node($self->{storecfg}, $targetsid, $self->{node});
+	my $scfg = PVE::Storage::storage_check_enabled($self->{storecfg}, $sid);
+	PVE::Storage::storage_check_enabled($self->{storecfg}, $targetsid, $self->{node});
 
 	if ($scfg->{shared}) {
 	    # PVE::Storage::activate_storage checks this for non-shared storages
@@ -402,7 +402,7 @@ sub scan_local_volumes {
 
 	    my $targetsid = PVE::QemuServer::map_storage($self->{opts}->{storagemap}, $storeid);
 	    # check if storage is available on target node
-	    PVE::Storage::storage_check_node($storecfg, $targetsid, $self->{node});
+	    PVE::Storage::storage_check_enabled($storecfg, $targetsid, $self->{node});
 
 	    # grandfather in existing mismatches
 	    if ($targetsid ne $storeid) {
@@ -467,8 +467,8 @@ sub scan_local_volumes {
 
 	    my $targetsid = PVE::QemuServer::map_storage($self->{opts}->{storagemap}, $sid);
 	    # check if storage is available on both nodes
-	    my $scfg = PVE::Storage::storage_check_node($storecfg, $sid);
-	    PVE::Storage::storage_check_node($storecfg, $targetsid, $self->{node});
+	    my $scfg = PVE::Storage::storage_check_enabled($storecfg, $sid);
+	    PVE::Storage::storage_check_enabled($storecfg, $targetsid, $self->{node});
 
 	    return if $scfg->{shared};
 
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 6d9cf2d..bd00af3 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2456,8 +2456,8 @@ sub check_storage_availability {
 	return if !$sid;
 
 	# check if storage is available on both nodes
-	my $scfg = PVE::Storage::storage_check_node($storecfg, $sid);
-	PVE::Storage::storage_check_node($storecfg, $sid, $node);
+	my $scfg = PVE::Storage::storage_check_enabled($storecfg, $sid);
+	PVE::Storage::storage_check_enabled($storecfg, $sid, $node);
    });
 }
 
-- 
2.30.2

[pve-devel] applied: [PATCH qemu-server 1/2] prefer storage_check_enabled over storage_check_node

[Thomas Lamprecht]

On 18.06.21 12:59, Fabian Ebner wrote:
> storage_check_enabled simply checks for the 'disable' option and then calls
> storage_check_node.
> 
> While not strictly necessary for a second call where only the storage differs,
> e.g. in case of clone, it is more future-proof: if support for a target storage
> is added at some point, it might be easy to miss adapting the call.
> 
> For the migration checks, the situation is improved by now always catching
> disabled (target) storages.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/API2/Qemu.pm   |  4 ++--
>  PVE/QemuMigrate.pm | 10 +++++-----
>  PVE/QemuServer.pm  |  4 ++--
>  3 files changed, 9 insertions(+), 9 deletions(-)
> 
>

applied, thanks!

[pve-devel] [PATCH qemu-server 2/2] migrate: enforce that image content type is available

[Fabian Ebner]

and use it for the vdisk_list call too. This avoids scanning (and picking up
volumes from!) storages that are not even configured to hold images.

Previously, the content type was only enforced when a storage map was present.

Also serves a bit as a preparation to enforce content type on guest startup,
because now migration failure happens early and not only when trying to start
the guest on the remote node.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/QemuMigrate.pm | 25 ++++++++++++++++---------
 PVE/QemuServer.pm  |  3 +++
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 2576196..5f37890 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -341,7 +341,14 @@ sub prepare {
 	my $targetsid = PVE::QemuServer::map_storage($self->{opts}->{storagemap}, $sid);
 
 	my $scfg = PVE::Storage::storage_check_enabled($self->{storecfg}, $sid);
-	PVE::Storage::storage_check_enabled($self->{storecfg}, $targetsid, $self->{node});
+	my $target_scfg = PVE::Storage::storage_check_enabled(
+	    $self->{storecfg},
+	    $targetsid,
+	    $self->{node},
+	);
+
+	die "content type 'images' is not available on storage '$targetsid'\n"
+	    if !$target_scfg->{content}->{images};
 
 	if ($scfg->{shared}) {
 	    # PVE::Storage::activate_storage checks this for non-shared storages
@@ -396,20 +403,20 @@ sub scan_local_volumes {
 	    next if !PVE::Storage::storage_check_enabled($storecfg, $storeid, undef, 1);
 
 	    # get list from PVE::Storage (for unused volumes)
-	    my $dl = PVE::Storage::vdisk_list($storecfg, $storeid, $vmid);
+	    my $dl = PVE::Storage::vdisk_list($storecfg, $storeid, $vmid, undef, 'images');
 
 	    next if @{$dl->{$storeid}} == 0;
 
 	    my $targetsid = PVE::QemuServer::map_storage($self->{opts}->{storagemap}, $storeid);
 	    # check if storage is available on target node
-	    PVE::Storage::storage_check_enabled($storecfg, $targetsid, $self->{node});
+	    my $target_scfg = PVE::Storage::storage_check_enabled(
+		$storecfg,
+		$targetsid,
+		$self->{node},
+	    );
 
-	    # grandfather in existing mismatches
-	    if ($targetsid ne $storeid) {
-		my $target_scfg = PVE::Storage::storage_config($storecfg, $targetsid);
-		die "content type 'images' is not available on storage '$targetsid'\n"
-		    if !$target_scfg->{content}->{images};
-	    }
+	    die "content type 'images' is not available on storage '$targetsid'\n"
+		if !$target_scfg->{content}->{images};
 
 	    my $bwlimit = PVE::Storage::get_bandwidth_limit(
 		'migration',
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index bd00af3..804cf79 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2458,6 +2458,9 @@ sub check_storage_availability {
 	# check if storage is available on both nodes
 	my $scfg = PVE::Storage::storage_check_enabled($storecfg, $sid);
 	PVE::Storage::storage_check_enabled($storecfg, $sid, $node);
+
+	die "content type 'images' is not available on storage '$sid'\n"
+	    if !$scfg->{content}->{images};
    });
 }
 
-- 
2.30.2

[pve-devel] applied: [PATCH qemu-server 2/2] migrate: enforce that image content type is available

[Thomas Lamprecht]

On 18.06.21 12:59, Fabian Ebner wrote:
> and use it for the vdisk_list call too. This avoids scanning (and picking up
> volumes from!) storages that are not even configured to hold images.
> 
> Previously, the content type was only enforced when a storage map was present.
> 
> Also serves a bit as a preparation to enforce content type on guest startup,
> because now migration failure happens early and not only when trying to start
> the guest on the remote node.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/QemuMigrate.pm | 25 ++++++++++++++++---------
>  PVE/QemuServer.pm  |  3 +++
>  2 files changed, 19 insertions(+), 9 deletions(-)
> 
>

applied, thanks!

[pve-devel] [PATCH storage 1/2] vdisk_list: only scan storages with the correct content type(s)

[Fabian Ebner]

The enabled check in the lower loop is now redundant and can be removed.

If storeid is provided, initialize the result hash accordingly, mainly for
backwards compatibility (needed by a caller in pve-manager's Ceph/Pools.pm and
the migration code in pve-container and qemu-server), but it also is less
surprising in general.

Remaining vdisk_list users that do not specify a content type are:
    1. pve-manager's Pool/Ceph.pm, but the content type for RBD can only be
       rootdir and images, so the storage is scanned (if enabled, same as
       before).
    2. pve-container migration
    3. qemu-server migration
For the latter two, it's planned to enforce content type, so the change is fine
too.

This also means that for iscsi(direct) plugins with content type 'none', i.e.
"use LUNs directly" does not return the list of images anymore, but that was
rather a bug anyways as they're not virtual disks then:
    0.0.0.scsi-36001405b8f2772e13a04b8e9390db13d
All of the remaining callers not using content types (see above) are fine with
that change too.

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

Breaks old migration behavior when there are unused/unreferenced/vmstate volumes
on a misconfigured storage!

 PVE/Storage.pm | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 3aa2100..7312eba 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -936,7 +936,7 @@ sub vdisk_list {
 
     storage_check_enabled($cfg, $storeid) if ($storeid);
 
-    my $res = {};
+    my $res = $storeid ? { $storeid => [] } : {};
 
     # prepare/activate/refresh all storages
 
@@ -963,12 +963,8 @@ sub vdisk_list {
 
     activate_storage_list($cfg, $storage_list, $cache);
 
-    # FIXME PVE 7.0: only scan storages with the correct content types
-    my $scan = defined($ctype) ? $storage_list : [ keys %{$ids} ];
-
-    foreach my $sid (@{$scan}) {
+    for my $sid ($storage_list->@*) {
 	next if $storeid && $storeid ne $sid;
-	next if !storage_check_enabled($cfg, $sid, undef, 1);
 
 	my $scfg = $ids->{$sid};
 	my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
-- 
2.30.2

[pve-devel] applied: [PATCH storage 1/2] vdisk_list: only scan storages with the correct content type(s)

[Thomas Lamprecht]

On 18.06.21 12:59, Fabian Ebner wrote:
> The enabled check in the lower loop is now redundant and can be removed.
> 
> If storeid is provided, initialize the result hash accordingly, mainly for
> backwards compatibility (needed by a caller in pve-manager's Ceph/Pools.pm and
> the migration code in pve-container and qemu-server), but it also is less
> surprising in general.
> 
> Remaining vdisk_list users that do not specify a content type are:
>     1. pve-manager's Pool/Ceph.pm, but the content type for RBD can only be
>        rootdir and images, so the storage is scanned (if enabled, same as
>        before).
>     2. pve-container migration
>     3. qemu-server migration
> For the latter two, it's planned to enforce content type, so the change is fine
> too.
> 
> This also means that for iscsi(direct) plugins with content type 'none', i.e.
> "use LUNs directly" does not return the list of images anymore, but that was
> rather a bug anyways as they're not virtual disks then:
>     0.0.0.scsi-36001405b8f2772e13a04b8e9390db13d
> All of the remaining callers not using content types (see above) are fine with
> that change too.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Breaks old migration behavior when there are unused/unreferenced/vmstate volumes
> on a misconfigured storage!
> 
>  PVE/Storage.pm | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
>

applied, thanks!

[pve-devel] [PATCH storage 2/2] config: prevent empty content list when content type 'none' is not supported

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/Storage/Plugin.pm | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index 318d13a..d25e23f 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -344,6 +344,10 @@ sub decode_value {
 	    die "unable to combine 'none' with other content types\n";
 	}
 
+	if (scalar(keys $res->%*) == 0 && !$valid_content->{none}) {
+	    die "storage does not support content type 'none'\n";
+	}
+
 	return $res;
     } elsif ($key eq 'format') {
 	my $valid_formats = $def->{format}->[0];
-- 
2.30.2

[pve-devel] applied: [PATCH storage 2/2] config: prevent empty content list when content type 'none' is not supported

[Thomas Lamprecht]

On 18.06.21 12:59, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/Storage/Plugin.pm | 4 ++++
>  1 file changed, 4 insertions(+)
> 
>

applied, thanks!

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

[Fabian Ebner]

migration 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>
---

If Lorenz's patches that enforce the correct content type on guest startup
are applied, the warning message should be extended/generalised here of course.

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

diff --git a/PVE/CLI/pve6to7.pm b/PVE/CLI/pve6to7.pm
index fc779e4f..3e8af0a0 100644
--- a/PVE/CLI/pve6to7.pm
+++ b/PVE/CLI/pve6to7.pm
@@ -660,6 +660,47 @@ sub check_custom_pool_roles {
     }
 }
 
+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 on migration, so migrating " .
+	    "guests referencing those volumes will not work anymore.");
+    } else {
+	log_pass("none found");
+    }
+}
+
 sub check_misc {
     print_header("MISCELLANEOUS CHECKS");
     my $ssh_config = eval { PVE::Tools::file_get_contents('/root/.ssh/config') };
@@ -753,6 +794,7 @@ sub check_misc {
     check_backup_retention_settings();
     check_cifs_credential_location();
     check_custom_pool_roles();
+    check_storage_content();
 }
 
 __PACKAGE__->register_method ({
-- 
2.20.1

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

[Thomas Lamprecht]

On 18.06.21 12:59, Fabian Ebner wrote:
> migration 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>
> ---
> 
> If Lorenz's patches that enforce the correct content type on guest startup
> are applied, the warning message should be extended/generalised here of course.
> 

at least the container ones are applied now, and this needs a rebase anyway
as the description length check was applied in-between.

>  PVE/CLI/pve6to7.pm | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/PVE/CLI/pve6to7.pm b/PVE/CLI/pve6to7.pm
> index fc779e4f..3e8af0a0 100644
> --- a/PVE/CLI/pve6to7.pm
> +++ b/PVE/CLI/pve6to7.pm
> @@ -660,6 +660,47 @@ sub check_custom_pool_roles {
>      }
>  }
>  
> +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 on migration, so migrating " .
> +	    "guests referencing those volumes will not work anymore.");
> +    } else {
> +	log_pass("none found");
> +    }
> +}
> +
>  sub check_misc {
>      print_header("MISCELLANEOUS CHECKS");
>      my $ssh_config = eval { PVE::Tools::file_get_contents('/root/.ssh/config') };
> @@ -753,6 +794,7 @@ sub check_misc {
>      check_backup_retention_settings();
>      check_cifs_credential_location();
>      check_custom_pool_roles();
> +    check_storage_content();
>  }
>  
>  __PACKAGE__->register_method ({
> 

[pve-devel] [PATCH 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

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

diff --git a/PVE/CLI/pve6to7.pm b/PVE/CLI/pve6to7.pm
index 3e8af0a0..890fbcbb 100644
--- a/PVE/CLI/pve6to7.pm
+++ b/PVE/CLI/pve6to7.pm
@@ -17,6 +17,7 @@ use PVE::INotify;
 use PVE::JSONSchema;
 use PVE::RPCEnvironment;
 use PVE::Storage;
+use PVE::Storage::Plugin;
 use PVE::Tools qw(run_command split_list);
 use PVE::QemuServer;
 use PVE::VZDump::Common;
@@ -661,7 +662,7 @@ sub check_custom_pool_roles {
 }
 
 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;
 
@@ -672,6 +673,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};
 
-- 
2.20.1