[pve-devel] [PATCH storage 1/2] vdisk list: only collect images from storages with an appropriate content type

[pve-devel] [PATCH storage 1/2] vdisk list: only collect images from storages with an appropriate content type

[Fabian Ebner]

Only these storages are activated in the first place, and it's bad behavior to
list images when no appropriate content type is not set.

For example, on VM destruction, this avoids unreferenced images to be deleted
from a storage with only 'backup' content type set, which is supposedly what
happened in this[0] forum thread.

(Some) callers expect all keys to be present and valid array references in the
result, so initialization is needed.

Now, the enabled check is already done by the preceding code for every element
that is iterated over, and thus isn't needed in the main loop anymore.

[0]: https://forum.proxmox.com/threads/erasing-all-vm-disks-after-a-failed-vm-migration-task.85068

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

Why isn't 'iterand' a word?

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

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 8ee2c92..18c03ec 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -938,7 +938,7 @@ sub vdisk_list {
 
     storage_check_enabled($cfg, $storeid) if ($storeid);
 
-    my $res = {};
+    my $res = { map { $_ => [] } keys %{$ids} };
 
     # prepare/activate/refresh all storages
 
@@ -964,9 +964,8 @@ sub vdisk_list {
 
     activate_storage_list($cfg, $storage_list, $cache);
 
-    foreach my $sid (keys %$ids) {
+    foreach 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.20.1

[pve-devel] [PATCH storage 2/2] tests: zfs: complain when a first sub-test dies

[Fabian Ebner]

Previously, $count was not increased and no test failure was reported.

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

Found by messing up in the previous patch at first with:
    my $res = map { $_ => [] } keys %{$ids};
instead of
    my $res = { map { $_ => [] } keys %{$ids} };
and wondering why it failed in "production", but the test ran through.

 test/run_test_zfspoolplugin.pl | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/test/run_test_zfspoolplugin.pl b/test/run_test_zfspoolplugin.pl
index 9c5e841..2f63f1b 100755
--- a/test/run_test_zfspoolplugin.pl
+++ b/test/run_test_zfspoolplugin.pl
@@ -2714,7 +2714,10 @@ for (my $i = $start_test; $i <= $end_test; $i++) {
     eval {
 	$tests->{$i}();
     };
-    warn $@ if $@;
+    if (my $err = $@) {
+	warn $err;
+	$count++;
+    }
     cleanup_zfs();
 }
 
-- 
2.20.1

[pve-devel] applied: [PATCH storage 2/2] tests: zfs: complain when a first sub-test dies

[Thomas Lamprecht]

On 12.03.21 10:50, Fabian Ebner wrote:
> Previously, $count was not increased and no test failure was reported.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Found by messing up in the previous patch at first with:
>     my $res = map { $_ => [] } keys %{$ids};
> instead of
>     my $res = { map { $_ => [] } keys %{$ids} };
> and wondering why it failed in "production", but the test ran through.
> 
>  test/run_test_zfspoolplugin.pl | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
>

applied, thanks!

Re: [pve-devel] [PATCH storage 1/2] vdisk list: only collect images from storages with an appropriate content type

[Thomas Lamprecht]

On 12.03.21 10:50, Fabian Ebner wrote:
> Only these storages are activated in the first place, and it's bad behavior to
> list images when no appropriate content type is not set.
> 
> For example, on VM destruction, this avoids unreferenced images to be deleted
> from a storage with only 'backup' content type set, which is supposedly what
> happened in this[0] forum thread.
> 

Seems to be related to a patch of mine, not that long ago:
https://git.proxmox.com/?p=qemu-server.git;a=commitdiff;h=75854662699b261915289f36e8fbe953c5f77b7c

That flag needs to still be made available in the UI, though

> (Some) callers expect all keys to be present and valid array references in the
> result, so initialization is needed.
> 
> Now, the enabled check is already done by the preceding code for every element
> that is iterated over, and thus isn't needed in the main loop anymore.
> 
> [0]: https://forum.proxmox.com/threads/erasing-all-vm-disks-after-a-failed-vm-migration-task.85068
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Why isn't 'iterand' a word?
> 
>  PVE/Storage.pm | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 8ee2c92..18c03ec 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -938,7 +938,7 @@ sub vdisk_list {
>  
>      storage_check_enabled($cfg, $storeid) if ($storeid);
>  
> -    my $res = {};
> +    my $res = { map { $_ => [] } keys %{$ids} };
>  
>      # prepare/activate/refresh all storages
>  
> @@ -964,9 +964,8 @@ sub vdisk_list {
>  
>      activate_storage_list($cfg, $storage_list, $cache);
>  
> -    foreach my $sid (keys %$ids) {
> +    foreach 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});
> 

[pve-devel] applied: [PATCH storage 1/2] vdisk list: only collect images from storages with an appropriate content type

[Thomas Lamprecht]

On 12.03.21 10:50, Fabian Ebner wrote:
> Only these storages are activated in the first place, and it's bad behavior to
> list images when no appropriate content type is not set.
> 
> For example, on VM destruction, this avoids unreferenced images to be deleted
> from a storage with only 'backup' content type set, which is supposedly what
> happened in this[0] forum thread.
> 
> (Some) callers expect all keys to be present and valid array references in the
> result, so initialization is needed.
> 
> Now, the enabled check is already done by the preceding code for every element
> that is iterated over, and thus isn't needed in the main loop anymore.
> 
> [0]: https://forum.proxmox.com/threads/erasing-all-vm-disks-after-a-failed-vm-migration-task.85068
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Why isn't 'iterand' a word?
> 

Seems like it has at least some use in the wild, and may be clear enough to
no cause confusion when used.
https://softwareengineering.stackexchange.com/a/310191

>  PVE/Storage.pm | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
>

applied, thanks!