all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [PATCH container v2] fix #7164: lxc: restore: apply password and ssh keys during container restore
@ 2026-07-29  8:25 Elias Huhsovitz
  2026-07-29 14:20 ` Erik Fastermann
  2026-07-31 12:08 ` superseded: " Elias Huhsovitz
  0 siblings, 2 replies; 5+ messages in thread
From: Elias Huhsovitz @ 2026-07-29  8:25 UTC (permalink / raw)
  To: pve-devel; +Cc: Elias Huhsovitz

When restoring a container with `pct restore`, the `--password` and
`--ssh-public-keys` options are silently ignored. Previously, the API
endpoint only invoked setup hooks during initial container creation or
cloning, skipping them entirely for restores.

Introduce a new `post_restore_hook` in the LXC setup plugin
architecture. This hook applies the provided credentials if they are
defined.

Add `check_systemd_nesting` to the `post_restore_hook`.

Move `template_fixup`from the API endpoint function to the
`post_restore_hook`, matching the `post_create_hook` logic.

Signed-off-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
---

v1: https://lore.proxmox.com/all/20260716153959.183045-1-e.huhsovitz@proxmox.com/

Changes made according to the feedback (big thanks):
https://lore.proxmox.com/all/95a38704-543e-4820-b36c-696cf192a684@proxmox.com/

changes v1->v2:
* move logic into new function `post_restore_hook`
* do not call `post_create_hook` unconditionally
* conditionally call functions to:
    - set password
    - set ssh keys
