[PATCH container 0/2] mountpoint: allow certain options to be hotplugged

[PATCH container 0/2] mountpoint: allow certain options to be hotplugged

[Thomas Ellmenreich]

Ticking the backup or replicate flag on a container mountpoint and the
replicate flag on the containers rootfs can be applied directly even if
the container is running. This should be possible because it does not
affect the actual mountpoint/disk.

This functionality is already present for VMs, but not for containers.

With this implementation a list of "fast pluggable" options is created,
and any change that only affects any of these options will be fast plugged.
Should a "fast pluggable" option change be accompanied by a
non-"fast pluggable" option change, then the whole change will be set as
pending so that it may be applied as a single unit.

Testing
-------

- Only changing the `backup`/`replicate` setting does precisely that,
  skipping the pending state.

- When combining `backup`/`replicate` with changing a different setting,
  they are both marked as pending.

Questions
---------

- I have rewritten the check for the fast pluggable option to use
  `print_volume` since that makes it a lot easier. `print_volume` calls
  `print_property_string` which has a 'skip' parameter that can only
  be used if I also change `print_ct_mountpoint`. For this reason I
  decided to not make the change and delete the keys instead. That said
  using this skip parameter is an option.

- The tests for the __mountpoint_config_fast_plug_safe can now be found
  in the src/test/ folder. I'm not sure if that is the correct spot or
  if I should have created a src/PVE/LXC/test subdirectory and placed the
  tests in there.

- I have added the option to also fast plug the rootfs options, as replicate
  applies there as well, although I am somewhat unsure if that is a good
  idea. Because of that I have placed it in a separate commit so that it may
  be skipped or reverted in the future.

Changelog
---------

RFC -> v1 (thanks @Wolfgang, @Filip)
    * Instead of comparing the configs key by key, I now delete the
      fast pluggable options, convert the config to a string with
      print_volume and then compare those strings.
    * Added replicate to the fast plug options.
    * Moved the list of fast pluggable keys out of the
      __mountpoint_config_fast_plug_safe subroutine.
    * The __mountpoint_config_fast_plug_safe helper, it not private
      anymore so that it may be called in tests, but it has gained
      the __ prefix, indicating that it should not be used.
    * Added tests for the __mountpoint_config_fast_plug_safe subroutine.
    * Removed an early return that has become unreachable due to the changes.
    * Added the option to also fast plug the rootfs disk.


pve-container:

Thomas Ellmenreich (2):
  mountpoint: allow changing mountpoint properties at runtime
  mountpoint: allow changing rootfs properties at runtime

 src/PVE/LXC/Config.pm    |  39 ++++++++++--
 src/test/Makefile        |   6 +-
 src/test/config_tests.pl | 131 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 169 insertions(+), 7 deletions(-)
 create mode 100755 src/test/config_tests.pl


Summary over all repositories:
  3 files changed, 169 insertions(+), 7 deletions(-)

-- 
Generated by murpp 0.12.0

[PATCH pve-container 1/2] mountpoint: allow changing mountpoint properties at runtime

[Thomas Ellmenreich]

Previously, setting the backup or replicate flags of a running
container would first place them in a pending state before applying
them at a later time. As the flags do not affect the actual mount
point, they are now applied directly.

However, in situations where multiple settings are changed at once
and 'backup' or 'replicate' happen to be among them, the flags are
not applied directly but instead marked as pending alongside the
other changes.

Suggested-by: Filip Schauer <f.schauer@proxmox.com>
Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 src/PVE/LXC/Config.pm    | 37 ++++++++++++++++++---
 src/test/Makefile        |  6 +++-
 src/test/config_tests.pl | 72 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 109 insertions(+), 6 deletions(-)
 create mode 100755 src/test/config_tests.pl

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 9823d00..60318b4 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1738,10 +1738,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};
@@ -1876,11 +1872,43 @@ my $rescan_volume = sub {
     warn "Could not rescan volume size - $@\n" if $@;
 };
 
