From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 3F4C51FF187 for ; Mon, 25 Aug 2025 14:02:22 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 92D3011139; Mon, 25 Aug 2025 14:02:25 +0200 (CEST) From: Maximiliano Sandoval To: Stoiko Ivanov In-Reply-To: <20250814020926.3266d3b1@rosa.proxmox.com> (Stoiko Ivanov's message of "Thu, 14 Aug 2025 02:09:26 +0200") References: <20250404131438.328396-1-m.sandoval@proxmox.com> <20250814020926.3266d3b1@rosa.proxmox.com> User-Agent: mu4e 1.12.9; emacs 30.1 Date: Mon, 25 Aug 2025 14:01:51 +0200 Message-ID: MIME-Version: 1.0 X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1756123307601 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.100 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pmg-devel] [PATCH pmg-api v5 01/11] pmgpolicy: move pid file into /run/pmgpolicy X-BeenThere: pmg-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Mail Gateway development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: pmg-devel@lists.proxmox.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pmg-devel-bounces@lists.proxmox.com Sender: "pmg-devel" Stoiko Ivanov 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 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 >> --- >> >> 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