From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id B8A711FF09F for ; Thu, 17 Sep 2026 12:05:23 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 942B721553; Thu, 17 Sep 2026 12:05:20 +0200 (CEST) Date: Thu, 17 Sep 2026 12:05:16 +0200 From: Wolfgang Bumiller To: Thomas Ellmenreich Subject: Re: [RFC pve-container 1/1] mountpoint backup: allow changing backup flag at runtime Message-ID: <2utjzzswvd7qte6xpafhknjo3ssfllikhxh7vg63qc6lcubdsl@epsuqq2o27h7> References: <20260827122655.158754-1-t.ellmenreich@proxmox.com> <26b4609b-3645-4087-bb98-4eda32a7d177@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1789639516674 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.693 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 RCVD_IN_MSPIKE_H2 0.001 Average reputation (+2) 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: E3TEGAIJKUUY62LRVEQY3LX7MOMKVMCN X-Message-ID-Hash: E3TEGAIJKUUY62LRVEQY3LX7MOMKVMCN X-MailFrom: w.bumiller@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 CC: pve-devel@lists.proxmox.com 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 Fri, Sep 04, 2026 at 12:11:19PM +0200, Thomas Ellmenreich wrote: > On Mon Aug 31, 2026 at 12:36 PM CEST, Filip Schauer wrote: > > [snip] > > >> +# 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 { Newer code can use `my sub` instead of variables. > >> + my ($new, $old) = @_; > >> + my @fast_plug_safe_keys = ("backup"); ↑ It's more convenient to have this as a hash, and quicker if it's outside the sub. my %FAST_PLUG_MP_OPTIONS = (backup => 1, replicate => 1); > >> + > >> + 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. > > Ack, thanks for this, stupid mistake. Not really. And in perl quite annoying. It should not be too difficult to implement a comparison, but on the other hand, we *could* look at it this way: - We have a fixed set of keys which can be changed. - We do not care which value they get. - But other keys need to remain the same. So we could just *delete* those keys from both parsed mount point hashes and then then call `print_volume` and string-compare the result. It utilizes `print_property_string` which should be sorting the values and thus produce a consistent result. > > > > >> + } > >> + > >> + 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? > > Good catch, yes. I think it will be unreachable, although I'm not sure what to > do with the open #TODO, especially since I don't quite understand what the > exact meaning of "changing" is in this context. Yeah we can then drop that line. The case above it is more obvious, the one below less so: they allow allocating or adding-in a *new* disk. Anything else is a "change" to something already mounted. But changes to mount points are tricky. If we only want to change flags, like "read-only", it technically requires making sure it gets applied to all mount *points* (from a VFS point of view), unless we can apply the change to the superblock instead. Changing file systems or replacing a disk is even worse, as then there are open file handles as well. > > ```perl > if ($mp->{type} eq 'volume' && $mp->{volume} =~ $PVE::LXC::NEW_DISK_RE) { > # more code > } else { > die "skip\n" if $running && defined($old); # TODO: "changing" mount points? > # more code > } > ``` > > [snip] > > > > > --