fix #889: api create: reserver config with create lock early

allows to remove some checks as we can be sure the config belongs to
us once we have it resered, either for restore or new creation.

This is similar to the qemu-server approach[0][1], adapted to the
LXC code. We need to cleanup a bit less if something fails, as the
LXC code path always removed the config and all created volumes in
this case, which means the 'create' reserve lock is gone too.

The early reserve on API entry, instead of doing it after forked
worker entry, allows to workaround the issues reported in #889 as
successful return from the API call means that the VMID is locked.

[0]: https://git.proxmox.com/?p=qemu-server.git;a=commit;h=8ba8418ca1d1a76a7e24c34045ca7702b0cd969d
[1]: https://git.proxmox.com/?p=qemu-server.git;a=commit;h=4fedc13b453d2011b35352df246cf9ea396e942b

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index d0e82ee0799668391981546d9a5ab0abea0a2037..27d26d5be5ceb480fc6cc4379e4037008b806cae 100644 (file)
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -180,6 +180,8 @@ __PACKAGE__->register_method({
     code => sub {
        my ($param) = @_;
 
+       PVE::Cluster::check_cfs_quorum();
+
        my $rpcenv = PVE::RPCEnvironment::get();
        my $authuser = $rpcenv->get_user();
 
@@ -205,6 +207,7 @@ __PACKAGE__->register_method({
        if (!($same_container_exists && $restore && $force)) {
            PVE::Cluster::check_vmid_unused($vmid);
        } else {
+           die "can't overwrite running container\n" if PVE::LXC::check_running($vmid);
            my $conf = PVE::LXC::Config->load_config($vmid);
            PVE::LXC::Config->check_protection($conf, "unable to restore CT $vmid");
        }
@@ -317,38 +320,19 @@ __PACKAGE__->register_method({
 
        $conf->{unprivileged} = 1 if $unprivileged;
 
-       my $check_vmid_usage = sub {
-           if ($force) {
-               die "can't overwrite running container\n"
-                   if PVE::LXC::check_running($vmid);
-           } else {
-               PVE::Cluster::check_vmid_unused($vmid);
-           }
-       };
-
-       my $code = sub {
-           &$check_vmid_usage(); # final check after locking
-           my $old_conf;
+       my $emsg = $restore ? "unable to restore CT $vmid -" : "unable to create CT $vmid -";
 
-           my $config_fn = PVE::LXC::Config->config_file($vmid);
-           if (-f $config_fn) {
-               die "container exists" if !$restore; # just to be sure
-               $old_conf = PVE::LXC::Config->load_config($vmid);
-           } else {
-               eval {
-                   # try to create empty config on local node, we have an flock
-                   PVE::LXC::Config->write_config($vmid, {});
-               };
+       eval { PVE::LXC::Config->create_and_lock_config($vmid, $force) };
+       die "$emsg $@" if $@;
 
-               # another node was faster, abort
-               die "Could not reserve ID $vmid, already taken\n" if $@;
-           }
+       my $code = sub {
+           my $old_conf = PVE::LXC::Config->load_config($vmid);
 
-           PVE::Cluster::check_cfs_quorum();
            my $vollist = [];
            eval {
                my $orig_mp_param; # only used if $restore
                if ($restore) {
+                   die "can't overwrite running container\n" if PVE::LXC::check_running($vmid);
                    (my $orig_conf, $orig_mp_param) = PVE::LXC::Create::recover_config($archive);
                    if ($is_root) {
                        # When we're root call 'restore_configuration' with ristricted=0,
@@ -397,9 +381,10 @@ __PACKAGE__->register_method({
 
                $vollist = PVE::LXC::create_disks($storage_cfg, $vmid, $mp_param, $conf);
 
-               if (defined($old_conf)) {
+               # we always have the 'create' lock so check for more than 1 entry
+               if (scalar(keys %$old_conf) > 1) {
                    # destroy old container volumes
-                   PVE::LXC::destroy_lxc_container($storage_cfg, $vmid, $old_conf, {});
+                   PVE::LXC::destroy_lxc_container($storage_cfg, $vmid, $old_conf, { lock => 'create' });
                }
 
                eval {
@@ -432,7 +417,7 @@ __PACKAGE__->register_method({
                PVE::LXC::destroy_disks($storage_cfg, $vollist);
                eval { PVE::LXC::destroy_config($vmid) };
                warn $@ if $@;
-               die $err;
+               die "$emsg $err";
            }
            PVE::AccessControl::add_vm_to_pool($vmid, $pool) if $pool;
 
@@ -443,8 +428,6 @@ __PACKAGE__->register_method({
        my $workername = $restore ? 'vzrestore' : 'vzcreate';
        my $realcmd = sub { PVE::LXC::Config->lock_config($vmid, $code); };
 
-       &$check_vmid_usage(); # first check before locking
-
        return $rpcenv->fork_worker($workername, $vmid, $authuser, $realcmd);
     }});