[pve-devel] [PATCH v5 container 0/4] post_clone_hook for containers

[pve-devel] [PATCH v5 container 0/4] post_clone_hook for containers

[Oguz Bektas]

add post clone hook, and fix #3443 to clear machine-id for cloned
containers


v4->v5:
* split patches for easy review/backport
* also lock when moving config to target node
* move create_and_lock_config outside the eval, this should be checked
before the other ones


Oguz Bektas (4):
  setup: add post_clone_hook for containers
  clone_vm: improve config locking
  run post_clone_hook in clone_vm API
  clone_vm: fix minor typo in error message

 src/PVE/API2/LXC.pm       | 55 +++++++++++++++++++++++++++------------
 src/PVE/LXC/Setup.pm      | 12 +++++++++
 src/PVE/LXC/Setup/Base.pm | 31 ++++++++++++++++++++++
 3 files changed, 81 insertions(+), 17 deletions(-)

-- 
2.20.1

[pve-devel] [PATCH v5 container 1/4] setup: add post_clone_hook for containers

[Oguz Bektas]

for now it calls the new clear_machine_id function.

this way when new containers are created they will have a unique
/etc/machine-id.

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v4->v5:
* no change


 src/PVE/LXC/Setup.pm      | 12 ++++++++++++
 src/PVE/LXC/Setup/Base.pm | 31 +++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/src/PVE/LXC/Setup.pm b/src/PVE/LXC/Setup.pm
index 8b8fee9..bcb995d 100644
--- a/src/PVE/LXC/Setup.pm
+++ b/src/PVE/LXC/Setup.pm
@@ -352,6 +352,18 @@ sub pre_start_hook {
     $self->protected_call($code);
 }
 
+sub post_clone_hook {
+    my ($self, $conf) = @_;
+
+    my $clone = 1;
+
+    my $code = sub {
+	$self->{plugin}->post_clone_hook($self->{conf}, $clone);
+    };
+    $self->protected_call($code);
+
+}
+
 sub post_create_hook {
     my ($self, $root_password, $ssh_keys) = @_;
 
diff --git a/src/PVE/LXC/Setup/Base.pm b/src/PVE/LXC/Setup/Base.pm
index d73335b..54cb9a8 100644
--- a/src/PVE/LXC/Setup/Base.pm
+++ b/src/PVE/LXC/Setup/Base.pm
@@ -476,6 +476,30 @@ sub set_timezone {
     }
 }
 
