public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pbs-devel] [PATCH/RFC proxmox-backup] rest server: daemon: update PID file before sending MAINPID notification
@ 2022-05-04 11:33 Fabian Ebner
  2022-05-12 10:04 ` [pbs-devel] applied: " Thomas Lamprecht
  0 siblings, 1 reply; 2+ messages in thread
From: Fabian Ebner @ 2022-05-04 11:33 UTC (permalink / raw)
  To: pbs-devel

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 <f.ebner@proxmox.com>
---

An alternative would be to not tell systemd about the PIDFile at all,
but there's two small downsides:
* The PID file isn't cleaned up automatically when the service exits.
* Having the PID file updated before sending the MAINPID notification
  feels a bit cleaner (even if the PID file is not used by systemd).

 .../examples/minimal-rest-server.rs           | 18 +++++++----
 proxmox-rest-server/src/daemon.rs             | 16 ++++++++--
 src/bin/proxmox-backup-api.rs                 | 30 +++++++++--------
 src/bin/proxmox-backup-proxy.rs               | 32 +++++++++++--------
 4 files changed, 60 insertions(+), 36 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<FnOnce> is unusable, so just use Box<Fn>...
 type BoxedStoreFunc = Box<dyn FnMut() -> Result<String, Error> + 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<CString>
         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<F, S>(
     address: std::net::SocketAddr,
     create_service: F,
+    pidfn: Option<&str>,
 ) -> Result<(), Error>
 where
     F: FnOnce(tokio::net::TcpListener) -> Result<S, Error>,
@@ -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)?;
-
-        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
-        })
-    });
+    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)?;
+
+                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 6a305b7c..1f0be623 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));
-
-        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
-        })
-    });
+    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)?;
+
+                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)?;
 
-- 
2.30.2





^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-05-12 10:04 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-04 11:33 [pbs-devel] [PATCH/RFC proxmox-backup] rest server: daemon: update PID file before sending MAINPID notification Fabian Ebner
2022-05-12 10:04 ` [pbs-devel] applied: " Thomas Lamprecht

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal