[pve-devel] [PATCH SERIES V4 pve-container/qemu-server/pve-manager 0/7] fix #3711 & adapt drive detach/remove behavior

[pve-devel] [PATCH SERIES V4 pve-container/qemu-server/pve-manager 0/7] fix #3711 & adapt drive detach/remove behavior

[Stefan Hrdlicka]

V3 -> V4:
# pve-container
* add check for linked clones to avoid partially deleting volumes

# pve-container
* remove ignore_storage_errors
* ignores now errors with PVE::Storage::path

# pve-manager
* remove checkbox ignore errors checkbox from VM delete

V2 -> V3:
# pve-storage
* change removed

# pve-container
* remove storage_exists
* reduce code lines used

# qemu-server
* reduce code lines used

# pve-manager
* no change


V1 -> V2:
# overall
* matched detaching/removing drives behavior for VM & containers
  It currently works this way:
  - Detach drive
  - drive shows up as unused
  - remove drive
  - drive will be removed without removing data (obviously)

# pve-storage
* added storage_exists function for matching detach/remove behavoir

# pve-container
* review fixes:
    * variable naming
    * desciption string adapted
    * moved eval further up the call chain
    * removed ticket number form cleanup
* check if storage exists for unused disks

# qemu-server
* add same force option as for containers
* match detach/remove behavoir between VM/container
* shorten line

# pve-manager
* added same option for VMs as for containers



Stefan Hrdlicka (4):
  fix #3711: optionally allow CT deletion to complete on disk volume
    removal errors
  adapt behavior for detaching/removing a mount point
  add linked clone check for LXC container template deletion
  cleanup: remove spaces from empty lines

 src/PVE/API2/LXC.pm   |  8 ++++++++
 src/PVE/LXC.pm        | 28 +++++++++++++++++++++-------
 src/PVE/LXC/Config.pm |  6 +++++-
 3 files changed, 34 insertions(+), 8 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH V4 pve-container 1/7] fix #3711: optionally allow CT deletion to complete on disk volume removal errors

[Stefan Hrdlicka]

Make it possible to delete a container whoes underlying storage is no
longer available. This will just write an warning instead of dying.

Without setting the option ignore-storage-errors=1 a delete will still
fail, like it did before the changes. With this option set it will try to
delete the volume from the storage. If this fails it writes a warning.

