From e9b9f33aee8c2181ba6571e31a75ce7742c5081d Mon Sep 17 00:00:00 2001 From: Fabian Ebner Date: Wed, 4 May 2022 13:33:24 +0200 Subject: [PATCH] rest server: daemon: update PID file before sending MAINPID notification There is a race upon reload, where it can happen that: 1. systemd forks off /bin/kill -HUP $MAINPID 2. Current instance forks off new one and notifies systemd with the new MAINPID. 3. systemd sets new MAINPID. 4. systemd receives SIGCHLD for the kill process (which is the current control process for the service) and reads the PID of the old instance from the PID file, resetting MAINPID to the PID of the old instance. 5. Old instance exits. 6. systemd receives SIGCHLD for the old instance, reads the PID of the old instance from the PID file once more. systemd sees that the MAINPID matches the child PID and considers the service exited. 7. systemd receivese notification from the new PID and is confused. The service won't get active, because the notification wasn't handled. To fix it, update the PID file before sending the MAINPID notification, similar to what a comment in systemd's src/core/service.c suggests: > /* Forking services may occasionally move to a new PID. > * As long as they update the PID file before exiting the old > * PID, they're fine. */ but for our Type=notify "before sending the notification" rather than "before exiting", because otherwise, the mix-up in 4. could still happen (although it might not actually be problematic without the mix-up in 6., it still seems better to avoid). Signed-off-by: Fabian Ebner --- .../examples/minimal-rest-server.rs | 18 +++++++----- proxmox-rest-server/src/daemon.rs | 16 +++++++++-- src/bin/proxmox-backup-api.rs | 26 +++++++++-------- src/bin/proxmox-backup-proxy.rs | 28 +++++++++++-------- 4 files changed, 56 insertions(+), 32 deletions(-) diff --git a/proxmox-rest-server/examples/minimal-rest-server.rs b/proxmox-rest-server/examples/minimal-rest-server.rs index 0b1bfd53..9bb03402 100644 --- a/proxmox-rest-server/examples/minimal-rest-server.rs +++ b/proxmox-rest-server/examples/minimal-rest-server.rs @@ -210,15 +210,19 @@ async fn run() -> Result<(), Error> { // then we have to create a daemon that listens, accepts and serves // the api to clients - proxmox_rest_server::daemon::create_daemon(([127, 0, 0, 1], 65000).into(), move |listener| { - let incoming = hyper::server::conn::AddrIncoming::from_listener(listener)?; + proxmox_rest_server::daemon::create_daemon( + ([127, 0, 0, 1], 65000).into(), + move |listener| { + let incoming = hyper::server::conn::AddrIncoming::from_listener(listener)?; - Ok(async move { - hyper::Server::builder(incoming).serve(rest_server).await?; + Ok(async move { + hyper::Server::builder(incoming).serve(rest_server).await?; - Ok(()) - }) - }) + Ok(()) + }) + }, + None, + ) .await?; Ok(()) diff --git a/proxmox-rest-server/src/daemon.rs b/proxmox-rest-server/src/daemon.rs index 4e09118d..4a5806bd 100644 --- a/proxmox-rest-server/src/daemon.rs +++ b/proxmox-rest-server/src/daemon.rs @@ -15,6 +15,7 @@ use nix::unistd::{fork, ForkResult}; use proxmox_io::{ReadExt, WriteExt}; use proxmox_sys::fd::{fd_change_cloexec, Fd}; +use proxmox_sys::fs::CreateOptions; // Unfortunately FnBox is nightly-only and Box is unusable, so just use Box... type BoxedStoreFunc = Box Result + UnwindSafe + Send>; @@ -81,7 +82,7 @@ impl Reloader { Ok(()) } - pub fn fork_restart(self) -> Result<(), Error> { + pub fn fork_restart(self, pid_fn: Option<&str>) -> Result<(), Error> { // Get our parameters as Vec let args = std::env::args_os(); let mut new_args = Vec::with_capacity(args.len()); @@ -179,6 +180,16 @@ impl Reloader { } }); + if let Some(pid_fn) = pid_fn { + let pid_str = format!("{}\n", child); + proxmox_sys::fs::replace_file( + pid_fn, + pid_str.as_bytes(), + CreateOptions::new(), + false, + )?; + } + if let Err(e) = systemd_notify(SystemdNotify::MainPid(child)) { log::error!("failed to notify systemd about the new main pid: {}", e); } @@ -250,6 +261,7 @@ impl Reloadable for tokio::net::TcpListener { pub async fn create_daemon( address: std::net::SocketAddr, create_service: F, + pidfn: Option<&str>, ) -> Result<(), Error> where F: FnOnce(tokio::net::TcpListener) -> Result, @@ -295,7 +307,7 @@ where log::error!("failed to wait on systemd-processing: {}", e); } - if let Err(e) = reloader.take().unwrap().fork_restart() { + if let Err(e) = reloader.take().unwrap().fork_restart(pidfn) { log::error!("error during reload: {}", e); let _ = systemd_notify(SystemdNotify::Status("error during reload".to_string())); } diff --git a/src/bin/proxmox-backup-api.rs b/src/bin/proxmox-backup-api.rs index f47656e2..dda4b638 100644 --- a/src/bin/proxmox-backup-api.rs +++ b/src/bin/proxmox-backup-api.rs @@ -138,19 +138,23 @@ async fn run() -> Result<(), Error> { )?; // http server future: - let server = daemon::create_daemon(([127, 0, 0, 1], 82).into(), move |listener| { - let incoming = hyper::server::conn::AddrIncoming::from_listener(listener)?; + let server = daemon::create_daemon( + ([127, 0, 0, 1], 82).into(), + move |listener| { + let incoming = hyper::server::conn::AddrIncoming::from_listener(listener)?; - Ok(async { - daemon::systemd_notify(daemon::SystemdNotify::Ready)?; + Ok(async { + daemon::systemd_notify(daemon::SystemdNotify::Ready)?; - hyper::Server::builder(incoming) - .serve(rest_server) - .with_graceful_shutdown(proxmox_rest_server::shutdown_future()) - .map_err(Error::from) - .await - }) - }); + hyper::Server::builder(incoming) + .serve(rest_server) + .with_graceful_shutdown(proxmox_rest_server::shutdown_future()) + .map_err(Error::from) + .await + }) + }, + Some(pbs_buildcfg::PROXMOX_BACKUP_API_PID_FN), + ); proxmox_rest_server::write_pid(pbs_buildcfg::PROXMOX_BACKUP_API_PID_FN)?; diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs index 7d32bb04..91b38336 100644 --- a/src/bin/proxmox-backup-proxy.rs +++ b/src/bin/proxmox-backup-proxy.rs @@ -301,20 +301,24 @@ async fn run() -> Result<(), Error> { Ok(Value::Null) })?; - let server = daemon::create_daemon(([0, 0, 0, 0, 0, 0, 0, 0], 8007).into(), move |listener| { - let connections = accept_connections(listener, acceptor, debug); - let connections = hyper::server::accept::from_stream(ReceiverStream::new(connections)); + let server = daemon::create_daemon( + ([0, 0, 0, 0, 0, 0, 0, 0], 8007).into(), + move |listener| { + let connections = accept_connections(listener, acceptor, debug); + let connections = hyper::server::accept::from_stream(ReceiverStream::new(connections)); - Ok(async { - daemon::systemd_notify(daemon::SystemdNotify::Ready)?; + Ok(async { + daemon::systemd_notify(daemon::SystemdNotify::Ready)?; - hyper::Server::builder(connections) - .serve(rest_server) - .with_graceful_shutdown(proxmox_rest_server::shutdown_future()) - .map_err(Error::from) - .await - }) - }); + hyper::Server::builder(connections) + .serve(rest_server) + .with_graceful_shutdown(proxmox_rest_server::shutdown_future()) + .map_err(Error::from) + .await + }) + }, + Some(pbs_buildcfg::PROXMOX_BACKUP_PROXY_PID_FN), + ); proxmox_rest_server::write_pid(pbs_buildcfg::PROXMOX_BACKUP_PROXY_PID_FN)?;