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 217731FF176 for ; Fri, 7 Feb 2025 15:36:05 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id F3D26E651; Fri, 7 Feb 2025 15:36:03 +0100 (CET) Date: Fri, 7 Feb 2025 15:35:59 +0100 From: Wolfgang Bumiller To: Dominik Csapak Message-ID: References: <20241203112349.2446383-1-d.csapak@proxmox.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20241203112349.2446383-1-d.csapak@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.082 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [server.rs] Subject: Re: [pbs-devel] [PATCH proxmox v2] daemon: clean up middle process of double fork X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Cc: pbs-devel@lists.proxmox.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" On Tue, Dec 03, 2024 at 12:23:49PM +0100, Dominik Csapak wrote: > so we don't leave around a zombie process when the old daemon still > needs to run, because of e.g. a running task. > > Since this is mostly a cosmetic issue though, only try a clean up with a > 10 second timeout, so we don't block forever. (It could happen that it > didn't exit at that point, but it's very unlikely.) > > In case we do run into the timeout here, the process will not be > collected until the parent process exits and the middle process is > collected by pid 1. > > Signed-off-by: Dominik Csapak > --- > changes from v1: > * use a timeout > * log the error > > proxmox-daemon/src/server.rs | 33 +++++++++++++++++++++++++++++++-- > 1 file changed, 31 insertions(+), 2 deletions(-) > > diff --git a/proxmox-daemon/src/server.rs b/proxmox-daemon/src/server.rs > index efea9078..27153fc2 100644 > --- a/proxmox-daemon/src/server.rs > +++ b/proxmox-daemon/src/server.rs > @@ -8,6 +8,7 @@ use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd, OwnedFd, RawFd}; > use std::panic::UnwindSafe; > use std::path::PathBuf; > use std::pin::{pin, Pin}; > +use std::time::Duration; > > use anyhow::{bail, format_err, Error}; > use futures::future::{self, Either}; > @@ -15,6 +16,7 @@ use nix::unistd::{fork, ForkResult}; > > use proxmox_sys::fd::fd_change_cloexec; > use proxmox_sys::fs::CreateOptions; > +use proxmox_sys::linux::timer; > > type BoxedStoreFunc = Box Result + UnwindSafe + Send>; > > @@ -165,10 +167,12 @@ impl Reloader { The error case above here does a `log::error!()` - that is literally the only thing which could possibly hang. We could have a 2nd socketpair for stringified errors (or turn the existing one into a datagram based socket with a type prefix for messages) and just use a blocking wait(). But if we really want to use a timeout, I'd rather add a pidfd wrapper, use `pidfd_open()` (it's our fork, so since we don't `wait()` for it, we should get the correct process, and we could - in the future - replace `fork()` with an equivalent `clone()` returning a pidfd). The pidfd can be used with poll/epoll (so it would even work for async code via `AsyncFd`). This is also a case where we could consider switching to a vfork() type of fork. That might even play nice with whatever logger we might be using, making it even less likely for anything to block... > // No matter how we managed to get here, this is the time where we bail out quickly: > unsafe { libc::_exit(-1) } > } > - Ok(ForkResult::Parent { child }) => { > + Ok(ForkResult::Parent { > + child: middle_child, > + }) => { > log::debug!( > "forked off a new server (first pid: {}), waiting for 2nd pid", > - child > + middle_child > ); > std::mem::drop(pnew); > let mut pold = std::fs::File::from(pold); > @@ -211,6 +215,10 @@ impl Reloader { > log::error!("child vanished during reload: {}", e); > } > > + if let Err(e) = waitpid_with_timeout(middle_child, Duration::from_secs(10)) { > + log::error!("waitpid for middle process failed: {e}"); > + } > + > Ok(()) > } > Err(e) => { > @@ -230,6 +238,27 @@ impl Reloader { > } > } > > +fn waitpid_with_timeout(pid: nix::unistd::Pid, timeout: std::time::Duration) -> Result<(), Error> { > + // unblock the timeout signal temporarily > + let _sigblock_guard = timer::unblock_timeout_signal(); > + > + // setup a timeout timer > + let mut timer = timer::Timer::create( > + timer::Clock::Realtime, > + timer::TimerEvent::ThisThreadSignal(timer::SIGTIMEOUT), > + )?; > + > + timer.arm( > + timer::TimerSpec::new() > + .value(Some(timeout)) > + .interval(Some(Duration::from_millis(10))), > + )?; > + > + nix::sys::wait::waitpid(pid, None)?; > + > + Ok(()) > +} > + > fn fd_store_func(fd: RawFd) -> Result { > let fd = unsafe { > OwnedFd::from_raw_fd(nix::fcntl::fcntl( > -- > 2.39.5 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel