From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 2437A1FF0E0 for ; Thu, 23 Jul 2026 16:16:23 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 9962721468; Thu, 23 Jul 2026 16:16:22 +0200 (CEST) MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Subject: Re: [PATCH proxmox-backup v3 2/3] api: add heartbeat endpoint for backup reader/writer http/2 clients From: Robert Obkircher To: Christian Ebner In-Reply-To: <20260721114806.505047-3-c.ebner@proxmox.com> References: <20260721114806.505047-1-c.ebner@proxmox.com> <20260721114806.505047-3-c.ebner@proxmox.com> Date: Thu, 23 Jul 2026 16:15:44 +0200 Message-Id: <178481614425.204362.18056299450161646361.b4-review@b4> X-Mailer: b4 0.16-dev X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784816117323 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.169 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: F63FYKTUN7F2K7T6N6KWX64BOLDAYAWU X-Message-ID-Hash: F63FYKTUN7F2K7T6N6KWX64BOLDAYAWU X-MailFrom: r.obkircher@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pbs-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: > Backup readers and writers can potentially have long periods of idle > connections, e.g. a reader if a backup snapshot has been mounted and > all relevant chunks are locally cached, a backup session with > previous metadata archive not needing to fetch new contents while the > backup is ongoing or a backup writer to a datastore with slow backend > storage. > > Proxies like e.g. HAProxy might however close idle connections for > better resource handling [0,1], even multiplexed HTTP/2 connections as > are being used for the Proxmox Backup Sever backup reader/writer > protocol. > > Therefore, perform heartbeat traffic in the HTTP/2 client when no > other requests are being send, tracked by an internal timeout, > being rest before sending a request. nit: typo: "rest" > > Since older servers do not provide the new API path, ignore errors > as the response is not strictly necessary for the connection to > remain established. > > The heartbeat is currently only being performed if the timeout value > in seconds is given via the PBS_READER_HEARTBEAT_TIMEOUT for readers > and PBS_WRITER_HEARTBEAT_TIMEOUT for writers. > > Testing was performed using HAProxy with the Proxmox Backup Server > as backend using the following 5 second connection idle timeouts > as configuration parameters in haproxy.cfg: > ``` > ... > defaults > ... > timeout connect 5000 > timeout client 5000 > timeout server 5000 > .. > > frontend http-in > bind *:8007 > mode tcp > default_backend pbs > > backend pbs > mode tcp > http-reuse always > server pbs :8007 verify none > ``` > > As command invocation: > ``` > PBS_READER_HEARTBEAT_TIMEOUT=1 proxmox-backup-client mount \ > --repository @: --verbose > ``` > > [0] https://www.haproxy.com/documentation/haproxy-configuration-manual/latest/#4-timeout%20client > [1] https://www.haproxy.com/documentation/haproxy-configuration-manual/latest/#4.2-timeout%20http-keep-alive > > Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=6373 > Signed-off-by: Christian Ebner > > diff --git a/pbs-client/src/backup_reader.rs b/pbs-client/src/backup_reader.rs > index 139659479..cd9071310 100644 > --- a/pbs-client/src/backup_reader.rs > +++ b/pbs-client/src/backup_reader.rs > @@ -5,6 +5,7 @@ use std::sync::Arc; > > use futures::future::AbortHandle; > use serde_json::{Value, json}; > +use tokio::sync::mpsc; > > use pbs_api_types::{BackupArchiveName, BackupDir, BackupNamespace, MANIFEST_BLOB_NAME}; > use pbs_datastore::data_blob::DataBlob; > @@ -16,28 +17,61 @@ use pbs_datastore::{BackupManifest, PROXMOX_BACKUP_READER_PROTOCOL_ID_V1}; > use pbs_tools::crypt_config::CryptConfig; > use pbs_tools::sha::sha256; > > -use super::{H2Client, HttpClient}; > +use super::{H2Client, HeartBeatMsg, HttpClient, parse_optional_heartbeat_env}; > > /// Backup Reader > pub struct BackupReader { > h2: H2Client, > abort: AbortHandle, > crypt_config: Option>, > + heartbeat: Option>, > } > > impl Drop for BackupReader { > fn drop(&mut self) { > + self.send_msg_heartbeat(HeartBeatMsg::Abort); > self.abort.abort(); > } > } > > impl BackupReader { > - fn new(h2: H2Client, abort: AbortHandle, crypt_config: Option>) -> Arc { > - Arc::new(Self { > - h2, > - abort, > - crypt_config, > - }) > + fn new( > + h2: H2Client, > + abort: AbortHandle, > + crypt_config: Option>, > + ) -> Result, Error> { > + if let Some(timeout) = parse_optional_heartbeat_env("PBS_READER_HEARTBEAT_TIMEOUT")? { > + let (send, mut recv) = mpsc::channel(1); > + let backup_reader = Arc::new(Self { > + h2, > + abort, > + crypt_config, > + heartbeat: Some(send), > + }); > + let reader_cloned = Arc::clone(&backup_reader); > + > + tokio::spawn(async move { > + loop { > + match tokio::time::timeout(timeout, recv.recv()).await { > + Ok(Some(HeartBeatMsg::Reset)) => (), > + Ok(Some(HeartBeatMsg::Abort)) | Ok(None) => break, > + Err(_elapsed) => { > + // connection idle timeout reached, send heatbeat > + let _ = reader_cloned.h2.get("heartbeat", None).await; > + } > + } Wouldn't this spam the server if someone sets timeout to 0? -- Robert Obkircher