Re: [RFC pve-container 1/1] mountpoint backup: allow changing backup flag at runtime

["Thomas Ellmenreich" <t.ellmenreich@proxmox.com>]

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:
>> 
>> [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.

Ah yes, thanks!

>> >> +    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.

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_OPTIONS'
still the best choice? If I understand correctly, the values in the hash have
no effect on what keys are deleted, only the keys do.

>> 
>> >
>> >> +    }
>> >> +
>> >> +    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]
>> 
>> 
>> 
>> 
>>