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

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

[Fabian Ebner]

This reverts commit a44c18925d223a971296801a0985db34707ada4d and adds a reminder
comment.

The mentioned commit is actually a backwards-incompatible change that leads to
slightly different behavior when migrating a VM with volumes on a misconfigured
storage. For example, unreferenced volumes on a misconfigured storage won't be
picked up, even though they were before. And for referenced volumes on a
misconfigured storage, the disk size would not be updated on migration anymore.

We should wait until the next major release for this change and then also
re-evaluate the migration behavior with misconfigured disks.

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

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 18c03ec..19f675d 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 = { map { $_ => [] } keys %{$ids} };
+    my $res = {};
 
     # prepare/activate/refresh all storages
 
@@ -964,8 +964,10 @@ sub vdisk_list {
 
     activate_storage_list($cfg, $storage_list, $cache);
 
-    foreach my $sid (@{$storage_list}) {
+    # FIXME PVE 7.0: only scan storages with the correct content types
+    foreach my $sid (keys %$ids) {
 	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/4] vdisk list: allow specifying content type

[Fabian Ebner]

and only scan storages that support it if specified.

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

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 19f675d..eaa86fb 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -932,7 +932,7 @@ sub vdisk_free {
 }
 
 sub vdisk_list {
-    my ($cfg, $storeid, $vmid, $vollist) = @_;
+    my ($cfg, $storeid, $vmid, $vollist, $ctype) = @_;
 
     my $ids = $cfg->{ids};
 
@@ -955,6 +955,7 @@ sub vdisk_list {
 	    next if $storeid && $storeid ne $sid;
 	    next if !storage_check_enabled($cfg, $sid, undef, 1);
 	    my $content = $ids->{$sid}->{content};
+	    next if defined($ctype) && !$content->{$ctype};
 	    next if !($content->{rootdir} || $content->{images});
 	    push @$storage_list, $sid;
 	}
@@ -965,7 +966,9 @@ sub vdisk_list {
     activate_storage_list($cfg, $storage_list, $cache);
 
     # FIXME PVE 7.0: only scan storages with the correct content types
-    foreach my $sid (keys %$ids) {
+    my $scan = defined($ctype) ? $storage_list : [ keys %{$ids} ];
+
+    foreach my $sid (@{$scan}) {
 	next if $storeid && $storeid ne $sid;
 	next if !storage_check_enabled($cfg, $sid, undef, 1);
 
-- 
2.20.1

[pve-devel] [PATCH container 3/4] filter by content type when using vdisk_list

[Fabian Ebner]

except for migration, where it would be subtly backwards-incompatible.

Also allows to get rid of the existing filtering hack in rescan().

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

Dependency bump for pve-storage is needed.

 src/PVE/LXC.pm | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 6395d12..7e6f378 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -836,7 +836,7 @@ sub destroy_lxc_container {
     });
 
     if ($purge_unreferenced) { # also remove unreferenced disk
-	my $vmdisks = PVE::Storage::vdisk_list($storage_cfg, undef, $vmid);
+	my $vmdisks = PVE::Storage::vdisk_list($storage_cfg, undef, $vmid, undef, 'rootdir');
 	PVE::Storage::foreach_volid($vmdisks, sub {
 	    my ($volid, $sid, $volname, $d) = @_;
 	    eval { PVE::Storage::vdisk_free($storage_cfg, $volid) };
@@ -2042,7 +2042,7 @@ sub update_unused {
 sub scan_volids {
     my ($cfg, $vmid) = @_;
 
-    my $info = PVE::Storage::vdisk_list($cfg, undef, $vmid);
+    my $info = PVE::Storage::vdisk_list($cfg, undef, $vmid, undef, 'rootdir');
 
     my $all_volumes = {};
     foreach my $storeid (keys %$info) {
@@ -2062,12 +2062,6 @@ sub rescan {
 
     my $cfg = PVE::Storage::config();
 
-    # FIXME: Remove once our RBD plugin can handle CT and VM on a single storage
-    # see: https://pve.proxmox.com/pipermail/pve-devel/2018-July/032900.html
-    foreach my $stor (keys %{$cfg->{ids}}) {
-	delete($cfg->{ids}->{$stor}) if !$cfg->{ids}->{$stor}->{content}->{rootdir};
-    }
-
     print "rescan volumes...\n";
     my $all_volumes = scan_volids($cfg, $vmid);
 
-- 
2.20.1

[pve-devel] [PATCH qemu-server 4/4] filter by content type when using vdisk_list

[Fabian Ebner]

except for migration, where it would be subtly backwards-incompatible. Since
there is a scan_volids call for migration, we can't default to filtering in
scan_volids just yet.

Also allows to get rid of the existing filtering hack in rescan().

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

Dependency bump for pve-storage is needed.

 PVE/QemuServer.pm | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 1c0b5c2..9fa5546 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2111,7 +2111,7 @@ sub destroy_vm {
     });
 
     if ($purge_unreferenced) { # also remove unreferenced disk
-	my $vmdisks = PVE::Storage::vdisk_list($storecfg, undef, $vmid);
+	my $vmdisks = PVE::Storage::vdisk_list($storecfg, undef, $vmid, undef, 'images');
 	PVE::Storage::foreach_volid($vmdisks, sub {
 	    my ($volid, $sid, $volname, $d) = @_;
 	    eval { PVE::Storage::vdisk_free($storecfg, $volid) };
@@ -5998,10 +5998,11 @@ my $restore_destroy_volumes = sub {
     }
 };
 
+# FIXME For PVE 7.0, remove $content_type and always use 'images'
 sub scan_volids {
-    my ($cfg, $vmid) = @_;
+    my ($cfg, $vmid, $content_type) = @_;
 
-    my $info = PVE::Storage::vdisk_list($cfg, undef, $vmid);
+    my $info = PVE::Storage::vdisk_list($cfg, undef, $vmid, undef, $content_type);
 
     my $volid_hash = {};
     foreach my $storeid (keys %$info) {
@@ -6094,14 +6095,8 @@ sub rescan {
 
     my $cfg = PVE::Storage::config();
 
-    # FIXME: Remove once our RBD plugin can handle CT and VM on a single storage
-    # see: https://pve.proxmox.com/pipermail/pve-devel/2018-July/032900.html
-    foreach my $stor (keys %{$cfg->{ids}}) {
-	delete($cfg->{ids}->{$stor}) if ! $cfg->{ids}->{$stor}->{content}->{images};
-    }
-
     print "rescan volumes...\n";
-    my $volid_hash = scan_volids($cfg, $vmid);
+    my $volid_hash = scan_volids($cfg, $vmid, 'images');
 
     my $updatefn =  sub {
 	my ($vmid) = @_;
-- 
2.20.1

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

[Thomas Lamprecht]

On 22.03.21 15:32, Fabian Ebner wrote:
> This reverts commit a44c18925d223a971296801a0985db34707ada4d and adds a reminder
> comment.
> 
> The mentioned commit is actually a backwards-incompatible change that leads to
> slightly different behavior when migrating a VM with volumes on a misconfigured
> storage. For example, unreferenced volumes on a misconfigured storage won't be
> picked up, even though they were before. And for referenced volumes on a
> misconfigured storage, the disk size would not be updated on migration anymore.
> 
> We should wait until the next major release for this change and then also
> re-evaluate the migration behavior with misconfigured disks.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/Storage.pm | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH storage 2/4] vdisk list: allow specifying content type

[Thomas Lamprecht]

On 22.03.21 15:32, Fabian Ebner wrote:
> and only scan storages that support it if specified.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/Storage.pm | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH container 3/4] filter by content type when using vdisk_list

[Thomas Lamprecht]

On 22.03.21 15:32, Fabian Ebner wrote:
> except for migration, where it would be subtly backwards-incompatible.
> 
> Also allows to get rid of the existing filtering hack in rescan().
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Dependency bump for pve-storage is needed.
> 
>  src/PVE/LXC.pm | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
>

applied, with d/control bump for pve-storage thanks!

[pve-devel] applied: [PATCH qemu-server 4/4] filter by content type when using vdisk_list

[Thomas Lamprecht]

On 22.03.21 15:32, Fabian Ebner wrote:
> except for migration, where it would be subtly backwards-incompatible. Since
> there is a scan_volids call for migration, we can't default to filtering in
> scan_volids just yet.
> 
> Also allows to get rid of the existing filtering hack in rescan().
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Dependency bump for pve-storage is needed.
> 
>  PVE/QemuServer.pm | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
>

applied, with d/control bump for pve-storage thanks!