+my @FAST_PLUG_SAFE_KEYS = qw(backup replicate);
+
+# 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.
+sub __mountpoint_config_fast_plug_safe {
+    my ($class, $key, $new, $old) = @_;
+
+    my %new = $new->%*;
+    my %old = $old->%*;
+    delete @new{@FAST_PLUG_SAFE_KEYS};
+    delete @old{@FAST_PLUG_SAFE_KEYS};
+
+    my $new_str = $class->print_volume($key, \%new);
+    my $old_str = $class->print_volume($key, \%old);
+
+    return $new_str eq $old_str;
+}
+
 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 (defined($old) && $running) {
+        if ($class->__mountpoint_config_fast_plug_safe(
+            $opt,
+            $mp,
+            $class->parse_volume($opt, $old),
+        )) {
+            return;
+        }
+        die "skip\n"; # don't try to hotplug over existing mp
+    }
+
     if ($mp->{type} eq 'volume' && $mp->{volume} =~ $PVE::LXC::NEW_DISK_RE) {
         my $original_value = $conf->{pending}->{$opt};
         my $vollist = PVE::LXC::create_disks(
@@ -1901,7 +1929,6 @@ sub apply_pending_mountpoint {
             }
         }
     } else {
-        die "skip\n" if $running && defined($old); # TODO: "changing" mount points?
         $rescan_volume->($storecfg, $mp) if $mp->{type} eq 'volume';
         if ($running) {
             PVE::LXC::mountpoint_hotplug($vmid, $conf, $opt, $mp, $storecfg);
diff --git a/src/test/Makefile b/src/test/Makefile
index 91ae6ff..a3728b5 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -2,7 +2,7 @@ RUN_USERNS := lxc-usernsexec -m "u:0:`id -u`:1" -m "g:0:`id -g`:1" --
 
 all: test
 
-test: test_setup test_snapshot test_bindmount test_idmap
+test: test_setup test_snapshot test_bindmount test_idmap test_config
 
 test_setup: run_setup_tests.pl
 	if test -e /run/lock/sbuild; then \
@@ -24,5 +24,9 @@ test_bindmount: bindmount_test.pl
 test_idmap: run_idmap_tests.pl
 	./run_idmap_tests.pl
 
+test_config: config_tests.pl
+	./config_tests.pl
+
 clean:
 	rm -rf tmprootfs
+
diff --git a/src/test/config_tests.pl b/src/test/config_tests.pl
new file mode 100755
index 0000000..43935a9
--- /dev/null
+++ b/src/test/config_tests.pl
@@ -0,0 +1,72 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use lib qw(..);
+
+use Test::More;
+
+use PVE::LXC::Config;
+
+my sub create_conf {
+    my ($common, $new, $old) = @_;
+
+    my $new_str = defined($new) ? $common . ',' . $new : $common;
+    my $old_str = defined($old) ? $common . ',' . $old : $common;
+    return (
+        PVE::LXC::Config->parse_volume('mp0', $new_str),
+        PVE::LXC::Config->parse_volume('mp0', $old_str),
+    );
+}
+
+subtest 'same config can be applied' => sub {
+    my ($new, $old) = create_conf("local:100/vm-100-disk-1.raw,mp=/mnt,backup=1");
+    my $result = PVE::LXC::Config->__mountpoint_config_fast_plug_safe("mp0", $new, $old);
+
+    ok($result, "can be hotplugged");
+};
+
+subtest 'config with only backup difference can be applied' => sub {
+    my ($new, $old) =
+        create_conf("local:100/vm-100-disk-1.raw,mp=/mnt", "backup=1", "backup=0");
+    my $result = PVE::LXC::Config->__mountpoint_config_fast_plug_safe("mp0", $new, $old);
+
+    ok($result, "can be hotplugged");
+};
+
+subtest 'fast plug option also ignored if previously not present' => sub {
+    my ($new, $old) = create_conf("local:100/vm-100-disk-1.raw,mp=/mnt", "replicate=1", undef);
+    my $result = PVE::LXC::Config->__mountpoint_config_fast_plug_safe("mp0", $new, $old);
+
+    ok($result, "can be hotplugged");
+};
+
+subtest 'difference in mountpoint cannot be applied' => sub {
+    my ($new, $old) =
+        create_conf("local:100/vm-100-disk-1.raw,replicate=1", "mp=/mnt/one", "mp=/mnt/two");
+    my $result = PVE::LXC::Config->__mountpoint_config_fast_plug_safe("mp0", $new, $old);
+
+    ok(!$result, "cannot be hotplugged");
+};
+
+subtest 'changing fast pluggable and non fast pluggable cannot be applied' => sub {
+    my ($new, $old) =
+        create_conf("local:100/vm-100-disk-1.raw", "mp=/mnt/one", "mp=/mnt/two,replicate=1");
+    my $result = PVE::LXC::Config->__mountpoint_config_fast_plug_safe("mp0", $new, $old);
+
+    ok(!$result, "cannot be hotplugged");
+};
+
+subtest 'idmap array does not throw off comparison' => sub {
+    my ($new, $old) = create_conf(
+        "local:100/vm-100-disk-1.raw,mp=/mnt,idmap=u:0:100000:65536;g:0:100000:65536",
+        "backup=1",
+        undef,
+    );
+    my $result = PVE::LXC::Config->__mountpoint_config_fast_plug_safe("mp0", $new, $old);
+
+    ok($result, "can be hotplugged");
+};
+
+done_testing();
-- 
2.47.3

[PATCH pve-container 2/2] mountpoint: allow changing rootfs properties at runtime

[Thomas Ellmenreich]

Just as has been implemented for mountpoints, properties like replicate
can now be applied at runtime for rootfs disks. If the change is
performed with any other non hot pluggable options, the whole changeset
is added as pending instead of mixing the two states.

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 src/PVE/LXC/Config.pm    |  2 +-
 src/test/config_tests.pl | 65 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 60318b4..5c35750 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1737,7 +1737,7 @@ sub vmconfig_hotplug_pending {
                 if (!$hotplug_memory_done) { # don't call twice if both opts are passed
                     $hotplug_memory->($conf->{pending}->{memory}, $conf->{pending}->{swap});
                 }
-            } elsif ($opt =~ m/^mp(\d+)$/) {
+            } elsif ($opt =~ m/^mp(\d+)$/ || $opt eq 'rootfs') {
                 $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};
diff --git a/src/test/config_tests.pl b/src/test/config_tests.pl
index 43935a9..5c5e868 100755
--- a/src/test/config_tests.pl
+++ b/src/test/config_tests.pl
@@ -9,14 +9,19 @@ use Test::More;
 
 use PVE::LXC::Config;
 
+# Creates new and old configurations based on the common core and just
+# the changes. The fourth parameter can change the kind of mountpoint it
+# is, instead of always using mp0.
 my sub create_conf {
-    my ($common, $new, $old) = @_;
+    my ($common, $new, $old, $mountpoint_name) = @_;
+
+    $mountpoint_name //= 'mp0';
 
     my $new_str = defined($new) ? $common . ',' . $new : $common;
     my $old_str = defined($old) ? $common . ',' . $old : $common;
     return (
-        PVE::LXC::Config->parse_volume('mp0', $new_str),
-        PVE::LXC::Config->parse_volume('mp0', $old_str),
+        PVE::LXC::Config->parse_volume($mountpoint_name, $new_str),
+        PVE::LXC::Config->parse_volume($mountpoint_name, $old_str),
     );
 }
 
@@ -27,6 +32,14 @@ subtest 'same config can be applied' => sub {
     ok($result, "can be hotplugged");
 };
 
+subtest 'rootfs same config can be applied' => sub {
+    my ($new, $old) =
+        create_conf("local:100/vm-100-disk-1.raw,replicate=1", undef, undef, "rootfs");
+    my $result = PVE::LXC::Config->__mountpoint_config_fast_plug_safe("rootfs", $new, $old);
+
+    ok($result, "can be hotplugged");
+};
+
 subtest 'config with only backup difference can be applied' => sub {
     my ($new, $old) =
         create_conf("local:100/vm-100-disk-1.raw,mp=/mnt", "backup=1", "backup=0");
@@ -35,6 +48,14 @@ subtest 'config with only backup difference can be applied' => sub {
     ok($result, "can be hotplugged");
 };
 
+subtest 'rootfs config with only backup difference can be applied' => sub {
+    my ($new, $old) =
+        create_conf("local:100/vm-100-disk-1.raw", "replicate=1", "replicate=0", "rootfs");
+    my $result = PVE::LXC::Config->__mountpoint_config_fast_plug_safe("rootfs", $new, $old);
+
+    ok($result, "can be hotplugged");
+};
+
 subtest 'fast plug option also ignored if previously not present' => sub {
     my ($new, $old) = create_conf("local:100/vm-100-disk-1.raw,mp=/mnt", "replicate=1", undef);
     my $result = PVE::LXC::Config->__mountpoint_config_fast_plug_safe("mp0", $new, $old);
@@ -42,6 +63,14 @@ subtest 'fast plug option also ignored if previously not present' => sub {
     ok($result, "can be hotplugged");
 };
 
+subtest 'rootfs fast plug option also ignored if previously not present' => sub {
+    my ($new, $old) =
+        create_conf("local:100/vm-100-disk-1.raw", "replicate=1", undef, "rootfs");
+    my $result = PVE::LXC::Config->__mountpoint_config_fast_plug_safe("rootfs", $new, $old);
+
+    ok($result, "can be hotplugged");
+};
+
 subtest 'difference in mountpoint cannot be applied' => sub {
     my ($new, $old) =
         create_conf("local:100/vm-100-disk-1.raw,replicate=1", "mp=/mnt/one", "mp=/mnt/two");
@@ -50,6 +79,15 @@ subtest 'difference in mountpoint cannot be applied' => sub {
     ok(!$result, "cannot be hotplugged");
 };
 
+subtest 'rootfs difference in acl cannot be applied' => sub {
+    my ($new, $old) = create_conf(
+        "local:100/vm-100-disk-1.raw,replicate=1", "acl=0", "acl=1", "rootfs",
+    );
+    my $result = PVE::LXC::Config->__mountpoint_config_fast_plug_safe("rootfs", $new, $old);
+
+    ok(!$result, "cannot be hotplugged");
+};
+
 subtest 'changing fast pluggable and non fast pluggable cannot be applied' => sub {
     my ($new, $old) =
         create_conf("local:100/vm-100-disk-1.raw", "mp=/mnt/one", "mp=/mnt/two,replicate=1");
@@ -58,6 +96,15 @@ subtest 'changing fast pluggable and non fast pluggable cannot be applied' => su
     ok(!$result, "cannot be hotplugged");
 };
 
+subtest 'rootfs changing fast pluggable and non fast pluggable cannot be applied' => sub {
+    my ($new, $old) = create_conf(
+        "local:100/vm-100-disk-1.raw", "acl=0", "acl=1,replicate=1", "rootfs",
+    );
+    my $result = PVE::LXC::Config->__mountpoint_config_fast_plug_safe("rootfs", $new, $old);
+
+    ok(!$result, "cannot be hotplugged");
+};
+
 subtest 'idmap array does not throw off comparison' => sub {
     my ($new, $old) = create_conf(
         "local:100/vm-100-disk-1.raw,mp=/mnt,idmap=u:0:100000:65536;g:0:100000:65536",
@@ -69,4 +116,16 @@ subtest 'idmap array does not throw off comparison' => sub {
     ok($result, "can be hotplugged");
 };
 
+subtest 'rootfs idmap array does not throw off comparison' => sub {
+    my ($new, $old) = create_conf(
+        "local:100/vm-100-disk-1.raw,idmap=u:0:100000:65536;g:0:100000:65536",
+        "replicate=1",
+        undef,
+        "rootfs",
+    );
+    my $result = PVE::LXC::Config->__mountpoint_config_fast_plug_safe("rootfs", $new, $old);
+
+    ok($result, "can be hotplugged");
+};
+
 done_testing();
-- 
2.47.3