all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH container/qemu-server v5 0/7] fix #3711 and adapt drive detach/remove behavior
@ 2025-05-13  8:03 Michael Köppl
  2025-05-13  8:03 ` [pve-devel] [PATCH container v5 1/4] fix #3711: lxc: print warning if storage for mounted volume does not exist anymore Michael Köppl
                   ` (6 more replies)
  0 siblings, 7 replies; 14+ messages in thread
From: Michael Köppl @ 2025-05-13  8:03 UTC (permalink / raw)
  To: pve-devel

This series aims to fix #3711 and streamline the detach/remove behavior 
around volumes that are either mounted into a container or attached to
a VM as a hard disk. It is a continuation of a series from 2022 [0],
but makes the following changes:

Changes v4 -> v5:
- Always ignore errors that originate from a removed storage and
  continue with destruction of a container or removal of a volume, 
  instead of adding an option to ignore these errors.
- Remove web UI checkbox
- Remove formatting patch
- Additionally allow removing a mount point with a removed storage
  from a running container (previously hotplug removal was not possible)
- Fix style nits from v4 review
- Print warnings for any errors that occur instead of ignoring them
- Add explicit check if storage still exists when destroying a container
  to differentiate between that case and other error cases (which should
  still fail)

The first change also means that this patch series should be held back
until PVE 9. In the original patch series the option to ignore these
errors was made to avoid breaking existing behavior. After some off-list
discussion it seems more reasonable to avoid an additional option and
instead allow users to remove containers with mount points even if the
underlying storage of the mount point was removed.

Where at least some of the implementation was taken from the previous
series, the patch was marked co-authored by the original author.

[0] https://lore.proxmox.com/pve-devel/20221125144008.2988072-1-s.hrdlicka@proxmox.com/t/#

pve-container:

Michael Köppl (4):
  fix #3711: lxc: print warning if storage for mounted volume does not
    exist anymore
  lxc: get volid through parse_volume()
  fix #3711: lxc: allow removing unused mp if storage no longer exists
  add linked clone check when destroying container

 src/PVE/LXC.pm        | 24 +++++++++++++++++++++++-
 src/PVE/LXC/Config.pm | 21 ++++++++++++++++-----
 2 files changed, 39 insertions(+), 6 deletions(-)


qemu-server:

Michael Köppl (3):
  adapt linked clone check to not die if an error occurs during check
  print warning for PVE::Storage::path errors instead of failing
  mark volumes pending detach as unused if storage was removed

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


Summary over all repositories:
  3 files changed, 50 insertions(+), 11 deletions(-)

-- 
Generated by git-murpp 0.8.0


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [pve-devel] [PATCH container v5 1/4] fix #3711: lxc: print warning if storage for mounted volume does not exist anymore
  2025-05-13  8:03 [pve-devel] [PATCH container/qemu-server v5 0/7] fix #3711 and adapt drive detach/remove behavior Michael Köppl
@ 2025-05-13  8:03 ` Michael Köppl
  2025-05-13  8:03 ` [pve-devel] [PATCH container v5 2/4] lxc: get volid through parse_volume() Michael Köppl
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Michael Köppl @ 2025-05-13  8:03 UTC (permalink / raw)
  To: pve-devel

An explicit check for the existence of the storage is added to print a
warning and continue with the removal of the container without deleting
the mount point in case the storage does not exist anymore. For other
errors, the function should still die.

