From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 046231FF09B for ; Mon, 31 Aug 2026 12:36:18 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 454AE2091E; Mon, 31 Aug 2026 12:36:17 +0200 (CEST) Message-ID: <26b4609b-3645-4087-bb98-4eda32a7d177@proxmox.com> Date: Mon, 31 Aug 2026 12:36:12 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC pve-container 1/1] mountpoint backup: allow changing backup flag at runtime To: Thomas Ellmenreich , pve-devel@lists.proxmox.com References: <20260827122655.158754-1-t.ellmenreich@proxmox.com> Content-Language: en-US From: Filip Schauer In-Reply-To: <20260827122655.158754-1-t.ellmenreich@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1788172559198 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.869 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: EIEFCNCGZDFJIDGKSFURHYE6DDZKPL6O X-Message-ID-Hash: EIEFCNCGZDFJIDGKSFURHYE6DDZKPL6O X-MailFrom: f.schauer@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 27/08/2026 14:27, Thomas Ellmenreich wrote: > Previously, setting the backup flag of a running container would first > place it into a pending state, before applying it at a later time. However, > since the flag does not affect the actual mount point, it is now applied > directly. > > In situations where multiple settings are changed at once and backup > happens to be one of them, the flag is not applied directly, but instead > marked as pending alongside the other changes. > > Suggested-by: Filip Schauer > Signed-off-by: Thomas Ellmenreich > --- > Ticking the backup flag on a running mountpoint can be directly applied, > even if the container is running, as it has no effect on the mountpoint > itself. > > This functionality is already present for VMs, but not for containers. > > As also mentioned in the commit message, a change to the backup flag is > only directly applied if it is the only change. In cases where other > configuration changes are also made, all of them are set to pending as > a single unit. > > Open Questions > -------------- > > - As far as I understand, qemu-server currently decides whether to change a > configuration at runtime based on a set of 'fast_plug_option's as well as a > set of 'non-hotpluggable options'. For the sake of simplicity, I have > decided to stick with a list of 'fast plug options'. Is this the right > approach? I considered creating an inverse list, but that would have > involved more work. Having 'pending' as the default is the correct approach, > in my opinion. Sounds reasonable to me. > > - Are there any other properties that could mabye be made fast-pluggable? The `replicate` ("Skip Replication") property comes to mind. > > - Instead of moving the skip and apply control flow into the > `apply_pending_mountpoint` subroutine, I considered leaving it where > it was and taking the config parsing to the caller. However, I decided > against this option, as delegating the config parsing to the caller > seemed like the wrong decision. > > Testing > ------- > > - Only changing the `backup` setting does precisely that, skipping the > pending state. > > - When combining `backup` with changing a different setting, they are both > marked as pending. > > src/PVE/LXC/Config.pm | 29 +++++++++++++++++++++++++---- > 1 file changed, 25 insertions(+), 4 deletions(-) > > diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm > index 7c81834..c28fb04 100644 > --- a/src/PVE/LXC/Config.pm > +++ b/src/PVE/LXC/Config.pm > @@ -3,6 +3,7 @@ package PVE::LXC::Config; > use strict; > use warnings; > > +use List::Util qw(uniq first); > use Fcntl qw(O_RDONLY); > > use PVE::AbstractConfig; > @@ -1741,10 +1742,6 @@ sub vmconfig_hotplug_pending { > $hotplug_memory->($conf->{pending}->{memory}, $conf->{pending}->{swap}); > } > } elsif ($opt =~ m/^mp(\d+)$/) { > - if (exists($conf->{$opt})) { > - die "skip\n"; # don't try to hotplug over existing mp > - } > - > $class->apply_pending_mountpoint($vmid, $conf, $opt, $storecfg, 1); > # apply_pending_mountpoint modifies the value if it creates a new disk > $value = $conf->{pending}->{$opt}; > @@ -1888,11 +1885,35 @@ my $rescan_volume = sub { > warn "Could not rescan volume size - $@\n" if $@; > }; > > +# Checks whether the changed properties can be applied at runtime. It only > +# returns true if all changes can be applied in this way. If even one > +# property cannot be applied, returns false to ensure changes are applied > +# as a unit. > +my $mountpoint_config_fast_plug_safe = sub { > + my ($new, $old) = @_; > + my @fast_plug_safe_keys = ("backup"); > + > + for my $key (uniq(keys %$new, keys %$old)) { > + next if first { $_ eq $key } @fast_plug_safe_keys; > + return 0 if ($new->{$key} // '') ne ($old->{$key} // ''); From testing I noticed that when toggling `backup` on a mount point specifying a custom `idmap`, the `backup` property is still marked as pending. The reason seems to be that this is comparing array references. Even if the arrays both hold the same data, the references still differ. > + } > + > + 1; > +}; > + > sub apply_pending_mountpoint { > my ($class, $vmid, $conf, $opt, $storecfg, $running) = @_; > > my $mp = $class->parse_volume($opt, $conf->{pending}->{$opt}); > my $old = $conf->{$opt}; > + > + if (exists($conf->{$opt}) && $running) { > + if ($mountpoint_config_fast_plug_safe->($mp, $class->parse_volume($opt, $old))) { > + return; > + } > + die "skip\n"; # don't try to hotplug over existing mp Doesn't this make the later `die "skip\n" if $running && defined($old);` unreachable? > + } > + > if ($mp->{type} eq 'volume' && $mp->{volume} =~ $PVE::LXC::NEW_DISK_RE) { > my $original_value = $conf->{pending}->{$opt}; > my $vollist = PVE::LXC::create_disks(