[pve-devel] [PATCH container 1/2] fix: clean up config when invalid ostemplate is given

[pve-devel] [PATCH container 1/2] fix: clean up config when invalid ostemplate is given

[Daniel Tschlatscher]

Before, if an invalid/non-existant ostemplate parameter was passed,
the task would abort, but would leave an empty config file behind.
This also applies to errors for invalid mount point configurations.
In both cases, the empty config will now be removed.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
 src/PVE/API2/LXC.pm | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 79aecaa..7cc64af 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -443,7 +443,14 @@ __PACKAGE__->register_method({
 		    }
 		}
 	    };
-	    die "$emsg $@" if $@;
+
+	    if (my $reason = $@) {
+		if (!$same_container_exists) {
+		    eval { PVE::LXC::Config->destroy_config($vmid) };
+		    $remove_lock = 0;
+		}
+		die "$emsg $reason";
+	    }
 
 	    # up until here we did not modify the container, besides the lock
 	    $remove_lock = 0;
-- 
2.30.2

[pve-devel] [PATCH container 2/2] fix: also remove firewall config after failed restore

[Daniel Tschlatscher]

Before, a failed restore would only remove the container config, but
the firewall config would remain.
Now, the firewall config is also removed, except for the case when the
user only has the VM.Backup permission. In this case the firewall
would not have been restored/changed by us and is left as is.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
 src/PVE/API2/LXC.pm | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 7cc64af..27b63f2 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -505,6 +505,9 @@ __PACKAGE__->register_method({
 	    if (my $err = $@) {
 		PVE::LXC::destroy_disks($storage_cfg, $vollist);
 		eval { PVE::LXC::Config->destroy_config($vmid) };
+		if (!$skip_fw_config_restore) { # Only if user has permission to change the firewall
+		    PVE::Firewall::remove_vmfw_conf($vmid);
+		}
 		warn $@ if $@;
 		die "$emsg $err";
 	    }
-- 
2.30.2

Re: [pve-devel] [PATCH container 1/2] fix: clean up config when invalid ostemplate is given

[Thomas Lamprecht]

the "fix" is unsual for non #bugids, rather tag the subsystem you're modifying, e.g.:

restore: clean up config when invalid source archive is given

Am 17/11/2022 um 10:39 schrieb Daniel Tschlatscher:
> Before, if an invalid/non-existant ostemplate parameter was passed,

s/existant/existent/

And it's only relevant in the restore case or? as otherwise there isn't
happening anything in that code part fwict; maybe hint that here.

> the task would abort, but would leave an empty config file behind.
> This also applies to errors for invalid mount point configurations.
> In both cases, the empty config will now be removed.
> 
> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
> ---
>  src/PVE/API2/LXC.pm | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 79aecaa..7cc64af 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -443,7 +443,14 @@ __PACKAGE__->register_method({
>  		    }
>  		}
>  	    };
> -	    die "$emsg $@" if $@;
> +
> +	    if (my $reason = $@) {
> +		if (!$same_container_exists) {
> +		    eval { PVE::LXC::Config->destroy_config($vmid) };

we already have such a section below, maybe you could move that out in the general cleanup
and set a flag similarly to $remove_lock, e.g., $destroy_config_on_error then in the error
path of the outher locked call it would look something like:

if ($destroy_config_on_error) {
    eval { PVE::LXC::Config->destroy_config($vmid) };
    warn $@ if $@;
} elsif ($remove_lock) {
    PVE::LXC::Config->remove_lock($vmid, 'create');
}

that way you wouldn't need to override remove_lock either. Not a strong preference
towards that, so just throwing the idea out there.

> +		    $remove_lock = 0;
> +		}
> +		die "$emsg $reason";
> +	    }
>  
>  	    # up until here we did not modify the container, besides the lock
>  	    $remove_lock = 0;