Re: [pmg-devel] [PATCH pmg-api v5 01/11] pmgpolicy: move pid file into /run/pmgpolicy

[Maximiliano Sandoval <m.sandoval@proxmox.com>]

Stoiko Ivanov <s.ivanov@proxmox.com> writes:

> Hi,
>
> writing the general feedback for the series here - as there's no
> cover-letter - hope that's ok!
>
> Thanks big time for tackling this and sending updates so often!
>
> The series looks quite nice already in general!
> A few nits (I'll send replies to the individual patches for those)
> Most of them are related to the commit-messages being a bit to terse, and
> lacking a few explanation, which might help reviewers - or people looking
> for bugs in the future.
>
> else - I rebased it (after the tree-wide run of proxmox-perltidy on
> pmg-api) - so if you want to spare yourself the hassle - it's on my staff
> repo
>
> comments inline:
> On Fri,  4 Apr 2025 15:14:28 +0200
> Maximiliano Sandoval <m.sandoval@proxmox.com> wrote:
>
>> We use systemd's RuntimeDirectory to ensure the directory exists when needed.
>> 
>> We also set $opt_pidfile using PIDFILE, see
>> https://www.freedesktop.org/software/systemd/man/latest/systemd.exec.html#%24PIDFILE.
>> 
>> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
>> ---
>> 
>> Differences from v4:
>>  - Run pmg-smtp-filter migration if coming from a version older than 9.0.0.
>>  - Instead of adding the pmgpolicy user to the pmg group, pmgproxy.service is
>>    started with SupplementaryGroups=systemd-journal
> is that a typo? (pmgpolicy is not added to the pmg-group therefore
> pmgproxy is started with an another group?)
> (in both cases - a short line as to why would help)

The pmgpolicy user is added to the pmg group later on however this
should read "Instead of adding the pmgpolicy user to the systemd-journal
group, pmgproxy.service is started with
SupplementaryGroups=systemd-journal"

>
>>  - Use $ENV{'PIDFILE'} instead of hardcoding PID path on binaries backed up
>>    with a systemd service
> Not 100% sure - is using the PIDFILE env-var here to address Fiona's
> feedback from
> https://lore.proxmox.com/all/38c4a43b-5f49-41a0-98ca-3911676a0232@proxmox.com/
> ? - If so - I'm not sure that this would be enough - as the pid-file is
> read by other services (pmgdaemon upon config-changes) - so I still see a
> theoretical potential for a race (but would assume that all services
> should be restarted one after the other while pmg-api (which ships
> all of the services) is upgraded - so I think it should be ok.
>

This is only done so that the pid file is hardcoded in only 2 (one for
the shared uses) places instead of 3.

>> 
>> Differences from v3:
>>  - Override rrdcached's systemd unit to add SOCKGROUP=pmg instead of
>>    modifying /etc/default/rrdcached.conf
>> 
>> Differences from v2:
>>  - Use systemd-sysusers for creating users
>> 
>>  debian/pmgpolicy.service | 3 ++-
>>  src/bin/pmgpolicy        | 2 +-
>>  2 files changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/debian/pmgpolicy.service b/debian/pmgpolicy.service
>> index 517a5d61..21a403f0 100644
>> --- a/debian/pmgpolicy.service
>> +++ b/debian/pmgpolicy.service
>> @@ -10,8 +10,9 @@ ExecStart=/usr/bin/pmgpolicy
>>  KillMode=mixed
>>  TimeoutStopSec=40
>>  ExecReload=/bin/kill -HUP $MAINPID
>> -PIDFile=/run/pmgpolicy.pid
>> +PIDFile=/run/pmgpolicy/pmgpolicy.pid
>>  Type=forking
>> +RuntimeDirectory=pmgpolicy
>>  
>>  [Install]
>>  WantedBy=multi-user.target
>> diff --git a/src/bin/pmgpolicy b/src/bin/pmgpolicy
>> index df2e66f4..3f976ff7 100755
>> --- a/src/bin/pmgpolicy
>> +++ b/src/bin/pmgpolicy
>> @@ -56,7 +56,7 @@ if (!GetOptions(%_opts)) {
>>      exit (-1);
>>  }
>>  
>> -$opt_pidfile = "/run/pmgpolicy.pid" if !$opt_pidfile;
>> +$opt_pidfile = $ENV{'PIDFILE'} if !$opt_pidfile;
>>  $opt_max_dequeue = 0 if $opt_testmode;
>>  
>>  initlog('pmgpolicy', 'mail');

-- 
Maximiliano


_______________________________________________
pmg-devel mailing list
pmg-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel