Re: [PATCH container v2] fix #7164: lxc: restore: apply password and ssh keys during container restore

["Elias Huhsovitz" <e.huhsovitz@proxmox.com>]

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;