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 BB3E31FF0AF for ; Thu, 24 Sep 2026 08:37:04 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id A97D821648; Thu, 24 Sep 2026 08:37:00 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 24 Sep 2026 08:36:54 +0200 Message-Id: Subject: Re: [RFC pve-container 1/1] mountpoint backup: allow changing backup flag at runtime From: "Thomas Ellmenreich" To: "Wolfgang Bumiller" X-Mailer: aerc 0.20.0 References: <20260827122655.158754-1-t.ellmenreich@proxmox.com> <26b4609b-3645-4087-bb98-4eda32a7d177@proxmox.com> <2utjzzswvd7qte6xpafhknjo3ssfllikhxh7vg63qc6lcubdsl@epsuqq2o27h7> In-Reply-To: <2utjzzswvd7qte6xpafhknjo3ssfllikhxh7vg63qc6lcubdsl@epsuqq2o27h7> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1790231814722 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.512 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: GLEQ4WFTPIWUKA3HL2IIOBF5CV5SQ5YU X-Message-ID-Hash: GLEQ4WFTPIWUKA3HL2IIOBF5CV5SQ5YU X-MailFrom: t.ellmenreich@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 Thu Sep 17, 2026 at 12:05 PM CEST, Wolfgang Bumiller wrote: > 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: >>=20 >> [snip] >>=20 >> >> +# Checks whether the changed properties can be applied at runtime. I= t only >> >> +# returns true if all changes can be applied in this way. If even on= e >> >> +# property cannot be applied, returns false to ensure changes are ap= plied >> >> +# as a unit. >> >> +my $mountpoint_config_fast_plug_safe =3D sub { > > Newer code can use `my sub` instead of variables. Ah yes, thanks! >> >> + my ($new, $old) =3D @_; >> >> + my @fast_plug_safe_keys =3D ("backup"); > > =E2=86=91 It's more convenient to have this as a hash, and quicker if it'= s > outside the sub. > > my %FAST_PLUG_MP_OPTIONS =3D (backup =3D> 1, replicate =3D> 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 i= f >> > the arrays both hold the same data, the references still differ. >>=20 >> 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. I actually considered doing this as well, but then discarded it because I thought a clone was necessary (I realize now that it is not), but I do have one question: Reading through the perldoc for delete, it mentions that one can delete multiple keys of a hash by using an array: @hash{qw(foo bar)} Which seems perfect for our use case. Is using a hash for 'FAST_PLUG_MP_OPT= IONS' still the best choice? If I understand correctly, the values in the hash ha= ve no effect on what keys are deleted, only the keys do. >>=20 >> > >> >> + } >> >> + >> >> + 1; >> >> +}; >> >> + >> >> sub apply_pending_mountpoint { >> >> my ($class, $vmid, $conf, $opt, $storecfg, $running) =3D @_; >> >> =20 >> >> my $mp =3D $class->parse_volume($opt, $conf->{pending}->{$opt})= ; >> >> my $old =3D $conf->{$opt}; >> >> + >> >> + if (exists($conf->{$opt}) && $running) { >> >> + if ($mountpoint_config_fast_plug_safe->($mp, $class->parse_v= olume($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? >>=20 >> Good catch, yes. I think it will be unreachable, although I'm not sure w= hat to >> do with the open #TODO, especially since I don't quite understand what t= he >> 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. > >>=20 >> ```perl >> if ($mp->{type} eq 'volume' && $mp->{volume} =3D~ $PVE::LXC::NEW_DISK_RE= ) { >> # more code >> } else { >> die "skip\n" if $running && defined($old); # TODO: "changing" mount = points? >> # more code >> } >> ``` >>=20 >> [snip] >>=20 >>=20 >>=20 >>=20 >>=20