+sub clear_machine_id {
+    my ($self, $conf, $clone) = @_;
+
+    my $uses_systemd = $self->ct_is_executable("/lib/systemd/systemd")
+	|| $self->ct_is_executable("/usr/lib/systemd/systemd");
+
+    my $dbus_machine_id_path = "/var/lib/dbus/machine-id";
+    my $machine_id_path = "/etc/machine-id";
+    if (
+	$self->ct_file_exists($dbus_machine_id_path)
+	&& !$self->ct_is_symlink($dbus_machine_id_path)
+	&& $uses_systemd
+    ) {
+        $self->ct_unlink($dbus_machine_id_path);
+    }
+
+    # don't remove file if container is being cloned
+    if ($clone) {
+	$self->ct_file_set_contents($machine_id_path, "\n");
+    } else {
+	$self->ct_unlink($machine_id_path);
+    }
+}
+
 sub pre_start_hook {
     my ($self, $conf) = @_;
 
@@ -488,9 +512,16 @@ sub pre_start_hook {
     # fixme: what else ?
 }
 
+sub post_clone_hook {
+    my ($self, $conf) = @_;
+
+    $self->clear_machine_id($conf, 1);
+}
+
 sub post_create_hook {
     my ($self, $conf, $root_password, $ssh_keys) = @_;
 
+    $self->clear_machine_id($conf);
     $self->template_fixup($conf);
 
     &$randomize_crontab($self, $conf);
-- 
2.20.1

[pve-devel] [PATCH v5 container 2/4] clone_vm: improve config locking

[Oguz Bektas]

cleaned up the locking situation with config files as Fabian G.
suggested in the review.

use the 'create_and_lock_config' helper in the beginning to ensure that
the target CTID is available, and that the target config is locked from
the beginning. in case any error happens during the initial checks, we
unlink this config in error handling.

firewall config is also now cloned inside the worker instead of before
the worker, in case the clone fails.

also lock the config file when renaming the conf (for moving to a target
node when the option is passed).

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v4->v5:
* move create_and_lock_config outside the eval
* also lock moving config to target node



 src/PVE/API2/LXC.pm | 44 ++++++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 936debb..ade109b 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -1394,6 +1394,9 @@ __PACKAGE__->register_method({
 	my $vollist = [];
 	my $running;
 
+	PVE::LXC::Config->create_and_lock_config($newid, 0);
+	$conffile = PVE::LXC::Config->config_file($newid);
+
 	PVE::LXC::Config->lock_config($vmid, sub {
 	    my $src_conf = PVE::LXC::Config->set_lock($vmid, 'disk');
 
@@ -1411,10 +1414,6 @@ __PACKAGE__->register_method({
 
 		my $src_conf = $snapname ? $src_conf->{snapshots}->{$snapname} : $src_conf;
 
-		$conffile = PVE::LXC::Config->config_file($newid);
-		die "unable to create CT $newid: config file already exists\n"
-		    if -f $conffile;
-
 		my $sharedvm = 1;
 		for my $opt (sort keys %$src_conf) {
 		    next if $opt =~ m/^unused\d+$/;
@@ -1482,11 +1481,23 @@ __PACKAGE__->register_method({
 		    $newconf->{description} = $param->{description};
 		}
 
-		# create empty/temp config - this fails if CT already exists on other node
-		PVE::LXC::Config->write_config($newid, $newconf);
+		PVE::LXC::Config->lock_config($newid, sub {
+		    # read empty config, lock needs to be still here
+		    my $conf = PVE::LXC::Config->load_config($newid);
+		    die "Lost 'create' config lock, aborting.\n"
+			if !PVE::LXC::Config->has_lock($conf, 'create');
+		    # write the actual new config now to disk
+		    PVE::LXC::Config->write_config($newid, $newconf);
+		});
 	    };
 	    if (my $err = $@) {
 		eval { PVE::LXC::Config->remove_lock($vmid, 'disk') };
+		PVE::LXC::Config->lock_config($newid, sub {
+		    my $conf = PVE::LXC::Config->load_config($newid);
+		    die "Lost 'create' config lock, aborting.\n"
+			if !PVE::LXC::Config->has_lock($conf, 'create');
+		    unlink($conffile);
+		});
 		warn $@ if $@;
 		die $err;
 	    }
@@ -1545,18 +1556,19 @@ __PACKAGE__->register_method({
 		PVE::AccessControl::add_vm_to_pool($newid, $pool) if $pool;
 		PVE::LXC::Config->remove_lock($newid, 'create');
 
-		if ($target) {
-		    # always deactivate volumes - avoid lvm LVs to be active on several nodes
-		    PVE::Storage::deactivate_volumes($storecfg, $vollist, $snapname) if !$running;
-		    PVE::Storage::deactivate_volumes($storecfg, $newvollist);
+		PVE::LXC::Config->lock_config($newid, sub {
+		    if ($target) {
+			# always deactivate volumes - avoid lvm LVs to be active on several nodes
+			PVE::Storage::deactivate_volumes($storecfg, $vollist, $snapname) if !$running;
+			PVE::Storage::deactivate_volumes($storecfg, $newvollist);
 
-		    my $newconffile = PVE::LXC::Config->config_file($newid, $target);
-		    die "Failed to move config to node '$target' - rename failed: $!\n"
-			if !rename($conffile, $newconffile);
-		}
+			my $newconffile = PVE::LXC::Config->config_file($newid, $target);
+			die "Failed to move config to node '$target' - rename failed: $!\n"
+			    if !rename($conffile, $newconffile);
+		    }
+		});
 	    };
 	    my $err = $@;
-
 	    # Unlock the source config in any case:
 	    eval { PVE::LXC::Config->remove_lock($vmid, 'disk') };
 	    warn $@ if $@;
@@ -1574,10 +1586,10 @@ __PACKAGE__->register_method({
 		die "clone failed: $err";
 	    }
 
+	    PVE::Firewall::clone_vmfw_conf($vmid, $newid);
 	    return;
 	};
 
-	PVE::Firewall::clone_vmfw_conf($vmid, $newid);
 	return $rpcenv->fork_worker('vzclone', $vmid, $authuser, $realcmd);
     }});
 
-- 
2.20.1

[pve-devel] [PATCH v5 container 3/4] run post_clone_hook in clone_vm API

[Oguz Bektas]

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v4->v5:
* no change


 src/PVE/API2/LXC.pm | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index ade109b..c2312f4 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -1554,6 +1554,15 @@ __PACKAGE__->register_method({
 		}
 
 		PVE::AccessControl::add_vm_to_pool($newid, $pool) if $pool;
+
+		$newconf = PVE::LXC::Config->load_config($newid);
+		die "Lost 'create' config lock, aborting.\n"
+		    if !PVE::LXC::Config->has_lock($newconf, 'create');
+		my $rootdir = PVE::LXC::mount_all($newid, $storecfg, $newconf, 1);
+		my $lxc_setup = PVE::LXC::Setup->new($newconf, $rootdir);
+		$lxc_setup->post_clone_hook($newconf);
+		PVE::LXC::umount_all($newid, $storecfg, $newconf, 1);
+
 		PVE::LXC::Config->remove_lock($newid, 'create');
 
 		PVE::LXC::Config->lock_config($newid, sub {
-- 
2.20.1

[pve-devel] [PATCH v5 container 4/4] clone_vm: fix minor typo in error message

[Oguz Bektas]

Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v4->v5:
* split

 src/PVE/API2/LXC.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index c2312f4..4dd692d 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -1451,7 +1451,7 @@ __PACKAGE__->register_method({
 
 			} else {
 			    # TODO: allow bind mounts?
-			    die "unable to clone mountpint '$opt' (type $mp->{type})\n";
+			    die "unable to clone mountpoint '$opt' (type $mp->{type})\n";
 			}
 		    } elsif ($opt =~ m/^net(\d+)$/) {
 			# always change MAC! address
-- 
2.20.1

[pve-devel] [PATCH container 0/6] clone_vm follow-ups

[Fabian Grünbichler]

some patches cleaning this up some more
- use existing helpers where applicable
- define new helpers for repeated code
- fix lock scoping
- correctly handle FW config and improve error handling

requires the series to be applied first ;)

Fabian Grünbichler (6):
  clone_vm: use move_config_to_node
  clone_vm: use destroy_config instead of manual unlink
  clone_vm: reduce source flock scope
  clone_vm: move linked clone check in eval
  clone_vm: refactor locking further
  clone_vm: rework firewall config cloning

 src/PVE/API2/LXC.pm | 237 ++++++++++++++++++++++++--------------------
 1 file changed, 128 insertions(+), 109 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH container 1/6] clone_vm: use move_config_to_node

[Fabian Grünbichler]

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/PVE/API2/LXC.pm | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 4dd692d..9865a6b 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -1571,9 +1571,7 @@ __PACKAGE__->register_method({
 			PVE::Storage::deactivate_volumes($storecfg, $vollist, $snapname) if !$running;
 			PVE::Storage::deactivate_volumes($storecfg, $newvollist);
 
-			my $newconffile = PVE::LXC::Config->config_file($newid, $target);
-			die "Failed to move config to node '$target' - rename failed: $!\n"
-			    if !rename($conffile, $newconffile);
+			PVE::LXC::Config->move_config_to_node($newid, $target);
 		    }
 		});
 	    };
-- 
2.30.2

[pve-devel] [PATCH container 2/6] clone_vm: use destroy_config instead of manual unlink

[Fabian Grünbichler]

and wrap the calls in an eval to preserve original errors causing us to
remove the config in the first place..

also, remove disks before removing the locked config (reverse order of
creation).

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/PVE/API2/LXC.pm | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 9865a6b..807709a 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -1387,7 +1387,6 @@ __PACKAGE__->register_method({
 
 	PVE::Cluster::check_cfs_quorum();
 
-	my $conffile;
 	my $newconf = {};
 	my $mountpoints = {};
 	my $fullclone = {};
@@ -1395,7 +1394,6 @@ __PACKAGE__->register_method({
 	my $running;
 
 	PVE::LXC::Config->create_and_lock_config($newid, 0);
-	$conffile = PVE::LXC::Config->config_file($newid);
 
 	PVE::LXC::Config->lock_config($vmid, sub {
 	    my $src_conf = PVE::LXC::Config->set_lock($vmid, 'disk');
@@ -1492,13 +1490,18 @@ __PACKAGE__->register_method({
 	    };
 	    if (my $err = $@) {
 		eval { PVE::LXC::Config->remove_lock($vmid, 'disk') };
-		PVE::LXC::Config->lock_config($newid, sub {
-		    my $conf = PVE::LXC::Config->load_config($newid);
-		    die "Lost 'create' config lock, aborting.\n"
-			if !PVE::LXC::Config->has_lock($conf, 'create');
-		    unlink($conffile);
-		});
-		warn $@ if $@;
+		warn "Failed to remove source CT config lock - $@\n" if $@;
+
+		eval {
+		    PVE::LXC::Config->lock_config($newid, sub {
+			my $conf = PVE::LXC::Config->load_config($newid);
+			die "Lost 'create' config lock, aborting.\n"
+			    if !PVE::LXC::Config->has_lock($conf, 'create');
+			PVE::LXC::Config->destroy_config($newid);
+		    });
+		};
+		warn "Failed to remove target CT config - $@\n" if $@;
+
 		die $err;
 	    }
 	});
@@ -1582,14 +1585,23 @@ __PACKAGE__->register_method({
 
 	    if ($err) {
 		# Now cleanup the config & disks:
-		unlink $conffile;
-
 		sleep 1; # some storages like rbd need to wait before release volume - really?
 
 		foreach my $volid (@$newvollist) {
 		    eval { PVE::Storage::vdisk_free($storecfg, $volid); };
 		    warn $@ if $@;
 		}
+
+		eval {
+		    PVE::LXC::Config->lock_config($newid, sub {
+			my $conf = PVE::LXC::Config->load_config($newid);
+			die "Lost 'create' config lock, aborting.\n"
+			    if !PVE::LXC::Config->has_lock($conf, 'create');
+			PVE::LXC::Config->destroy_config($newid);
+		    });
+		};
+		warn "Failed to remove target CT config - $@\n" if $@;
+
 		die "clone failed: $err";
 	    }
 
-- 
2.30.2

[pve-devel] [PATCH container 3/6] clone_vm: reduce source flock scope

[Fabian Grünbichler]

set_lock already obtains the flock (since it does a read-modify-write
cycle), and the rest of this code does not touch the config file in any
fashion so no need to hold the flock either..

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    best viewed with -w ;)

 src/PVE/API2/LXC.pm | 163 ++++++++++++++++++++++----------------------
 1 file changed, 81 insertions(+), 82 deletions(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 807709a..1554ef2 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -1395,116 +1395,115 @@ __PACKAGE__->register_method({
 
 	PVE::LXC::Config->create_and_lock_config($newid, 0);
 
-	PVE::LXC::Config->lock_config($vmid, sub {
-	    my $src_conf = PVE::LXC::Config->set_lock($vmid, 'disk');
-
-	    $running = PVE::LXC::check_running($vmid) || 0;
 
-	    my $full = extract_param($param, 'full');
-	    if (!defined($full)) {
-		$full = !PVE::LXC::Config->is_template($src_conf);
-	    }
-	    die "parameter 'storage' not allowed for linked clones\n" if defined($storage) && !$full;
+	my $src_conf = PVE::LXC::Config->set_lock($vmid, 'disk');
 
-	    eval {
-		die "snapshot '$snapname' does not exist\n"
-		    if $snapname && !defined($src_conf->{snapshots}->{$snapname});
+	$running = PVE::LXC::check_running($vmid) || 0;
 
-		my $src_conf = $snapname ? $src_conf->{snapshots}->{$snapname} : $src_conf;
+	my $full = extract_param($param, 'full');
+	if (!defined($full)) {
+	    $full = !PVE::LXC::Config->is_template($src_conf);
+	}
+	die "parameter 'storage' not allowed for linked clones\n" if defined($storage) && !$full;
 
-		my $sharedvm = 1;
-		for my $opt (sort keys %$src_conf) {
-		    next if $opt =~ m/^unused\d+$/;
+	eval {
+	    die "snapshot '$snapname' does not exist\n"
+		if $snapname && !defined($src_conf->{snapshots}->{$snapname});
 
-		    my $value = $src_conf->{$opt};
+	    my $src_conf = $snapname ? $src_conf->{snapshots}->{$snapname} : $src_conf;
 
-		    if (($opt eq 'rootfs') || ($opt =~ m/^mp\d+$/)) {
-			my $mp = PVE::LXC::Config->parse_volume($opt, $value);
+	    my $sharedvm = 1;
+	    for my $opt (sort keys %$src_conf) {
+		next if $opt =~ m/^unused\d+$/;
 
-			if ($mp->{type} eq 'volume') {
-			    my $volid = $mp->{volume};
+		my $value = $src_conf->{$opt};
 
-			    my ($sid, $volname) = PVE::Storage::parse_volume_id($volid);
-			    $sid = $storage if defined($storage);
-			    my $scfg = PVE::Storage::storage_config($storecfg, $sid);
-			    if (!$scfg->{shared}) {
-				$sharedvm = 0;
-				warn "found non-shared volume: $volid\n" if $target;
-			    }
+		if (($opt eq 'rootfs') || ($opt =~ m/^mp\d+$/)) {
+		    my $mp = PVE::LXC::Config->parse_volume($opt, $value);
 
-			    $rpcenv->check($authuser, "/storage/$sid", ['Datastore.AllocateSpace']);
+		    if ($mp->{type} eq 'volume') {
+			my $volid = $mp->{volume};
 
-			    if ($full) {
-				die "Cannot do full clones on a running container without snapshots\n"
-				    if $running && !defined($snapname);
-				$fullclone->{$opt} = 1;
-			    } else {
-				# not full means clone instead of copy
-				die "Linked clone feature for '$volid' is not available\n"
-				    if !PVE::Storage::volume_has_feature($storecfg, 'clone', $volid, $snapname, $running, {'valid_target_formats' => ['raw', 'subvol']});
-			    }
+			my ($sid, $volname) = PVE::Storage::parse_volume_id($volid);
+			$sid = $storage if defined($storage);
+			my $scfg = PVE::Storage::storage_config($storecfg, $sid);
+			if (!$scfg->{shared}) {
+			    $sharedvm = 0;
+			    warn "found non-shared volume: $volid\n" if $target;
+			}
 
-			    $mountpoints->{$opt} = $mp;
-			    push @$vollist, $volid;
+			$rpcenv->check($authuser, "/storage/$sid", ['Datastore.AllocateSpace']);
 
+			if ($full) {
+			    die "Cannot do full clones on a running container without snapshots\n"
+				if $running && !defined($snapname);
+			    $fullclone->{$opt} = 1;
 			} else {
-			    # TODO: allow bind mounts?
-			    die "unable to clone mountpoint '$opt' (type $mp->{type})\n";
+			    # not full means clone instead of copy
+			    die "Linked clone feature for '$volid' is not available\n"
+				if !PVE::Storage::volume_has_feature($storecfg, 'clone', $volid, $snapname, $running, {'valid_target_formats' => ['raw', 'subvol']});
 			}
-		    } elsif ($opt =~ m/^net(\d+)$/) {
-			# always change MAC! address
-			my $dc = PVE::Cluster::cfs_read_file('datacenter.cfg');
-			my $net = PVE::LXC::Config->parse_lxc_network($value);
-			$net->{hwaddr} = PVE::Tools::random_ether_addr($dc->{mac_prefix});
-			$newconf->{$opt} = PVE::LXC::Config->print_lxc_network($net);
+
+			$mountpoints->{$opt} = $mp;
+			push @$vollist, $volid;
+
 		    } else {
-			# copy everything else
-			$newconf->{$opt} = $value;
+			# TODO: allow bind mounts?
+			die "unable to clone mountpoint '$opt' (type $mp->{type})\n";
 		    }
+		} elsif ($opt =~ m/^net(\d+)$/) {
+		    # always change MAC! address
+		    my $dc = PVE::Cluster::cfs_read_file('datacenter.cfg');
+		    my $net = PVE::LXC::Config->parse_lxc_network($value);
+		    $net->{hwaddr} = PVE::Tools::random_ether_addr($dc->{mac_prefix});
+		    $newconf->{$opt} = PVE::LXC::Config->print_lxc_network($net);
+		} else {
+		    # copy everything else
+		    $newconf->{$opt} = $value;
 		}
-		die "can't clone CT to node '$target' (CT uses local storage)\n"
-		    if $target && !$sharedvm;
+	    }
+	    die "can't clone CT to node '$target' (CT uses local storage)\n"
+		if $target && !$sharedvm;
 
-		# Replace the 'disk' lock with a 'create' lock.
-		$newconf->{lock} = 'create';
+	    # Replace the 'disk' lock with a 'create' lock.
+	    $newconf->{lock} = 'create';
 
-		delete $newconf->{snapshots};
-		delete $newconf->{pending};
-		delete $newconf->{template};
-		if ($param->{hostname}) {
-		    $newconf->{hostname} = $param->{hostname};
-		}
+	    delete $newconf->{snapshots};
+	    delete $newconf->{pending};
+	    delete $newconf->{template};
+	    if ($param->{hostname}) {
+		$newconf->{hostname} = $param->{hostname};
+	    }
 
-		if ($param->{description}) {
-		    $newconf->{description} = $param->{description};
-		}
+	    if ($param->{description}) {
+		$newconf->{description} = $param->{description};
+	    }
 
+	    PVE::LXC::Config->lock_config($newid, sub {
+		# read empty config, lock needs to be still here
+		my $conf = PVE::LXC::Config->load_config($newid);
+		die "Lost 'create' config lock, aborting.\n"
+		    if !PVE::LXC::Config->has_lock($conf, 'create');
+		# write the actual new config now to disk
+		PVE::LXC::Config->write_config($newid, $newconf);
+	    });
+	};
+	if (my $err = $@) {
+	    eval { PVE::LXC::Config->remove_lock($vmid, 'disk') };
+	    warn "Failed to remove source CT config lock - $@\n" if $@;
+
+	    eval {
 		PVE::LXC::Config->lock_config($newid, sub {
-		    # read empty config, lock needs to be still here
 		    my $conf = PVE::LXC::Config->load_config($newid);
 		    die "Lost 'create' config lock, aborting.\n"
 			if !PVE::LXC::Config->has_lock($conf, 'create');
-		    # write the actual new config now to disk
-		    PVE::LXC::Config->write_config($newid, $newconf);
+		    PVE::LXC::Config->destroy_config($newid);
 		});
 	    };
-	    if (my $err = $@) {
-		eval { PVE::LXC::Config->remove_lock($vmid, 'disk') };
-		warn "Failed to remove source CT config lock - $@\n" if $@;
-
-		eval {
-		    PVE::LXC::Config->lock_config($newid, sub {
-			my $conf = PVE::LXC::Config->load_config($newid);
-			die "Lost 'create' config lock, aborting.\n"
-			    if !PVE::LXC::Config->has_lock($conf, 'create');
-			PVE::LXC::Config->destroy_config($newid);
-		    });
-		};
-		warn "Failed to remove target CT config - $@\n" if $@;
+	    warn "Failed to remove target CT config - $@\n" if $@;
 
-		die $err;
-	    }
-	});
+	    die $err;
+	}
 
 	my $update_conf = sub {
 	    my ($key, $value) = @_;
-- 
2.30.2

[pve-devel] [PATCH container 4/6] clone_vm: move linked clone check in eval

[Fabian Grünbichler]

so that the source config is properly cleaned up/unlocked

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/PVE/API2/LXC.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 1554ef2..5406923 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -1404,9 +1404,11 @@ __PACKAGE__->register_method({
 	if (!defined($full)) {
 	    $full = !PVE::LXC::Config->is_template($src_conf);
 	}
-	die "parameter 'storage' not allowed for linked clones\n" if defined($storage) && !$full;
 
 	eval {
+	    die "parameter 'storage' not allowed for linked clones\n"
+		if defined($storage) && !$full;
+
 	    die "snapshot '$snapname' does not exist\n"
 		if $snapname && !defined($src_conf->{snapshots}->{$snapname});
 
-- 
2.30.2

[pve-devel] [PATCH container 5/6] clone_vm: refactor locking further

[Fabian Grünbichler]

introduce a new helper handling
- obtaining the flock
- (re)loading the config
- checking that the 'create' lock is still there

before calling a passed-in sub with the current config, since this
pattern was used quite a lot here.

intentionally changed behaviour:
- flock is now held for the post_clone hook call
- failure to remove the 'create' lock or to move the config to the
  target node if applicable will not undo the clone, since either is
  trivially fixable ('pct unlock' or a no-op migration), and copying all
  those volumes might have been quite expensive..

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
there are probably quite a few places in the container/qemu-server code
where we should employ this or a similar mechanism with digest tracking
to be 100% on the safe side..

 src/PVE/API2/LXC.pm | 76 ++++++++++++++++++++++++---------------------
 1 file changed, 41 insertions(+), 35 deletions(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 5406923..4877dd9 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -1395,6 +1395,16 @@ __PACKAGE__->register_method({
 
 	PVE::LXC::Config->create_and_lock_config($newid, 0);
 
+	my $lock_and_reload = sub {
+	    my ($vmid, $code) = @_;
+	    return PVE::LXC::Config->lock_config($vmid, sub {
+		my $conf = PVE::LXC::Config->load_config($vmid);
+		die "Lost 'create' config lock, aborting.\n"
+		    if !PVE::LXC::Config->has_lock($conf, 'create');
+
+		return $code->($conf);
+	    });
+	};
 
 	my $src_conf = PVE::LXC::Config->set_lock($vmid, 'disk');
 
@@ -1481,12 +1491,7 @@ __PACKAGE__->register_method({
 		$newconf->{description} = $param->{description};
 	    }
 
-	    PVE::LXC::Config->lock_config($newid, sub {
-		# read empty config, lock needs to be still here
-		my $conf = PVE::LXC::Config->load_config($newid);
-		die "Lost 'create' config lock, aborting.\n"
-		    if !PVE::LXC::Config->has_lock($conf, 'create');
-		# write the actual new config now to disk
+	    $lock_and_reload->($newid, sub {
 		PVE::LXC::Config->write_config($newid, $newconf);
 	    });
 	};
@@ -1495,10 +1500,7 @@ __PACKAGE__->register_method({
 	    warn "Failed to remove source CT config lock - $@\n" if $@;
 
 	    eval {
-		PVE::LXC::Config->lock_config($newid, sub {
-		    my $conf = PVE::LXC::Config->load_config($newid);
-		    die "Lost 'create' config lock, aborting.\n"
-			if !PVE::LXC::Config->has_lock($conf, 'create');
+		$lock_and_reload->($newid, sub {
 		    PVE::LXC::Config->destroy_config($newid);
 		});
 	    };
@@ -1509,10 +1511,8 @@ __PACKAGE__->register_method({
 
 	my $update_conf = sub {
 	    my ($key, $value) = @_;
-	    return PVE::LXC::Config->lock_config($newid, sub {
-		my $conf = PVE::LXC::Config->load_config($newid);
-		die "Lost 'create' config lock, aborting.\n"
-		    if !PVE::LXC::Config->has_lock($conf, 'create');
+	    return $lock_and_reload->($newid, sub {
+		my $conf = shift;
 		$conf->{$key} = $value;
 		PVE::LXC::Config->write_config($newid, $conf);
 	    });
@@ -1559,23 +1559,20 @@ __PACKAGE__->register_method({
 
 		PVE::AccessControl::add_vm_to_pool($newid, $pool) if $pool;
 
-		$newconf = PVE::LXC::Config->load_config($newid);
-		die "Lost 'create' config lock, aborting.\n"
-		    if !PVE::LXC::Config->has_lock($newconf, 'create');
-		my $rootdir = PVE::LXC::mount_all($newid, $storecfg, $newconf, 1);
-		my $lxc_setup = PVE::LXC::Setup->new($newconf, $rootdir);
-		$lxc_setup->post_clone_hook($newconf);
-		PVE::LXC::umount_all($newid, $storecfg, $newconf, 1);
-
-		PVE::LXC::Config->remove_lock($newid, 'create');
-
-		PVE::LXC::Config->lock_config($newid, sub {
-		    if ($target) {
-			# always deactivate volumes - avoid lvm LVs to be active on several nodes
-			PVE::Storage::deactivate_volumes($storecfg, $vollist, $snapname) if !$running;
-			PVE::Storage::deactivate_volumes($storecfg, $newvollist);
-
-			PVE::LXC::Config->move_config_to_node($newid, $target);
+		$lock_and_reload->($newid, sub {
+		    my $conf = shift;
+		    my $rootdir = PVE::LXC::mount_all($newid, $storecfg, $conf, 1);
+		    eval {
+			my $lxc_setup = PVE::LXC::Setup->new($conf, $rootdir);
+			$lxc_setup->post_clone_hook($conf);
+		    };
+		    my $err = $@;
+		    eval { PVE::LXC::umount_all($newid, $storecfg, $conf, 1); };
+		    if ($err) {
+			warn "$@\n" if $@;
+			die $err;
+		    } else {
+			die $@ if $@;
 		    }
 		});
 	    };
@@ -1594,10 +1591,7 @@ __PACKAGE__->register_method({
 		}
 
 		eval {
-		    PVE::LXC::Config->lock_config($newid, sub {
-			my $conf = PVE::LXC::Config->load_config($newid);
-			die "Lost 'create' config lock, aborting.\n"
-			    if !PVE::LXC::Config->has_lock($conf, 'create');
+		    $lock_and_reload->($newid, sub {
 			PVE::LXC::Config->destroy_config($newid);
 		    });
 		};
@@ -1606,6 +1600,18 @@ __PACKAGE__->register_method({
 		die "clone failed: $err";
 	    }
 
+	    $lock_and_reload->($newid, sub {
+		PVE::LXC::Config->remove_lock($newid, 'create');
+
+		if ($target) {
+		    # always deactivate volumes - avoid lvm LVs to be active on several nodes
+		    PVE::Storage::deactivate_volumes($storecfg, $vollist, $snapname) if !$running;
+		    PVE::Storage::deactivate_volumes($storecfg, $newvollist);
+
+		    PVE::LXC::Config->move_config_to_node($newid, $target);
+		}
+	    });
+
 	    PVE::Firewall::clone_vmfw_conf($vmid, $newid);
 	    return;
 	};
-- 
2.30.2

[pve-devel] [PATCH container 6/6] clone_vm: rework firewall config cloning

[Fabian Grünbichler]

we need to clone the firewall config before doing any actual work, else
we risk partially aborting and leaving a non-firewalled container
around. accordingly, we need to (attempt to) remove the cloned FW config
after successfully removing the guest config in error handling.

partially reverts/fixes 4925b86a920a862f25f0d93d243ce099c922979d clone_vm: improve config locking

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/PVE/API2/LXC.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 4877dd9..0d4d91a 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -1394,6 +1394,7 @@ __PACKAGE__->register_method({
 	my $running;
 
 	PVE::LXC::Config->create_and_lock_config($newid, 0);
+	PVE::Firewall::clone_vmfw_conf($vmid, $newid);
 
 	my $lock_and_reload = sub {
 	    my ($vmid, $code) = @_;
@@ -1502,6 +1503,7 @@ __PACKAGE__->register_method({
 	    eval {
 		$lock_and_reload->($newid, sub {
 		    PVE::LXC::Config->destroy_config($newid);
+		    PVE::Firewall::remove_vmfw_conf($newid);
 		});
 	    };
 	    warn "Failed to remove target CT config - $@\n" if $@;
@@ -1593,6 +1595,7 @@ __PACKAGE__->register_method({
 		eval {
 		    $lock_and_reload->($newid, sub {
 			PVE::LXC::Config->destroy_config($newid);
+			PVE::Firewall::remove_vmfw_conf($newid);
 		    });
 		};
 		warn "Failed to remove target CT config - $@\n" if $@;
@@ -1612,7 +1615,6 @@ __PACKAGE__->register_method({
 		}
 	    });
 
-	    PVE::Firewall::clone_vmfw_conf($vmid, $newid);
 	    return;
 	};
 
-- 
2.30.2

Re: [pve-devel] [PATCH v5 container 1/4] setup: add post_clone_hook for containers

[Thomas Lamprecht]

On 17.06.21 12:51, Oguz Bektas wrote:
> --- a/src/PVE/LXC/Setup.pm
> +++ b/src/PVE/LXC/Setup.pm
> @@ -352,6 +352,18 @@ sub pre_start_hook {
>      $self->protected_call($code);
>  }
>  
> +sub post_clone_hook {
> +    my ($self, $conf) = @_;> +
> +    my $clone = 1;


what is with this parameter? The post_clone_hook does not have it in it's
signature and it does not look like it would be useful in general...

> +
> +    my $code = sub {
> +	$self->{plugin}->post_clone_hook($self->{conf}, $clone);

why pass $conf if then $self->{conf} is passed to the actual hook, leaving $conf
unused?

> +    };
> +    $self->protected_call($code);
> +
> +}
> +
>  sub post_create_hook {
>      my ($self, $root_password, $ssh_keys) = @_;
>  

[pve-devel] applied: [PATCH v5 container 0/4] post_clone_hook for containers

[Thomas Lamprecht]

On 17.06.21 12:51, Oguz Bektas wrote:
> add post clone hook, and fix #3443 to clear machine-id for cloned
> containers
> 
> 
> v4->v5:
> * split patches for easy review/backport
> * also lock when moving config to target node
> * move create_and_lock_config outside the eval, this should be checked
> before the other ones
> 
> 
> Oguz Bektas (4):
>   setup: add post_clone_hook for containers
>   clone_vm: improve config locking
>   run post_clone_hook in clone_vm API
>   clone_vm: fix minor typo in error message
> 
>  src/PVE/API2/LXC.pm       | 55 +++++++++++++++++++++++++++------------
>  src/PVE/LXC/Setup.pm      | 12 +++++++++
>  src/PVE/LXC/Setup/Base.pm | 31 ++++++++++++++++++++++
>  3 files changed, 81 insertions(+), 17 deletions(-)
> 



applied with Fabians followups (thanks for those!), but it is still buggy then, i.e.,
- LXC::Setup post_clone_hook was just completely off
- if a (non-systemd) CT without any /etc/machine-id present gets cloned it suddenly
  gets an empty one?

so pushed out two followups to fix that mess...