Co-authored-by: Stefan Hrdlicka
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/LXC.pm | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 2b9f0cf..6a1ce92 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -953,7 +953,17 @@ sub destroy_lxc_container {
 	return if $volids->{$volume};
 	$volids->{$volume} = 1;
 
-	delete_mountpoint_volume($storage_cfg, $vmid, $volume);
+	# explicitly check if storage still exists to avoid failing during
+	# deletion of the mountpoint volume. instead, only a warning is
+	# printed and destroying the container continues.
+	my ($storeid) = PVE::Storage::parse_volume_id($volume);
+	eval { PVE::Storage::storage_config($storage_cfg, $storeid) };
+	my $err = $@;
+	PVE::RESTEnvironment::log_warn("failed to delete $volume, $err") if $err;
+
+	if (!$err) {
+	    delete_mountpoint_volume($storage_cfg, $vmid, $volume);
+	}
     };
     PVE::LXC::Config->foreach_volume_full($conf, {include_unused => 1}, $remove_volume);
 
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [pve-devel] [PATCH container v5 2/4] lxc: get volid through parse_volume()
  2025-05-13  8:03 [pve-devel] [PATCH container/qemu-server v5 0/7] fix #3711 and adapt drive detach/remove behavior Michael Köppl
  2025-05-13  8:03 ` [pve-devel] [PATCH container v5 1/4] fix #3711: lxc: print warning if storage for mounted volume does not exist anymore Michael Köppl
@ 2025-05-13  8:03 ` Michael Köppl
  2025-05-13 13:37   ` Daniel Kral
  2025-05-13 13:48   ` Daniel Kral
  2025-05-13  8:03 ` [pve-devel] [PATCH container v5 3/4] fix #3711: lxc: allow removing unused mp if storage no longer exists Michael Köppl
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 14+ messages in thread
From: Michael Köppl @ 2025-05-13  8:03 UTC (permalink / raw)
  To: pve-devel

The value in $conf->{opt} is not necessarily a volume ID. To ensure that
a valid volume ID is used, it is retrieved by calling parse_volume().

Co-authored-by: Stefan Hrdlicka
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/LXC/Config.pm | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 0740e8c..7f76b34 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1557,15 +1557,16 @@ sub vmconfig_apply_pending {
     foreach my $opt (sort keys %$pending_delete_hash) {
 	next if $selection && !$selection->{$opt};
 	eval {
+	    my $mp = $class->parse_volume($opt, $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+)$/) {
 		PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $conf->{$opt})
-		    if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
+		    if !$class->is_volume_in_use($conf, $mp->{volume}, 1, 1);
 	    } elsif ($opt =~ m/^net(\d+)$/) {
 		if ($have_sdn) {
 		    my $net = $class->parse_lxc_network($conf->{$opt});
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [pve-devel] [PATCH container v5 3/4] fix #3711: lxc: allow removing unused mp if storage no longer exists
  2025-05-13  8:03 [pve-devel] [PATCH container/qemu-server v5 0/7] fix #3711 and adapt drive detach/remove behavior Michael Köppl
  2025-05-13  8:03 ` [pve-devel] [PATCH container v5 1/4] fix #3711: lxc: print warning if storage for mounted volume does not exist anymore Michael Köppl
  2025-05-13  8:03 ` [pve-devel] [PATCH container v5 2/4] lxc: get volid through parse_volume() Michael Köppl
@ 2025-05-13  8:03 ` Michael Köppl
  2025-05-13  8:03 ` [pve-devel] [PATCH container v5 4/4] add linked clone check when destroying container Michael Köppl
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Michael Köppl @ 2025-05-13  8:03 UTC (permalink / raw)
  To: pve-devel

Detaching a mount point with a removed underlying storage causes it to
be labeled as an unused disk. With this change, removing an unused disk
with a removed underlying storage causes it to be removed from the
configuration instead of failing. Deleting the mount point is skipped in
this case to avoid failing and to allow removing the unused volume from
the CT configuration and to achieve behavior consistent with destroying
a CT.

Co-authored-by: Stefan Hrdlicka
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/LXC/Config.pm | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 7f76b34..15d52cb 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1470,11 +1470,17 @@ sub vmconfig_hotplug_pending {
     foreach my $opt (sort keys %$pending_delete_hash) {
 	next if $selection && !$selection->{$opt};
 	eval {
+	    my $mp = $class->parse_volume($opt, $conf->{$opt});
+	    my ($storeid, undef) = PVE::Storage::parse_volume_id($mp->{volume});
+
 	    if ($LXC_FASTPLUG_OPTIONS->{$opt}) {
 		# pass
-	    } 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);
+		    if !$class->is_volume_in_use($conf, $mp->{volume}, 1, 1);
 	    } elsif ($opt eq 'swap') {
 		$hotplug_memory->(undef, 0);
 	    } elsif ($opt eq 'cpulimit') {
@@ -1558,13 +1564,17 @@ sub vmconfig_apply_pending {
 	next if $selection && !$selection->{$opt};
 	eval {
 	    my $mp = $class->parse_volume($opt, $conf->{$opt});
+	    my ($storeid, undef) = PVE::Storage::parse_volume_id($mp->{volume});
 
 	    if ($opt =~ m/^mp(\d+)$/) {
 		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, $mp->{volume}, 1, 1);
 	    } elsif ($opt =~ m/^net(\d+)$/) {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [pve-devel] [PATCH container v5 4/4] add linked clone check when destroying container
  2025-05-13  8:03 [pve-devel] [PATCH container/qemu-server v5 0/7] fix #3711 and adapt drive detach/remove behavior Michael Köppl
                   ` (2 preceding siblings ...)
  2025-05-13  8:03 ` [pve-devel] [PATCH container v5 3/4] fix #3711: lxc: allow removing unused mp if storage no longer exists Michael Köppl
@ 2025-05-13  8:03 ` Michael Köppl
  2025-05-13  8:03 ` [pve-devel] [PATCH qemu-server v5 1/3] adapt linked clone check to not die if an error occurs during check Michael Köppl
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Michael Köppl @ 2025-05-13  8:03 UTC (permalink / raw)
  To: pve-devel

This behavior matches the behavior already implemented for VMs and
prevents partial storage deletion if a container template has a linked
clone. In such cases, the destruction of the container template will
now fail, informing the user that the base volume is still in use by
the linked clone.

Co-authored-by: Stefan Hrdlicka
Signed-off-by: Michael Köppl <m.koeppl@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 6a1ce92..7bc566a 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -945,6 +945,18 @@ sub destroy_lxc_container {
     my ($storage_cfg, $vmid, $conf, $replacement_conf, $purge_unreferenced) = @_;
 
     my $volids = {};
+
+    if ($conf->{template}) {
+	PVE::LXC::Config->foreach_volume_full($conf, {include_unused => 1}, sub {
+	   my ($ms, $mountpoint) = @_;
+	   my $volid = $mountpoint->{volume};
+	   return if !$volid || PVE::LXC::Config->classify_mountpoint($volid) ne 'volume';
+	   my $result = eval { PVE::Storage::volume_is_base_and_used($storage_cfg, $volid) };
+	   die "failed to delete volume '$volid': $@\n" if $@;
+	   die "base volume '$volid' is still in use by linked clone\n" if $result;
+	});
+    }
+
     my $remove_volume = sub {
 	my ($ms, $mountpoint) = @_;
 
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [pve-devel] [PATCH qemu-server v5 1/3] adapt linked clone check to not die if an error occurs during check
  2025-05-13  8:03 [pve-devel] [PATCH container/qemu-server v5 0/7] fix #3711 and adapt drive detach/remove behavior Michael Köppl
                   ` (3 preceding siblings ...)
  2025-05-13  8:03 ` [pve-devel] [PATCH container v5 4/4] add linked clone check when destroying container Michael Köppl
@ 2025-05-13  8:03 ` Michael Köppl
  2025-05-13  8:03 ` [pve-devel] [PATCH qemu-server v5 2/3] print warning for PVE::Storage::path errors instead of failing Michael Köppl
  2025-05-13  8:03 ` [pve-devel] [PATCH qemu-server v5 3/3] mark volumes pending detach as unused if storage was removed Michael Köppl
  6 siblings, 0 replies; 14+ messages in thread
