[pve-devel] [PATCH v2 container] create_vm: avoid premature write_config caused by update_pct_config

[Fabian Ebner]

by moving the write_config calls from vmconfig_*_pending to their
call sites. The single other call site for update_pct_config in
update_vm is also adapted. The first write_config call in
vmconfig_hotplug_pending was redundant as the code between it and
the second write_config call shouldn't be able to die. This allowed
to drop the $changes variable as well.

The update_pct_config call lead to a write_config call and so the
configuration file was created before it was intended to be created.

When the CFS is updated in between the write_config call and the
PVE::Cluster::check_vmid_unused call in create_and_lock_config,
the container file would already exist and so creation would
fail after writing out a basically empty config.
(Meaning this is necessary for the proposed "update CFS after
obtaining a configuration file lock" changes)

Even worse, a race was possible for two containers created with the
same ID at the same time:
Assuming the initial PVE::Cluster::check_vmid_unused check in the
parameter verification passes for both create_vm calls, the later one
would potentially overwrite the earlier configuration file with its
update_pct_config call.

Additionally, the file read for $old_config was always the one written
by update_pct_config. Meaning that for a create_vm call with force=1,
already existing old volumes were not removed.

Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
---

Changes from v1:
    * instead of re-ordering, move the write_config
      calls to the (grand-)parent call sites.

There are now two places where write_config is immediately followed
by load_config. I didn't want to change the semantics for the
non-create_vm call sites, so I left them there. Are there any
possible side effects? Otherwise, the load_config calls could be
dropped as a follow-up. Note that the third place in lxc-pve-poststop-hook
has no load_config call.

 src/PVE/API2/LXC/Config.pm |  1 +
 src/PVE/LXC.pm             |  1 +
 src/PVE/LXC/Config.pm      | 10 ----------
 src/lxc-pve-poststop-hook  |  5 ++++-
 4 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/src/PVE/API2/LXC/Config.pm b/src/PVE/API2/LXC/Config.pm
index 42e16d1..73fec36 100644
--- a/src/PVE/API2/LXC/Config.pm
+++ b/src/PVE/API2/LXC/Config.pm
@@ -204,6 +204,7 @@ __PACKAGE__->register_method({
 	    my $running = PVE::LXC::check_running($vmid);
 
 	    my $errors = PVE::LXC::Config->update_pct_config($vmid, $conf, $running, $param, \@delete, \@revert);
+	    PVE::LXC::Config->write_config($vmid, $conf);
 	    $conf = PVE::LXC::Config->load_config($vmid);
 
 	    PVE::LXC::update_lxc_config($vmid, $conf);
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index efbd1d9..2b41ae6 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -2173,6 +2173,7 @@ sub vm_start {
     if (scalar(keys %{$conf->{pending}})) {
 	my $storecfg = PVE::Storage::config();
 	PVE::LXC::Config->vmconfig_apply_pending($vmid, $conf, $storecfg);
+	PVE::LXC::Config->write_config($vmid, $conf);
 	$conf = PVE::LXC::Config->load_config($vmid); # update/reload
     }
 
diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 1443f87..dcc8755 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1161,19 +1161,13 @@ sub vmconfig_hotplug_pending {
 	$errors->{$opt} = "unable to hotplug $opt: $msg";
     };
 
-    my $changes;
     foreach my $opt (sort keys %{$conf->{pending}}) { # add/change
 	next if $selection && !$selection->{$opt};
 	if ($LXC_FASTPLUG_OPTIONS->{$opt}) {
 	    $conf->{$opt} = delete $conf->{pending}->{$opt};
-	    $changes = 1;
 	}
     }
 
-    if ($changes) {
-	$class->write_config($vmid, $conf);
-    }
-
     my $cgroup = PVE::LXC::CGroup->new($vmid);
 
     # There's no separate swap size to configure, there's memory and "total"
@@ -1258,8 +1252,6 @@ sub vmconfig_hotplug_pending {
 	    delete $conf->{pending}->{$opt};
 	}
     }
-
-    $class->write_config($vmid, $conf);
 }
 
 sub vmconfig_apply_pending {
@@ -1316,8 +1308,6 @@ sub vmconfig_apply_pending {
 	    $conf->{$opt} = delete $conf->{pending}->{$opt};
 	}
     }
-
-    $class->write_config($vmid, $conf);
 }
 
 my $rescan_volume = sub {
diff --git a/src/lxc-pve-poststop-hook b/src/lxc-pve-poststop-hook
index 1dba48c..2683ae2 100755
--- a/src/lxc-pve-poststop-hook
+++ b/src/lxc-pve-poststop-hook
@@ -51,7 +51,10 @@ PVE::LXC::Tools::lxc_hook('post-stop', 'lxc', sub {
 
     my $config_updated = 0;
     if ($conf->{pending}) {
-	eval { PVE::LXC::Config->vmconfig_apply_pending($vmid, $conf, $storage_cfg) };
+	eval {
+	    PVE::LXC::Config->vmconfig_apply_pending($vmid, $conf, $storage_cfg);
+	    PVE::LXC::Config->write_config($vmid, $conf);
+	};
 	warn "$@" if $@;
 	PVE::LXC::update_lxc_config($vmid, $conf);
 	$config_updated = 1;
-- 
2.20.1