Re: [PATCH pve-manager] api: nodes: dns: restart postfix after changing name servers

[Jonas Theisen <j.theisen@proxmox.com>]

On 11/10/26 09:32, Dominik Csapak wrote:
> Hi,
>
> first, does postfix really need a restart or would a 'reload' be enough?

Tried that but no a reload does not seem to rewrite the copied config.
Only a restart refreshed the settings.
>
> if so, would it be possible to use try-reload-or-restart instead?

Thanks for that pointer. tbh i wasn't even aware that these existed
with the try prefix. Switched that out for a try-restart, since the
try-reload-or-restart would just issue a reload since postfix is capable 
of it.
>
> some comments inline:
>
> On 11/9/26 11:51 AM, Jonas Theisen wrote:
>> <snip>
>> --- a/PVE/API2/Nodes.pm
>> +++ b/PVE/API2/Nodes.pm
>> @@ -1671,6 +1671,17 @@ __PACKAGE__->register_method({
>>             PVE::INotify::update_file('resolvconf', $param);
>>   +        # postfix copies resolv.conf into its chroot only on 
>> start, so a running instance needs a
>> +        # restart to use the new name servers
>> +        eval {
>> +            my $active = PVE::Tools::run_command(
>
> please use `PVE::Cmd::run` for new code instead of `run_command`
> (we recently moved that code into it's own perl module)
> interface is identical

Ack switched that out.
>> +                ['systemctl', 'is-active', '--quiet', 'postfix@-'],
>> +                noerr => 1,
>> +            );
>
> on several machines of mine this does return an error:
>
> ```
> root@pve:~# systemctl is-active --quiet postfix@-
> root@pve:~# echo $?
> 3
> ```
>
> sure it's the right unit to check and restart?

Right, sorry for whatever reason i tried this on a PVE 8 and
from there to PVE 9 the units changed.
Fixed that in v2.
>
> also, '$active' sounds like must be set to >0 if the unit is active
> and code below looks like we restart it if it's not running
>
> better name could be
>
> $is_active_error
>
> (or something like that, i'm not the best at naming either ;) )

Ack will keep that in mind for the future.
With the current v2 this is not an issue anymore.
>
>> + PVE::Tools::run_command(['systemctl', 'restart', 'postfix@-']) if 
>> $active == 0;
>> +        };
>> +        warn "failed to restart postfix - $@" if $@;
>> +
>>           return;
>>       },
>>   });
>