From: Michael Köppl @ 2025-05-13  8:03 UTC (permalink / raw)
  To: pve-devel

Align error handling behavior when checking for linked clones with the
rest of destroy_vm's error handling approach. Unlike the LXC destroy
implementation, this version continues execution when encountering
errors during the check, since:

1. The same validation occurs later in the process
2. This prevents potential crashes
3. The VM removal will still be blocked if the volume has linked clones

Align error handling behavior when checking for linked clones with the
rest of destroy_vm's error handling approach. Unlike the LXC implementation,
this version continues execution when encountering errors during the check,
since:

Co-authored-by: Stefan Hrdlicka
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 PVE/QemuServer.pm | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 577959a4..e02bf7d4 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2116,9 +2116,10 @@ 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);
-
+		# errors that occur during this check are not propagated since the
+		# same check is done again in vdisk_free
+		my $result = eval { PVE::Storage::volume_is_base_and_used($storecfg, $volid) };
+		die "base volume '$volid' is still in use by linked cloned\n" if $result;
 	});
     }
 
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [pve-devel] [PATCH qemu-server v5 2/3] print warning for PVE::Storage::path errors instead of failing
  2025-05-13  8:03 [pve-devel] [PATCH container/qemu-server v5 0/7] fix #3711 and adapt drive detach/remove behavior Michael Köppl
                   ` (4 preceding siblings ...)
  2025-05-13  8:03 ` [pve-devel] [PATCH qemu-server v5 1/3] adapt linked clone check to not die if an error occurs during check Michael Köppl