* call `check_systemd_nesting()` for pct restore

 src/PVE/API2/LXC.pm            | 2 +-
 src/PVE/LXC/Setup.pm           | 9 +++++++++
 src/PVE/LXC/Setup/Base.pm      | 9 +++++++++
 src/PVE/LXC/Setup/Plugin.pm    | 5 +++++
 src/PVE/LXC/Setup/Unmanaged.pm | 4 ++++
 5 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 22320ba..ad3b635 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -590,7 +590,7 @@ __PACKAGE__->register_method({
                         );
                         PVE::LXC::create_ifaces_ipams_ips($conf, $vmid) if $unique;
                         my $lxc_setup = PVE::LXC::Setup->new($conf, $rootdir);
-                        $lxc_setup->template_fixup($conf);
+                        $lxc_setup->post_restore_hook($password, $ssh_keys);
                     } else {
                         my $lxc_setup = PVE::LXC::Setup->new($conf, $rootdir); # detect OS
                         PVE::LXC::Config->write_config($vmid, $conf); # safe config (after OS detection)
diff --git a/src/PVE/LXC/Setup.pm b/src/PVE/LXC/Setup.pm
index d936af2..58cadba 100644
--- a/src/PVE/LXC/Setup.pm
+++ b/src/PVE/LXC/Setup.pm
@@ -329,6 +329,15 @@ sub post_create_hook {
     $self->check_systemd_nesting();
 }
 
+sub post_restore_hook {
+    my ($self, $root_password, $ssh_keys) = @_;
+
+    $self->protected_call(sub {
+        $self->{plugin}->post_restore_hook($self->{conf}, $root_password, $ssh_keys);
+    });
+    $self->check_systemd_nesting();
+}
+
 sub unified_cgroupv2_support {
     my ($self) = @_;
 
diff --git a/src/PVE/LXC/Setup/Base.pm b/src/PVE/LXC/Setup/Base.pm
index 567f9d6..f0e1f01 100644
--- a/src/PVE/LXC/Setup/Base.pm
+++ b/src/PVE/LXC/Setup/Base.pm
@@ -735,6 +735,15 @@ sub post_create_hook {
     # fixme: what else ?
 }
 
+sub post_restore_hook {
+    my ($self, $conf, $root_password, $ssh_keys) = @_;
+
+    $self->template_fixup($conf);
+
+    $self->set_user_password($conf, 'root', $root_password) if defined($root_password);
+    $self->set_user_authorized_ssh_keys($conf, 'root', $ssh_keys) if defined($ssh_keys);
+}
+
 # File access wrappers for container setup code.
 # NOTE: those are not direct part of the Plugin API (yet), avoid using them outside the child plugins
 # For user-namespace support these might need to take uid and gid maps into account.
diff --git a/src/PVE/LXC/Setup/Plugin.pm b/src/PVE/LXC/Setup/Plugin.pm
index 95a633f..6ba656b 100644
--- a/src/PVE/LXC/Setup/Plugin.pm
+++ b/src/PVE/LXC/Setup/Plugin.pm
@@ -99,4 +99,9 @@ sub post_create_hook {
     croak "implement me in sub-class";
 }
 
+sub post_restore_hook {
+    my ($self, $conf, $root_password, $ssh_keys) = @_;
+    croak "implement me in sub-class";
+}
+
 1;
diff --git a/src/PVE/LXC/Setup/Unmanaged.pm b/src/PVE/LXC/Setup/Unmanaged.pm
index b51be55..0b440cf 100644
--- a/src/PVE/LXC/Setup/Unmanaged.pm
+++ b/src/PVE/LXC/Setup/Unmanaged.pm
@@ -84,4 +84,8 @@ sub post_create_hook {
     my ($self, $conf, $root_password, $ssh_keys) = @_;
 }
 
+sub post_restore_hook {
+    my ($self, $conf, $root_password, $ssh_keys) = @_;
+}
+
 1;
-- 
2.47.3





^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH container v2] fix #7164: lxc: restore: apply password and ssh keys during container restore
  2026-07-29  8:25 [PATCH container v2] fix #7164: lxc: restore: apply password and ssh keys during container restore Elias Huhsovitz
@ 2026-07-29 14:20 ` Erik Fastermann
  2026-07-30 10:43   ` Elias Huhsovitz
  2026-07-31 12:08 ` superseded: " Elias Huhsovitz
  1 sibling, 1 reply; 5+ messages in thread
From: Erik Fastermann @ 2026-07-29 14:20 UTC (permalink / raw)
  To: Elias Huhsovitz, pve-devel

Thank you for working on this. See my comments below.


The commit message is a little long. I would just shorten it to 'fix 
#7164: restore: apply password and ssh keys', as lxc is obvious since we 
are in pve-container and 'during container restore' is clear from the 
'restore:' tag.

> When restoring a container with `pct restore`, the `--password` and
> `--ssh-public-keys` options are silently ignored. Previously, the API
> endpoint only invoked setup hooks during initial container creation or
> cloning, skipping them entirely for restores.
> 
> Introduce a new `post_restore_hook` in the LXC setup plugin
> architecture. This hook applies the provided credentials if they are
> defined.
> 
> Add `check_systemd_nesting` to the `post_restore_hook`.

Why is this added? A rational here would be helpful. If the reason is 
just symmetry with `post_create_hook`, we might need to be careful here, 
as this could introduce new unwanted failure conditions. I have not 
checked in detail, but some of the code can die.

> 
> Move `template_fixup`from the API endpoint function to the

Nit: Missing space (`template_fixup`from)

> `post_restore_hook`, matching the `post_create_hook` logic.
> 
> Signed-off-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
> ---
> 
> v1: https://lore.proxmox.com/all/20260716153959.183045-1-e.huhsovitz@proxmox.com/
> 
> Changes made according to the feedback (big thanks):
> https://lore.proxmox.com/all/95a38704-543e-4820-b36c-696cf192a684@proxmox.com/
> 
> changes v1->v2:
> * move logic into new function `post_restore_hook`
> * do not call `post_create_hook` unconditionally
> * conditionally call functions to:
>      - set password
>      - set ssh keys
> * call `check_systemd_nesting()` for pct restore
> 
>   src/PVE/API2/LXC.pm            | 2 +-
>   src/PVE/LXC/Setup.pm           | 9 +++++++++
>   src/PVE/LXC/Setup/Base.pm      | 9 +++++++++
>   src/PVE/LXC/Setup/Plugin.pm    | 5 +++++
>   src/PVE/LXC/Setup/Unmanaged.pm | 4 ++++
>   5 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 22320ba..ad3b635 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -590,7 +590,7 @@ __PACKAGE__->register_method({
>                           );
>                           PVE::LXC::create_ifaces_ipams_ips($conf, $vmid) if $unique;
>                           my $lxc_setup = PVE::LXC::Setup->new($conf, $rootdir);
> -                        $lxc_setup->template_fixup($conf);
> +                        $lxc_setup->post_restore_hook($password, $ssh_keys);
>                       } else {
>                           my $lxc_setup = PVE::LXC::Setup->new($conf, $rootdir); # detect OS
>                           PVE::LXC::Config->write_config($vmid, $conf); # safe config (after OS detection)
> diff --git a/src/PVE/LXC/Setup.pm b/src/PVE/LXC/Setup.pm
> index d936af2..58cadba 100644
> --- a/src/PVE/LXC/Setup.pm
> +++ b/src/PVE/LXC/Setup.pm
> @@ -329,6 +329,15 @@ sub post_create_hook {
>       $self->check_systemd_nesting();
>   }
>   
> +sub post_restore_hook {
> +    my ($self, $root_password, $ssh_keys) = @_;
> +
> +    $self->protected_call(sub {
> +        $self->{plugin}->post_restore_hook($self->{conf}, $root_password, $ssh_keys);
> +    });
> +    $self->check_systemd_nesting();
> +}
> +
>   sub unified_cgroupv2_support {
>       my ($self) = @_;
>   
> diff --git a/src/PVE/LXC/Setup/Base.pm b/src/PVE/LXC/Setup/Base.pm
> index 567f9d6..f0e1f01 100644
> --- a/src/PVE/LXC/Setup/Base.pm
> +++ b/src/PVE/LXC/Setup/Base.pm
> @@ -735,6 +735,15 @@ sub post_create_hook {
>       # fixme: what else ?
>   }
>   
> +sub post_restore_hook {
> +    my ($self, $conf, $root_password, $ssh_keys) = @_;
> +
> +    $self->template_fixup($conf);
> +
> +    $self->set_user_password($conf, 'root', $root_password) if defined($root_password);
> +    $self->set_user_authorized_ssh_keys($conf, 'root', $ssh_keys) if defined($ssh_keys);

The `post_create_hook` uses `if $ssh_keys`, so I think this should also 
be used here instead of `if defined($ssh_keys)`?

> +}
> +
>   # File access wrappers for container setup code.
>   # NOTE: those are not direct part of the Plugin API (yet), avoid using them outside the child plugins
>   # For user-namespace support these might need to take uid and gid maps into account.
> diff --git a/src/PVE/LXC/Setup/Plugin.pm b/src/PVE/LXC/Setup/Plugin.pm
> index 95a633f..6ba656b 100644
> --- a/src/PVE/LXC/Setup/Plugin.pm
> +++ b/src/PVE/LXC/Setup/Plugin.pm
> @@ -99,4 +99,9 @@ sub post_create_hook {
>       croak "implement me in sub-class";
>   }
>   
> +sub post_restore_hook {
> +    my ($self, $conf, $root_password, $ssh_keys) = @_;
> +    croak "implement me in sub-class";
> +}
> +
>   1;
> diff --git a/src/PVE/LXC/Setup/Unmanaged.pm b/src/PVE/LXC/Setup/Unmanaged.pm
> index b51be55..0b440cf 100644
> --- a/src/PVE/LXC/Setup/Unmanaged.pm
> +++ b/src/PVE/LXC/Setup/Unmanaged.pm
> @@ -84,4 +84,8 @@ sub post_create_hook {
>       my ($self, $conf, $root_password, $ssh_keys) = @_;
>   }
>   
> +sub post_restore_hook {
> +    my ($self, $conf, $root_password, $ssh_keys) = @_;
> +}
> +
>   1;





^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH container v2] fix #7164: lxc: restore: apply password and ssh keys during container restore
  2026-07-29 14:20 ` Erik Fastermann
@ 2026-07-30 10:43   ` Elias Huhsovitz
  2026-07-31  8:24     ` Erik Fastermann
  0 siblings, 1 reply; 5+ messages in thread
From: Elias Huhsovitz @ 2026-07-30 10:43 UTC (permalink / raw)
  To: Erik Fastermann, pve-devel

Thanks for the quick reply, i have added my comments below.

On Wed Jul 29, 2026 at 4:20 PM CEST, Erik Fastermann wrote:
> The commit message is a little long. I would just shorten it to 'fix 
> #7164: restore: apply password and ssh keys', as lxc is obvious since we 
> are in pve-container and 'during container restore' is clear from the 
> 'restore:' tag.

Fixed, thanks.

>> When restoring a container with `pct restore`, the `--password` and
>> `--ssh-public-keys` options are silently ignored. Previously, the API
>> endpoint only invoked setup hooks during initial container creation or
>> cloning, skipping them entirely for restores.
>> 
>> Introduce a new `post_restore_hook` in the LXC setup plugin
>> architecture. This hook applies the provided credentials if they are
>> defined.
>> 
>> Add `check_systemd_nesting` to the `post_restore_hook`.
>
> Why is this added? A rational here would be helpful. If the reason is 
> just symmetry with `post_create_hook`, we might need to be careful here, 
> as this could introduce new unwanted failure conditions. I have not 
> checked in detail, but some of the code can die.

I believed adding this check might make the restore process more resiliant
since the user might get important warnings early, thus reducing the amount 
of troubleshooting needed.

But as you pointed out, this could lead to a regression.

Since a restore is technically a creating a new contaienrs, i felt as 
though adding this check made sense. Perhaps the `die` is a good thing? 
Alerting the user early regarding potential issues, might be beneficial.

I am curious to know what your opinion is on this! After more consideration 
I am strongly considering omitting the check.

>> 
>> Move `template_fixup`from the API endpoint function to the
>
> Nit: Missing space (`template_fixup`from)

Fixed, thanks.

>
>> `post_restore_hook`, matching the `post_create_hook` logic.
>> 
>> Signed-off-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
>> ---
>> 
>> v1: https://lore.proxmox.com/all/20260716153959.183045-1-e.huhsovitz@proxmox.com/
>> 
>> Changes made according to the feedback (big thanks):
>> https://lore.proxmox.com/all/95a38704-543e-4820-b36c-696cf192a684@proxmox.com/
>> 
>> changes v1->v2:
>> * move logic into new function `post_restore_hook`
>> * do not call `post_create_hook` unconditionally
>> * conditionally call functions to:
>>      - set password
>>      - set ssh keys
>> * call `check_systemd_nesting()` for pct restore
>> 
>>   src/PVE/API2/LXC.pm            | 2 +-
>>   src/PVE/LXC/Setup.pm           | 9 +++++++++
>>   src/PVE/LXC/Setup/Base.pm      | 9 +++++++++
>>   src/PVE/LXC/Setup/Plugin.pm    | 5 +++++
>>   src/PVE/LXC/Setup/Unmanaged.pm | 4 ++++
>>   5 files changed, 28 insertions(+), 1 deletion(-)
>> 
>> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
>> index 22320ba..ad3b635 100644
>> --- a/src/PVE/API2/LXC.pm
>> +++ b/src/PVE/API2/LXC.pm
>> @@ -590,7 +590,7 @@ __PACKAGE__->register_method({
>>                           );
>>                           PVE::LXC::create_ifaces_ipams_ips($conf, $vmid) if $unique;
>>                           my $lxc_setup = PVE::LXC::Setup->new($conf, $rootdir);
>> -                        $lxc_setup->template_fixup($conf);
>> +                        $lxc_setup->post_restore_hook($password, $ssh_keys);
>>                       } else {
>>                           my $lxc_setup = PVE::LXC::Setup->new($conf, $rootdir); # detect OS
>>                           PVE::LXC::Config->write_config($vmid, $conf); # safe config (after OS detection)
>> diff --git a/src/PVE/LXC/Setup.pm b/src/PVE/LXC/Setup.pm
>> index d936af2..58cadba 100644
>> --- a/src/PVE/LXC/Setup.pm
>> +++ b/src/PVE/LXC/Setup.pm
>> @@ -329,6 +329,15 @@ sub post_create_hook {
>>       $self->check_systemd_nesting();
>>   }
>>   
>> +sub post_restore_hook {
>> +    my ($self, $root_password, $ssh_keys) = @_;
>> +
>> +    $self->protected_call(sub {
>> +        $self->{plugin}->post_restore_hook($self->{conf}, $root_password, $ssh_keys);
>> +    });
>> +    $self->check_systemd_nesting();
>> +}
>> +
>>   sub unified_cgroupv2_support {
>>       my ($self) = @_;
>>   
>> diff --git a/src/PVE/LXC/Setup/Base.pm b/src/PVE/LXC/Setup/Base.pm
>> index 567f9d6..f0e1f01 100644
>> --- a/src/PVE/LXC/Setup/Base.pm
>> +++ b/src/PVE/LXC/Setup/Base.pm
>> @@ -735,6 +735,15 @@ sub post_create_hook {
>>       # fixme: what else ?
>>   }
>>   
>> +sub post_restore_hook {
>> +    my ($self, $conf, $root_password, $ssh_keys) = @_;
>> +
>> +    $self->template_fixup($conf);
>> +
>> +    $self->set_user_password($conf, 'root', $root_password) if defined($root_password);
>> +    $self->set_user_authorized_ssh_keys($conf, 'root', $ssh_keys) if defined($ssh_keys);
>
> The `post_create_hook` uses `if $ssh_keys`, so I think this should also 
> be used here instead of `if defined($ssh_keys)`?

Yes, i agree. Originally i wanted to avoid truthy/falsy checks, but my
implementation would return true if the user passed an empty string,
leading to an unnecessary operation.

I will change it to `if $ssh_keys` in v3.

>> +}
>> +
>>   # File access wrappers for container setup code.
>>   # NOTE: those are not direct part of the Plugin API (yet), avoid using them outside the child plugins
>>   # For user-namespace support these might need to take uid and gid maps into account.
>> diff --git a/src/PVE/LXC/Setup/Plugin.pm b/src/PVE/LXC/Setup/Plugin.pm
>> index 95a633f..6ba656b 100644
>> --- a/src/PVE/LXC/Setup/Plugin.pm
>> +++ b/src/PVE/LXC/Setup/Plugin.pm
>> @@ -99,4 +99,9 @@ sub post_create_hook {
>>       croak "implement me in sub-class";
>>   }
>>   
>> +sub post_restore_hook {
>> +    my ($self, $conf, $root_password, $ssh_keys) = @_;
>> +    croak "implement me in sub-class";
>> +}
>> +
>>   1;
>> diff --git a/src/PVE/LXC/Setup/Unmanaged.pm b/src/PVE/LXC/Setup/Unmanaged.pm
>> index b51be55..0b440cf 100644
>> --- a/src/PVE/LXC/Setup/Unmanaged.pm
>> +++ b/src/PVE/LXC/Setup/Unmanaged.pm
>> @@ -84,4 +84,8 @@ sub post_create_hook {
>>       my ($self, $conf, $root_password, $ssh_keys) = @_;
>>   }
>>   
>> +sub post_restore_hook {
>> +    my ($self, $conf, $root_password, $ssh_keys) = @_;
>> +}
>> +
>>   1;





^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH container v2] fix #7164: lxc: restore: apply password and ssh keys during container restore
  2026-07-30 10:43   ` Elias Huhsovitz
@ 2026-07-31  8:24     ` Erik Fastermann
  0 siblings, 0 replies; 5+ messages in thread
From: Erik Fastermann @ 2026-07-31  8:24 UTC (permalink / raw)
  To: Elias Huhsovitz, pve-devel

[snip]

>>> Add `check_systemd_nesting` to the `post_restore_hook`.
>>
>> Why is this added? A rational here would be helpful. If the reason is
>> just symmetry with `post_create_hook`, we might need to be careful here,
>> as this could introduce new unwanted failure conditions. I have not
>> checked in detail, but some of the code can die.
> 
> I believed adding this check might make the restore process more resiliant
> since the user might get important warnings early, thus reducing the amount
> of troubleshooting needed.
> 
> But as you pointed out, this could lead to a regression.
> 
> Since a restore is technically a creating a new contaienrs, i felt as
> though adding this check made sense. Perhaps the `die` is a good thing?
> Alerting the user early regarding potential issues, might be beneficial.
> 
> I am curious to know what your opinion is on this! After more consideration
> I am strongly considering omitting the check.
> 

More checks are usually good and I currently don't see a concrete 
use-case where having this check would break something. I still would 
prefer if this was an additional patch as it is unrelated to the 
original fix.

[snip]




^ permalink raw reply	[flat|nested] 5+ messages in thread

* superseded: [PATCH container v2] fix #7164: lxc: restore: apply password and ssh keys during container restore
  2026-07-29  8:25 [PATCH container v2] fix #7164: lxc: restore: apply password and ssh keys during container restore Elias Huhsovitz
  2026-07-29 14:20 ` Erik Fastermann
@ 2026-07-31 12:08 ` Elias Huhsovitz
  1 sibling, 0 replies; 5+ messages in thread
From: Elias Huhsovitz @ 2026-07-31 12:08 UTC (permalink / raw)
  To: Elias Huhsovitz, pve-devel

Superseded-by: https://lore.proxmox.com/pve-devel/20260731115943.94756-1-e.huhsovitz@proxmox.com/




^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-07-31 12:08 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-29  8:25 [PATCH container v2] fix #7164: lxc: restore: apply password and ssh keys during container restore Elias Huhsovitz
2026-07-29 14:20 ` Erik Fastermann
2026-07-30 10:43   ` Elias Huhsovitz
2026-07-31  8:24     ` Erik Fastermann
2026-07-31 12:08 ` superseded: " Elias Huhsovitz

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.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal