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

[Stoiko Ivanov]

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 at 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 at 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)

>  - 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.


> 
> 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');