@ 2025-05-13  8:03 ` Michael Köppl
  2025-05-13  8:03 ` [pve-devel] [PATCH qemu-server v5 3/3] mark volumes pending detach as unused if storage was removed Michael Köppl
  6 siblings, 0 replies; 14+ messages in thread
From: Michael Köppl @ 2025-05-13  8:03 UTC (permalink / raw)
  To: pve-devel

Co-authored-by: Stefan Hrdlicka
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 PVE/QemuServer.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index e02bf7d4..68bbf4ce 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2132,7 +2132,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 { PVE::Storage::path($storecfg, $volid) };
+	log_warn("failed to get path and owner of volume '$volid': $@\n") if $@;
 	return if !$path || !$owner || ($owner != $vmid);
 
 	$volids->{$volid} = 1;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [pve-devel] [PATCH qemu-server v5 3/3] mark volumes pending detach as unused if storage was removed
  2025-05-13  8:03 [pve-devel] [PATCH container/qemu-server v5 0/7] fix #3711 and adapt drive detach/remove behavior Michael Köppl
                   ` (5 preceding siblings ...)
  2025-05-13  8:03 ` [pve-devel] [PATCH qemu-server v5 2/3] print warning for PVE::Storage::path errors instead of failing Michael Köppl
@ 2025-05-13  8:03 ` Michael Köppl
  6 siblings, 0 replies; 14+ messages in thread
From: Michael Köppl @ 2025-05-13  8:03 UTC (permalink / raw)
  To: pve-devel

If the storage backing the volume was removed, the volume will be marked
as unused when pending actions are processed (e.g. volume was detached).
Previously, the volume would simply disappear from the config.

Co-authored-by: Stefan Hrdlicka
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 PVE/QemuServer.pm | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 68bbf4ce..d47fa51c 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -1838,7 +1838,11 @@ sub vmconfig_register_unused_drive {
 	delete $conf->{'special-sections'}->{cloudinit};
     } elsif (!drive_is_cdrom($drive)) {
 	my $volid = $drive->{file};
-	if (vm_is_volid_owner($storecfg, $vmid, $volid)) {
+	my ($storeid, undef) = PVE::Storage::parse_volume_id($volid);
+	if (
+	    !PVE::Storage::storage_config($storecfg, $storeid, 1)
+	    || vm_is_volid_owner($storecfg, $vmid, $volid)
+	) {
 	    PVE::QemuConfig->add_unused_volume($conf, $volid, $vmid);
 	}
     }
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [pve-devel] [PATCH container v5 2/4] lxc: get volid through parse_volume()
  2025-05-13  8:03 ` [pve-devel] [PATCH container v5 2/4] lxc: get volid through parse_volume() Michael Köppl
