* [pve-devel] [PATCH container] fix: cloning a locked container creates an empty config @ 2022-06-14 12:22 Daniel Tschlatscher 2022-06-14 12:51 ` Fabian Grünbichler 0 siblings, 1 reply; 4+ messages in thread From: Daniel Tschlatscher @ 2022-06-14 12:22 UTC (permalink / raw) To: pve-devel When an attempt was made to clone a locked container the API would correctly present the error 'CT is locked (disk)' but create the config files for the new container anyway and then abort. The fix is to simply check whether the CT config is locked before creating the configs for the new container. Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com> --- src/PVE/API2/LXC.pm | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm index 64724cb..e1b4cd3 100644 --- a/src/PVE/API2/LXC.pm +++ b/src/PVE/API2/LXC.pm @@ -1461,9 +1461,6 @@ __PACKAGE__->register_method({ my $vollist = []; 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) = @_; return PVE::LXC::Config->lock_config($vmid, sub { @@ -1477,6 +1474,9 @@ __PACKAGE__->register_method({ my $src_conf = PVE::LXC::Config->set_lock($vmid, 'disk'); + PVE::LXC::Config->create_and_lock_config($newid, 0); + PVE::Firewall::clone_vmfw_conf($vmid, $newid); + $running = PVE::LXC::check_running($vmid) || 0; my $full = extract_param($param, 'full'); -- 2.30.2 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH container] fix: cloning a locked container creates an empty config 2022-06-14 12:22 [pve-devel] [PATCH container] fix: cloning a locked container creates an empty config Daniel Tschlatscher @ 2022-06-14 12:51 ` Fabian Grünbichler 2022-06-14 13:43 ` Daniel Tschlatscher 0 siblings, 1 reply; 4+ messages in thread From: Fabian Grünbichler @ 2022-06-14 12:51 UTC (permalink / raw) To: Proxmox VE development discussion On June 14, 2022 2:22 pm, Daniel Tschlatscher wrote: > When an attempt was made to clone a locked container the API would > correctly present the error 'CT is locked (disk)' but create the > config files for the new container anyway and then abort. > > The fix is to simply check whether the CT config is locked before > creating the configs for the new container. is there a reason for not just moving it to the start of the eval block to avoid the same problem being re-introduced in the future? any error occuring inside the eval block will then trigger a cleanup.. > > Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com> > --- > src/PVE/API2/LXC.pm | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm > index 64724cb..e1b4cd3 100644 > --- a/src/PVE/API2/LXC.pm > +++ b/src/PVE/API2/LXC.pm > @@ -1461,9 +1461,6 @@ __PACKAGE__->register_method({ > my $vollist = []; > 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) = @_; > return PVE::LXC::Config->lock_config($vmid, sub { > @@ -1477,6 +1474,9 @@ __PACKAGE__->register_method({ > > my $src_conf = PVE::LXC::Config->set_lock($vmid, 'disk'); > > + PVE::LXC::Config->create_and_lock_config($newid, 0); > + PVE::Firewall::clone_vmfw_conf($vmid, $newid); > + > $running = PVE::LXC::check_running($vmid) || 0; > > my $full = extract_param($param, 'full'); > -- > 2.30.2 > > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH container] fix: cloning a locked container creates an empty config 2022-06-14 12:51 ` Fabian Grünbichler @ 2022-06-14 13:43 ` Daniel Tschlatscher 2022-06-15 8:11 ` Fabian Grünbichler 0 siblings, 1 reply; 4+ messages in thread From: Daniel Tschlatscher @ 2022-06-14 13:43 UTC (permalink / raw) To: pve-devel On 6/14/22 14:51, Fabian Grünbichler wrote: > On June 14, 2022 2:22 pm, Daniel Tschlatscher wrote: >> When an attempt was made to clone a locked container the API would >> correctly present the error 'CT is locked (disk)' but create the >> config files for the new container anyway and then abort. >> >> The fix is to simply check whether the CT config is locked before >> creating the configs for the new container. > > is there a reason for not just moving it to the start of the eval block > to avoid the same problem being re-introduced in the future? any error > occuring inside the eval block will then trigger a cleanup.. > When an error occurs and the cleanup is triggered, the cleanup tries to release the lock again. Moving the set_lock function into the eval block would create a problem here: If the lock was created by another process (and if it is a 'disk' lock) set_lock would emit an error and the lock would be incorrectly released by this process, which did not originally acquire it. >> >> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com> >> --- >> src/PVE/API2/LXC.pm | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm >> index 64724cb..e1b4cd3 100644 >> --- a/src/PVE/API2/LXC.pm >> +++ b/src/PVE/API2/LXC.pm >> @@ -1461,9 +1461,6 @@ __PACKAGE__->register_method({ >> my $vollist = []; >> 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) = @_; >> return PVE::LXC::Config->lock_config($vmid, sub { >> @@ -1477,6 +1474,9 @@ __PACKAGE__->register_method({ >> >> my $src_conf = PVE::LXC::Config->set_lock($vmid, 'disk'); >> >> + PVE::LXC::Config->create_and_lock_config($newid, 0); >> + PVE::Firewall::clone_vmfw_conf($vmid, $newid); >> + >> $running = PVE::LXC::check_running($vmid) || 0; >> >> my $full = extract_param($param, 'full'); >> -- >> 2.30.2 >> >> >> >> _______________________________________________ >> pve-devel mailing list >> pve-devel@lists.proxmox.com >> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >> >> >> > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH container] fix: cloning a locked container creates an empty config 2022-06-14 13:43 ` Daniel Tschlatscher @ 2022-06-15 8:11 ` Fabian Grünbichler 0 siblings, 0 replies; 4+ messages in thread From: Fabian Grünbichler @ 2022-06-15 8:11 UTC (permalink / raw) To: Proxmox VE development discussion On June 14, 2022 3:43 pm, Daniel Tschlatscher wrote: > > > On 6/14/22 14:51, Fabian Grünbichler wrote: >> On June 14, 2022 2:22 pm, Daniel Tschlatscher wrote: >>> When an attempt was made to clone a locked container the API would >>> correctly present the error 'CT is locked (disk)' but create the >>> config files for the new container anyway and then abort. >>> >>> The fix is to simply check whether the CT config is locked before >>> creating the configs for the new container. >> >> is there a reason for not just moving it to the start of the eval block >> to avoid the same problem being re-introduced in the future? any error >> occuring inside the eval block will then trigger a cleanup.. >> > > When an error occurs and the cleanup is triggered, the cleanup tries to > release the lock again. > > Moving the set_lock function into the eval block would create a problem > here: > If the lock was created by another process (and if it is a 'disk' lock) > set_lock would emit an error and the lock would be incorrectly released > by this process, which did not originally acquire it. my suggestion was not to move it *into* the eval block, but next to the *start* of the eval block, like so: my $newconf = {}; my $mountpoints = {}; my $fullclone = {}; my $vollist = []; my $running; 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); } 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); }); }; PVE::LXC::Config->create_and_lock_config($newid, 0); PVE::Firewall::clone_vmfw_conf($vmid, $newid); # error handling for this block will cleanup configs eval { die "parameter 'storage' not allowed for linked clones\n" if defined($storage) && !$full; to reduce the chances of re-introducing the bug by adding potentially failing calls between create_and_lock_config and the eval block. and now that I took a second look I realized that clone_vm_fw_conf is exactly such a call, and should therefore be moved into the eval ;) > >>> >>> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com> >>> --- >>> src/PVE/API2/LXC.pm | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm >>> index 64724cb..e1b4cd3 100644 >>> --- a/src/PVE/API2/LXC.pm >>> +++ b/src/PVE/API2/LXC.pm >>> @@ -1461,9 +1461,6 @@ __PACKAGE__->register_method({ >>> my $vollist = []; >>> 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) = @_; >>> return PVE::LXC::Config->lock_config($vmid, sub { >>> @@ -1477,6 +1474,9 @@ __PACKAGE__->register_method({ >>> >>> my $src_conf = PVE::LXC::Config->set_lock($vmid, 'disk'); >>> >>> + PVE::LXC::Config->create_and_lock_config($newid, 0); >>> + PVE::Firewall::clone_vmfw_conf($vmid, $newid); >>> + >>> $running = PVE::LXC::check_running($vmid) || 0; >>> >>> my $full = extract_param($param, 'full'); >>> -- >>> 2.30.2 >>> >>> >>> >>> _______________________________________________ >>> pve-devel mailing list >>> pve-devel@lists.proxmox.com >>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >>> >>> >>> >> >> >> _______________________________________________ >> pve-devel mailing list >> pve-devel@lists.proxmox.com >> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >> >> > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-06-15 8:12 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-14 12:22 [pve-devel] [PATCH container] fix: cloning a locked container creates an empty config Daniel Tschlatscher 2022-06-14 12:51 ` Fabian Grünbichler 2022-06-14 13:43 ` Daniel Tschlatscher 2022-06-15 8:11 ` Fabian Grünbichler
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.