review fixes
- rename parameter to ignore-storage-errors
- move eval further up the call chain

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 src/PVE/API2/LXC.pm | 8 ++++++++
 src/PVE/LXC.pm      | 6 ++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 79aecaa..19806fa 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -700,6 +700,13 @@ __PACKAGE__->register_method({
 		    ." enabled storages which are not referenced in the config.",
 		optional => 1,
 	    },
+	    'ignore-storage-errors' => {
+		type => 'boolean',
+		description => 'If set, this will ignore errors when trying to remove'
+		    . ' container storage.',
+		default => 0,
+		optional => 1,
+	    }
 	},
     },
     returns => {
@@ -761,6 +768,7 @@ __PACKAGE__->register_method({
 		$conf,
 		{ lock => 'destroyed' },
 		$param->{'destroy-unreferenced-disks'},
+		$param->{'ignore-storage-errors'},
 	    );
 
 	    PVE::AccessControl::remove_vm_access($vmid);
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 4bbd739..fe68f75 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -861,7 +861,8 @@ sub delete_mountpoint_volume {
 }
 
 sub destroy_lxc_container {
-    my ($storage_cfg, $vmid, $conf, $replacement_conf, $purge_unreferenced) = @_;
+    my ($storage_cfg, $vmid, $conf, $replacement_conf,
+	$purge_unreferenced, $ignore_storage_errors) = @_;
 
     my $volids = {};
     my $remove_volume = sub {
@@ -872,7 +873,8 @@ sub destroy_lxc_container {
 	return if $volids->{$volume};
 	$volids->{$volume} = 1;
 
-	delete_mountpoint_volume($storage_cfg, $vmid, $volume);
+	eval { delete_mountpoint_volume($storage_cfg, $vmid, $volume); };
+	die $@ if !$ignore_storage_errors && $@;
     };
     PVE::LXC::Config->foreach_volume_full($conf, {include_unused => 1}, $remove_volume);
 
-- 
2.30.2

[pve-devel] [PATCH V4 pve-container 2/7] adapt behavior for detaching/removing a mount point

[Stefan Hrdlicka]

detach of a mount point with a removed underlying storage causes it to
be labeled as a an 'unused disk'
remove of a 'unused disk' with a removed underlying storage causes it to
be removed from the configuration

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 src/PVE/LXC/Config.pm | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index b1f779b..e8fbd86 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1421,13 +1421,17 @@ sub vmconfig_apply_pending {
     foreach my $opt (sort keys %$pending_delete_hash) {
 	next if $selection && !$selection->{$opt};
 	eval {
+	    my ($storeid, $volname) = PVE::Storage::parse_volume_id($conf->{$opt});
 	    if ($opt =~ m/^mp(\d+)$/) {
 		my $mp = $class->parse_volume($opt, $conf->{$opt});
 		if ($mp->{type} eq 'volume') {
 		    $class->add_unused_volume($conf, $mp->{volume})
 			if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
 		}
-	    } elsif ($opt =~ m/^unused(\d+)$/) {
+	    } elsif (
+		$opt =~ m/^unused(\d+)$/
+		&& PVE::Storage::storage_config($storecfg, $storeid, 1)
+	    ) {
 		PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $conf->{$opt})
 		    if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
 	    }
-- 
2.30.2

[pve-devel] [PATCH V4 pve-container 3/7] add linked clone check for LXC container template deletion

[Stefan Hrdlicka]

prevent partial storage deletion if the template has a linked clone
container

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 src/PVE/LXC.pm | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index fe68f75..7164462 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -865,6 +865,18 @@ sub destroy_lxc_container {
 	$purge_unreferenced, $ignore_storage_errors) = @_;
 
     my $volids = {};
+
+    if ($conf->{template}) {
+	PVE::LXC::Config->foreach_volume_full($conf, {incldue_unused => 1}, sub {
+	    my ($ms, $mountpoint) = @_;
+	    my $volid = $mountpoint->{volume};
+	    return if !$volid || $volid =~ m|^/|;
+	    my $result;
+	    eval{ $result = PVE::Storage::volume_is_base_and_used($storage_cfg, $volid) };
+	    die "base volume '$volid' is still in use by linked cloned\n" if $result;
+	});
+    }
+
     my $remove_volume = sub {
 	my ($ms, $mountpoint) = @_;
 
-- 
2.30.2

[pve-devel] [PATCH V4 pve-container 4/7] cleanup: remove spaces from empty lines

[Stefan Hrdlicka]

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 src/PVE/LXC.pm | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 7164462..7527106 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -668,7 +668,7 @@ sub update_lxc_config {
 
     # some init scripts expect a linux terminal (turnkey).
     $raw .= "lxc.environment = TERM=linux\n";
-    
+
     my $utsname = $conf->{hostname} || "CT$vmid";
     $raw .= "lxc.uts.name = $utsname\n";
 
@@ -1702,14 +1702,14 @@ sub __mountpoint_mount {
     my $type = $mountpoint->{type};
     my $quota = !$snapname && !$mountpoint->{ro} && $mountpoint->{quota};
     my $mounted_dev;
-    
+
     return if !$volid || !$mount;
 
     $mount =~ s!/+!/!g;
 
     my $mount_path;
     my ($mpfd, $parentfd, $last_dir);
-    
+
     if (defined($rootdir)) {
 	($rootdir, $mount_path, $mpfd, $parentfd, $last_dir) =
 	    __mount_prepare_rootdir($rootdir, $mount, $rootuid, $rootgid);
@@ -1718,7 +1718,7 @@ sub __mountpoint_mount {
     if (defined($stage_mount)) {
 	$mount_path = $rootdir;
     }
-    
+
     my ($storage, $volname) = PVE::Storage::parse_volume_id($volid, 1);
 
     die "unknown snapshot path for '$volid'" if !$storage && defined($snapname);
@@ -1827,7 +1827,7 @@ sub __mountpoint_mount {
 	warn "cannot enable quota control for bind mounts\n" if $quota;
 	return wantarray ? ($volid, 0, undef) : $volid;
     }
-    
+
     die "unsupported storage";
 }
 
-- 
2.30.2

[pve-devel] [PATCH V4 qemu-server 5/7] ignore PVE::Storage::path errors when deleting VMs

[Stefan Hrdlicka]

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 PVE/QemuServer.pm | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 51e9a51..331677f 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2341,10 +2341,9 @@ sub destroy_vm {
 
 		my $volid = $drive->{file};
 		return if !$volid || $volid =~ m|^/|;
-
-		die "base volume '$volid' is still in use by linked cloned\n"
-		    if PVE::Storage::volume_is_base_and_used($storecfg, $volid);
-
+		my $result;
+		eval { $result = PVE::Storage::volume_is_base_and_used($storecfg, $volid) };
+		die "base volume '$volid' is still in use by linked cloned\n" if $result;
 	});
     }
 
@@ -2357,7 +2356,8 @@ sub destroy_vm {
 	return if !$volid || $volid =~ m|^/|;
 	return if $volids->{$volid};
 
-	my ($path, $owner) = PVE::Storage::path($storecfg, $volid);
+	my ($path, $owner);
+	eval { ($path, $owner) = PVE::Storage::path($storecfg, $volid); };
 	return if !$path || !$owner || ($owner != $vmid);
 
 	$volids->{$volid} = 1;
-- 
2.30.2

[pve-devel] [PATCH V4 qemu-server 6/7] adapt behavior for detaching drives to deatching container mount points

[Stefan Hrdlicka]

if a storage is not available a volume will be added to the container
config as unused. before it would just disappear from the config

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 PVE/QemuServer.pm | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 331677f..b57226b 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2038,7 +2038,11 @@ sub vmconfig_register_unused_drive {
 	delete $conf->{cloudinit};
     } elsif (!drive_is_cdrom($drive)) {
 	my $volid = $drive->{file};
-	if (vm_is_volid_owner($storecfg, $vmid, $volid)) {
+	my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid);
+	if (!PVE::Storage::storage_config($storecfg, $storeid, 1)) {
+	    # if storage is not in config, skip owner check and add as unused
+	    PVE::QemuConfig->add_unused_volume($conf, $volid, $vmid);
+	} elsif (vm_is_volid_owner($storecfg, $vmid, $volid)) {
 	    PVE::QemuConfig->add_unused_volume($conf, $volid, $vmid);
 	}
     }
-- 
2.30.2

[pve-devel] [PATCH V4 pve-manager 7/7] fix #3711: optionally allow CT deletion to complete on disk volume removal errors

[Stefan Hrdlicka]

Add a checkbox to the remove dialog of LXC containers to force
deleting a container if the storage it uses has been removed.

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 www/manager6/lxc/Config.js              |  1 +
 www/manager6/window/SafeDestroyGuest.js | 34 +++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/www/manager6/lxc/Config.js b/www/manager6/lxc/Config.js
index 93f385db..a00e1927 100644
--- a/www/manager6/lxc/Config.js
+++ b/www/manager6/lxc/Config.js
@@ -154,6 +154,7 @@ Ext.define('PVE.lxc.Config', {
 			    url: base_url,
 			    item: { type: 'CT', id: vmid },
 			    taskName: 'vzdestroy',
+			    showForceRemoveMissingStorage: true,
 			}).show();
 		    },
 		    iconCls: 'fa fa-trash-o',
diff --git a/www/manager6/window/SafeDestroyGuest.js b/www/manager6/window/SafeDestroyGuest.js
index 3328293a..6cffa2bb 100644
--- a/www/manager6/window/SafeDestroyGuest.js
+++ b/www/manager6/window/SafeDestroyGuest.js
@@ -28,8 +28,37 @@ Ext.define('PVE.window.SafeDestroyGuest', {
 		'data-qtip': gettext('Scan all enabled storages for unreferenced disks and delete them.'),
 	    },
 	},
+	{
+	    xtype: 'proxmoxcheckbox',
+	    name: 'forceStorageRemove',
+	    reference: 'forceRemoveMissingStorage',
+	    boxLabel: gettext('Ignore missing/unavailable storage.'),
+	    checked: false,
+	    submitValue: false,
+	    hidden: true,
+	    autoEl: {
+		tag: 'div',
+		'data-qtip': gettext('Force remove container when storage is unavailable. ' +
+		    'The disk is not cleaned if storage still exists.'),
+	    },
+	},
     ],
 
+    config: {
+        showForceRemoveMissingStorage: false,
+    },
+
+    initComponent: function() {
+	let me = this;
+
+	me.callParent();
+
+	if (me.showForceRemoveMissingStorage) {
+	    let frms = me.lookupReference('forceRemoveMissingStorage');
+	    frms.hidden = !me.showForceRemoveMissingStorage;
+	}
+    },
+
     note: gettext('Referenced disks will always be destroyed.'),
 
     getParams: function() {
@@ -41,6 +70,11 @@ Ext.define('PVE.window.SafeDestroyGuest', {
 	const destroyUnreferencedCheckbox = me.lookupReference('destroyUnreferencedCheckbox');
 	me.params["destroy-unreferenced-disks"] = destroyUnreferencedCheckbox.checked ? 1 : 0;
 
+	if (me.showForceRemoveMissingStorage) {
+	    const forceRemoveMissingStorage = me.lookupReference('forceRemoveMissingStorage');
+	    me.params['ignore-storage-errors'] = forceRemoveMissingStorage.checked ? 1 : 0;
+	}
+
 	return me.callParent();
     },
 });
-- 
2.30.2

Re: [pve-devel] [PATCH V4 pve-container 1/7] fix #3711: optionally allow CT deletion to complete on disk volume removal errors

[Fiona Ebner]

Am 25.11.22 um 15:40 schrieb Stefan Hrdlicka:
> review fixes
> - rename parameter to ignore-storage-errors
> - move eval further up the call chain

This should go...

> 
> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---

...here to not clobber up the commit message

>  src/PVE/API2/LXC.pm | 8 ++++++++
>  src/PVE/LXC.pm      | 6 ++++--
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 79aecaa..19806fa 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -700,6 +700,13 @@ __PACKAGE__->register_method({
>  		    ." enabled storages which are not referenced in the config.",
>  		optional => 1,
>  	    },
> +	    'ignore-storage-errors' => {
> +		type => 'boolean',
> +		description => 'If set, this will ignore errors when trying to remove'
> +		    . ' container storage.',

I'd rather use 'volumes' than 'storage' here. Or 'disks' which is
already used in the 'destroy-unreferenced-disks' option.

> +		default => 0,
> +		optional => 1,
> +	    }

Style nit: missing comma (then the line doesn't need to be touched the
next time an option is added ;))

>  	},
>      },
>      returns => {
> @@ -761,6 +768,7 @@ __PACKAGE__->register_method({
>  		$conf,
>  		{ lock => 'destroyed' },
>  		$param->{'destroy-unreferenced-disks'},
> +		$param->{'ignore-storage-errors'},
>  	    );
>  
>  	    PVE::AccessControl::remove_vm_access($vmid);
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 4bbd739..fe68f75 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -861,7 +861,8 @@ sub delete_mountpoint_volume {
>  }
>  
>  sub destroy_lxc_container {
> -    my ($storage_cfg, $vmid, $conf, $replacement_conf, $purge_unreferenced) = @_;
> +    my ($storage_cfg, $vmid, $conf, $replacement_conf,
> +	$purge_unreferenced, $ignore_storage_errors) = @_;
>  
>      my $volids = {};
>      my $remove_volume = sub {
> @@ -872,7 +873,8 @@ sub destroy_lxc_container {
>  	return if $volids->{$volume};
>  	$volids->{$volume} = 1;
>  
> -	delete_mountpoint_volume($storage_cfg, $vmid, $volume);
> +	eval { delete_mountpoint_volume($storage_cfg, $vmid, $volume); };
> +	die $@ if !$ignore_storage_errors && $@;

The commit message states that a warning is printed (which would be
nice), but the error is completely ignored here?

>      };
>      PVE::LXC::Config->foreach_volume_full($conf, {include_unused => 1}, $remove_volume);
>  

Re: [pve-devel] [PATCH V4 pve-container 2/7] adapt behavior for detaching/removing a mount point

[Fiona Ebner]

Am 25.11.22 um 15:40 schrieb Stefan Hrdlicka:
> detach of a mount point with a removed underlying storage causes it to
> be labeled as a an 'unused disk'
> remove of a 'unused disk' with a removed underlying storage causes it to
> be removed from the configuration
> 
> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---
>  src/PVE/LXC/Config.pm | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> index b1f779b..e8fbd86 100644
> --- a/src/PVE/LXC/Config.pm
> +++ b/src/PVE/LXC/Config.pm
> @@ -1421,13 +1421,17 @@ sub vmconfig_apply_pending {
>      foreach my $opt (sort keys %$pending_delete_hash) {
>  	next if $selection && !$selection->{$opt};
>  	eval {
> +	    my ($storeid, $volname) = PVE::Storage::parse_volume_id($conf->{$opt});

Nit: I'd prefer my ($storeid, undef) to avoid introducing an unused variable

$conf->{opt} is not necessarily a volume ID. That's only true with the
current format for unused entries. Please use $class->parse_volume() to
get the volume ID first.

>  	    if ($opt =~ m/^mp(\d+)$/) {
>  		my $mp = $class->parse_volume($opt, $conf->{$opt});
>  		if ($mp->{type} eq 'volume') {
>  		    $class->add_unused_volume($conf, $mp->{volume})
>  			if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
>  		}
> -	    } elsif ($opt =~ m/^unused(\d+)$/) {
> +	    } elsif (
> +		$opt =~ m/^unused(\d+)$/
> +		&& PVE::Storage::storage_config($storecfg, $storeid, 1)
> +	    ) {
>  		PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $conf->{$opt})
>  		    if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);

Please also change the parameter to is_volume_in_use() to use $volid
rather than $conf->{opt} (should be a separate patch).

>  	    }

Re: [pve-devel] [PATCH V4 pve-container 3/7] add linked clone check for LXC container template deletion

[Fiona Ebner]

Am 25.11.22 um 15:40 schrieb Stefan Hrdlicka:
> prevent partial storage deletion if the template has a linked clone
> container
> 

Nit: not too important, but you could mention that we already do the
same for VMs

> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---
>  src/PVE/LXC.pm | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index fe68f75..7164462 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -865,6 +865,18 @@ sub destroy_lxc_container {
>  	$purge_unreferenced, $ignore_storage_errors) = @_;
>  
>      my $volids = {};
> +
> +    if ($conf->{template}) {
> +	PVE::LXC::Config->foreach_volume_full($conf, {incldue_unused => 1}, sub {
> +	    my ($ms, $mountpoint) = @_;
> +	    my $volid = $mountpoint->{volume};
> +	    return if !$volid || $volid =~ m|^/|;

Could/should use
PVE::LXC::Config->classify_mountpoint($volume) ne 'volume';
rather than
$volid =~ m|^/|;
to be more future-proof.

> +	    my $result;
> +	    eval{ $result = PVE::Storage::volume_is_base_and_used($storage_cfg, $volid) };

Style nit: my $result = eval { PVE::... }; saves a line

> +	    die "base volume '$volid' is still in use by linked cloned\n" if $result;

If the check dies, we cannot tell if the volume is actually an in-use
base volume. So we shouldn't just move on with the removal. The error
should be propagated except if $ignore_storage_errors is true.

> +	});
> +    }
> +
>      my $remove_volume = sub {
>  	my ($ms, $mountpoint) = @_;
>  

Re: [pve-devel] [PATCH V4 qemu-server 5/7] ignore PVE::Storage::path errors when deleting VMs

[Fiona Ebner]

Some rationale in the commit message would be nice, along the lines of
"we already ignore storage errors except these, let's finish the job"

Am 25.11.22 um 15:40 schrieb Stefan Hrdlicka:
> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---
>  PVE/QemuServer.pm | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 51e9a51..331677f 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -2341,10 +2341,9 @@ sub destroy_vm {
>  
>  		my $volid = $drive->{file};
>  		return if !$volid || $volid =~ m|^/|;
> -
> -		die "base volume '$volid' is still in use by linked cloned\n"
> -		    if PVE::Storage::volume_is_base_and_used($storecfg, $volid);
> -
> +		my $result;
> +		eval { $result = PVE::Storage::volume_is_base_and_used($storecfg, $volid) };

Same style nit as for the container patch: my $result = eval {...};

But here it's fine to not propagate the error, because
1. We already ignore storage errors by default
2. The check is done again inside vdisk_free()

It would be nice to add a comment in the code here mentioning point 2.
We could also log a warning here, but no big deal.

> +		die "base volume '$volid' is still in use by linked cloned\n" if $result;
>  	});
>      }
>  
> @@ -2357,7 +2356,8 @@ sub destroy_vm {
>  	return if !$volid || $volid =~ m|^/|;
>  	return if $volids->{$volid};
>  
> -	my ($path, $owner) = PVE::Storage::path($storecfg, $volid);
> +	my ($path, $owner);
> +	eval { ($path, $owner) = PVE::Storage::path($storecfg, $volid); };

Same style nit my ($path, $owner) = eval {...};

Should log a warning if the eval fails.

>  	return if !$path || !$owner || ($owner != $vmid);
>  
>  	$volids->{$volid} = 1;

Re: [pve-devel] [PATCH V4 qemu-server 6/7] adapt behavior for detaching drives to deatching container mount points

[Fiona Ebner]

The info from this commit title is good to have as part of the commit
message, but it would be nicer if the title would describe the change
rather than just referencing another repository.

Am 25.11.22 um 15:40 schrieb Stefan Hrdlicka:
> if a storage is not available a volume will be added to the container

s/container/VM/

> config as unused. before it would just disappear from the config
> 
> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---
>  PVE/QemuServer.pm | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 331677f..b57226b 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -2038,7 +2038,11 @@ sub vmconfig_register_unused_drive {
>  	delete $conf->{cloudinit};
>      } elsif (!drive_is_cdrom($drive)) {
>  	my $volid = $drive->{file};
> -	if (vm_is_volid_owner($storecfg, $vmid, $volid)) {
> +	my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid);
> +	if (!PVE::Storage::storage_config($storecfg, $storeid, 1)) {
> +	    # if storage is not in config, skip owner check and add as unused
> +	    PVE::QemuConfig->add_unused_volume($conf, $volid, $vmid);
> +	} elsif (vm_is_volid_owner($storecfg, $vmid, $volid)) {
>  	    PVE::QemuConfig->add_unused_volume($conf, $volid, $vmid);
>  	}
>      }