@ 2025-05-13 13:37   ` Daniel Kral
  2025-05-13 13:48   ` Daniel Kral
  1 sibling, 0 replies; 14+ messages in thread
From: Daniel Kral @ 2025-05-13 13:37 UTC (permalink / raw)
  To: Proxmox VE development discussion, Michael Köppl

On 5/13/25 10:03, Michael Köppl wrote:
> The value in $conf->{opt} is not necessarily a volume ID. To ensure that
> a valid volume ID is used, it is retrieved by calling parse_volume().

The patch summary and/or description should contain more information 
about the whereabouts, e.g. for here:

config: apply_pending: get unused volid through parse_volume()

nit: If there are no functional changes here, it's always nice to state 
that in the commit message, so it's clear from the start ;).

> 
> Co-authored-by: Stefan Hrdlicka
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>   src/PVE/LXC/Config.pm | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> index 0740e8c..7f76b34 100644
> --- a/src/PVE/LXC/Config.pm
> +++ b/src/PVE/LXC/Config.pm
> @@ -1557,15 +1557,16 @@ sub vmconfig_apply_pending {
>       foreach my $opt (sort keys %$pending_delete_hash) {
>   	next if $selection && !$selection->{$opt};
>   	eval {
> +	    my $mp = $class->parse_volume($opt, $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+)$/) {
>   		PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $conf->{$opt})
> -		    if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
> +		    if !$class->is_volume_in_use($conf, $mp->{volume}, 1, 1);

I was a little confused why $mp->{volume} needs to be passed here but at 
other places (like is_volume_in_use for mp* above) $conf->{$opt} is fine.

AFAICS that is because valid_volume_keys(...) for the LXC config defines 
rootfs and mp* as valid volume keys, but not unused*, which is then used 
by foreach_volume(...) in the is_volume_in_use(...) helper, where it can 
be found to be used.

"Coincidentally" $conf->{$opt} and $mp->{volume} have the same value 
here, so this patch doesn't change the behavior here AFAICS, but a 
$class->parse_volume(...) makes it more future proof if anything changes 
here, because that helper is aware of unused and will correctly parse it 
with $parse_ct_mountpoint_full->($class, $unused_desc, ...).

It would be great if this was better documented here, but that could be 
a follow-up as it isn't really related to the series itself.

>   	    } elsif ($opt =~ m/^net(\d+)$/) {
>   		if ($have_sdn) {
>   		    my $net = $class->parse_lxc_network($conf->{$opt});



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [pve-devel] [PATCH container v5 2/4] lxc: get volid through parse_volume()
  2025-05-13  8:03 ` [pve-devel] [PATCH container v5 2/4] lxc: get volid through parse_volume() Michael Köppl
  2025-05-13 13:37   ` Daniel Kral
@ 2025-05-13 13:48   ` Daniel Kral
  2025-05-14 14:18     ` Thomas Lamprecht
  1 sibling, 1 reply; 14+ messages in thread
From: Daniel Kral @ 2025-05-13 13:48 UTC (permalink / raw)
  To: Proxmox VE development discussion, Michael Köppl

On 5/13/25 10:03, Michael Köppl wrote:
> The value in $conf->{opt} is not necessarily a volume ID. To ensure that
> a valid volume ID is used, it is retrieved by calling parse_volume().
> 
> Co-authored-by: Stefan Hrdlicka

As already discussed in person, the email here is omitted because git 
send-email seems to know about the Co-authored-by git trailer here and 
wants to append it to the CC, but since the email doesn't exist anymore, 
it doesn't work and will not go through with sending the email.

I wonder if it'd be more appropriate to use "Originally-by" here as it 
still shows who worked on this originally and then the email could be 
added because it doesn't try to send a copy to that email. But that's 
just nit-picking, because I have not seen trailers without email 
addresses yet.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [pve-devel] [PATCH container v5 2/4] lxc: get volid through parse_volume()
  2025-05-13 13:48   ` Daniel Kral
@ 2025-05-14 14:18     ` Thomas Lamprecht
  2025-05-19  7:50       ` Michael Köppl
  0 siblings, 1 reply; 14+ messages in thread
From: Thomas Lamprecht @ 2025-05-14 14:18 UTC (permalink / raw)
  To: Proxmox VE development discussion, Daniel Kral, Michael Köppl

Am 13.05.25 um 15:48 schrieb Daniel Kral:
> On 5/13/25 10:03, Michael Köppl wrote:
>> The value in $conf->{opt} is not necessarily a volume ID. To ensure that
>> a valid volume ID is used, it is retrieved by calling parse_volume().
>>
>> Co-authored-by: Stefan Hrdlicka
> 
> As already discussed in person, the email here is omitted because git 
> send-email seems to know about the Co-authored-by git trailer here and 
> wants to append it to the CC, but since the email doesn't exist anymore, 
> it doesn't work and will not go through with sending the email.

Should be resolved using git send-email's `--suppress-cc=all` option
when sending this patch(es), for details see:

https://git-scm.com/docs/git-send-email#Documentation/git-send-email.txt---suppress-ccltcategorygt


> 
> I wonder if it'd be more appropriate to use "Originally-by" here as it 
> still shows who worked on this originally and then the email could be 
> added because it doesn't try to send a copy to that email. But that's 
> just nit-picking, because I have not seen trailers without email 
> addresses yet.

Depends on the changes and always a bit of a gut-call for such things,
but would be also OK in this case.



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [pve-devel] [PATCH container v5 2/4] lxc: get volid through parse_volume()
  2025-05-14 14:18     ` Thomas Lamprecht
@ 2025-05-19  7:50       ` Michael Köppl
  2025-05-19  8:09         ` Thomas Lamprecht
  0 siblings, 1 reply; 14+ messages in thread
From: Michael Köppl @ 2025-05-19  7:50 UTC (permalink / raw)
  To: Thomas Lamprecht, Proxmox VE development discussion, Daniel Kral

On 5/14/25 16:18, Thomas Lamprecht wrote:
> Am 13.05.25 um 15:48 schrieb Daniel Kral:
>> On 5/13/25 10:03, Michael Köppl wrote:
>>> The value in $conf->{opt} is not necessarily a volume ID. To ensure that
>>> a valid volume ID is used, it is retrieved by calling parse_volume().
>>>
>>> Co-authored-by: Stefan Hrdlicka
>>
>> As already discussed in person, the email here is omitted because git 
>> send-email seems to know about the Co-authored-by git trailer here and 
>> wants to append it to the CC, but since the email doesn't exist anymore, 
>> it doesn't work and will not go through with sending the email.
> 
> Should be resolved using git send-email's `--suppress-cc=all` option
> when sending this patch(es), for details see:
> 
> https://git-scm.com/docs/git-send-email#Documentation/git-send-email.txt---suppress-ccltcategorygt

Thanks for the hint! It seems I have missed that option, wasn't sure how
to handle it otherwise. Will use it in the future. If a v6 of this
series becomes necessary, I'll add Stefan's former email address again.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [pve-devel] [PATCH container v5 2/4] lxc: get volid through parse_volume()
  2025-05-19  7:50       ` Michael Köppl
@ 2025-05-19  8:09         ` Thomas Lamprecht
  2025-05-20  9:12           ` Michael Köppl
  0 siblings, 1 reply; 14+ messages in thread
From: Thomas Lamprecht @ 2025-05-19  8:09 UTC (permalink / raw)
  To: Michael Köppl, Proxmox VE development discussion, Daniel Kral

Am 19.05.25 um 09:50 schrieb Michael Köppl:
> Thanks for the hint! It seems I have missed that option, wasn't sure how
> to handle it otherwise. Will use it in the future. If a v6 of this
> series becomes necessary, I'll add Stefan's former email address again.

Please send a v6 in any case with the trailers being complete even if
nothing else comes up over a day or so. That way it's easier to avoid that
something bogus will be pushed out. We currently have no automation to
check these things, and they can be somewhat easy to overlook. And it's
always great if one can avoid that maintainers need to do extra work on
applying/pushing things, as often their plate is rather full as is already.

And FWIW, automation here wouldn't even be that hard to add using gitolite
hooks, but it doesn't come up often, so nobody see any benefit in doing so.
Albeit it can make sense  to start a general check for S-o-b existing and
maybe some other simple low false-positive checks and then adding an
"incomplete trailer" check would be rather cheap.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [pve-devel] [PATCH container v5 2/4] lxc: get volid through parse_volume()
  2025-05-19  8:09         ` Thomas Lamprecht
@ 2025-05-20  9:12           ` Michael Köppl
  0 siblings, 0 replies; 14+ messages in thread
From: Michael Köppl @ 2025-05-20  9:12 UTC (permalink / raw)
  To: Thomas Lamprecht, Proxmox VE development discussion, Daniel Kral

On 5/19/25 10:09, Thomas Lamprecht wrote:
> Am 19.05.25 um 09:50 schrieb Michael Köppl:
>> Thanks for the hint! It seems I have missed that option, wasn't sure how
>> to handle it otherwise. Will use it in the future. If a v6 of this
>> series becomes necessary, I'll add Stefan's former email address again.
> 
> Please send a v6 in any case with the trailers being complete even if
> nothing else comes up over a day or so. That way it's easier to avoid that
> something bogus will be pushed out. We currently have no automation to
> check these things, and they can be somewhat easy to overlook. And it's
> always great if one can avoid that maintainers need to do extra work on
> applying/pushing things, as often their plate is rather full as is already.

Ack, sent out a v6:
https://lore.proxmox.com/pve-devel/20250520090818.44881-1-m.koeppl@proxmox.com/T/#


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2025-05-20  9:12 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-05-13  8:03 [pve-devel] [PATCH container/qemu-server v5 0/7] fix #3711 and adapt drive detach/remove behavior Michael Köppl
2025-05-13  8:03 ` [pve-devel] [PATCH container v5 1/4] fix #3711: lxc: print warning if storage for mounted volume does not exist anymore Michael Köppl
2025-05-13  8:03 ` [pve-devel] [PATCH container v5 2/4] lxc: get volid through parse_volume() Michael Köppl
2025-05-13 13:37   ` Daniel Kral
2025-05-13 13:48   ` Daniel Kral
2025-05-14 14:18     ` Thomas Lamprecht
2025-05-19  7:50       ` Michael Köppl
2025-05-19  8:09         ` Thomas Lamprecht
2025-05-20  9:12           ` Michael Köppl
2025-05-13  8:03 ` [pve-devel] [PATCH container v5 3/4] fix #3711: lxc: allow removing unused mp if storage no longer exists Michael Köppl
2025-05-13  8:03 ` [pve-devel] [PATCH container v5 4/4] add linked clone check when destroying container Michael Köppl
2025-05-13  8:03 ` [pve-devel] [PATCH qemu-server v5 1/3] adapt linked clone check to not die if an error occurs during check Michael Köppl
2025-05-13  8:03 ` [pve-devel] [PATCH qemu-server v5 2/3] print warning for PVE::Storage::path errors instead of failing Michael Köppl
2025-05-13  8:03 ` [pve-devel] [PATCH qemu-server v5 3/3] mark volumes pending detach as unused if storage was removed Michael Köppl

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal